[PATCH v3 0/3] Changes, clean-ups and documentation for spin_is_locked()

[PATCH v3 0/3] Changes, clean-ups and documentation for spin_is_locked()

[Andrea Parri]

Hi Paul,

This series gathers the change to the arm64 implementation of spin_is_locked()
and the clean-up to the generic (q)spinlock presented in [1] together with the
patch adding the docbook header to spin_is_locked() [2].

Apart from minor adjustments to the commit messages, the patches are unchanged.

Cheers,
  Andrea

[1] https://marc.info/?l=linux-kernel&m=152223038924258&w=2
[2] https://marc.info/?l=linux-kernel&m=151981440005264&w=2

Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>

Andrea Parri (3):
  arm64: Remove smp_mb() from arch_spin_is_locked()
  locking: Clean-up comment and #ifndef for {,queued_}spin_is_locked()
  Documentation/locking: Document the semantics of spin_is_locked()

 arch/arm64/include/asm/spinlock.h |  5 -----
 include/asm-generic/qspinlock.h   |  2 --
 include/linux/mutex.h             |  3 ---
 include/linux/spinlock.h          | 11 +++++++++++
 4 files changed, 11 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[Andrea Parri]

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().

A recent auditing [1] of the callers of the primitive confirmed that
none of them are relying on particular ordering guarantees; document
this semantics by adding a docbook header to spin_is_locked().

[1] https://marc.info/?l=linux-kernel&m=151981440005264&w=2

Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Jade Alglave <j.alglave@ucl.ac.uk>
Cc: Luc Maranget <luc.maranget@inria.fr>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Akira Yokosawa <akiyks@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
---
 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.
