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

["Paul E. McKenney" <paulmck@kernel.org>]

On Thu, Oct 19, 2023 at 02:20:35PM +0800, Hou Tao wrote:
> Hi Paul,
> 
> On 10/19/2023 12:54 PM, Paul E. McKenney wrote:
> > On Thu, Oct 19, 2023 at 09:07:07AM +0800, Hou Tao wrote:
> >> 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 ?
> > The real reason is that smp_mb__before_atomic() is not defined to do
> > anything unless followed by an atomic read-modify-write operation,
> > and atomic_read(), atomic_64read(), atomic_set(), and so on are not
> > read-modify-write operations.
> 
> I see. Thanks for explanation. It seems I did not read
> Documentation/atomic_t.txt carefully, it said:
> 
>     The barriers:
> 
>     smp_mb__{before,after}_atomic()
> 
>     only apply to the RMW atomic ops and can be used to augment/upgrade the
>     ordering inherent to the op.

That is the place!

> > As you point out, one implementation consequence of this is that
> > smp_mb__before_atomic() is nothingness on x86.
> >
> >> 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
> > The above two lines are supposed to be accessing the same field, correct?
> > If so, process Y's store really should be WRITE_ONCE().
> 
> Yes. These two processes are accessing the same field (namely
> timer->timer). Is WRITE_ONCE(xx) still necessary when the write of
> timer->time in process Y is protected by a spin-lock ?

If there is any possibility of a concurrent reader, that is, a reader
not holding that same lock, then yes, you should use WRITE_ONCE().

Compilers can do pretty vicious things to unmarked reads and writes.
But don't take my word for it, here are a few writeups:

o	"Who's afraid of a big bad optimizing compiler?" (series)
	https://lwn.net/Articles/793253, https://lwn.net/Articles/799218

o	"An introduction to lockless algorithms" (Paolo Bonzini series)
	https://lwn.net/Articles/844224, https://lwn.net/Articles/846700,
	https://lwn.net/Articles/847481, https://lwn.net/Articles/847973,
	https://lwn.net/Articles/849237, https://lwn.net/Articles/850202

o	"Is Parallel Programming Hard, And, If So, What Can You Do About It?"
	Section 4.3.4 ("Accessing Shared Variables")
	https://mirrors.edge.kernel.org/pub/linux/kernel/people/paulmck/perfbook/
perfbook.html

> >>                                             // 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 ?
> > Yes, because smp_mb() will order the prior store against that later load.
> 
> Thanks. Will fix the patch.

Very good!

							Thanx, Paul

> Regards,
> Hou
> >
> > 							Thanx, Paul
> >
> >> Regards,
> >> Hou
> >>
> >> [1]:
> >> https://lore.kernel.org/bpf/20231017125717.241101-2-houtao@huaweicloud.com/
> >>                                                                 
> >>
> >>> 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@kernel.org>
> >>> Acked-by: David Vernet <void@manifault.com>
> >>> Cc: Andrii Nakryiko <andrii@kernel.org>
> >>> Cc: Alexei Starovoitov <ast@kernel.org>
> >>> Cc: Daniel Borkmann <daniel@iogearbox.net>
> >>> Cc: Martin KaFai Lau <martin.lau@linux.dev>
> >>> Cc: Song Liu <song@kernel.org>
> >>> Cc: Yonghong Song <yonghong.song@linux.dev>
> >>> Cc: John Fastabend <john.fastabend@gmail.com>
> >>> Cc: KP Singh <kpsingh@kernel.org>
> >>> Cc: Stanislav Fomichev <sdf@google.com>
> >>> Cc: Hao Luo <haoluo@google.com>
> >>> Cc: Jiri Olsa <jolsa@kernel.org>
> >>> Cc: <bpf@vger.kernel.org>
> >>>
> >>> 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);
> >>>
> >>> .
>