Date:   Fri, 12 Apr 2019 11:01:45 +0200
From:   Magnus Karlsson <magnus.karlsson@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Y Song <ys114321@...il.com>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        Björn Töpel <bjorn.topel@...el.com>,
        Alexei Starovoitov <ast@...nel.org>,
        netdev <netdev@...r.kernel.org>, bpf@...r.kernel.org,
        bruce.richardson@...el.com, ciara.loftus@...el.com,
        ilias.apalodimas@...aro.org, Ye Xiaolong <xiaolong.ye@...el.com>,
        ferruh.yigit@...el.com, "Zhang, Qi Z" <qi.z.zhang@...el.com>,
        georgmueller@....net
Subject: Re: [PATCH bpf v2 2/2] libbpf: remove dependency on barrier.h in xsk.h

On Thu, Apr 11, 2019 at 10:23 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 04/11/2019 09:54 AM, Magnus Karlsson wrote:
> > On Wed, Apr 10, 2019 at 9:08 PM Y Song <ys114321@...il.com> wrote:
> >> On Wed, Apr 10, 2019 at 12:21 AM Magnus Karlsson
> >> <magnus.karlsson@...el.com> wrote:
> >>>
> >>> The use of smp_rmb() and smp_wmb() creates a Linux header dependency
> >>> on barrier.h that is uneccessary in most parts. This patch implements
> >>> the two small defines that are needed from barrier.h. As a bonus, the
> >>> new implementations are faster than the default ones as they default
> >>> to sfence and lfence for x86, while we only need a compiler barrier in
> >>> our case. Just as it is when the same ring access code is compiled in
> >>> the kernel.
> >>>
> >>> Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets")
> >>> Signed-off-by: Magnus Karlsson <magnus.karlsson@...el.com>
> >>> ---
> >>>  tools/lib/bpf/xsk.h | 20 ++++++++++++++++++--
> >>>  1 file changed, 18 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
> >>> index 3638147..69136d9 100644
> >>> --- a/tools/lib/bpf/xsk.h
> >>> +++ b/tools/lib/bpf/xsk.h
> >>> @@ -39,6 +39,22 @@ DEFINE_XSK_RING(xsk_ring_cons);
> >>>  struct xsk_umem;
> >>>  struct xsk_socket;
> >>>
> >>> +#if !defined bpf_smp_rmb && !defined bpf_smp_wmb
> >>
> >> Maybe add some comments to explain the different between bpf_smp_{r,w}mb
> >> and smp_{r,w}mb so later users will have a better idea which to pick?
> >
> > Ouch, that is a hard one. I would just recommend people to read
> > Documentation/memory-barriers.txt. My attempt at explaining all this
> > would not be pretty and likely sprinkled with errors ;-).
>
> I think Yonghong meant here place a comment wrt when to use the below versus
> when to use smp_{r,w}mb(). Both are essentially the same just that the main
> difference here would be that this header needs to be installed in the system
> so users need to have it. I think it indeed makes sense to add a comment about
> this specific fact otherwise we might forget about it in few months.

Will do.

> >>> +# if defined(__i386__) || defined(__x86_64__)
> >>> +#  define bpf_smp_rmb() asm volatile("" : : : "memory")
> >>> +#  define bpf_smp_wmb() asm volatile("" : : : "memory")
> >>> +# elif defined(__aarch64__)
> >>> +#  define bpf_smp_rmb() asm volatile("dmb ishld" : : : "memory")
> >>> +#  define bpf_smp_wmb() asm volatile("dmb ishst" : : : "memory")
> >>> +# elif defined(__arm__)
> >>> +/* These are only valid for armv7 and above */
> >>> +#  define bpf_smp_rmb() asm volatile("dmb ish" : : : "memory")
> >>> +#  define bpf_smp_wmb() asm volatile("dmb ishst" : : : "memory")
> >>> +# else
> >>> +#  error Architecture not supported by the XDP socket code in libbpf.
> >>> +# endif
> >>> +#endif
> >>
> >> Since this is generic enough and could be used by other files as well,
> >> maybe put it into libbpf_util.h?
>
> Hmm, maybe a good point. We could place it into libbpf.h as there is already
> various misc helpers and xsk.h includes it anyway. But: if we do that, then
> the above 'else' part would need some generic fallback (__sync_synchronize()
> plus a warning?) as otherwise compilation would break for everyone with 'error'.
> Ideally this should then cover as much as possible from mainstream archs though.

