Re: [PATCH] Documentation/locking: Document the semantics of spin_is_locked()

[Paul E. McKenney]

On Wed, Feb 28, 2018 at 01:15:23PM +0100, Andrea Parri wrote:
> On Wed, Feb 28, 2018 at 11:34:56AM +0000, Will Deacon wrote:
> > On Wed, Feb 28, 2018 at 12:24:03PM +0100, Andrea Parri wrote:
> > > On Wed, Feb 28, 2018 at 10:56:32AM +0000, Will Deacon wrote:
> > > > On Wed, Feb 28, 2018 at 11:39:32AM +0100, Andrea Parri wrote:
> > > > > There appeared to be a certain, recurrent uncertainty concerning the
> > > > > semantics of spin_is_locked(), likely a consequence of the fact that
> > > > > this semantics remains undocumented or that it has been historically
> > > > > linked to the (likewise unclear) semantics of spin_unlock_wait().
> > > > > 
> > > > > Document this semantics.
> > > > > 
> > > > > Signed-off-by: Andrea Parri <parri.andrea@xxxxxxxxx>
> > > > > Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > > > > Cc: Will Deacon <will.deacon@xxxxxxx>
> > > > > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > > > > Cc: Boqun Feng <boqun.feng@xxxxxxxxx>
> > > > > Cc: Nicholas Piggin <npiggin@xxxxxxxxx>
> > > > > Cc: David Howells <dhowells@xxxxxxxxxx>
> > > > > Cc: Jade Alglave <j.alglave@xxxxxxxxx>
> > > > > Cc: Luc Maranget <luc.maranget@xxxxxxxx>
> > > > > Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx>
> > > > > Cc: Akira Yokosawa <akiyks@xxxxxxxxx>
> > > > > ---
> > > > >  include/linux/spinlock.h | 11 +++++++++++
> > > > >  1 file changed, 11 insertions(+)
> > > > > 
> > > > > diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> > > > > index 4894d322d2584..2639fdc9a916c 100644
> > > > > --- a/include/linux/spinlock.h
> > > > > +++ b/include/linux/spinlock.h
> > > > > @@ -380,6 +380,17 @@ static __always_inline int spin_trylock_irq(spinlock_t *lock)
> > > > >  	raw_spin_trylock_irqsave(spinlock_check(lock), flags); \
> > > > >  })
> > > > >  
> > > > > +/**
> > > > > + * spin_is_locked() - Check whether a spinlock is locked.
> > > > > + * @lock: Pointer to the spinlock.
> > > > > + *
> > > > > + * This function is NOT required to provide any memory ordering
> > > > > + * guarantees; it could be used for debugging purposes or, when
> > > > > + * additional synchronization is needed, accompanied with other
> > > > > + * constructs (memory barriers) enforcing the synchronization.
> > > > > + *
> > > > > + * Return: 1, if @lock is (found to be) locked; 0, otherwise.
> > > > > + */
> > > > 
> > > > I also don't think this is quite right, since the spin_is_locked check
> > > > must be ordered after all prior lock acquisitions (to any lock) on the same
> > > > CPU. That's why we have an smp_mb() in there on arm64 (see 38b850a73034f).
> > > 
> > > So, arm64 (and powerpc) complies to the semantics I _have_ in mind ...
> > 
> > Sure, but they're offering more than that at present. If I can remove the
> > smp_mb() in our spin_is_locked implementation, I will, but we need to know
> > what that will break even if you consider that code to be broken because it
> > relies on something undocumented.
> > 
> > > > So this is a change in semantics and we need to audit the users before
> > > > proceeding. We should also keep spin_is_locked consistent with the versions
> > > > for mutex, rwsem, bit_spin.
> > > 
> > > Well, strictly speaking, it isn't (given that the current semantics is,
> > > as reported above, currently undocumented); for the same reason, cases
> > > relying on anything more than _nothing_ (if any) are already broken ...
> > 
> > I suppose it depends on whether you consider the code or the documentation
> > to be authoritative. I tend to err on the side of the former for the kernel.
> > To be clear: I'm perfectly ok relaxing the semantics, but only if there's
> > some evidence that you've looked at the callsites and determined that they
> > won't break.  That's why I think a better first step would be to convert a
> > bunch of them to using lockdep for the "assert that I hold this lock"
> > checks, so we can start to see where the interesting cases are.
> 
> Sure, I'll do (queued after the RISC-V patches I'm currently working on).
> 
> So I think that we could all agree that the semantics I'm proposing here
> would be very simple to reason with ;-).  You know, OTOH, this auditing
> could turn out to be all but "simple"...
> 
>   https://marc.info/?l=linux-kernel&m=149910202928559&w=2
>   https://marc.info/?l=linux-kernel&m=149886113629263&w=2
>   https://marc.info/?l=linux-kernel&m=149912971028729&w=2

Indeed, if it was easy, we probably would have already done it.  ;-)

> but I'll have a try, IAC.  Perhaps, a temporary solution/workaround can
> be to simplify/clarify the semantics and to insert the smp_mb() (or the
> smp_mb__before_islocked(), ...) in the "dubious" use cases.

One approach that can be quite helpful is to instrument the code, perhaps
using tracing or perhaps as Will suggests using lockdep, to make it tell
you what it is up to.  Another approach is to find peope who actually
use kdgb and see if any of them mess with CPU hotplug.

							Thanx, Paul