LinuxLists.cc - [PATCH v3 0/3] Changes, clean-ups and documentation for spin_is

[permalink] [raw]

Subject: [PATCH v2 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 <[email protected]>
Cc: Alan Stern <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Boqun Feng <[email protected]>
Cc: Nicholas Piggin <[email protected]>
Cc: David Howells <[email protected]>
Cc: Jade Alglave <[email protected]>
Cc: Luc Maranget <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Akira Yokosawa <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
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

2018-04-01 16:44:49

[permalink] [raw]

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

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 <[email protected]>
Acked-by: Will Deacon <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Linus Torvalds <[email protected]>
---
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

2018-04-01 16:44:52

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Andrea Parri <[email protected]>
Acked-by: Will Deacon <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Linus Torvalds <[email protected]>
---
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

2018-04-01 18:25:49

[permalink] [raw]

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

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" <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
>
> 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
>

2018-04-02 14:04:47

by Alan Stern

[permalink] [raw]

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

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 <[email protected]>
> Cc: Alan Stern <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Boqun Feng <[email protected]>
> Cc: Nicholas Piggin <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: Jade Alglave <[email protected]>
> Cc: Luc Maranget <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: Akira Yokosawa <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> ---
> 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);
>

2018-04-02 19:46:13

[permalink] [raw]

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

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 <[email protected]>
> > Cc: Alan Stern <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Boqun Feng <[email protected]>
> > Cc: Nicholas Piggin <[email protected]>
> > Cc: David Howells <[email protected]>
> > Cc: Jade Alglave <[email protected]>
> > Cc: Luc Maranget <[email protected]>
> > Cc: "Paul E. McKenney" <[email protected]>
> > Cc: Akira Yokosawa <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > ---
> > 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);
> >
>

2018-04-03 12:50:37

[permalink] [raw]

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

Andrea Parri <[email protected]> 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

2018-04-03 13:38:32

[permalink] [raw]

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

On Tue, Apr 03, 2018 at 01:49:09PM +0100, David Howells wrote:
> Andrea Parri <[email protected]> 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

2018-04-03 13:55:35

[permalink] [raw]

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

Andrea Parri <[email protected]> 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

2018-04-03 14:09:11

[permalink] [raw]

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

On Tue, Apr 03, 2018 at 02:52:33PM +0100, David Howells wrote:
> Andrea Parri <[email protected]> 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

2018-04-03 14:18:00

[permalink] [raw]

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

On Tue, Apr 03, 2018 at 02:52:33PM +0100, David Howells wrote:
> Andrea Parri <[email protected]> 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

2018-04-03 14:44:48

by Peter Zijlstra

[permalink] [raw]

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

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.

2018-04-03 15:04:51

[permalink] [raw]

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

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

2018-04-03 15:25:51

[permalink] [raw]

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

Andrea Parri <[email protected]> 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

2018-04-03 16:12:41

[permalink] [raw]

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

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

2018-04-03 19:33:35

[permalink] [raw]

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

On Tue, Apr 03, 2018 at 04:23:07PM +0100, David Howells wrote:
> Andrea Parri <[email protected]> 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 <[email protected]>
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 <[email protected]>
Cc: Alan Stern <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Boqun Feng <[email protected]>
Cc: Nicholas Piggin <[email protected]>
Cc: David Howells <[email protected]>
Cc: Jade Alglave <[email protected]>
Cc: Luc Maranget <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Akira Yokosawa <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
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

2018-04-03 20:06:37

by Alan Stern

[permalink] [raw]

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

On Tue, 3 Apr 2018, Andrea Parri wrote:

> On Tue, Apr 03, 2018 at 04:23:07PM +0100, David Howells wrote:
> > Andrea Parri <[email protected]> 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

2018-04-03 21:44:41

[permalink] [raw]

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

Alan Stern <[email protected]> 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

2018-04-03 21:50:21

[permalink] [raw]

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

On 04/03/2018 02:43 PM, David Howells wrote:
> Alan Stern <[email protected]> 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

2018-04-04 12:49:29

[permalink] [raw]

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

On Tue, Apr 03, 2018 at 10:43:14PM +0100, David Howells wrote:
> Alan Stern <[email protected]> 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

2018-04-04 21:24:42

[permalink] [raw]

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

Randy Dunlap <[email protected]> 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".

2018-04-05 07:49:16

by Christoph Hellwig

[permalink] [raw]

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

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.

2018-04-05 08:58:10

[permalink] [raw]

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

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
>

2018-04-06 19:52:23

[permalink] [raw]

