Re: [PATCH bpf-next 2/2] libbpf, xsk: add libbpf_smp_store_release libbpf_smp_load_acquire

On Wed, 3 Mar 2021 at 08:14, Björn Töpel [off-list ref] wrote:

On 2021-03-03 05:38, Andrii Nakryiko wrote:

quoted

On Mon, Mar 1, 2021 at 2:43 AM Björn Töpel [off-list ref] wrote:

quoted

From: Björn Töpel <redacted>

Now that the AF_XDP rings have load-acquire/store-release semantics,
move libbpf to that as well.

The library-internal libbpf_smp_{load_acquire,store_release} are only
valid for 32-bit words on ARM64.

Also, remove the barriers that are no longer in use.

Signed-off-by: Björn Töpel <redacted>
---
  tools/lib/bpf/libbpf_util.h | 72 +++++++++++++++++++++++++------------
  tools/lib/bpf/xsk.h         | 17 +++------
  2 files changed, 55 insertions(+), 34 deletions(-)

diff --git a/tools/lib/bpf/libbpf_util.h b/tools/lib/bpf/libbpf_util.h
index 59c779c5790c..94a0d7bb6f3c 100644
--- a/tools/lib/bpf/libbpf_util.h
+++ b/tools/lib/bpf/libbpf_util.h

@@ -5,6 +5,7 @@
  #define __LIBBPF_LIBBPF_UTIL_H

  #include <stdbool.h>
+#include <linux/compiler.h>

  #ifdef __cplusplus
  extern "C" {

@@ -15,29 +16,56 @@ extern "C" {
   * application that uses libbpf.
   */
  #if defined(__i386__) || defined(__x86_64__)
-# define libbpf_smp_rmb() asm volatile("" : : : "memory")
-# define libbpf_smp_wmb() asm volatile("" : : : "memory")
-# define libbpf_smp_mb() \
-       asm volatile("lock; addl $0,-4(%%rsp)" : : : "memory", "cc")
-/* Hinders stores to be observed before older loads. */
-# define libbpf_smp_rwmb() asm volatile("" : : : "memory")

So, technically, these four are part of libbpf's API, as libbpf_util.h
is actually installed on target hosts. Seems like xsk.h is the only
one that is using them, though.

So the question is whether it's ok to remove them now?

I would say that. Ideally, the barriers shouldn't be visible at all,
since they're only used as an implementation detail for the static
inline functions.

quoted

And also, why wasn't this part of xsk.h in the first place?

I guess there was a "maybe it can be useful for more than the XDP socket
parts of libbpf"-idea. I'll move them to xsk.h for the v2, which will
make the migration easier.

Clarification! The reason for not having them in xsk.h, was that the
idea was that only the APIs allowed from the application should reside
there. IOW, libbpf_utils.h is only "implementation details". Again,
the static-inline function messes things up. Maybe moving to an
LTO-only world would be better, so we can get rid of the inlining all
together.

Björn

quoted

quoted

+# define libbpf_smp_store_release(p, v)                                        \
+       do {                                                            \
+               asm volatile("" : : : "memory");                        \
+               WRITE_ONCE(*p, v);                                      \
+       } while (0)
+# define libbpf_smp_load_acquire(p)                                    \
+       ({                                                              \
+               typeof(*p) ___p1 = READ_ONCE(*p);                       \
+               asm volatile("" : : : "memory");                        \
+               ___p1;                                                  \
+       })

[...]