| Message ID | 1554880154-30791-3-git-send-email-magnus.karlsson@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | libbpf: remove two dependencies on Linux kernel headers and improve performance as a bonus | expand |
On Wed, Apr 10, 2019 at 12:21 AM Magnus Karlsson <magnus.karlsson@intel.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@intel.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? > +# 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? > + > 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. > > *idx = cons->cached_cons; > cons->cached_cons += entries; > -- > 2.7.4 >
On Wed, Apr 10, 2019 at 9:08 PM Y Song <ys114321@gmail.com> wrote: > > On Wed, Apr 10, 2019 at 12:21 AM Magnus Karlsson > <magnus.karlsson@intel.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@intel.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 ;-). > > +# 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? 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. [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. /Magnus > > > > *idx = cons->cached_cons; > > cons->cached_cons += entries; > > -- > > 2.7.4 > >
On 04/11/2019 09:54 AM, Magnus Karlsson wrote: > On Wed, Apr 10, 2019 at 9:08 PM Y Song <ys114321@gmail.com> wrote: >> On Wed, Apr 10, 2019 at 12:21 AM Magnus Karlsson >> <magnus.karlsson@intel.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@intel.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. >>> +# 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. > 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. Thanks, Daniel > /Magnus > >>> >>> *idx = cons->cached_cons; >>> cons->cached_cons += entries; >>> -- >>> 2.7.4 >>>
On 04/11/2019 10:23 PM, Daniel Borkmann wrote: > On 04/11/2019 09:54 AM, Magnus Karlsson wrote: >> On Wed, Apr 10, 2019 at 9:08 PM Y Song <ys114321@gmail.com> wrote: >>> On Wed, Apr 10, 2019 at 12:21 AM Magnus Karlsson >>> <magnus.karlsson@intel.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@intel.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. > >>>> +# 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. (And if so then prefixed with libbpf_smp_{r,w}mb() to denote it's misc libbpf internal function.) >> 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. > > Thanks, > Daniel > >> /Magnus >> >>>> >>>> *idx = cons->cached_cons; >>>> cons->cached_cons += entries; >>>> -- >>>> 2.7.4 >>>> >
On Thu, Apr 11, 2019 at 10:23 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 04/11/2019 09:54 AM, Magnus Karlsson wrote: > > On Wed, Apr 10, 2019 at 9:08 PM Y Song <ys114321@gmail.com> wrote: > >> On Wed, Apr 10, 2019 at 12:21 AM Magnus Karlsson > >> <magnus.karlsson@intel.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@intel.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 > >>> >
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 +# 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 + 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(); *idx = cons->cached_cons; cons->cached_cons += entries;
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@intel.com> --- tools/lib/bpf/xsk.h | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)