diff mbox series

[v2,02/11] powerpc: qspinlock: Enforce qnode writes prior to publishing to queue

Message ID 20230510033117.1395895-3-rmclure@linux.ibm.com (mailing list archive)
State Accepted
Commit 6f3136326ee47ae2dd5dac9306c9b08ccbc7e81e
Headers show
Series powerpc: KCSAN fix warnings and mark accesses | expand

Commit Message

Rohan McLure May 10, 2023, 3:31 a.m. UTC
Annotate the release barrier and memory clobber (in effect, producing a
compiler barrier) in the publish_tail_cpu call. These barriers have the
effect of ensuring that qnode attributes are all written to prior to
publish the node to the waitqueue.

Even while the initial write to the 'locked' attribute is guaranteed to
terminate prior to the node being visible, KCSAN still complains that
the write is reorderable by the compiler. Issue a kcsan_release() to
inform KCSAN of the release barrier contained in publish_tail_cpu().

Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
v2: Remove extraneous compiler barrier, but annotate release-barrier
contained in call publish_tail_cpu(), and include kcsan_release().
---
 arch/powerpc/lib/qspinlock.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Nicholas Piggin May 15, 2023, 5:46 a.m. UTC | #1
On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
> Annotate the release barrier and memory clobber (in effect, producing a
> compiler barrier) in the publish_tail_cpu call. These barriers have the
> effect of ensuring that qnode attributes are all written to prior to
> publish the node to the waitqueue.
>
> Even while the initial write to the 'locked' attribute is guaranteed to
> terminate prior to the node being visible, KCSAN still complains that
> the write is reorderable by the compiler. Issue a kcsan_release() to
> inform KCSAN of the release barrier contained in publish_tail_cpu().
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> v2: Remove extraneous compiler barrier, but annotate release-barrier
> contained in call publish_tail_cpu(), and include kcsan_release().
> ---
>  arch/powerpc/lib/qspinlock.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c
> index b76c1f6acce5..253620979d0c 100644
> --- a/arch/powerpc/lib/qspinlock.c
> +++ b/arch/powerpc/lib/qspinlock.c
> @@ -161,6 +161,8 @@ static __always_inline u32 publish_tail_cpu(struct qspinlock *lock, u32 tail)
>  {
>  	u32 prev, tmp;
>  
> +	kcsan_release();
> +
>  	asm volatile(
>  "\t"	PPC_RELEASE_BARRIER "						\n"
>  "1:	lwarx	%0,0,%2		# publish_tail_cpu			\n"
> @@ -570,6 +572,11 @@ static __always_inline void queued_spin_lock_mcs_queue(struct qspinlock *lock, b
>  
>  	tail = encode_tail_cpu(node->cpu);
>  
> +	/*
> +	 * Assign all attributes of a node before it can be published.
> +	 * Issues an lwsync, serving as a release barrier, as well as a
> +	 * compiler barrier.
> +	 */
>  	old = publish_tail_cpu(lock, tail);

Possibly better to be with the publish function, hopefully the name
gives away it has a release barrier then store that makes it visible.
But that's nitpicking.

Thanks for the qspinlock fixes.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
diff mbox series

Patch

diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c
index b76c1f6acce5..253620979d0c 100644
--- a/arch/powerpc/lib/qspinlock.c
+++ b/arch/powerpc/lib/qspinlock.c
@@ -161,6 +161,8 @@  static __always_inline u32 publish_tail_cpu(struct qspinlock *lock, u32 tail)
 {
 	u32 prev, tmp;
 
+	kcsan_release();
+
 	asm volatile(
 "\t"	PPC_RELEASE_BARRIER "						\n"
 "1:	lwarx	%0,0,%2		# publish_tail_cpu			\n"
@@ -570,6 +572,11 @@  static __always_inline void queued_spin_lock_mcs_queue(struct qspinlock *lock, b
 
 	tail = encode_tail_cpu(node->cpu);
 
+	/*
+	 * Assign all attributes of a node before it can be published.
+	 * Issues an lwsync, serving as a release barrier, as well as a
+	 * compiler barrier.
+	 */
 	old = publish_tail_cpu(lock, tail);
 
 	/*