| Message ID | 20210301104318.263262-1-bjorn.topel@gmail.com |
|---|---|
| Headers | show |
| Series | load-acquire/store-release semantics for AF_XDP rings | expand |
On 2021-03-01 17:08, Toke Høiland-Jørgensen wrote: > Björn Töpel <bjorn.topel@gmail.com> writes: > >> From: Björn Töpel <bjorn.topel@intel.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@intel.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? 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. Björn > -Toke >
Björn Töpel <bjorn.topel@intel.com> writes: > On 2021-03-01 17:08, Toke Høiland-Jørgensen wrote: >> Björn Töpel <bjorn.topel@gmail.com> writes: >> >>> From: Björn Töpel <bjorn.topel@intel.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@intel.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? -Toke
On Tue, 2 Mar 2021 at 11:23, Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Björn Töpel <bjorn.topel@intel.com> writes: > > > On 2021-03-01 17:08, Toke Høiland-Jørgensen wrote: > >> Björn Töpel <bjorn.topel@gmail.com> writes: > >> > >>> From: Björn Töpel <bjorn.topel@intel.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@intel.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 >
This two-patch series introduces load-acquire/store-release semantics for the AF_XDP rings. For most contemporary architectures, this is more effective than a SPSC ring based on smp_{r,w,}mb() barriers. More importantly, load-acquire/store-release semantics make the ring code easier to follow. This is effectively the change done in commit 6c43c091bdc5 ("documentation: Update circular buffer for load-acquire/store-release"), but for the AF_XDP rings. Both libbpf and the kernel-side are updated. More details in each commit. Thanks, Björn Björn Töpel (2): xsk: update rings for load-acquire/store-release semantics libbpf, xsk: add libbpf_smp_store_release libbpf_smp_load_acquire net/xdp/xsk_queue.h | 27 ++++++-------- tools/lib/bpf/libbpf_util.h | 72 +++++++++++++++++++++++++------------ tools/lib/bpf/xsk.h | 17 +++------ 3 files changed, 66 insertions(+), 50 deletions(-) base-commit: 85e142cb42a1e7b33971bf035dae432d8670c46b