Re: perf events ring buffer memory barrier on powerpc

[Michael Neuling <mikey@neuling.org>]

> I would argue for:
> 
>   READ ->data_tail			READ ->data_head
>   smp_rmb()	(A)			smp_rmb()	(C)
>   WRITE $data				READ $data
>   smp_wmb()	(B)			smp_mb()	(D)
>   STORE ->data_head			WRITE ->data_tail
> 
> Where A pairs with D, and B pairs with C.
> 
> I don't think A needs to be a full barrier because we won't in fact
> write data until we see the store from userspace. So we simply don't
> issue the data WRITE until we observe it.
> 
> OTOH, D needs to be a full barrier since it separates the data READ from
> the tail WRITE.
> 
> For B a WMB is sufficient since it separates two WRITEs, and for C an
> RMB is sufficient since it separates two READs.

FWIW the testing Victor did confirms WMB is good enough on powerpc.

Thanks,
Mikey

> 
> ---
>  kernel/events/ring_buffer.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> index cd55144270b5..c91274ef4e23 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -87,10 +87,31 @@ static void perf_output_put_handle(struct perf_output_handle *handle)
>  		goto out;
>  
>  	/*
> -	 * Publish the known good head. Rely on the full barrier implied
> -	 * by atomic_dec_and_test() order the rb->head read and this
> -	 * write.
> +	 * Since the mmap() consumer (userspace) can run on a different CPU:
> +	 *
> +	 *   kernel				user
> +	 *
> +	 *   READ ->data_tail			READ ->data_head
> +	 *   smp_rmb()	(A)			smp_rmb()	(C)
> +	 *   WRITE $data			READ $data
> +	 *   smp_wmb()	(B)			smp_mb()	(D)
> +	 *   STORE ->data_head			WRITE ->data_tail
> +	 * 
> +	 * Where A pairs with D, and B pairs with C.
> +	 * 
> +	 * I don't think A needs to be a full barrier because we won't in fact
> +	 * write data until we see the store from userspace. So we simply don't
> +	 * issue the data WRITE until we observe it.
> +	 * 
> +	 * OTOH, D needs to be a full barrier since it separates the data READ
> +	 * from the tail WRITE.
> +	 * 
> +	 * For B a WMB is sufficient since it separates two WRITEs, and for C
> +	 * an RMB is sufficient since it separates two READs.
> +	 *
> +	 * See perf_output_begin().
>  	 */
> +	smp_wmb();
>  	rb->user_page->data_head = head;
>  
>  	/*
> @@ -154,6 +175,8 @@ int perf_output_begin(struct perf_output_handle *handle,
>  		 * Userspace could choose to issue a mb() before updating the
>  		 * tail pointer. So that all reads will be completed before the
>  		 * write is issued.
> +		 *
> +		 * See perf_output_put_handle().
>  		 */
>  		tail = ACCESS_ONCE(rb->user_page->data_tail);
>  		smp_rmb();
>