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

From: "Nicholas Piggin" <[email-masked]>
Date: Mon, 15 May 2023 15:46:17 +1000

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 <[email-masked]>
> ---
> 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 <[email-masked]>