netdev - Re: [PATCH bpf-next 1/2] xsk: update rings for load-acquire/store-release semantics

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 3 Mar 2021 08:56:04 +0100
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Björn Töpel <bjorn.topel@...el.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Netdev <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        paulmck@...nel.org, "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>, maximmi@...dia.com,
        Andrii Nakryiko <andrii@...nel.org>
Subject: Re: [PATCH bpf-next 1/2] xsk: update rings for load-acquire/store-release
 semantics

On Tue, 2 Mar 2021 at 11:23, Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> Björn Töpel <bjorn.topel@...el.com> writes:
>
> > On 2021-03-01 17:08, Toke Høiland-Jørgensen wrote:
> >> Björn Töpel <bjorn.topel@...il.com> writes:
> >>
> >>> From: Björn Töpel <bjorn.topel@...el.com>
> >>>
> >>> Currently, the AF_XDP rings uses smp_{r,w,}mb() fences on the
> >>> kernel-side. By updating the rings for load-acquire/store-release
> >>> semantics, the full barrier on the consumer side can be replaced with
> >>> improved performance as a nice side-effect.
> >>>
> >>> Note that this change does *not* require similar changes on the
> >>> libbpf/userland side, however it is recommended [1].
> >>>
> >>> On x86-64 systems, by removing the smp_mb() on the Rx and Tx side, the
> >>> l2fwd AF_XDP xdpsock sample performance increases by
> >>> 1%. Weakly-ordered platforms, such as ARM64 might benefit even more.
> >>>
> >>> [1] https://lore.kernel.org/bpf/20200316184423.GA14143@willie-the-truck/
> >>>
> >>> Signed-off-by: Björn Töpel <bjorn.topel@...el.com>
> >>> ---
> >>>   net/xdp/xsk_queue.h | 27 +++++++++++----------------
> >>>   1 file changed, 11 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
> >>> index 2823b7c3302d..e24279d8d845 100644
> >>> --- a/net/xdp/xsk_queue.h
> >>> +++ b/net/xdp/xsk_queue.h
> >>> @@ -47,19 +47,18 @@ struct xsk_queue {
> >>>     u64 queue_empty_descs;
> >>>   };
> >>>
> >>> -/* The structure of the shared state of the rings are the same as the
> >>> - * ring buffer in kernel/events/ring_buffer.c. For the Rx and completion
> >>> - * ring, the kernel is the producer and user space is the consumer. For
> >>> - * the Tx and fill rings, the kernel is the consumer and user space is
> >>> - * the producer.
> >>> +/* The structure of the shared state of the rings are a simple
> >>> + * circular buffer, as outlined in
> >>> + * Documentation/core-api/circular-buffers.rst. For the Rx and
> >>> + * completion ring, the kernel is the producer and user space is the
> >>> + * consumer. For the Tx and fill rings, the kernel is the consumer and
> >>> + * user space is the producer.
> >>>    *
> >>>    * producer                         consumer
> >>>    *
> >>> - * if (LOAD ->consumer) {           LOAD ->producer
> >>> - *                    (A)           smp_rmb()       (C)
> >>> + * if (LOAD ->consumer) {  (A)      LOAD.acq ->producer  (C)
> >>
> >> Why is LOAD.acq not needed on the consumer side?
> >>
> >
> > You mean why LOAD.acq is not needed on the *producer* side, i.e. the
> > ->consumer?
>
> Yes, of course! The two words were, like, right next to each other ;)
>
> > The ->consumer is a control dependency for the store, so there is no
> > ordering constraint for ->consumer at producer side. If there's no
> > space, no data is written. So, no barrier is needed there -- at least
> > that has been my perspective.
> >
> > This is very similar to the buffer in
> > Documentation/core-api/circular-buffers.rst. Roping in Paul for some
> > guidance.
>
> Yeah, I did read that, but got thrown off by this bit: "Therefore, the
> unlock-lock pair between consecutive invocations of the consumer
> provides the necessary ordering between the read of the index indicating
> that the consumer has vacated a given element and the write by the
> producer to that same element."
>
> Since there is no lock in the XSK, what provides that guarantee here?
>
>
> Oh, and BTW, when I re-read the rest of the comment in xsk_queue.h
> (below the diagram you are changing in this patch), the text still talks
> about "memory barriers" - maybe that should be updated to
> release/acquire as well while you're changing things?
>

Make sense! I'll make sure to do that for the V2!

Björn

> -Toke
>