Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1678171

Re: spin_unlock_wait() in ata_scsi_cmd_error_handler()?

Show all headers | View raw

Hello,

On Thu, Jun 29, 2017 at 01:14:43PM -0700, Paul E. McKenney wrote:
> On Thu, Jun 29, 2017 at 03:53:22PM -0400, Tejun Heo wrote:
> > Hello, Paul.
> > 
> > On Thu, Jun 29, 2017 at 11:10:57AM -0700, Paul E. McKenney wrote:
> > > If this code fragment doesn't deadlock, then CPU 0's spin_unlock_wait()
> > > must have executed before CPU 1's spin_lock().  However, even on x86,
> > > CPU 0's prior writes can be reordered with its subsequent reads, which
> > > means that r1 == 0 is possible, which means that the above condition
> > > could hold, even on x86.
> > 
> > I see.  Ah, that's a mind bender.
> 
> It has indeed been providing at least its share of entertainment over
> the past little while.  ;-)

lol :)

> > That part of the code should be dead now.  I don't think we no longer
> > have any driver which doesn't have error handler set.  I should rip
> > out that if/else.  Also, ACQUIRE semantics should be enough there.
> > Nothing changes from the EH side there.
> 
> It looks like we actually might get rid of spin_unlock_wait entirely.
> But how about if I just pull the spin_lock_irqsave() before the "if"
> and the spin_lock_irqrestore() after the "if"?  Same effect, only
> difference is that the "if" and the "ap->eh_tries = ATA_EH_MAX_TRIES"
> end up under the lock, and I bet that you won't be able to measure
> the difference.  (Please see below.)
> 
> I will do this because I just now happened to be editing that file on
> my "eradicate spin_unlock_wait()" quest, but can easily rework the
> patch as desired.  If you want something different, just let me know!

Sounds good to me.  That path isn't hot at all.  No change made at
this level is gonna have any actual impact.  Please go for whatever is
the simplest.  For moving out the lock/unlock outside if/else,

 Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

spin_unlock_wait() in ata_scsi_cmd_error_handler()? "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-29 20:20 +0200
  Re: spin_unlock_wait() in ata_scsi_cmd_error_handler()? Tejun Heo <tj@kernel.org> - 2017-06-29 22:00 +0200
    Re: spin_unlock_wait() in ata_scsi_cmd_error_handler()? "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-29 22:20 +0200
      Re: spin_unlock_wait() in ata_scsi_cmd_error_handler()? Tejun Heo <tj@kernel.org> - 2017-06-29 22:20 +0200
        Re: spin_unlock_wait() in ata_scsi_cmd_error_handler()? "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-29 22:50 +0200

csiph-web