Re: [PATCH bpf] Fold smp_mb__before_atomic() into atomic_set_release()

[Hou Tao]

Hi Paul,

On 10/19/2023 6:28 AM, Paul E. McKenney wrote:
> bpf: Fold smp_mb__before_atomic() into atomic_set_release()
>
> The bpf_user_ringbuf_drain() BPF_CALL function uses an atomic_set()
> immediately preceded by smp_mb__before_atomic() so as to order storing
> of ring-buffer consumer and producer positions prior to the atomic_set()
> call's clearing of the ->busy flag, as follows:
>
>         smp_mb__before_atomic();
>         atomic_set(&rb->busy, 0);
>
> Although this works given current architectures and implementations, and
> given that this only needs to order prior writes against a later write.
> However, it does so by accident because the smp_mb__before_atomic()
> is only guaranteed to work with read-modify-write atomic operations,
> and not at all with things like atomic_set() and atomic_read().
>
> Note especially that smp_mb__before_atomic() will not, repeat *not*,
> order the prior write to "a" before the subsequent non-read-modify-write
> atomic read from "b", even on strongly ordered systems such as x86:
>
>         WRITE_ONCE(a, 1);
>         smp_mb__before_atomic();
>         r1 = atomic_read(&b);

The reason is smp_mb__before_atomic() is defined as noop and
atomic_read() in x86-64 is just READ_ONCE(), right ?

And it seems that I also used smp_mb__before_atomic() in a wrong way for
patch [1]. The memory order in the posted patch is

process X                                    process Y
    atomic64_dec_and_test(&map->usercnt)
    READ_ONCE(timer->timer)
                                            timer->time = t
                                            // it won't work
                                            smp_mb__before_atomic()
                                            atomic64_read(&map->usercnt)

For the problem, it seems I need to replace smp_mb__before_atomic() by
smp_mb() to fix the memory order, right ?

Regards,
Hou

[1]:
https://lore.kernel.org/bpf/20231017125717.241101-2-houtao@xxxxxxxxxxxxxxx/
                                                                

>
> Therefore, replace the smp_mb__before_atomic() and atomic_set() with
> atomic_set_release() as follows:
>
>         atomic_set_release(&rb->busy, 0);
>
> This is no slower (and sometimes is faster) than the original, and also
> provides a formal guarantee of ordering that the original lacks.
>
> Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> Acked-by: David Vernet <void@xxxxxxxxxxxxx>
> Cc: Andrii Nakryiko <andrii@xxxxxxxxxx>
> Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
> Cc: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> Cc: Martin KaFai Lau <martin.lau@xxxxxxxxx>
> Cc: Song Liu <song@xxxxxxxxxx>
> Cc: Yonghong Song <yonghong.song@xxxxxxxxx>
> Cc: John Fastabend <john.fastabend@xxxxxxxxx>
> Cc: KP Singh <kpsingh@xxxxxxxxxx>
> Cc: Stanislav Fomichev <sdf@xxxxxxxxxx>
> Cc: Hao Luo <haoluo@xxxxxxxxxx>
> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> Cc: <bpf@xxxxxxxxxxxxxxx>
>
> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> index f045fde632e5..0ee653a936ea 100644
> --- a/kernel/bpf/ringbuf.c
> +++ b/kernel/bpf/ringbuf.c
> @@ -770,8 +770,7 @@ BPF_CALL_4(bpf_user_ringbuf_drain, struct bpf_map *, map,
>  	/* Prevent the clearing of the busy-bit from being reordered before the
>  	 * storing of any rb consumer or producer positions.
>  	 */
> -	smp_mb__before_atomic();
> -	atomic_set(&rb->busy, 0);
> +	atomic_set_release(&rb->busy, 0);
>  
>  	if (flags & BPF_RB_FORCE_WAKEUP)
>  		irq_work_queue(&rb->work);
>
> .