Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Subject: Re: [PATCH bpf] Fold smp_mb__before_atomic() into
 atomic_set_release()
To:     paulmck@kernel.org, bpf@vger.kernel.org
Cc:     David Vernet <void@manifault.com>,
        Andrii Nakryiko <andrii@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>,
        Yonghong Song <yonghong.song@linux.dev>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
        linux-kernel@vger.kernel.org
References: <ec86d38e-cfb4-44aa-8fdb-6c925922d93c@paulmck-laptop>
From:   Hou Tao <houtao@huaweicloud.com>
Message-ID: <722b64d7-281b-b4ab-4d4d-403abc41a36b@huaweicloud.com>
Date:   Thu, 19 Oct 2023 09:07:07 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <ec86d38e-cfb4-44aa-8fdb-6c925922d93c@paulmck-laptop>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

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@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);
>
> .