* [PATCH v3 0/3] Changes, clean-ups and documentation for spin_is_locked()
@ 2018-04-01 16:41 Andrea Parri
2018-04-01 16:41 ` [PATCH v2 1/3] locking: Document the semantics of spin_is_locked() Andrea Parri
` (4 more replies)
0 siblings, 5 replies; 33+ messages in thread
From: Andrea Parri @ 2018-04-01 16:41 UTC (permalink / raw)
To: paulmck, Ingo Molnar, Peter Zijlstra
Cc: linux-kernel, Andrea Parri, Will Deacon
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-01 16:41 [PATCH v3 0/3] Changes, clean-ups and documentation for spin_is_locked() Andrea Parri
@ 2018-04-01 16:41 ` Andrea Parri
2018-04-02 14:03 ` Alan Stern
2018-04-06 19:47 ` [PATCH v4 " Andrea Parri
2018-04-01 16:41 ` [PATCH v3 2/3] arm64: Remove smp_mb() from arch_spin_is_locked() Andrea Parri
` (3 subsequent siblings)
4 siblings, 2 replies; 33+ messages in thread
From: Andrea Parri @ 2018-04-01 16:41 UTC (permalink / raw)
To: paulmck, Ingo Molnar, Peter Zijlstra
Cc: linux-kernel, Andrea Parri, Andrea Parri, Alan Stern,
Will Deacon, Boqun Feng, Nicholas Piggin, David Howells,
Jade Alglave, Luc Maranget, Akira Yokosawa
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v3 2/3] arm64: Remove smp_mb() from arch_spin_is_locked()
2018-04-01 16:41 [PATCH v3 0/3] Changes, clean-ups and documentation for spin_is_locked() Andrea Parri
2018-04-01 16:41 ` [PATCH v2 1/3] locking: Document the semantics of spin_is_locked() Andrea Parri
@ 2018-04-01 16:41 ` Andrea Parri
2018-04-01 16:41 ` [PATCH v3 3/3] locking: Clean up comment and #ifndef for {,queued_}spin_is_locked() Andrea Parri
` (2 subsequent siblings)
4 siblings, 0 replies; 33+ messages in thread
From: Andrea Parri @ 2018-04-01 16:41 UTC (permalink / raw)
To: paulmck, Ingo Molnar, Peter Zijlstra
Cc: linux-kernel, Andrea Parri, Catalin Marinas, Will Deacon, Linus Torvalds
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v3 3/3] locking: Clean up comment and #ifndef for {,queued_}spin_is_locked()
2018-04-01 16:41 [PATCH v3 0/3] Changes, clean-ups and documentation for spin_is_locked() Andrea Parri
2018-04-01 16:41 ` [PATCH v2 1/3] locking: Document the semantics of spin_is_locked() Andrea Parri
2018-04-01 16:41 ` [PATCH v3 2/3] arm64: Remove smp_mb() from arch_spin_is_locked() Andrea Parri
@ 2018-04-01 16:41 ` Andrea Parri
2018-04-01 18:24 ` [PATCH v3 0/3] Changes, clean-ups and documentation for spin_is_locked() Paul E. McKenney
2018-04-03 12:49 ` [PATCH v2 1/3] locking: Document the semantics of spin_is_locked() David Howells
4 siblings, 0 replies; 33+ messages in thread
From: Andrea Parri @ 2018-04-01 16:41 UTC (permalink / raw)
To: paulmck, Ingo Molnar, Peter Zijlstra
Cc: linux-kernel, Andrea Parri, Will Deacon, Linus Torvalds
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v3 0/3] Changes, clean-ups and documentation for spin_is_locked()
2018-04-01 16:41 [PATCH v3 0/3] Changes, clean-ups and documentation for spin_is_locked() Andrea Parri
` (2 preceding siblings ...)
2018-04-01 16:41 ` [PATCH v3 3/3] locking: Clean up comment and #ifndef for {,queued_}spin_is_locked() Andrea Parri
@ 2018-04-01 18:24 ` Paul E. McKenney
2018-04-03 12:49 ` [PATCH v2 1/3] locking: Document the semantics of spin_is_locked() David Howells
4 siblings, 0 replies; 33+ messages in thread
From: Paul E. McKenney @ 2018-04-01 18:24 UTC (permalink / raw)
To: Andrea Parri; +Cc: Ingo Molnar, Peter Zijlstra, linux-kernel, Will Deacon
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
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-01 16:41 ` [PATCH v2 1/3] locking: Document the semantics of spin_is_locked() Andrea Parri
@ 2018-04-02 14:03 ` Alan Stern
2018-04-02 19:35 ` Paul E. McKenney
2018-04-06 19:47 ` [PATCH v4 " Andrea Parri
1 sibling, 1 reply; 33+ messages in thread
From: Alan Stern @ 2018-04-02 14:03 UTC (permalink / raw)
To: Andrea Parri
Cc: paulmck, Ingo Molnar, Peter Zijlstra, linux-kernel, Andrea Parri,
Will Deacon, Boqun Feng, Nicholas Piggin, David Howells,
Jade Alglave, Luc Maranget, Akira Yokosawa
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);
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-02 14:03 ` Alan Stern
@ 2018-04-02 19:35 ` Paul E. McKenney
0 siblings, 0 replies; 33+ messages in thread
From: Paul E. McKenney @ 2018-04-02 19:35 UTC (permalink / raw)
To: Alan Stern
Cc: Andrea Parri, Ingo Molnar, Peter Zijlstra, linux-kernel,
Andrea Parri, Will Deacon, Boqun Feng, Nicholas Piggin,
David Howells, Jade Alglave, Luc Maranget, Akira Yokosawa
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);
> >
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-01 16:41 [PATCH v3 0/3] Changes, clean-ups and documentation for spin_is_locked() Andrea Parri
` (3 preceding siblings ...)
2018-04-01 18:24 ` [PATCH v3 0/3] Changes, clean-ups and documentation for spin_is_locked() Paul E. McKenney
@ 2018-04-03 12:49 ` David Howells
2018-04-03 13:35 ` Andrea Parri
` (2 more replies)
4 siblings, 3 replies; 33+ messages in thread
From: David Howells @ 2018-04-03 12:49 UTC (permalink / raw)
To: Andrea Parri
Cc: dhowells, paulmck, Ingo Molnar, Peter Zijlstra, linux-kernel,
Andrea Parri, Alan Stern, Will Deacon, Boqun Feng,
Nicholas Piggin, Jade Alglave, Luc Maranget, Akira Yokosawa
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-03 12:49 ` [PATCH v2 1/3] locking: Document the semantics of spin_is_locked() David Howells
@ 2018-04-03 13:35 ` Andrea Parri
2018-04-03 13:52 ` David Howells
2018-04-05 7:47 ` Christoph Hellwig
2 siblings, 0 replies; 33+ messages in thread
From: Andrea Parri @ 2018-04-03 13:35 UTC (permalink / raw)
To: David Howells
Cc: paulmck, Ingo Molnar, Peter Zijlstra, linux-kernel, Andrea Parri,
Alan Stern, Will Deacon, Boqun Feng, Nicholas Piggin,
Jade Alglave, Luc Maranget, Akira Yokosawa
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-03 12:49 ` [PATCH v2 1/3] locking: Document the semantics of spin_is_locked() David Howells
2018-04-03 13:35 ` Andrea Parri
@ 2018-04-03 13:52 ` David Howells
2018-04-03 14:07 ` Andrea Parri
` (2 more replies)
2018-04-05 7:47 ` Christoph Hellwig
2 siblings, 3 replies; 33+ messages in thread
From: David Howells @ 2018-04-03 13:52 UTC (permalink / raw)
To: Andrea Parri
Cc: dhowells, paulmck, Ingo Molnar, Peter Zijlstra, linux-kernel,
Andrea Parri, Alan Stern, Will Deacon, Boqun Feng,
Nicholas Piggin, Jade Alglave, Luc Maranget, Akira Yokosawa
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-03 13:52 ` David Howells
@ 2018-04-03 14:07 ` Andrea Parri
2018-04-03 14:17 ` Paul E. McKenney
2018-04-03 15:23 ` David Howells
2 siblings, 0 replies; 33+ messages in thread
From: Andrea Parri @ 2018-04-03 14:07 UTC (permalink / raw)
To: David Howells
Cc: paulmck, Ingo Molnar, Peter Zijlstra, linux-kernel, Andrea Parri,
Alan Stern, Will Deacon, Boqun Feng, Nicholas Piggin,
Jade Alglave, Luc Maranget, Akira Yokosawa
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-03 13:52 ` David Howells
2018-04-03 14:07 ` Andrea Parri
@ 2018-04-03 14:17 ` Paul E. McKenney
2018-04-03 14:43 ` Peter Zijlstra
2018-04-03 15:23 ` David Howells
2 siblings, 1 reply; 33+ messages in thread
From: Paul E. McKenney @ 2018-04-03 14:17 UTC (permalink / raw)
To: David Howells
Cc: Andrea Parri, Ingo Molnar, Peter Zijlstra, linux-kernel,
Andrea Parri, Alan Stern, Will Deacon, Boqun Feng,
Nicholas Piggin, Jade Alglave, Luc Maranget, Akira Yokosawa
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-03 14:17 ` Paul E. McKenney
@ 2018-04-03 14:43 ` Peter Zijlstra
2018-04-03 15:03 ` Paul E. McKenney
0 siblings, 1 reply; 33+ messages in thread
From: Peter Zijlstra @ 2018-04-03 14:43 UTC (permalink / raw)
To: Paul E. McKenney
Cc: David Howells, Andrea Parri, Ingo Molnar, linux-kernel,
Andrea Parri, Alan Stern, Will Deacon, Boqun Feng,
Nicholas Piggin, Jade Alglave, Luc Maranget, Akira Yokosawa
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.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-03 14:43 ` Peter Zijlstra
@ 2018-04-03 15:03 ` Paul E. McKenney
2018-04-03 16:11 ` Paul E. McKenney
0 siblings, 1 reply; 33+ messages in thread
From: Paul E. McKenney @ 2018-04-03 15:03 UTC (permalink / raw)
To: Peter Zijlstra
Cc: David Howells, Andrea Parri, Ingo Molnar, linux-kernel,
Andrea Parri, Alan Stern, Will Deacon, Boqun Feng,
Nicholas Piggin, Jade Alglave, Luc Maranget, Akira Yokosawa
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-03 13:52 ` David Howells
2018-04-03 14:07 ` Andrea Parri
2018-04-03 14:17 ` Paul E. McKenney
@ 2018-04-03 15:23 ` David Howells
2018-04-03 19:31 ` Andrea Parri
2 siblings, 1 reply; 33+ messages in thread
From: David Howells @ 2018-04-03 15:23 UTC (permalink / raw)
To: Andrea Parri
Cc: dhowells, paulmck, Ingo Molnar, Peter Zijlstra, linux-kernel,
Andrea Parri, Alan Stern, Will Deacon, Boqun Feng,
Nicholas Piggin, Jade Alglave, Luc Maranget, Akira Yokosawa
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-03 15:03 ` Paul E. McKenney
@ 2018-04-03 16:11 ` Paul E. McKenney
0 siblings, 0 replies; 33+ messages in thread
From: Paul E. McKenney @ 2018-04-03 16:11 UTC (permalink / raw)
To: Peter Zijlstra
Cc: David Howells, Andrea Parri, Ingo Molnar, linux-kernel,
Andrea Parri, Alan Stern, Will Deacon, Boqun Feng,
Nicholas Piggin, Jade Alglave, Luc Maranget, Akira Yokosawa
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-03 15:23 ` David Howells
@ 2018-04-03 19:31 ` Andrea Parri
2018-04-03 20:04 ` Alan Stern
0 siblings, 1 reply; 33+ messages in thread
From: Andrea Parri @ 2018-04-03 19:31 UTC (permalink / raw)
To: David Howells
Cc: paulmck, Ingo Molnar, Peter Zijlstra, linux-kernel, Andrea Parri,
Alan Stern, Will Deacon, Boqun Feng, Nicholas Piggin,
Jade Alglave, Luc Maranget, Akira Yokosawa
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-03 19:31 ` Andrea Parri
@ 2018-04-03 20:04 ` Alan Stern
2018-04-03 21:43 ` David Howells
0 siblings, 1 reply; 33+ messages in thread
From: Alan Stern @ 2018-04-03 20:04 UTC (permalink / raw)
To: Andrea Parri
Cc: David Howells, paulmck, Ingo Molnar, Peter Zijlstra,
linux-kernel, Andrea Parri, Will Deacon, Boqun Feng,
Nicholas Piggin, Jade Alglave, Luc Maranget, Akira Yokosawa
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-03 20:04 ` Alan Stern
@ 2018-04-03 21:43 ` David Howells
2018-04-03 21:47 ` Randy Dunlap
` (2 more replies)
0 siblings, 3 replies; 33+ messages in thread
From: David Howells @ 2018-04-03 21:43 UTC (permalink / raw)
To: Alan Stern
Cc: dhowells, Andrea Parri, paulmck, Ingo Molnar, Peter Zijlstra,
linux-kernel, Andrea Parri, Will Deacon, Boqun Feng,
Nicholas Piggin, Jade Alglave, Luc Maranget, Akira Yokosawa
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-03 21:43 ` David Howells
@ 2018-04-03 21:47 ` Randy Dunlap
2018-04-04 12:47 ` Andrea Parri
2018-04-04 21:22 ` David Howells
2 siblings, 0 replies; 33+ messages in thread
From: Randy Dunlap @ 2018-04-03 21:47 UTC (permalink / raw)
To: David Howells, Alan Stern
Cc: Andrea Parri, paulmck, Ingo Molnar, Peter Zijlstra, linux-kernel,
Andrea Parri, Will Deacon, Boqun Feng, Nicholas Piggin,
Jade Alglave, Luc Maranget, Akira Yokosawa
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-03 21:43 ` David Howells
2018-04-03 21:47 ` Randy Dunlap
@ 2018-04-04 12:47 ` Andrea Parri
2018-04-04 21:22 ` David Howells
2 siblings, 0 replies; 33+ messages in thread
From: Andrea Parri @ 2018-04-04 12:47 UTC (permalink / raw)
To: David Howells
Cc: Alan Stern, paulmck, Ingo Molnar, Peter Zijlstra, linux-kernel,
Andrea Parri, Will Deacon, Boqun Feng, Nicholas Piggin,
Jade Alglave, Luc Maranget, Akira Yokosawa, Randy Dunlap
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-03 21:43 ` David Howells
2018-04-03 21:47 ` Randy Dunlap
2018-04-04 12:47 ` Andrea Parri
@ 2018-04-04 21:22 ` David Howells
2 siblings, 0 replies; 33+ messages in thread
From: David Howells @ 2018-04-04 21:22 UTC (permalink / raw)
To: Randy Dunlap
Cc: dhowells, Alan Stern, Andrea Parri, paulmck, Ingo Molnar,
Peter Zijlstra, linux-kernel, Andrea Parri, Will Deacon,
Boqun Feng, Nicholas Piggin, Jade Alglave, Luc Maranget,
Akira Yokosawa
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".
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-03 12:49 ` [PATCH v2 1/3] locking: Document the semantics of spin_is_locked() David Howells
2018-04-03 13:35 ` Andrea Parri
2018-04-03 13:52 ` David Howells
@ 2018-04-05 7:47 ` Christoph Hellwig
2018-04-05 8:56 ` Andrea Parri
2 siblings, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2018-04-05 7:47 UTC (permalink / raw)
To: David Howells
Cc: Andrea Parri, paulmck, Ingo Molnar, Peter Zijlstra, linux-kernel,
Andrea Parri, Alan Stern, Will Deacon, Boqun Feng,
Nicholas Piggin, Jade Alglave, Luc Maranget, Akira Yokosawa
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.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/3] locking: Document the semantics of spin_is_locked()
2018-04-05 7:47 ` Christoph Hellwig
@ 2018-04-05 8:56 ` Andrea Parri
0 siblings, 0 replies; 33+ messages in thread
From: Andrea Parri @ 2018-04-05 8:56 UTC (permalink / raw)
To: Christoph Hellwig
Cc: David Howells, paulmck, Ingo Molnar, Peter Zijlstra,
linux-kernel, Andrea Parri, Alan Stern, Will Deacon, Boqun Feng,
Nicholas Piggin, Jade Alglave, Luc Maranget, Akira Yokosawa
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
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()
2018-04-01 16:41 ` [PATCH v2 1/3] locking: Document the semantics of spin_is_locked() Andrea Parri
2018-04-02 14:03 ` Alan Stern
@ 2018-04-06 19:47 ` Andrea Parri
2018-04-06 21:00 ` Paul E. McKenney
2018-04-06 21:01 ` Randy Dunlap
1 sibling, 2 replies; 33+ messages in thread
From: Andrea Parri @ 2018-04-06 19:47 UTC (permalink / raw)
To: paulmck
Cc: linux-kernel, Alan Stern, David Howells, Andrea Parri,
Will Deacon, Peter Zijlstra, Boqun Feng, Nicholas Piggin,
Jade Alglave, Luc Maranget, Akira Yokosawa, Ingo Molnar
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()
2018-04-06 19:47 ` [PATCH v4 " Andrea Parri
@ 2018-04-06 21:00 ` Paul E. McKenney
2018-04-06 21:01 ` Randy Dunlap
1 sibling, 0 replies; 33+ messages in thread
From: Paul E. McKenney @ 2018-04-06 21:00 UTC (permalink / raw)
To: Andrea Parri
Cc: linux-kernel, Alan Stern, David Howells, Will Deacon,
Peter Zijlstra, Boqun Feng, Nicholas Piggin, Jade Alglave,
Luc Maranget, Akira Yokosawa, Ingo Molnar
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
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()
2018-04-06 19:47 ` [PATCH v4 " Andrea Parri
2018-04-06 21:00 ` Paul E. McKenney
@ 2018-04-06 21:01 ` Randy Dunlap
2018-04-06 21:07 ` Paul E. McKenney
1 sibling, 1 reply; 33+ messages in thread
From: Randy Dunlap @ 2018-04-06 21:01 UTC (permalink / raw)
To: Andrea Parri, paulmck
Cc: linux-kernel, Alan Stern, David Howells, Will Deacon,
Peter Zijlstra, Boqun Feng, Nicholas Piggin, Jade Alglave,
Luc Maranget, Akira Yokosawa, Ingo Molnar
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()
2018-04-06 21:01 ` Randy Dunlap
@ 2018-04-06 21:07 ` Paul E. McKenney
2018-04-06 21:08 ` Randy Dunlap
0 siblings, 1 reply; 33+ messages in thread
From: Paul E. McKenney @ 2018-04-06 21:07 UTC (permalink / raw)
To: Randy Dunlap
Cc: Andrea Parri, linux-kernel, Alan Stern, David Howells,
Will Deacon, Peter Zijlstra, Boqun Feng, Nicholas Piggin,
Jade Alglave, Luc Maranget, Akira Yokosawa, Ingo Molnar
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
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()
2018-04-06 21:07 ` Paul E. McKenney
@ 2018-04-06 21:08 ` Randy Dunlap
2018-04-06 21:58 ` Andrea Parri
0 siblings, 1 reply; 33+ messages in thread
From: Randy Dunlap @ 2018-04-06 21:08 UTC (permalink / raw)
To: paulmck
Cc: Andrea Parri, linux-kernel, Alan Stern, David Howells,
Will Deacon, Peter Zijlstra, Boqun Feng, Nicholas Piggin,
Jade Alglave, Luc Maranget, Akira Yokosawa, Ingo Molnar
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()
2018-04-06 21:08 ` Randy Dunlap
@ 2018-04-06 21:58 ` Andrea Parri
2018-04-08 21:14 ` Paul E. McKenney
0 siblings, 1 reply; 33+ messages in thread
From: Andrea Parri @ 2018-04-06 21:58 UTC (permalink / raw)
To: Randy Dunlap
Cc: paulmck, linux-kernel, Alan Stern, David Howells, Will Deacon,
Peter Zijlstra, Boqun Feng, Nicholas Piggin, Jade Alglave,
Luc Maranget, Akira Yokosawa, Ingo Molnar
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()
2018-04-06 21:58 ` Andrea Parri
@ 2018-04-08 21:14 ` Paul E. McKenney
2018-04-08 21:32 ` Randy Dunlap
0 siblings, 1 reply; 33+ messages in thread
From: Paul E. McKenney @ 2018-04-08 21:14 UTC (permalink / raw)
To: Andrea Parri
Cc: Randy Dunlap, linux-kernel, Alan Stern, David Howells,
Will Deacon, Peter Zijlstra, Boqun Feng, Nicholas Piggin,
Jade Alglave, Luc Maranget, Akira Yokosawa, Ingo Molnar
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
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()
2018-04-08 21:14 ` Paul E. McKenney
@ 2018-04-08 21:32 ` Randy Dunlap
2018-04-08 22:00 ` Paul E. McKenney
0 siblings, 1 reply; 33+ messages in thread
From: Randy Dunlap @ 2018-04-08 21:32 UTC (permalink / raw)
To: paulmck, Andrea Parri
Cc: linux-kernel, Alan Stern, David Howells, Will Deacon,
Peter Zijlstra, Boqun Feng, Nicholas Piggin, Jade Alglave,
Luc Maranget, Akira Yokosawa, Ingo Molnar
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] locking: Document the semantics of spin_is_locked()
2018-04-08 21:32 ` Randy Dunlap
@ 2018-04-08 22:00 ` Paul E. McKenney
0 siblings, 0 replies; 33+ messages in thread
From: Paul E. McKenney @ 2018-04-08 22:00 UTC (permalink / raw)
To: Randy Dunlap
Cc: Andrea Parri, linux-kernel, Alan Stern, David Howells,
Will Deacon, Peter Zijlstra, Boqun Feng, Nicholas Piggin,
Jade Alglave, Luc Maranget, Akira Yokosawa, Ingo Molnar
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
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2018-04-08 21:59 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-01 16:41 [PATCH v3 0/3] Changes, clean-ups and documentation for spin_is_locked() Andrea Parri
2018-04-01 16:41 ` [PATCH v2 1/3] locking: Document the semantics of spin_is_locked() Andrea Parri
2018-04-02 14:03 ` Alan Stern
2018-04-02 19:35 ` Paul E. McKenney
2018-04-06 19:47 ` [PATCH v4 " Andrea Parri
2018-04-06 21:00 ` Paul E. McKenney
2018-04-06 21:01 ` Randy Dunlap
2018-04-06 21:07 ` Paul E. McKenney
2018-04-06 21:08 ` Randy Dunlap
2018-04-06 21:58 ` Andrea Parri
2018-04-08 21:14 ` Paul E. McKenney
2018-04-08 21:32 ` Randy Dunlap
2018-04-08 22:00 ` Paul E. McKenney
2018-04-01 16:41 ` [PATCH v3 2/3] arm64: Remove smp_mb() from arch_spin_is_locked() Andrea Parri
2018-04-01 16:41 ` [PATCH v3 3/3] locking: Clean up comment and #ifndef for {,queued_}spin_is_locked() Andrea Parri
2018-04-01 18:24 ` [PATCH v3 0/3] Changes, clean-ups and documentation for spin_is_locked() Paul E. McKenney
2018-04-03 12:49 ` [PATCH v2 1/3] locking: Document the semantics of spin_is_locked() David Howells
2018-04-03 13:35 ` Andrea Parri
2018-04-03 13:52 ` David Howells
2018-04-03 14:07 ` Andrea Parri
2018-04-03 14:17 ` Paul E. McKenney
2018-04-03 14:43 ` Peter Zijlstra
2018-04-03 15:03 ` Paul E. McKenney
2018-04-03 16:11 ` Paul E. McKenney
2018-04-03 15:23 ` David Howells
2018-04-03 19:31 ` Andrea Parri
2018-04-03 20:04 ` Alan Stern
2018-04-03 21:43 ` David Howells
2018-04-03 21:47 ` Randy Dunlap
2018-04-04 12:47 ` Andrea Parri
2018-04-04 21:22 ` David Howells
2018-04-05 7:47 ` Christoph Hellwig
2018-04-05 8:56 ` Andrea Parri
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).