Re: spin_unlock_wait() in ata_scsi_cmd_error_handler()?

[Paul E. McKenney]

On Thu, Jun 29, 2017 at 04:17:54PM -0400, Tejun Heo wrote:
> 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@xxxxxxxxxx>

Applied, and thank you!

							Thanx, Paul