+ */
 static __always_inline int spin_is_locked(spinlock_t *lock)
 {
 	return raw_spin_is_locked(&lock->rlock);
-- 
2.7.4

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[Alan Stern]

On Sun, 1 Apr 2018, 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().
> 
> A recent auditing [1] of the callers of the primitive confirmed that
> none of them are relying on particular ordering guarantees; document
> this semantics by adding a docbook header to spin_is_locked().
> 
> [1] https://marc.info/?l=linux-kernel&m=151981440005264&w=2
> 
> Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Jade Alglave <j.alglave@ucl.ac.uk>
> Cc: Luc Maranget <luc.maranget@inria.fr>
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Cc: Akira Yokosawa <akiyks@gmail.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> ---
>  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.

This is a good addition.  But please remove the parenthetical phrase.  
Or if you prefer to keep it, at least remove the parentheses.

Alan

> + */
>  static __always_inline int spin_is_locked(spinlock_t *lock)
>  {
>  	return raw_spin_is_locked(&lock->rlock);
> 

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[Paul E. McKenney]

On Mon, Apr 02, 2018 at 10:03:22AM -0400, Alan Stern wrote:
> On Sun, 1 Apr 2018, 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().
> > 
> > A recent auditing [1] of the callers of the primitive confirmed that
> > none of them are relying on particular ordering guarantees; document
> > this semantics by adding a docbook header to spin_is_locked().
> > 
> > [1] https://marc.info/?l=linux-kernel&m=151981440005264&w=2
> > 
> > Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Boqun Feng <boqun.feng@gmail.com>
> > Cc: Nicholas Piggin <npiggin@gmail.com>
> > Cc: David Howells <dhowells@redhat.com>
> > Cc: Jade Alglave <j.alglave@ucl.ac.uk>
> > Cc: Luc Maranget <luc.maranget@inria.fr>
> > Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > Cc: Akira Yokosawa <akiyks@gmail.com>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > ---
> >  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.
> 
> This is a good addition.  But please remove the parenthetical phrase.  
> Or if you prefer to keep it, at least remove the parentheses.

Unless someone objects or proposes a different course of action, I will
make this change in -rcu.

							Thanx, Paul

> Alan
> 
> > + */
> >  static __always_inline int spin_is_locked(spinlock_t *lock)
> >  {
> >  	return raw_spin_is_locked(&lock->rlock);
> > 
> 

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[David Howells]

Andrea Parri <andrea.parri@amarulasolutions.com> wrote:

> +/**
> + * 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.

It's more complicated than that.  This function is dangerous and should be
used with extreme care.  In the case where CONFIG_SMP=n the value is locked
one way or the other and it might be the wrong way.

David

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[Andrea Parri]

On Tue, Apr 03, 2018 at 01:49:09PM +0100, David Howells wrote:
> Andrea Parri <andrea.parri@amarulasolutions.com> wrote:
> 
> > +/**
> > + * 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.
> 
> It's more complicated than that.  This function is dangerous and should be
> used with extreme care.  In the case where CONFIG_SMP=n the value is locked
> one way or the other and it might be the wrong way.

You mean "unlocked"? (aka, return 0)

  Andrea


> 
> David

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[David Howells]

Andrea Parri <andrea.parri@amarulasolutions.com> wrote:

> > It's more complicated than that.  This function is dangerous and should be
> > used with extreme care.  In the case where CONFIG_SMP=n the value is locked
> > one way or the other and it might be the wrong way.
> 
> You mean "unlocked"? (aka, return 0)

No, I mean "fixed", sorry.  We've had problems stemming from this before on UP
systems.

David

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[Andrea Parri]

On Tue, Apr 03, 2018 at 02:52:33PM +0100, David Howells wrote:
> Andrea Parri <andrea.parri@amarulasolutions.com> wrote:
> 
> > > It's more complicated than that.  This function is dangerous and should be
> > > used with extreme care.  In the case where CONFIG_SMP=n the value is locked
> > > one way or the other and it might be the wrong way.
> > 
> > You mean "unlocked"? (aka, return 0)
> 
> No, I mean "fixed", sorry.  We've had problems stemming from this before on UP
> systems.

Sorry, but I don't understand your objection: are you suggesting to add
something like "Always return 0 on !SMP" to the comment?  what else?

  Andrea


> 
> David

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[David Howells]

Andrea Parri <andrea.parri@amarulasolutions.com> wrote:

> Sorry, but I don't understand your objection: are you suggesting to add
> something like "Always return 0 on !SMP" to the comment?  what else?

Something like that, possibly along with a warning that this might not be what
you want.  You might actually want it to return true on !SMP, it depends on
what you're using it for.

David

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[Andrea Parri]

On Tue, Apr 03, 2018 at 04:23:07PM +0100, David Howells wrote:
> Andrea Parri <andrea.parri@amarulasolutions.com> wrote:
> 
> > Sorry, but I don't understand your objection: are you suggesting to add
> > something like "Always return 0 on !SMP" to the comment?  what else?
> 
> Something like that, possibly along with a warning that this might not be what
> you want.  You might actually want it to return true on !SMP, it depends on
> what you're using it for.

I ended up with the following revision.  I hesitated on whether to refer
to 'include/linux/spinlock_up.h' or not, but in the end I decided to not
include the reference.  Please let me know what you think about this.

  Andrea


>From 85f2d12d4ad9769cc9f69cc5f447fdb8c5ed4d14 Mon Sep 17 00:00:00 2001
From: Andrea Parri <andrea.parri@amarulasolutions.com>
Date: Tue, 3 Apr 2018 21:23:07 +0200
Subject: [PATCH v3 1/3] locking: Document the semantics of spin_is_locked()

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().

A recent auditing [1] of the callers of the primitive confirmed that
none of them are relying on particular ordering guarantees; document
this semantics by adding a docbook header to spin_is_locked().

[1] https://marc.info/?l=linux-kernel&m=151981440005264&w=2

Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Jade Alglave <j.alglave@ucl.ac.uk>
Cc: Luc Maranget <luc.maranget@inria.fr>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Akira Yokosawa <akiyks@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
---
 include/linux/spinlock.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 4894d322d2584..636a4436191c1 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -380,6 +380,20 @@ 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.
+ *
+ *	   Remark that this primitve can return a fixed value
+ *	   under certain !SMP configurations.
+ */
 static __always_inline int spin_is_locked(spinlock_t *lock)
 {
 	return raw_spin_is_locked(&lock->rlock);
-- 
2.7.4



> 
> David

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[Alan Stern]

On Tue, 3 Apr 2018, Andrea Parri wrote:

> On Tue, Apr 03, 2018 at 04:23:07PM +0100, David Howells wrote:
> > Andrea Parri <andrea.parri@amarulasolutions.com> wrote:
> > 
> > > Sorry, but I don't understand your objection: are you suggesting to add
> > > something like "Always return 0 on !SMP" to the comment?  what else?
> > 
> > Something like that, possibly along with a warning that this might not be what
> > you want.  You might actually want it to return true on !SMP, it depends on
> > what you're using it for.
> 
> I ended up with the following revision.  I hesitated on whether to refer
> to 'include/linux/spinlock_up.h' or not, but in the end I decided to not
> include the reference.  Please let me know what you think about this.


> +/**
> + * 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.
> + *
> + *	   Remark that this primitve can return a fixed value
> + *	   under certain !SMP configurations.

I would change these last two paragraphs as follows:

+ * Returns: 1 if @lock is locked, 0 otherwise.
+ * However, on !CONFIG_SMP builds with !CONFIG_DEBUG_SPINLOCK,
+ * the return value is always 0 (see include/linux/spinlock_up.h).
+ * Therefore you should not rely heavily on the return value.

Alan

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[David Howells]

Alan Stern <stern@rowland.harvard.edu> wrote:

> + * Returns: 1 if @lock is locked, 0 otherwise.
> + * However, on !CONFIG_SMP builds with !CONFIG_DEBUG_SPINLOCK,
> + * the return value is always 0 (see include/linux/spinlock_up.h).
> + * Therefore you should not rely heavily on the return value.

Seems reasonable.

It might also want to include a note that the lock isn't necessarily held by
your own CPU.  I would also use "=n" rather than "!", so maybe something like:

 * Returns: 1 if @lock is locked, 0 otherwise.
 *
 * Note that the function only tells you that the CPU is seen to be locked,
 * not that it is locked on your CPU.
 *
 * Further, on CONFIG_SMP=n builds with CONFIG_DEBUG_SPINLOCK=n, the return
 * value is always 0 (see include/linux/spinlock_up.h).  Therefore you should
 * not rely heavily on the return value.

David

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[Randy Dunlap]

On 04/03/2018 02:43 PM, David Howells wrote:
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
>> + * Returns: 1 if @lock is locked, 0 otherwise.
>> + * However, on !CONFIG_SMP builds with !CONFIG_DEBUG_SPINLOCK,
>> + * the return value is always 0 (see include/linux/spinlock_up.h).
>> + * Therefore you should not rely heavily on the return value.
> 
> Seems reasonable.
> 
> It might also want to include a note that the lock isn't necessarily held by
> your own CPU.  I would also use "=n" rather than "!", so maybe something like:
> 
>  * Returns: 1 if @lock is locked, 0 otherwise.
>  *
>  * Note that the function only tells you that the CPU is seen to be locked,

the CPU is locked??

>  * not that it is locked on your CPU.
>  *
>  * Further, on CONFIG_SMP=n builds with CONFIG_DEBUG_SPINLOCK=n, the return
>  * value is always 0 (see include/linux/spinlock_up.h).  Therefore you should
>  * not rely heavily on the return value.
> 
> David
> 


-- 
~Randy

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[David Howells]

Randy Dunlap <rdunlap@infradead.org> wrote:

> >  * Note that the function only tells you that the CPU is seen to be locked,
> 
> the CPU is locked??
> 
> >  * not that it is locked on your CPU.

Sorry, yes: "... that the lock is seen to be locked, not that it is locked on
your CPU".

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[Andrea Parri]

On Tue, Apr 03, 2018 at 10:43:14PM +0100, David Howells wrote:
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > + * Returns: 1 if @lock is locked, 0 otherwise.
> > + * However, on !CONFIG_SMP builds with !CONFIG_DEBUG_SPINLOCK,
> > + * the return value is always 0 (see include/linux/spinlock_up.h).
> > + * Therefore you should not rely heavily on the return value.
> 
> Seems reasonable.
> 
> It might also want to include a note that the lock isn't necessarily held by
> your own CPU.  I would also use "=n" rather than "!", so maybe something like:
> 
>  * Returns: 1 if @lock is locked, 0 otherwise.
>  *
>  * Note that the function only tells you that the CPU is seen to be locked,
>  * not that it is locked on your CPU.
>  *
>  * Further, on CONFIG_SMP=n builds with CONFIG_DEBUG_SPINLOCK=n, the return
>  * value is always 0 (see include/linux/spinlock_up.h).  Therefore you should
>  * not rely heavily on the return value.

Thank you all for the suggestions.  I plan to integrate these in the next
version of the patch, which should also include your Co-developed-by:

  Andrea


> 
> David

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[Paul E. McKenney]

On Tue, Apr 03, 2018 at 02:52:33PM +0100, David Howells wrote:
> Andrea Parri <andrea.parri@amarulasolutions.com> wrote:
> 
> > > It's more complicated than that.  This function is dangerous and should be
> > > used with extreme care.  In the case where CONFIG_SMP=n the value is locked
> > > one way or the other and it might be the wrong way.
> > 
> > You mean "unlocked"? (aka, return 0)
> 
> No, I mean "fixed", sorry.  We've had problems stemming from this before on UP
> systems.

Oooh...  I had forgotten about spinlocks disappearing on UP systems,
good catch!

Suggestions for a fix?  Clearly great care is required when using it
in things like WARN_ON()...

							Thanx, Paul

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[Peter Zijlstra]

On Tue, Apr 03, 2018 at 07:17:18AM -0700, Paul E. McKenney wrote:
> Suggestions for a fix?  Clearly great care is required when using it
> in things like WARN_ON()...

Yeah, don't use it there, use lockdep_assert_held().

As I stated before in this thread, ideally we'd make *_is_locked() go
away entirely.

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[Paul E. McKenney]

On Tue, Apr 03, 2018 at 04:43:00PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 03, 2018 at 07:17:18AM -0700, Paul E. McKenney wrote:
> > Suggestions for a fix?  Clearly great care is required when using it
> > in things like WARN_ON()...
> 
> Yeah, don't use it there, use lockdep_assert_held().

Good point, -ETOOEARLY.  ;-)

> As I stated before in this thread, ideally we'd make *_is_locked() go
> away entirely.

After being reminded of the issues on UP systems, I now have much more
sympathy for that view...

							Thanx, Paul

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[Paul E. McKenney]

On Tue, Apr 03, 2018 at 08:03:56AM -0700, Paul E. McKenney wrote:
> On Tue, Apr 03, 2018 at 04:43:00PM +0200, Peter Zijlstra wrote:
> > On Tue, Apr 03, 2018 at 07:17:18AM -0700, Paul E. McKenney wrote:
> > > Suggestions for a fix?  Clearly great care is required when using it
> > > in things like WARN_ON()...
> > 
> > Yeah, don't use it there, use lockdep_assert_held().
> 
> Good point, -ETOOEARLY.  ;-)
> 
> > As I stated before in this thread, ideally we'd make *_is_locked() go
> > away entirely.
> 
> After being reminded of the issues on UP systems, I now have much more
> sympathy for that view...

And so the main remaining use case is debug prints on !PROVE_LOCKING
builds.  Which need some thought about the UP case.

Or am I missing something here?

							Thanx, Paul

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[Christoph Hellwig]

Can't we just kill off spin_is_locked?  It seems pretty much all uses
should simply be replaced with lockdep annotations, and there aren't
many to start with.

Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()

[Andrea Parri]

On Thu, Apr 05, 2018 at 12:47:49AM -0700, Christoph Hellwig wrote:
> Can't we just kill off spin_is_locked?  It seems pretty much all uses
> should simply be replaced with lockdep annotations, and there aren't
> many to start with.

Yeah, this is not the first time such a question has been raised ;) In fact,
some people (see, e.g., Peter's comment in this thread) have also suggested
extending it to {mutex,rwsem,bit_spin}_is_locked  (the number of uses would
then increase to a few hundreds, and lockdep would need some extensions...).
Of course, removing the docbook headers should not cause particular trouble,
once/if the removal of these primitives will be realized... ;)

  Andrea
> 

[PATCH v4 1/3] locking: Document the semantics of spin_is_locked()

[Andrea Parri]

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().

A recent auditing [1] of the callers of the primitive confirmed that
none of them are relying on particular ordering guarantees; document
this semantics by adding a docbook header to spin_is_locked(). Also,
describe behaviors specific to certain CONFIG_SMP=n builds.

[1] https://marc.info/?l=linux-kernel&m=151981440005264&w=2
    https://marc.info/?l=linux-kernel&m=152042843808540&w=2
    https://marc.info/?l=linux-kernel&m=152043346110262&w=2

Co-Developed-by: Andrea Parri <andrea.parri@amarulasolutions.com>
Co-Developed-by: Alan Stern <stern@rowland.harvard.edu>
Co-Developed-by: David Howells <dhowells@redhat.com>
Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: David Howells <dhowells@redhat.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Jade Alglave <j.alglave@ucl.ac.uk>
Cc: Luc Maranget <luc.maranget@inria.fr>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Akira Yokosawa <akiyks@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
---
 include/linux/spinlock.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 4894d322d2584..1e8a464358384 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -380,6 +380,24 @@ 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.
+ *
+ * Returns: 1 if @lock is locked, 0 otherwise.
+ *
+ * Note that the function only tells you that the spinlock is
+ * seen to be locked, not that it is locked on your CPU.
+ *
+ * Further, on CONFIG_SMP=n builds with CONFIG_DEBUG_SPINLOCK=n,
+ * the return value is always 0 (see include/linux/spinlock_up.h).
+ * Therefore you should not rely heavily on the return value.
+ */
 static __always_inline int spin_is_locked(spinlock_t *lock)
 {
 	return raw_spin_is_locked(&lock->rlock);
-- 
2.7.4

Re: [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()

[Paul E. McKenney]

On Fri, Apr 06, 2018 at 09:47:39PM +0200, 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().
> 
> A recent auditing [1] of the callers of the primitive confirmed that
> none of them are relying on particular ordering guarantees; document
> this semantics by adding a docbook header to spin_is_locked(). Also,
> describe behaviors specific to certain CONFIG_SMP=n builds.
> 
> [1] https://marc.info/?l=linux-kernel&m=151981440005264&w=2
>     https://marc.info/?l=linux-kernel&m=152042843808540&w=2
>     https://marc.info/?l=linux-kernel&m=152043346110262&w=2
> 
> Co-Developed-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> Co-Developed-by: Alan Stern <stern@rowland.harvard.edu>
> Co-Developed-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Jade Alglave <j.alglave@ucl.ac.uk>
> Cc: Luc Maranget <luc.maranget@inria.fr>
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Cc: Akira Yokosawa <akiyks@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>

Queued in place of its predecessor, thank you all!

							Thanx, Paul

> ---
>  include/linux/spinlock.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> index 4894d322d2584..1e8a464358384 100644
> --- a/include/linux/spinlock.h
> +++ b/include/linux/spinlock.h
> @@ -380,6 +380,24 @@ 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.
> + *
> + * Returns: 1 if @lock is locked, 0 otherwise.
> + *
> + * Note that the function only tells you that the spinlock is
> + * seen to be locked, not that it is locked on your CPU.
> + *
> + * Further, on CONFIG_SMP=n builds with CONFIG_DEBUG_SPINLOCK=n,
> + * the return value is always 0 (see include/linux/spinlock_up.h).
> + * Therefore you should not rely heavily on the return value.
> + */
>  static __always_inline int spin_is_locked(spinlock_t *lock)
>  {
>  	return raw_spin_is_locked(&lock->rlock);
> -- 
> 2.7.4
> 

Re: [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()

[Randy Dunlap]

On 04/06/2018 12:47 PM, 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().
> 
> A recent auditing [1] of the callers of the primitive confirmed that
> none of them are relying on particular ordering guarantees; document
> this semantics by adding a docbook header to spin_is_locked(). Also,
> describe behaviors specific to certain CONFIG_SMP=n builds.
> 
> [1] https://marc.info/?l=linux-kernel&m=151981440005264&w=2
>     https://marc.info/?l=linux-kernel&m=152042843808540&w=2
>     https://marc.info/?l=linux-kernel&m=152043346110262&w=2
> 
> Co-Developed-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> Co-Developed-by: Alan Stern <stern@rowland.harvard.edu>
> Co-Developed-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Jade Alglave <j.alglave@ucl.ac.uk>
> Cc: Luc Maranget <luc.maranget@inria.fr>
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Cc: Akira Yokosawa <akiyks@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> ---
>  include/linux/spinlock.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> index 4894d322d2584..1e8a464358384 100644
> --- a/include/linux/spinlock.h
> +++ b/include/linux/spinlock.h
> @@ -380,6 +380,24 @@ 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.
> + *
> + * Returns: 1 if @lock is locked, 0 otherwise.

Sorry, minor nit:
s/Returns:/Return:/
(according to kernel-doc.rst)

although I agree that "Returns:" is better.
[I should have changed that years ago.]

> + *
> + * Note that the function only tells you that the spinlock is
> + * seen to be locked, not that it is locked on your CPU.
> + *
> + * Further, on CONFIG_SMP=n builds with CONFIG_DEBUG_SPINLOCK=n,
> + * the return value is always 0 (see include/linux/spinlock_up.h).
> + * Therefore you should not rely heavily on the return value.
> + */
>  static __always_inline int spin_is_locked(spinlock_t *lock)
>  {
>  	return raw_spin_is_locked(&lock->rlock);
> 


-- 
~Randy

Re: [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()

[Paul E. McKenney]

On Fri, Apr 06, 2018 at 02:01:41PM -0700, Randy Dunlap wrote:
> On 04/06/2018 12:47 PM, 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().
> > 
> > A recent auditing [1] of the callers of the primitive confirmed that
> > none of them are relying on particular ordering guarantees; document
> > this semantics by adding a docbook header to spin_is_locked(). Also,
> > describe behaviors specific to certain CONFIG_SMP=n builds.
> > 
> > [1] https://marc.info/?l=linux-kernel&m=151981440005264&w=2
> >     https://marc.info/?l=linux-kernel&m=152042843808540&w=2
> >     https://marc.info/?l=linux-kernel&m=152043346110262&w=2
> > 
> > Co-Developed-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> > Co-Developed-by: Alan Stern <stern@rowland.harvard.edu>
> > Co-Developed-by: David Howells <dhowells@redhat.com>
> > Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Boqun Feng <boqun.feng@gmail.com>
> > Cc: Nicholas Piggin <npiggin@gmail.com>
> > Cc: Jade Alglave <j.alglave@ucl.ac.uk>
> > Cc: Luc Maranget <luc.maranget@inria.fr>
> > Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > Cc: Akira Yokosawa <akiyks@gmail.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > ---
> >  include/linux/spinlock.h | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> > index 4894d322d2584..1e8a464358384 100644
> > --- a/include/linux/spinlock.h
> > +++ b/include/linux/spinlock.h
> > @@ -380,6 +380,24 @@ 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.
> > + *
> > + * Returns: 1 if @lock is locked, 0 otherwise.
> 
> Sorry, minor nit:
> s/Returns:/Return:/
> (according to kernel-doc.rst)
> 
> although I agree that "Returns:" is better.
> [I should have changed that years ago.]

Agreed, English grammar and templates often seem to conflict.

So should we change this comment, or are you instead proposing to add
"Returns:" as valid kernel-doc?

							Thanx, Paul

> > + *
> > + * Note that the function only tells you that the spinlock is
> > + * seen to be locked, not that it is locked on your CPU.
> > + *
> > + * Further, on CONFIG_SMP=n builds with CONFIG_DEBUG_SPINLOCK=n,
> > + * the return value is always 0 (see include/linux/spinlock_up.h).
> > + * Therefore you should not rely heavily on the return value.
> > + */
> >  static __always_inline int spin_is_locked(spinlock_t *lock)
> >  {
> >  	return raw_spin_is_locked(&lock->rlock);
> > 
> 
> 
> -- 
> ~Randy
> 

Re: [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()

[Randy Dunlap]

On 04/06/2018 02:07 PM, Paul E. McKenney wrote:
> On Fri, Apr 06, 2018 at 02:01:41PM -0700, Randy Dunlap wrote:
>> On 04/06/2018 12:47 PM, 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().
>>>
>>> A recent auditing [1] of the callers of the primitive confirmed that
>>> none of them are relying on particular ordering guarantees; document
>>> this semantics by adding a docbook header to spin_is_locked(). Also,
>>> describe behaviors specific to certain CONFIG_SMP=n builds.
>>>
>>> [1] https://marc.info/?l=linux-kernel&m=151981440005264&w=2
>>>     https://marc.info/?l=linux-kernel&m=152042843808540&w=2
>>>     https://marc.info/?l=linux-kernel&m=152043346110262&w=2
>>>
>>> Co-Developed-by: Andrea Parri <andrea.parri@amarulasolutions.com>
>>> Co-Developed-by: Alan Stern <stern@rowland.harvard.edu>
>>> Co-Developed-by: David Howells <dhowells@redhat.com>
>>> Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
>>> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
>>> Signed-off-by: David Howells <dhowells@redhat.com>
>>> Cc: Will Deacon <will.deacon@arm.com>
>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>> Cc: Boqun Feng <boqun.feng@gmail.com>
>>> Cc: Nicholas Piggin <npiggin@gmail.com>
>>> Cc: Jade Alglave <j.alglave@ucl.ac.uk>
>>> Cc: Luc Maranget <luc.maranget@inria.fr>
>>> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
>>> Cc: Akira Yokosawa <akiyks@gmail.com>
>>> Cc: Ingo Molnar <mingo@kernel.org>
>>> ---
>>>  include/linux/spinlock.h | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
>>> index 4894d322d2584..1e8a464358384 100644
>>> --- a/include/linux/spinlock.h
>>> +++ b/include/linux/spinlock.h
>>> @@ -380,6 +380,24 @@ 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.
>>> + *
>>> + * Returns: 1 if @lock is locked, 0 otherwise.
>>
>> Sorry, minor nit:
>> s/Returns:/Return:/
>> (according to kernel-doc.rst)
>>
>> although I agree that "Returns:" is better.
>> [I should have changed that years ago.]
> 
> Agreed, English grammar and templates often seem to conflict.
> 
> So should we change this comment, or are you instead proposing to add
> "Returns:" as valid kernel-doc?

Please change this patch to current doc syntax.
Any changes to kernel-doc syntax would come later.

Thanks.

> 							Thanx, Paul
> 
>>> + *
>>> + * Note that the function only tells you that the spinlock is
>>> + * seen to be locked, not that it is locked on your CPU.
>>> + *
>>> + * Further, on CONFIG_SMP=n builds with CONFIG_DEBUG_SPINLOCK=n,
>>> + * the return value is always 0 (see include/linux/spinlock_up.h).
>>> + * Therefore you should not rely heavily on the return value.
>>> + */
>>>  static __always_inline int spin_is_locked(spinlock_t *lock)
>>>  {
>>>  	return raw_spin_is_locked(&lock->rlock);
>>>
>>
>>
>> -- 
>> ~Randy
>>
> 


-- 
~Randy

Re: [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()

[Andrea Parri]

On Fri, Apr 06, 2018 at 02:08:16PM -0700, Randy Dunlap wrote:
> On 04/06/2018 02:07 PM, Paul E. McKenney wrote:
> > On Fri, Apr 06, 2018 at 02:01:41PM -0700, Randy Dunlap wrote:
> >> On 04/06/2018 12:47 PM, 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().
> >>>
> >>> A recent auditing [1] of the callers of the primitive confirmed that
> >>> none of them are relying on particular ordering guarantees; document
> >>> this semantics by adding a docbook header to spin_is_locked(). Also,
> >>> describe behaviors specific to certain CONFIG_SMP=n builds.
> >>>
> >>> [1] https://marc.info/?l=linux-kernel&m=151981440005264&w=2
> >>>     https://marc.info/?l=linux-kernel&m=152042843808540&w=2
> >>>     https://marc.info/?l=linux-kernel&m=152043346110262&w=2
> >>>
> >>> Co-Developed-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> >>> Co-Developed-by: Alan Stern <stern@rowland.harvard.edu>
> >>> Co-Developed-by: David Howells <dhowells@redhat.com>
> >>> Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> >>> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> >>> Signed-off-by: David Howells <dhowells@redhat.com>
> >>> Cc: Will Deacon <will.deacon@arm.com>
> >>> Cc: Peter Zijlstra <peterz@infradead.org>
> >>> Cc: Boqun Feng <boqun.feng@gmail.com>
> >>> Cc: Nicholas Piggin <npiggin@gmail.com>
> >>> Cc: Jade Alglave <j.alglave@ucl.ac.uk>
> >>> Cc: Luc Maranget <luc.maranget@inria.fr>
> >>> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> >>> Cc: Akira Yokosawa <akiyks@gmail.com>
> >>> Cc: Ingo Molnar <mingo@kernel.org>
> >>> ---
> >>>  include/linux/spinlock.h | 18 ++++++++++++++++++
> >>>  1 file changed, 18 insertions(+)
> >>>
> >>> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> >>> index 4894d322d2584..1e8a464358384 100644
> >>> --- a/include/linux/spinlock.h
> >>> +++ b/include/linux/spinlock.h
> >>> @@ -380,6 +380,24 @@ 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.
> >>> + *
> >>> + * Returns: 1 if @lock is locked, 0 otherwise.
> >>
> >> Sorry, minor nit:
> >> s/Returns:/Return:/
> >> (according to kernel-doc.rst)
> >>
> >> although I agree that "Returns:" is better.
> >> [I should have changed that years ago.]
> > 
> > Agreed, English grammar and templates often seem to conflict.
> > 
> > So should we change this comment, or are you instead proposing to add
> > "Returns:" as valid kernel-doc?
> 
> Please change this patch to current doc syntax.
> Any changes to kernel-doc syntax would come later.

Paul: I understand that you're going to do this change "in place"; please
let me know if I'm wrong/if you need a new submission.

Thanks,
  Andrea


> 
> Thanks.
> 
> > 							Thanx, Paul
> > 
> >>> + *
> >>> + * Note that the function only tells you that the spinlock is
> >>> + * seen to be locked, not that it is locked on your CPU.
> >>> + *
> >>> + * Further, on CONFIG_SMP=n builds with CONFIG_DEBUG_SPINLOCK=n,
> >>> + * the return value is always 0 (see include/linux/spinlock_up.h).
> >>> + * Therefore you should not rely heavily on the return value.
> >>> + */
> >>>  static __always_inline int spin_is_locked(spinlock_t *lock)
> >>>  {
> >>>  	return raw_spin_is_locked(&lock->rlock);
> >>>
> >>
> >>
> >> -- 
> >> ~Randy
> >>
> > 
> 
> 
> -- 
> ~Randy

Re: [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()

[Paul E. McKenney]

On Fri, Apr 06, 2018 at 11:58:25PM +0200, Andrea Parri wrote:
> On Fri, Apr 06, 2018 at 02:08:16PM -0700, Randy Dunlap wrote:
> > On 04/06/2018 02:07 PM, Paul E. McKenney wrote:
> > > On Fri, Apr 06, 2018 at 02:01:41PM -0700, Randy Dunlap wrote:
> > >> On 04/06/2018 12:47 PM, 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().
> > >>>
> > >>> A recent auditing [1] of the callers of the primitive confirmed that
> > >>> none of them are relying on particular ordering guarantees; document
> > >>> this semantics by adding a docbook header to spin_is_locked(). Also,
> > >>> describe behaviors specific to certain CONFIG_SMP=n builds.
> > >>>
> > >>> [1] https://marc.info/?l=linux-kernel&m=151981440005264&w=2
> > >>>     https://marc.info/?l=linux-kernel&m=152042843808540&w=2
> > >>>     https://marc.info/?l=linux-kernel&m=152043346110262&w=2
> > >>>
> > >>> Co-Developed-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> > >>> Co-Developed-by: Alan Stern <stern@rowland.harvard.edu>
> > >>> Co-Developed-by: David Howells <dhowells@redhat.com>
> > >>> Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> > >>> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > >>> Signed-off-by: David Howells <dhowells@redhat.com>
> > >>> Cc: Will Deacon <will.deacon@arm.com>
> > >>> Cc: Peter Zijlstra <peterz@infradead.org>
> > >>> Cc: Boqun Feng <boqun.feng@gmail.com>
> > >>> Cc: Nicholas Piggin <npiggin@gmail.com>
> > >>> Cc: Jade Alglave <j.alglave@ucl.ac.uk>
> > >>> Cc: Luc Maranget <luc.maranget@inria.fr>
> > >>> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > >>> Cc: Akira Yokosawa <akiyks@gmail.com>
> > >>> Cc: Ingo Molnar <mingo@kernel.org>
> > >>> ---
> > >>>  include/linux/spinlock.h | 18 ++++++++++++++++++
> > >>>  1 file changed, 18 insertions(+)
> > >>>
> > >>> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> > >>> index 4894d322d2584..1e8a464358384 100644
> > >>> --- a/include/linux/spinlock.h
> > >>> +++ b/include/linux/spinlock.h
> > >>> @@ -380,6 +380,24 @@ 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.
> > >>> + *
> > >>> + * Returns: 1 if @lock is locked, 0 otherwise.
> > >>
> > >> Sorry, minor nit:
> > >> s/Returns:/Return:/
> > >> (according to kernel-doc.rst)
> > >>
> > >> although I agree that "Returns:" is better.
> > >> [I should have changed that years ago.]
> > > 
> > > Agreed, English grammar and templates often seem to conflict.
> > > 
> > > So should we change this comment, or are you instead proposing to add
> > > "Returns:" as valid kernel-doc?
> > 
> > Please change this patch to current doc syntax.
> > Any changes to kernel-doc syntax would come later.

Are you sure?

$ git grep "\* Returns:" | wc -l
2470
$ git grep "\* Return:" | wc -l
4144

Looks like more than a third of them are already "Returns:".  ;-)

> Paul: I understand that you're going to do this change "in place"; please
> let me know if I'm wrong/if you need a new submission.

If Randy is certain that he would like to continue propagating
this grammatical infelicity, sure.  ;-)

							Thanx, Paul

> Thanks,
>   Andrea
> 
> 
> > 
> > Thanks.
> > 
> > > 							Thanx, Paul
> > > 
> > >>> + *
> > >>> + * Note that the function only tells you that the spinlock is
> > >>> + * seen to be locked, not that it is locked on your CPU.
> > >>> + *
> > >>> + * Further, on CONFIG_SMP=n builds with CONFIG_DEBUG_SPINLOCK=n,
> > >>> + * the return value is always 0 (see include/linux/spinlock_up.h).
> > >>> + * Therefore you should not rely heavily on the return value.
> > >>> + */
> > >>>  static __always_inline int spin_is_locked(spinlock_t *lock)
> > >>>  {
> > >>>  	return raw_spin_is_locked(&lock->rlock);
> > >>>
> > >>
> > >>
> > >> -- 
> > >> ~Randy
> > >>
> > > 
> > 
> > 
> > -- 
> > ~Randy
> 

Re: [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()

[Randy Dunlap]

On 04/08/2018 02:14 PM, Paul E. McKenney wrote:
> On Fri, Apr 06, 2018 at 11:58:25PM +0200, Andrea Parri wrote:
>> On Fri, Apr 06, 2018 at 02:08:16PM -0700, Randy Dunlap wrote:
>>> On 04/06/2018 02:07 PM, Paul E. McKenney wrote:
>>>> On Fri, Apr 06, 2018 at 02:01:41PM -0700, Randy Dunlap wrote:
>>>>> On 04/06/2018 12:47 PM, 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().
>>>>>>
>>>>>> A recent auditing [1] of the callers of the primitive confirmed that
>>>>>> none of them are relying on particular ordering guarantees; document
>>>>>> this semantics by adding a docbook header to spin_is_locked(). Also,
>>>>>> describe behaviors specific to certain CONFIG_SMP=n builds.
>>>>>>
>>>>>> [1] https://marc.info/?l=linux-kernel&m=151981440005264&w=2
>>>>>>     https://marc.info/?l=linux-kernel&m=152042843808540&w=2
>>>>>>     https://marc.info/?l=linux-kernel&m=152043346110262&w=2
>>>>>>
>>>>>> Co-Developed-by: Andrea Parri <andrea.parri@amarulasolutions.com>
>>>>>> Co-Developed-by: Alan Stern <stern@rowland.harvard.edu>
>>>>>> Co-Developed-by: David Howells <dhowells@redhat.com>
>>>>>> Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
>>>>>> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
>>>>>> Signed-off-by: David Howells <dhowells@redhat.com>
>>>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>>>> Cc: Boqun Feng <boqun.feng@gmail.com>
>>>>>> Cc: Nicholas Piggin <npiggin@gmail.com>
>>>>>> Cc: Jade Alglave <j.alglave@ucl.ac.uk>
>>>>>> Cc: Luc Maranget <luc.maranget@inria.fr>
>>>>>> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
>>>>>> Cc: Akira Yokosawa <akiyks@gmail.com>
>>>>>> Cc: Ingo Molnar <mingo@kernel.org>
>>>>>> ---
>>>>>>  include/linux/spinlock.h | 18 ++++++++++++++++++
>>>>>>  1 file changed, 18 insertions(+)
>>>>>>
>>>>>> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
>>>>>> index 4894d322d2584..1e8a464358384 100644
>>>>>> --- a/include/linux/spinlock.h
>>>>>> +++ b/include/linux/spinlock.h
>>>>>> @@ -380,6 +380,24 @@ 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.
>>>>>> + *
>>>>>> + * Returns: 1 if @lock is locked, 0 otherwise.
>>>>>
>>>>> Sorry, minor nit:
>>>>> s/Returns:/Return:/
>>>>> (according to kernel-doc.rst)
>>>>>
>>>>> although I agree that "Returns:" is better.
>>>>> [I should have changed that years ago.]
>>>>
>>>> Agreed, English grammar and templates often seem to conflict.
>>>>
>>>> So should we change this comment, or are you instead proposing to add
>>>> "Returns:" as valid kernel-doc?
>>>
>>> Please change this patch to current doc syntax.
>>> Any changes to kernel-doc syntax would come later.
> 
> Are you sure?
> 
> $ git grep "\* Returns:" | wc -l
> 2470
> $ git grep "\* Return:" | wc -l
> 4144
> 
> Looks like more than a third of them are already "Returns:".  ;-)
> 
>> Paul: I understand that you're going to do this change "in place"; please
>> let me know if I'm wrong/if you need a new submission.
> 
> If Randy is certain that he would like to continue propagating
> this grammatical infelicity, sure.  ;-)

eh?
Well, Documentation/doc-guide/kernel-doc.rst says "Return:", but it appears
that it does not matter to scripts/kernel-doc -- it's just the name of a
"section" of the documentation and can be spelled any way!  oh well.  :)

Acked-by: Randy Dunlap <rdunlap@infradead.org>

Thanks.

-- 
~Randy

Re: [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()

[Paul E. McKenney]

On Sun, Apr 08, 2018 at 02:32:53PM -0700, Randy Dunlap wrote:
> On 04/08/2018 02:14 PM, Paul E. McKenney wrote:
> > On Fri, Apr 06, 2018 at 11:58:25PM +0200, Andrea Parri wrote:
> >> On Fri, Apr 06, 2018 at 02:08:16PM -0700, Randy Dunlap wrote:
> >>> On 04/06/2018 02:07 PM, Paul E. McKenney wrote:
> >>>> On Fri, Apr 06, 2018 at 02:01:41PM -0700, Randy Dunlap wrote:
> >>>>> On 04/06/2018 12:47 PM, 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().
> >>>>>>
> >>>>>> A recent auditing [1] of the callers of the primitive confirmed that
> >>>>>> none of them are relying on particular ordering guarantees; document
> >>>>>> this semantics by adding a docbook header to spin_is_locked(). Also,
> >>>>>> describe behaviors specific to certain CONFIG_SMP=n builds.
> >>>>>>
> >>>>>> [1] https://marc.info/?l=linux-kernel&m=151981440005264&w=2
> >>>>>>     https://marc.info/?l=linux-kernel&m=152042843808540&w=2
> >>>>>>     https://marc.info/?l=linux-kernel&m=152043346110262&w=2
> >>>>>>
> >>>>>> Co-Developed-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> >>>>>> Co-Developed-by: Alan Stern <stern@rowland.harvard.edu>
> >>>>>> Co-Developed-by: David Howells <dhowells@redhat.com>
> >>>>>> Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> >>>>>> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> >>>>>> Signed-off-by: David Howells <dhowells@redhat.com>
> >>>>>> Cc: Will Deacon <will.deacon@arm.com>
> >>>>>> Cc: Peter Zijlstra <peterz@infradead.org>
> >>>>>> Cc: Boqun Feng <boqun.feng@gmail.com>
> >>>>>> Cc: Nicholas Piggin <npiggin@gmail.com>
> >>>>>> Cc: Jade Alglave <j.alglave@ucl.ac.uk>
> >>>>>> Cc: Luc Maranget <luc.maranget@inria.fr>
> >>>>>> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> >>>>>> Cc: Akira Yokosawa <akiyks@gmail.com>
> >>>>>> Cc: Ingo Molnar <mingo@kernel.org>
> >>>>>> ---
> >>>>>>  include/linux/spinlock.h | 18 ++++++++++++++++++
> >>>>>>  1 file changed, 18 insertions(+)
> >>>>>>
> >>>>>> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> >>>>>> index 4894d322d2584..1e8a464358384 100644
> >>>>>> --- a/include/linux/spinlock.h
> >>>>>> +++ b/include/linux/spinlock.h
> >>>>>> @@ -380,6 +380,24 @@ 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.
> >>>>>> + *
> >>>>>> + * Returns: 1 if @lock is locked, 0 otherwise.
> >>>>>
> >>>>> Sorry, minor nit:
> >>>>> s/Returns:/Return:/
> >>>>> (according to kernel-doc.rst)
> >>>>>
> >>>>> although I agree that "Returns:" is better.
> >>>>> [I should have changed that years ago.]
> >>>>
> >>>> Agreed, English grammar and templates often seem to conflict.
> >>>>
> >>>> So should we change this comment, or are you instead proposing to add
> >>>> "Returns:" as valid kernel-doc?
> >>>
> >>> Please change this patch to current doc syntax.
> >>> Any changes to kernel-doc syntax would come later.
> > 
> > Are you sure?
> > 
> > $ git grep "\* Returns:" | wc -l
> > 2470
> > $ git grep "\* Return:" | wc -l
> > 4144
> > 
> > Looks like more than a third of them are already "Returns:".  ;-)
> > 
> >> Paul: I understand that you're going to do this change "in place"; please
> >> let me know if I'm wrong/if you need a new submission.
> > 
> > If Randy is certain that he would like to continue propagating
> > this grammatical infelicity, sure.  ;-)
> 
> eh?
> Well, Documentation/doc-guide/kernel-doc.rst says "Return:", but it appears
> that it does not matter to scripts/kernel-doc -- it's just the name of a
> "section" of the documentation and can be spelled any way!  oh well.  :)
> 
> Acked-by: Randy Dunlap <rdunlap@infradead.org>

Applied, thank you both!

							Thanx, Paul

[PATCH v3 2/3] arm64: Remove smp_mb() from arch_spin_is_locked()

[Andrea Parri]

Commit 38b850a73034f ("arm64: spinlock: order spin_{is_locked,unlock_wait}
against local locks") added an smp_mb() to arch_spin_is_locked(), in order
"to ensure that the lock value is always loaded after any other locks have
been taken by the current CPU", and reported one example (the "insane case"
in ipc/sem.c) relying on such guarantee.

It is however understood that spin_is_locked() is not required to provide
such an ordering guarantee (a guarantee that is currently not provided by
all the implementations/archs), and that callers relying on such ordering
should instead insert suitable memory barriers before acting on the result
of spin_is_locked().

Following a recent auditing [1] of the callers of {,raw_}spin_is_locked(),
revealing that none of them are relying on the ordering guarantee anymore,
this commit removes the leading smp_mb() from the primitive thus reverting
38b850a73034f.

[1] https://marc.info/?l=linux-kernel&m=151981440005264&w=2

Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
 arch/arm64/include/asm/spinlock.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h
index ebdae15d665de..26c5bd7d88d8d 100644
--- a/arch/arm64/include/asm/spinlock.h
+++ b/arch/arm64/include/asm/spinlock.h
@@ -122,11 +122,6 @@ static inline int arch_spin_value_unlocked(arch_spinlock_t lock)
 
 static inline int arch_spin_is_locked(arch_spinlock_t *lock)
 {
-	/*
-	 * Ensure prior spin_lock operations to other locks have completed
-	 * on this CPU before we test whether "lock" is locked.
-	 */
-	smp_mb(); /* ^^^ */
 	return !arch_spin_value_unlocked(READ_ONCE(*lock));
 }
 
-- 
2.7.4

[PATCH v3 3/3] locking: Clean up comment and #ifndef for {,queued_}spin_is_locked()

[Andrea Parri]

Removes "#ifndef queued_spin_is_locked" from the generic code: this is
unused and it's reasonable to conclude that it won't in a near future.

Also removes the comment about spin_is_locked() from mutex_is_locked():
the comment remains valid but not particularly useful.

Suggested-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
 include/asm-generic/qspinlock.h | 2 --
 include/linux/mutex.h           | 3 ---
 2 files changed, 5 deletions(-)

diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
index b37b4ad7eb946..dc4e4ac4937ea 100644
--- a/include/asm-generic/qspinlock.h
+++ b/include/asm-generic/qspinlock.h
@@ -26,7 +26,6 @@
  * @lock: Pointer to queued spinlock structure
  * Return: 1 if it is locked, 0 otherwise
  */
-#ifndef queued_spin_is_locked
 static __always_inline int queued_spin_is_locked(struct qspinlock *lock)
 {
 	/*
@@ -35,7 +34,6 @@ static __always_inline int queued_spin_is_locked(struct qspinlock *lock)
 	 */
 	return atomic_read(&lock->val);
 }
-#endif
 
 /**
  * queued_spin_value_unlocked - is the spinlock structure unlocked?
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 14bc0d5d0ee59..3093dd1624243 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -146,9 +146,6 @@ extern void __mutex_init(struct mutex *lock, const char *name,
  */
 static inline bool mutex_is_locked(struct mutex *lock)
 {
-	/*
-	 * XXX think about spin_is_locked
-	 */
 	return __mutex_owner(lock) != NULL;
 }
 
-- 
2.7.4

Re: [PATCH v3 0/3] Changes, clean-ups and documentation for spin_is_locked()

[Paul E. McKenney]

On Sun, Apr 01, 2018 at 06:41:49PM +0200, Andrea Parri wrote:
> Hi Paul,
> 
> This series gathers the change to the arm64 implementation of spin_is_locked()
> and the clean-up to the generic (q)spinlock presented in [1] together with the
> patch adding the docbook header to spin_is_locked() [2].
> 
> Apart from minor adjustments to the commit messages, the patches are unchanged.

I queued these on my lkmm branch and pushed them to -rcu, thank you!

I did rework the commit logs a bit, most notably to add the URLs of
the specific messages discussion the spin_is_locked() investigation.
Please take a look and let me know if I have broken something.

							Thanx, Paul

> Cheers,
>   Andrea
> 
> [1] https://marc.info/?l=linux-kernel&m=152223038924258&w=2
> [2] https://marc.info/?l=linux-kernel&m=151981440005264&w=2
> 
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> 
> Andrea Parri (3):
>   arm64: Remove smp_mb() from arch_spin_is_locked()
>   locking: Clean-up comment and #ifndef for {,queued_}spin_is_locked()
>   Documentation/locking: Document the semantics of spin_is_locked()
> 
>  arch/arm64/include/asm/spinlock.h |  5 -----
>  include/asm-generic/qspinlock.h   |  2 --
>  include/linux/mutex.h             |  3 ---
>  include/linux/spinlock.h          | 11 +++++++++++
>  4 files changed, 11 insertions(+), 10 deletions(-)
> 
> -- 
> 2.7.4
>