Subject: [PATCH v4 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(). 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 <[email protected]>
Co-Developed-by: Alan Stern <[email protected]>
Co-Developed-by: David Howells <[email protected]>
Signed-off-by: Andrea Parri <[email protected]>
Signed-off-by: Alan Stern <[email protected]>
Signed-off-by: David Howells <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Boqun Feng <[email protected]>
Cc: Nicholas Piggin <[email protected]>
Cc: Jade Alglave <[email protected]>
Cc: Luc Maranget <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Akira Yokosawa <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
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

2018-04-06 21:03:18

[permalink] [raw]

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

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 <[email protected]>
> Co-Developed-by: Alan Stern <[email protected]>
> Co-Developed-by: David Howells <[email protected]>
> Signed-off-by: Andrea Parri <[email protected]>
> Signed-off-by: Alan Stern <[email protected]>
> Signed-off-by: David Howells <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Boqun Feng <[email protected]>
> Cc: Nicholas Piggin <[email protected]>
> Cc: Jade Alglave <[email protected]>
> Cc: Luc Maranget <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: Akira Yokosawa <[email protected]>
> Cc: Ingo Molnar <[email protected]>

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
>

2018-04-06 21:06:10

[permalink] [raw]

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

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 <[email protected]>
> Co-Developed-by: Alan Stern <[email protected]>
> Co-Developed-by: David Howells <[email protected]>
> Signed-off-by: Andrea Parri <[email protected]>
> Signed-off-by: Alan Stern <[email protected]>
> Signed-off-by: David Howells <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Boqun Feng <[email protected]>
> Cc: Nicholas Piggin <[email protected]>
> Cc: Jade Alglave <[email protected]>
> Cc: Luc Maranget <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: Akira Yokosawa <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> ---
> 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

2018-04-06 21:11:17

[permalink] [raw]

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

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 <[email protected]>
> > Co-Developed-by: Alan Stern <[email protected]>
> > Co-Developed-by: David Howells <[email protected]>
> > Signed-off-by: Andrea Parri <[email protected]>
> > Signed-off-by: Alan Stern <[email protected]>
> > Signed-off-by: David Howells <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Boqun Feng <[email protected]>
> > Cc: Nicholas Piggin <[email protected]>
> > Cc: Jade Alglave <[email protected]>
> > Cc: Luc Maranget <[email protected]>
> > Cc: "Paul E. McKenney" <[email protected]>
> > Cc: Akira Yokosawa <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > ---
> > 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
>

2018-04-06 21:12:03

[permalink] [raw]

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

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 <[email protected]>
>>> Co-Developed-by: Alan Stern <[email protected]>
>>> Co-Developed-by: David Howells <[email protected]>
>>> Signed-off-by: Andrea Parri <[email protected]>
>>> Signed-off-by: Alan Stern <[email protected]>
>>> Signed-off-by: David Howells <[email protected]>
>>> Cc: Will Deacon <[email protected]>
>>> Cc: Peter Zijlstra <[email protected]>
>>> Cc: Boqun Feng <[email protected]>
>>> Cc: Nicholas Piggin <[email protected]>
>>> Cc: Jade Alglave <[email protected]>
>>> Cc: Luc Maranget <[email protected]>
>>> Cc: "Paul E. McKenney" <[email protected]>
>>> Cc: Akira Yokosawa <[email protected]>
>>> Cc: Ingo Molnar <[email protected]>
>>> ---
>>> 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

2018-04-06 22:02:41

[permalink] [raw]

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

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 <[email protected]>
> >>> Co-Developed-by: Alan Stern <[email protected]>
> >>> Co-Developed-by: David Howells <[email protected]>
> >>> Signed-off-by: Andrea Parri <[email protected]>
> >>> Signed-off-by: Alan Stern <[email protected]>
> >>> Signed-off-by: David Howells <[email protected]>
> >>> Cc: Will Deacon <[email protected]>
> >>> Cc: Peter Zijlstra <[email protected]>
> >>> Cc: Boqun Feng <[email protected]>
> >>> Cc: Nicholas Piggin <[email protected]>
> >>> Cc: Jade Alglave <[email protected]>
> >>> Cc: Luc Maranget <[email protected]>
> >>> Cc: "Paul E. McKenney" <[email protected]>
> >>> Cc: Akira Yokosawa <[email protected]>
> >>> Cc: Ingo Molnar <[email protected]>
> >>> ---
> >>> 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

2018-04-08 22:00:11

[permalink] [raw]

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

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 <[email protected]>
> > >>> Co-Developed-by: Alan Stern <[email protected]>
> > >>> Co-Developed-by: David Howells <[email protected]>
> > >>> Signed-off-by: Andrea Parri <[email protected]>
> > >>> Signed-off-by: Alan Stern <[email protected]>
> > >>> Signed-off-by: David Howells <[email protected]>
> > >>> Cc: Will Deacon <[email protected]>
> > >>> Cc: Peter Zijlstra <[email protected]>
> > >>> Cc: Boqun Feng <[email protected]>
> > >>> Cc: Nicholas Piggin <[email protected]>
> > >>> Cc: Jade Alglave <[email protected]>
> > >>> Cc: Luc Maranget <[email protected]>
> > >>> Cc: "Paul E. McKenney" <[email protected]>
> > >>> Cc: Akira Yokosawa <[email protected]>
> > >>> Cc: Ingo Molnar <[email protected]>
> > >>> ---
> > >>> 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
>

2018-04-08 22:01:12

[permalink] [raw]

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

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 <[email protected]>
>>>>>> Co-Developed-by: Alan Stern <[email protected]>
>>>>>> Co-Developed-by: David Howells <[email protected]>
>>>>>> Signed-off-by: Andrea Parri <[email protected]>
>>>>>> Signed-off-by: Alan Stern <[email protected]>
>>>>>> Signed-off-by: David Howells <[email protected]>
>>>>>> Cc: Will Deacon <[email protected]>
>>>>>> Cc: Peter Zijlstra <[email protected]>
>>>>>> Cc: Boqun Feng <[email protected]>
>>>>>> Cc: Nicholas Piggin <[email protected]>
>>>>>> Cc: Jade Alglave <[email protected]>
>>>>>> Cc: Luc Maranget <[email protected]>
>>>>>> Cc: "Paul E. McKenney" <[email protected]>
>>>>>> Cc: Akira Yokosawa <[email protected]>
>>>>>> Cc: Ingo Molnar <[email protected]>
>>>>>> ---
>>>>>> 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 <[email protected]>

Thanks.

--
~Randy

2018-04-08 22:50:11