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 [off-list ref] wrote:

Björn Töpel [off-list ref] writes:

quoted

On 2021-03-01 17:08, Toke Høiland-Jørgensen wrote:

quoted

Björn Töpel [off-list ref] writes:

quoted

From: Björn Töpel <redacted>

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/ (local)

Signed-off-by: Björn Töpel <redacted>
---
  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 ;)

quoted

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