I will give this a stab for the mainstream archs then fall back and
warn as you suggests. Will use the libbpf_ prefix.

> > Good question. Do not know. Daniel suggested introducing [0] and
> > perhaps that can be used by the broader libbpf code base? The
> > important part for this patch set is that these operations match the
> > ones in the kernel on the other end of the ring.
>
> Yeah, it can be used generally except for headers that are going to be
> installed where these are present in inline helper functions.
>
> > [0] https://lore.kernel.org/netdev/20181017144156.16639-2-daniel@iogearbox.net/
> >
> >>> +
> >>>  static inline __u64 *xsk_ring_prod__fill_addr(struct xsk_ring_prod *fill,
> >>>                                               __u32 idx)
> >>>  {
> >>> @@ -119,7 +135,7 @@ static inline void xsk_ring_prod__submit(struct xsk_ring_prod *prod, size_t nb)
> >>>         /* Make sure everything has been written to the ring before signalling
> >>>          * this to the kernel.
> >>>          */
> >>> -       smp_wmb();
> >>> +       bpf_smp_wmb();
> >>>
> >>>         *prod->producer += nb;
> >>>  }
> >>> @@ -133,7 +149,7 @@ static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons,
> >>>                 /* Make sure we do not speculatively read the data before
> >>>                  * we have received the packet buffers from the ring.
> >>>                  */
> >>> -               smp_rmb();
> >>> +               bpf_smp_rmb();
> >>
> >> Could you explain why a compiler barrier is good enough here on x86? Note that
> >> the load cons->cached_cons could be reordered with earlier
> >> non-overlapping stores
> >> at runtime.
> >
> > The bpf_smp_rmb() is there to protect the data in the ring itself to
> > be read by the consumer before the producer has signaled that it has
> > finished “producing” them by updating the producer (head) pointer. As
> > stores are not reordered with other stores on x86 (nor loads with
> > other loads), the update of the producer pointer will always be
> > observed after the writing of the data in the ring, as that is done
> > before the update of the producer pointer in xsk_ring_prod__submit().
> > One side only updates and the other side only reads. cached_cons is a
> > local variable and only for operations done by another core can we
> > observe loads being reordered with older stores to different
> > locations. Since no one else is touching cached_cons, this will not
> > happen.
>
> From perf RB side, I found this one kernel/events/ring_buffer.c +72 to
> be very helpful. It's independent of this series, but I would appreciate
> if you could make similar scheme / comment somewhere in the AF_XDP code
> such that all barriers in there can be more easily followed wrt how they
> pair to user space.

That was very useful indeed and as we have the same structure this
also uncovered that we are missing an smp_mb in the (D) location below
for the ring to work on PPC64 and likely ARM too. On x86, it will work
without one since stores are not reordered with older loads (as
observed from another core), but there is only rmb, wmb and mb in
Linux (no rwmb or wrmb), so we are going to take a hit on x86 here.
But correctness for everyone always goes first. Will submit a new
patch set with this fix in the kernel also. The question is, can I
just refer to the perf_ring comments in the xsk.h code, or should I
largely copy it into the xsk code so it is there too?

As an optimization for the future, maybe we could do something along
the lines of Documentation/core-api/circular-buffers.rst, where
load_aqcuire and store_release are used instead of expensive memory
barriers. Do you guys have any experience using these primitives
instead and their performance on various platforms?

from kernel/events/ring_buffer.c:
*   kernel                             user
*
*   if (LOAD ->data_tail) {            LOAD ->data_head
*                      (A)             smp_rmb()       (C)
*      STORE $data                     LOAD $data
*      smp_wmb()       (B)             smp_mb()        (D)
*      STORE ->data_head               STORE ->data_tail
*   }
*
* Where A pairs with D, and B pairs with C.


> Thanks,
> Daniel
>
> > /Magnus
> >
> >>>
> >>>                 *idx = cons->cached_cons;
> >>>                 cons->cached_cons += entries;
> >>> --
> >>> 2.7.4
> >>>
>

Powered by blists - more mailing lists