bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/6] Move bpf_helpers and add BPF_CORE_READ macros
@ 2019-09-30 18:58 Andrii Nakryiko
  2019-09-30 18:58 ` [PATCH bpf-next 1/6] selftests/bpf: undo GCC-specific bpf_helpers.h changes Andrii Nakryiko
                   ` (5 more replies)
  0 siblings, 6 replies; 47+ messages in thread
From: Andrii Nakryiko @ 2019-09-30 18:58 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

This patch set make bpf_helpers.h and bpf_endian.h part of libbpf itself for
consumption by user BPF programs, not just selftests. All the selftests are
switched to use libbpf's bpf_helpers/bpf_endian and selftests' ones are
removed.

As part of this patch set we also add BPF_CORE_READ variadic macros, that are
simplifying BPF CO-RE reads, especially the ones that have to follow few
pointers. E.g., what in non-BPF world (and when using BCC) would be:

int x = s->a->b.c->d; /* s, a, and b.c are pointers */

today would have to be written using explicit bpf_probe_read() calls as:

  void *t;
  int x;
  bpf_probe_read(&t, sizeof(t), s->a);
  bpf_probe_read(&t, sizeof(t), ((struct b *)t)->b.c);
  bpf_probe_read(&x, sizeof(x), ((struct c *)t)->d);

This is super inconvenient and distracts from program logic a lot. Now, with
added BPF_CORE_READ() macros, you can write the above as:

  int x = BPF_CORE_READ(s, a, b.c, d);

Up to 9 levels of pointer chasing are supported, which should be enough for
any practical purpose, hopefully, without adding too much boilerplate macro
definitions (though there is admittedly some, given how variadic and recursive
C macro have to be implemented).

There is also BPF_CORE_READ_INTO() variant, which relies on caller to allocate
space for result:

  int x;
  BPF_CORE_READ_INTO(&x, s, a, b.c, d);

Result of last bpf_probe_read() call in the chain of calls is the result of
BPF_CORE_READ_INTO(). If any intermediate bpf_probe_read() aall fails, then
all the subsequent ones will fail too, so this is sufficient to know whether
overall "operation" succeeded or not. No short-circuiting of bpf_probe_read()s
is done, though.

BPF_CORE_READ_STR_INTO() is added as well, which differs from
BPF_CORE_READ_INTO() only in that last bpf_probe_read() call (to read final
field after chasing pointers) is replaced with bpf_probe_read_str(). Result of
bpf_probe_read_str() is returned as a result of BPF_CORE_READ_STR_INTO() macro
itself, so that applications can track return code and/or length of read
string.

Patch set outline:
- patch #1 undoes previously added GCC-specific bpf-helpers.h include;
- patch #2 adds bpf_helper.h and bpf_endian.h into libbpf sources;
- patch #3 adjusts selftests (Makefile only) and removes
  bpf_helpers.h/bpf_endian.h from seftests/bpf;
- patch #4 adds variadic BPF_CORE_READ() macro family, as described above;
- patch #5 fixes existing BPF CO-RE reloc selftest as it previously re-used
  BPF_CORE_READ() macro name with slightly different syntax;
- patch #6 adds tests to verify all possible levels of pointer nestedness for
  BPF_CORE_READ(), as well as correctness test for BPF_CORE_READ_STR_INTO().

Andrii Nakryiko (6):
  selftests/bpf: undo GCC-specific bpf_helpers.h changes
  libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  selftests/bpf: switch test to use libbpf's helpers
  libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers
  selftests/bpf: adjust CO-RE reloc tests for new BPF_CORE_READ macro
  selftests/bpf: add BPF_CORE_READ and BPF_CORE_READ_STR_INTO macro
    tests

 tools/lib/bpf/Makefile                        |   4 +-
 .../selftests => lib}/bpf/bpf_endian.h        |   0
 .../selftests => lib}/bpf/bpf_helpers.h       | 159 ++++++++++++++++--
 tools/testing/selftests/bpf/Makefile          |   2 +-
 .../selftests/bpf/prog_tests/core_reloc.c     |   8 +-
 .../selftests/bpf/progs/core_reloc_types.h    |   9 +
 .../bpf/progs/test_core_reloc_arrays.c        |  10 +-
 .../bpf/progs/test_core_reloc_flavors.c       |   8 +-
 .../bpf/progs/test_core_reloc_ints.c          |  18 +-
 .../bpf/progs/test_core_reloc_kernel.c        |  60 ++++++-
 .../bpf/progs/test_core_reloc_misc.c          |   8 +-
 .../bpf/progs/test_core_reloc_mods.c          |  18 +-
 .../bpf/progs/test_core_reloc_nesting.c       |   6 +-
 .../bpf/progs/test_core_reloc_primitives.c    |  12 +-
 .../bpf/progs/test_core_reloc_ptr_as_arr.c    |   4 +-
 15 files changed, 273 insertions(+), 53 deletions(-)
 rename tools/{testing/selftests => lib}/bpf/bpf_endian.h (100%)
 rename tools/{testing/selftests => lib}/bpf/bpf_helpers.h (77%)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 47+ messages in thread

* [PATCH bpf-next 1/6] selftests/bpf: undo GCC-specific bpf_helpers.h changes
  2019-09-30 18:58 [PATCH bpf-next 0/6] Move bpf_helpers and add BPF_CORE_READ macros Andrii Nakryiko
@ 2019-09-30 18:58 ` Andrii Nakryiko
  2019-09-30 22:53   ` Song Liu
                     ` (2 more replies)
  2019-09-30 18:58 ` [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf Andrii Nakryiko
                   ` (4 subsequent siblings)
  5 siblings, 3 replies; 47+ messages in thread
From: Andrii Nakryiko @ 2019-09-30 18:58 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Having GCC provide its own bpf-helper.h is not the right approach and is
going to be changed. Undo bpf_helpers.h change before moving
bpf_helpers.h into libbpf.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/testing/selftests/bpf/bpf_helpers.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index 54a50699bbfd..a1d9b97b8e15 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -13,8 +13,6 @@
 			 ##__VA_ARGS__);		\
 })
 
-#ifdef __clang__
-
 /* helper macro to place programs, maps, license in
  * different sections in elf_bpf file. Section names
  * are interpreted by elf_bpf loader
@@ -258,12 +256,6 @@ struct bpf_map_def {
 	unsigned int numa_node;
 };
 
-#else
-
-#include <bpf-helpers.h>
-
-#endif
-
 #define BPF_ANNOTATE_KV_PAIR(name, type_key, type_val)		\
 	struct ____btf_map_##name {				\
 		type_key key;					\
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-09-30 18:58 [PATCH bpf-next 0/6] Move bpf_helpers and add BPF_CORE_READ macros Andrii Nakryiko
  2019-09-30 18:58 ` [PATCH bpf-next 1/6] selftests/bpf: undo GCC-specific bpf_helpers.h changes Andrii Nakryiko
@ 2019-09-30 18:58 ` Andrii Nakryiko
  2019-09-30 22:55   ` Song Liu
                     ` (3 more replies)
  2019-09-30 18:58 ` [PATCH bpf-next 3/6] selftests/bpf: switch test to use libbpf's helpers Andrii Nakryiko
                   ` (3 subsequent siblings)
  5 siblings, 4 replies; 47+ messages in thread
From: Andrii Nakryiko @ 2019-09-30 18:58 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
are installed along the other libbpf headers.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/Makefile      |   4 +-
 tools/lib/bpf/bpf_endian.h  |  72 +++++
 tools/lib/bpf/bpf_helpers.h | 527 ++++++++++++++++++++++++++++++++++++
 3 files changed, 602 insertions(+), 1 deletion(-)
 create mode 100644 tools/lib/bpf/bpf_endian.h
 create mode 100644 tools/lib/bpf/bpf_helpers.h

diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index c6f94cffe06e..2ff345981803 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -240,7 +240,9 @@ install_headers:
 		$(call do_install,libbpf.h,$(prefix)/include/bpf,644); \
 		$(call do_install,btf.h,$(prefix)/include/bpf,644); \
 		$(call do_install,libbpf_util.h,$(prefix)/include/bpf,644); \
-		$(call do_install,xsk.h,$(prefix)/include/bpf,644);
+		$(call do_install,xsk.h,$(prefix)/include/bpf,644); \
+		$(call do_install,bpf_helpers.h,$(prefix)/include/bpf,644); \
+		$(call do_install,bpf_endian.h,$(prefix)/include/bpf,644);
 
 install_pkgconfig: $(PC_FILE)
 	$(call QUIET_INSTALL, $(PC_FILE)) \
diff --git a/tools/lib/bpf/bpf_endian.h b/tools/lib/bpf/bpf_endian.h
new file mode 100644
index 000000000000..fbe28008450f
--- /dev/null
+++ b/tools/lib/bpf/bpf_endian.h
@@ -0,0 +1,72 @@
+/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
+#ifndef __BPF_ENDIAN__
+#define __BPF_ENDIAN__
+
+#include <linux/stddef.h>
+#include <linux/swab.h>
+
+/* LLVM's BPF target selects the endianness of the CPU
+ * it compiles on, or the user specifies (bpfel/bpfeb),
+ * respectively. The used __BYTE_ORDER__ is defined by
+ * the compiler, we cannot rely on __BYTE_ORDER from
+ * libc headers, since it doesn't reflect the actual
+ * requested byte order.
+ *
+ * Note, LLVM's BPF target has different __builtin_bswapX()
+ * semantics. It does map to BPF_ALU | BPF_END | BPF_TO_BE
+ * in bpfel and bpfeb case, which means below, that we map
+ * to cpu_to_be16(). We could use it unconditionally in BPF
+ * case, but better not rely on it, so that this header here
+ * can be used from application and BPF program side, which
+ * use different targets.
+ */
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+# define __bpf_ntohs(x)			__builtin_bswap16(x)
+# define __bpf_htons(x)			__builtin_bswap16(x)
+# define __bpf_constant_ntohs(x)	___constant_swab16(x)
+# define __bpf_constant_htons(x)	___constant_swab16(x)
+# define __bpf_ntohl(x)			__builtin_bswap32(x)
+# define __bpf_htonl(x)			__builtin_bswap32(x)
+# define __bpf_constant_ntohl(x)	___constant_swab32(x)
+# define __bpf_constant_htonl(x)	___constant_swab32(x)
+# define __bpf_be64_to_cpu(x)		__builtin_bswap64(x)
+# define __bpf_cpu_to_be64(x)		__builtin_bswap64(x)
+# define __bpf_constant_be64_to_cpu(x)	___constant_swab64(x)
+# define __bpf_constant_cpu_to_be64(x)	___constant_swab64(x)
+#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+# define __bpf_ntohs(x)			(x)
+# define __bpf_htons(x)			(x)
+# define __bpf_constant_ntohs(x)	(x)
+# define __bpf_constant_htons(x)	(x)
+# define __bpf_ntohl(x)			(x)
+# define __bpf_htonl(x)			(x)
+# define __bpf_constant_ntohl(x)	(x)
+# define __bpf_constant_htonl(x)	(x)
+# define __bpf_be64_to_cpu(x)		(x)
+# define __bpf_cpu_to_be64(x)		(x)
+# define __bpf_constant_be64_to_cpu(x)  (x)
+# define __bpf_constant_cpu_to_be64(x)  (x)
+#else
+# error "Fix your compiler's __BYTE_ORDER__?!"
+#endif
+
+#define bpf_htons(x)				\
+	(__builtin_constant_p(x) ?		\
+	 __bpf_constant_htons(x) : __bpf_htons(x))
+#define bpf_ntohs(x)				\
+	(__builtin_constant_p(x) ?		\
+	 __bpf_constant_ntohs(x) : __bpf_ntohs(x))
+#define bpf_htonl(x)				\
+	(__builtin_constant_p(x) ?		\
+	 __bpf_constant_htonl(x) : __bpf_htonl(x))
+#define bpf_ntohl(x)				\
+	(__builtin_constant_p(x) ?		\
+	 __bpf_constant_ntohl(x) : __bpf_ntohl(x))
+#define bpf_cpu_to_be64(x)			\
+	(__builtin_constant_p(x) ?		\
+	 __bpf_constant_cpu_to_be64(x) : __bpf_cpu_to_be64(x))
+#define bpf_be64_to_cpu(x)			\
+	(__builtin_constant_p(x) ?		\
+	 __bpf_constant_be64_to_cpu(x) : __bpf_be64_to_cpu(x))
+
+#endif /* __BPF_ENDIAN__ */
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
new file mode 100644
index 000000000000..a1d9b97b8e15
--- /dev/null
+++ b/tools/lib/bpf/bpf_helpers.h
@@ -0,0 +1,527 @@
+/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
+#ifndef __BPF_HELPERS__
+#define __BPF_HELPERS__
+
+#define __uint(name, val) int (*name)[val]
+#define __type(name, val) val *name
+
+/* helper macro to print out debug messages */
+#define bpf_printk(fmt, ...)				\
+({							\
+	char ____fmt[] = fmt;				\
+	bpf_trace_printk(____fmt, sizeof(____fmt),	\
+			 ##__VA_ARGS__);		\
+})
+
+/* helper macro to place programs, maps, license in
+ * different sections in elf_bpf file. Section names
+ * are interpreted by elf_bpf loader
+ */
+#define SEC(NAME) __attribute__((section(NAME), used))
+
+/* helper functions called from eBPF programs written in C */
+static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
+	(void *) BPF_FUNC_map_lookup_elem;
+static int (*bpf_map_update_elem)(void *map, const void *key, const void *value,
+				  unsigned long long flags) =
+	(void *) BPF_FUNC_map_update_elem;
+static int (*bpf_map_delete_elem)(void *map, const void *key) =
+	(void *) BPF_FUNC_map_delete_elem;
+static int (*bpf_map_push_elem)(void *map, const void *value,
+				unsigned long long flags) =
+	(void *) BPF_FUNC_map_push_elem;
+static int (*bpf_map_pop_elem)(void *map, void *value) =
+	(void *) BPF_FUNC_map_pop_elem;
+static int (*bpf_map_peek_elem)(void *map, void *value) =
+	(void *) BPF_FUNC_map_peek_elem;
+static int (*bpf_probe_read)(void *dst, int size, const void *unsafe_ptr) =
+	(void *) BPF_FUNC_probe_read;
+static unsigned long long (*bpf_ktime_get_ns)(void) =
+	(void *) BPF_FUNC_ktime_get_ns;
+static int (*bpf_trace_printk)(const char *fmt, int fmt_size, ...) =
+	(void *) BPF_FUNC_trace_printk;
+static void (*bpf_tail_call)(void *ctx, void *map, int index) =
+	(void *) BPF_FUNC_tail_call;
+static unsigned long long (*bpf_get_smp_processor_id)(void) =
+	(void *) BPF_FUNC_get_smp_processor_id;
+static unsigned long long (*bpf_get_current_pid_tgid)(void) =
+	(void *) BPF_FUNC_get_current_pid_tgid;
+static unsigned long long (*bpf_get_current_uid_gid)(void) =
+	(void *) BPF_FUNC_get_current_uid_gid;
+static int (*bpf_get_current_comm)(void *buf, int buf_size) =
+	(void *) BPF_FUNC_get_current_comm;
+static unsigned long long (*bpf_perf_event_read)(void *map,
+						 unsigned long long flags) =
+	(void *) BPF_FUNC_perf_event_read;
+static int (*bpf_clone_redirect)(void *ctx, int ifindex, int flags) =
+	(void *) BPF_FUNC_clone_redirect;
+static int (*bpf_redirect)(int ifindex, int flags) =
+	(void *) BPF_FUNC_redirect;
+static int (*bpf_redirect_map)(void *map, int key, int flags) =
+	(void *) BPF_FUNC_redirect_map;
+static int (*bpf_perf_event_output)(void *ctx, void *map,
+				    unsigned long long flags, void *data,
+				    int size) =
+	(void *) BPF_FUNC_perf_event_output;
+static int (*bpf_get_stackid)(void *ctx, void *map, int flags) =
+	(void *) BPF_FUNC_get_stackid;
+static int (*bpf_probe_write_user)(void *dst, const void *src, int size) =
+	(void *) BPF_FUNC_probe_write_user;
+static int (*bpf_current_task_under_cgroup)(void *map, int index) =
+	(void *) BPF_FUNC_current_task_under_cgroup;
+static int (*bpf_skb_get_tunnel_key)(void *ctx, void *key, int size, int flags) =
+	(void *) BPF_FUNC_skb_get_tunnel_key;
+static int (*bpf_skb_set_tunnel_key)(void *ctx, void *key, int size, int flags) =
+	(void *) BPF_FUNC_skb_set_tunnel_key;
+static int (*bpf_skb_get_tunnel_opt)(void *ctx, void *md, int size) =
+	(void *) BPF_FUNC_skb_get_tunnel_opt;
+static int (*bpf_skb_set_tunnel_opt)(void *ctx, void *md, int size) =
+	(void *) BPF_FUNC_skb_set_tunnel_opt;
+static unsigned long long (*bpf_get_prandom_u32)(void) =
+	(void *) BPF_FUNC_get_prandom_u32;
+static int (*bpf_xdp_adjust_head)(void *ctx, int offset) =
+	(void *) BPF_FUNC_xdp_adjust_head;
+static int (*bpf_xdp_adjust_meta)(void *ctx, int offset) =
+	(void *) BPF_FUNC_xdp_adjust_meta;
+static int (*bpf_get_socket_cookie)(void *ctx) =
+	(void *) BPF_FUNC_get_socket_cookie;
+static int (*bpf_setsockopt)(void *ctx, int level, int optname, void *optval,
+			     int optlen) =
+	(void *) BPF_FUNC_setsockopt;
+static int (*bpf_getsockopt)(void *ctx, int level, int optname, void *optval,
+			     int optlen) =
+	(void *) BPF_FUNC_getsockopt;
+static int (*bpf_sock_ops_cb_flags_set)(void *ctx, int flags) =
+	(void *) BPF_FUNC_sock_ops_cb_flags_set;
+static int (*bpf_sk_redirect_map)(void *ctx, void *map, int key, int flags) =
+	(void *) BPF_FUNC_sk_redirect_map;
+static int (*bpf_sk_redirect_hash)(void *ctx, void *map, void *key, int flags) =
+	(void *) BPF_FUNC_sk_redirect_hash;
+static int (*bpf_sock_map_update)(void *map, void *key, void *value,
+				  unsigned long long flags) =
+	(void *) BPF_FUNC_sock_map_update;
+static int (*bpf_sock_hash_update)(void *map, void *key, void *value,
+				   unsigned long long flags) =
+	(void *) BPF_FUNC_sock_hash_update;
+static int (*bpf_perf_event_read_value)(void *map, unsigned long long flags,
+					void *buf, unsigned int buf_size) =
+	(void *) BPF_FUNC_perf_event_read_value;
+static int (*bpf_perf_prog_read_value)(void *ctx, void *buf,
+				       unsigned int buf_size) =
+	(void *) BPF_FUNC_perf_prog_read_value;
+static int (*bpf_override_return)(void *ctx, unsigned long rc) =
+	(void *) BPF_FUNC_override_return;
+static int (*bpf_msg_redirect_map)(void *ctx, void *map, int key, int flags) =
+	(void *) BPF_FUNC_msg_redirect_map;
+static int (*bpf_msg_redirect_hash)(void *ctx,
+				    void *map, void *key, int flags) =
+	(void *) BPF_FUNC_msg_redirect_hash;
+static int (*bpf_msg_apply_bytes)(void *ctx, int len) =
+	(void *) BPF_FUNC_msg_apply_bytes;
+static int (*bpf_msg_cork_bytes)(void *ctx, int len) =
+	(void *) BPF_FUNC_msg_cork_bytes;
+static int (*bpf_msg_pull_data)(void *ctx, int start, int end, int flags) =
+	(void *) BPF_FUNC_msg_pull_data;
+static int (*bpf_msg_push_data)(void *ctx, int start, int end, int flags) =
+	(void *) BPF_FUNC_msg_push_data;
+static int (*bpf_msg_pop_data)(void *ctx, int start, int cut, int flags) =
+	(void *) BPF_FUNC_msg_pop_data;
+static int (*bpf_bind)(void *ctx, void *addr, int addr_len) =
+	(void *) BPF_FUNC_bind;
+static int (*bpf_xdp_adjust_tail)(void *ctx, int offset) =
+	(void *) BPF_FUNC_xdp_adjust_tail;
+static int (*bpf_skb_get_xfrm_state)(void *ctx, int index, void *state,
+				     int size, int flags) =
+	(void *) BPF_FUNC_skb_get_xfrm_state;
+static int (*bpf_sk_select_reuseport)(void *ctx, void *map, void *key, __u32 flags) =
+	(void *) BPF_FUNC_sk_select_reuseport;
+static int (*bpf_get_stack)(void *ctx, void *buf, int size, int flags) =
+	(void *) BPF_FUNC_get_stack;
+static int (*bpf_fib_lookup)(void *ctx, struct bpf_fib_lookup *params,
+			     int plen, __u32 flags) =
+	(void *) BPF_FUNC_fib_lookup;
+static int (*bpf_lwt_push_encap)(void *ctx, unsigned int type, void *hdr,
+				 unsigned int len) =
+	(void *) BPF_FUNC_lwt_push_encap;
+static int (*bpf_lwt_seg6_store_bytes)(void *ctx, unsigned int offset,
+				       void *from, unsigned int len) =
+	(void *) BPF_FUNC_lwt_seg6_store_bytes;
+static int (*bpf_lwt_seg6_action)(void *ctx, unsigned int action, void *param,
+				  unsigned int param_len) =
+	(void *) BPF_FUNC_lwt_seg6_action;
+static int (*bpf_lwt_seg6_adjust_srh)(void *ctx, unsigned int offset,
+				      unsigned int len) =
+	(void *) BPF_FUNC_lwt_seg6_adjust_srh;
+static int (*bpf_rc_repeat)(void *ctx) =
+	(void *) BPF_FUNC_rc_repeat;
+static int (*bpf_rc_keydown)(void *ctx, unsigned int protocol,
+			     unsigned long long scancode, unsigned int toggle) =
+	(void *) BPF_FUNC_rc_keydown;
+static unsigned long long (*bpf_get_current_cgroup_id)(void) =
+	(void *) BPF_FUNC_get_current_cgroup_id;
+static void *(*bpf_get_local_storage)(void *map, unsigned long long flags) =
+	(void *) BPF_FUNC_get_local_storage;
+static unsigned long long (*bpf_skb_cgroup_id)(void *ctx) =
+	(void *) BPF_FUNC_skb_cgroup_id;
+static unsigned long long (*bpf_skb_ancestor_cgroup_id)(void *ctx, int level) =
+	(void *) BPF_FUNC_skb_ancestor_cgroup_id;
+static struct bpf_sock *(*bpf_sk_lookup_tcp)(void *ctx,
+					     struct bpf_sock_tuple *tuple,
+					     int size, unsigned long long netns_id,
+					     unsigned long long flags) =
+	(void *) BPF_FUNC_sk_lookup_tcp;
+static struct bpf_sock *(*bpf_skc_lookup_tcp)(void *ctx,
+					     struct bpf_sock_tuple *tuple,
+					     int size, unsigned long long netns_id,
+					     unsigned long long flags) =
+	(void *) BPF_FUNC_skc_lookup_tcp;
+static struct bpf_sock *(*bpf_sk_lookup_udp)(void *ctx,
+					     struct bpf_sock_tuple *tuple,
+					     int size, unsigned long long netns_id,
+					     unsigned long long flags) =
+	(void *) BPF_FUNC_sk_lookup_udp;
+static int (*bpf_sk_release)(struct bpf_sock *sk) =
+	(void *) BPF_FUNC_sk_release;
+static int (*bpf_skb_vlan_push)(void *ctx, __be16 vlan_proto, __u16 vlan_tci) =
+	(void *) BPF_FUNC_skb_vlan_push;
+static int (*bpf_skb_vlan_pop)(void *ctx) =
+	(void *) BPF_FUNC_skb_vlan_pop;
+static int (*bpf_rc_pointer_rel)(void *ctx, int rel_x, int rel_y) =
+	(void *) BPF_FUNC_rc_pointer_rel;
+static void (*bpf_spin_lock)(struct bpf_spin_lock *lock) =
+	(void *) BPF_FUNC_spin_lock;
+static void (*bpf_spin_unlock)(struct bpf_spin_lock *lock) =
+	(void *) BPF_FUNC_spin_unlock;
+static struct bpf_sock *(*bpf_sk_fullsock)(struct bpf_sock *sk) =
+	(void *) BPF_FUNC_sk_fullsock;
+static struct bpf_tcp_sock *(*bpf_tcp_sock)(struct bpf_sock *sk) =
+	(void *) BPF_FUNC_tcp_sock;
+static struct bpf_sock *(*bpf_get_listener_sock)(struct bpf_sock *sk) =
+	(void *) BPF_FUNC_get_listener_sock;
+static int (*bpf_skb_ecn_set_ce)(void *ctx) =
+	(void *) BPF_FUNC_skb_ecn_set_ce;
+static int (*bpf_tcp_check_syncookie)(struct bpf_sock *sk,
+	    void *ip, int ip_len, void *tcp, int tcp_len) =
+	(void *) BPF_FUNC_tcp_check_syncookie;
+static int (*bpf_sysctl_get_name)(void *ctx, char *buf,
+				  unsigned long long buf_len,
+				  unsigned long long flags) =
+	(void *) BPF_FUNC_sysctl_get_name;
+static int (*bpf_sysctl_get_current_value)(void *ctx, char *buf,
+					   unsigned long long buf_len) =
+	(void *) BPF_FUNC_sysctl_get_current_value;
+static int (*bpf_sysctl_get_new_value)(void *ctx, char *buf,
+				       unsigned long long buf_len) =
+	(void *) BPF_FUNC_sysctl_get_new_value;
+static int (*bpf_sysctl_set_new_value)(void *ctx, const char *buf,
+				       unsigned long long buf_len) =
+	(void *) BPF_FUNC_sysctl_set_new_value;
+static int (*bpf_strtol)(const char *buf, unsigned long long buf_len,
+			 unsigned long long flags, long *res) =
+	(void *) BPF_FUNC_strtol;
+static int (*bpf_strtoul)(const char *buf, unsigned long long buf_len,
+			  unsigned long long flags, unsigned long *res) =
+	(void *) BPF_FUNC_strtoul;
+static void *(*bpf_sk_storage_get)(void *map, struct bpf_sock *sk,
+				   void *value, __u64 flags) =
+	(void *) BPF_FUNC_sk_storage_get;
+static int (*bpf_sk_storage_delete)(void *map, struct bpf_sock *sk) =
+	(void *)BPF_FUNC_sk_storage_delete;
+static int (*bpf_send_signal)(unsigned sig) = (void *)BPF_FUNC_send_signal;
+static long long (*bpf_tcp_gen_syncookie)(struct bpf_sock *sk, void *ip,
+					  int ip_len, void *tcp, int tcp_len) =
+	(void *) BPF_FUNC_tcp_gen_syncookie;
+
+/* llvm builtin functions that eBPF C program may use to
+ * emit BPF_LD_ABS and BPF_LD_IND instructions
+ */
+struct sk_buff;
+unsigned long long load_byte(void *skb,
+			     unsigned long long off) asm("llvm.bpf.load.byte");
+unsigned long long load_half(void *skb,
+			     unsigned long long off) asm("llvm.bpf.load.half");
+unsigned long long load_word(void *skb,
+			     unsigned long long off) asm("llvm.bpf.load.word");
+
+/* a helper structure used by eBPF C program
+ * to describe map attributes to elf_bpf loader
+ */
+struct bpf_map_def {
+	unsigned int type;
+	unsigned int key_size;
+	unsigned int value_size;
+	unsigned int max_entries;
+	unsigned int map_flags;
+	unsigned int inner_map_idx;
+	unsigned int numa_node;
+};
+
+#define BPF_ANNOTATE_KV_PAIR(name, type_key, type_val)		\
+	struct ____btf_map_##name {				\
+		type_key key;					\
+		type_val value;					\
+	};							\
+	struct ____btf_map_##name				\
+	__attribute__ ((section(".maps." #name), used))		\
+		____btf_map_##name = { }
+
+static int (*bpf_skb_load_bytes)(void *ctx, int off, void *to, int len) =
+	(void *) BPF_FUNC_skb_load_bytes;
+static int (*bpf_skb_load_bytes_relative)(void *ctx, int off, void *to, int len, __u32 start_header) =
+	(void *) BPF_FUNC_skb_load_bytes_relative;
+static int (*bpf_skb_store_bytes)(void *ctx, int off, void *from, int len, int flags) =
+	(void *) BPF_FUNC_skb_store_bytes;
+static int (*bpf_l3_csum_replace)(void *ctx, int off, int from, int to, int flags) =
+	(void *) BPF_FUNC_l3_csum_replace;
+static int (*bpf_l4_csum_replace)(void *ctx, int off, int from, int to, int flags) =
+	(void *) BPF_FUNC_l4_csum_replace;
+static int (*bpf_csum_diff)(void *from, int from_size, void *to, int to_size, int seed) =
+	(void *) BPF_FUNC_csum_diff;
+static int (*bpf_skb_under_cgroup)(void *ctx, void *map, int index) =
+	(void *) BPF_FUNC_skb_under_cgroup;
+static int (*bpf_skb_change_head)(void *, int len, int flags) =
+	(void *) BPF_FUNC_skb_change_head;
+static int (*bpf_skb_pull_data)(void *, int len) =
+	(void *) BPF_FUNC_skb_pull_data;
+static unsigned int (*bpf_get_cgroup_classid)(void *ctx) =
+	(void *) BPF_FUNC_get_cgroup_classid;
+static unsigned int (*bpf_get_route_realm)(void *ctx) =
+	(void *) BPF_FUNC_get_route_realm;
+static int (*bpf_skb_change_proto)(void *ctx, __be16 proto, __u64 flags) =
+	(void *) BPF_FUNC_skb_change_proto;
+static int (*bpf_skb_change_type)(void *ctx, __u32 type) =
+	(void *) BPF_FUNC_skb_change_type;
+static unsigned int (*bpf_get_hash_recalc)(void *ctx) =
+	(void *) BPF_FUNC_get_hash_recalc;
+static unsigned long long (*bpf_get_current_task)(void) =
+	(void *) BPF_FUNC_get_current_task;
+static int (*bpf_skb_change_tail)(void *ctx, __u32 len, __u64 flags) =
+	(void *) BPF_FUNC_skb_change_tail;
+static long long (*bpf_csum_update)(void *ctx, __u32 csum) =
+	(void *) BPF_FUNC_csum_update;
+static void (*bpf_set_hash_invalid)(void *ctx) =
+	(void *) BPF_FUNC_set_hash_invalid;
+static int (*bpf_get_numa_node_id)(void) =
+	(void *) BPF_FUNC_get_numa_node_id;
+static int (*bpf_probe_read_str)(void *ctx, __u32 size,
+				 const void *unsafe_ptr) =
+	(void *) BPF_FUNC_probe_read_str;
+static unsigned int (*bpf_get_socket_uid)(void *ctx) =
+	(void *) BPF_FUNC_get_socket_uid;
+static unsigned int (*bpf_set_hash)(void *ctx, __u32 hash) =
+	(void *) BPF_FUNC_set_hash;
+static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
+				  unsigned long long flags) =
+	(void *) BPF_FUNC_skb_adjust_room;
+
+/* Scan the ARCH passed in from ARCH env variable (see Makefile) */
+#if defined(__TARGET_ARCH_x86)
+	#define bpf_target_x86
+	#define bpf_target_defined
+#elif defined(__TARGET_ARCH_s390)
+	#define bpf_target_s390
+	#define bpf_target_defined
+#elif defined(__TARGET_ARCH_arm)
+	#define bpf_target_arm
+	#define bpf_target_defined
+#elif defined(__TARGET_ARCH_arm64)
+	#define bpf_target_arm64
+	#define bpf_target_defined
+#elif defined(__TARGET_ARCH_mips)
+	#define bpf_target_mips
+	#define bpf_target_defined
+#elif defined(__TARGET_ARCH_powerpc)
+	#define bpf_target_powerpc
+	#define bpf_target_defined
+#elif defined(__TARGET_ARCH_sparc)
+	#define bpf_target_sparc
+	#define bpf_target_defined
+#else
+	#undef bpf_target_defined
+#endif
+
+/* Fall back to what the compiler says */
+#ifndef bpf_target_defined
+#if defined(__x86_64__)
+	#define bpf_target_x86
+#elif defined(__s390__)
+	#define bpf_target_s390
+#elif defined(__arm__)
+	#define bpf_target_arm
+#elif defined(__aarch64__)
+	#define bpf_target_arm64
+#elif defined(__mips__)
+	#define bpf_target_mips
+#elif defined(__powerpc__)
+	#define bpf_target_powerpc
+#elif defined(__sparc__)
+	#define bpf_target_sparc
+#endif
+#endif
+
+#if defined(bpf_target_x86)
+
+#ifdef __KERNEL__
+#define PT_REGS_PARM1(x) ((x)->di)
+#define PT_REGS_PARM2(x) ((x)->si)
+#define PT_REGS_PARM3(x) ((x)->dx)
+#define PT_REGS_PARM4(x) ((x)->cx)
+#define PT_REGS_PARM5(x) ((x)->r8)
+#define PT_REGS_RET(x) ((x)->sp)
+#define PT_REGS_FP(x) ((x)->bp)
+#define PT_REGS_RC(x) ((x)->ax)
+#define PT_REGS_SP(x) ((x)->sp)
+#define PT_REGS_IP(x) ((x)->ip)
+#else
+#ifdef __i386__
+/* i386 kernel is built with -mregparm=3 */
+#define PT_REGS_PARM1(x) ((x)->eax)
+#define PT_REGS_PARM2(x) ((x)->edx)
+#define PT_REGS_PARM3(x) ((x)->ecx)
+#define PT_REGS_PARM4(x) 0
+#define PT_REGS_PARM5(x) 0
+#define PT_REGS_RET(x) ((x)->esp)
+#define PT_REGS_FP(x) ((x)->ebp)
+#define PT_REGS_RC(x) ((x)->eax)
+#define PT_REGS_SP(x) ((x)->esp)
+#define PT_REGS_IP(x) ((x)->eip)
+#else
+#define PT_REGS_PARM1(x) ((x)->rdi)
+#define PT_REGS_PARM2(x) ((x)->rsi)
+#define PT_REGS_PARM3(x) ((x)->rdx)
+#define PT_REGS_PARM4(x) ((x)->rcx)
+#define PT_REGS_PARM5(x) ((x)->r8)
+#define PT_REGS_RET(x) ((x)->rsp)
+#define PT_REGS_FP(x) ((x)->rbp)
+#define PT_REGS_RC(x) ((x)->rax)
+#define PT_REGS_SP(x) ((x)->rsp)
+#define PT_REGS_IP(x) ((x)->rip)
+#endif
+#endif
+
+#elif defined(bpf_target_s390)
+
+/* s390 provides user_pt_regs instead of struct pt_regs to userspace */
+struct pt_regs;
+#define PT_REGS_S390 const volatile user_pt_regs
+#define PT_REGS_PARM1(x) (((PT_REGS_S390 *)(x))->gprs[2])
+#define PT_REGS_PARM2(x) (((PT_REGS_S390 *)(x))->gprs[3])
+#define PT_REGS_PARM3(x) (((PT_REGS_S390 *)(x))->gprs[4])
+#define PT_REGS_PARM4(x) (((PT_REGS_S390 *)(x))->gprs[5])
+#define PT_REGS_PARM5(x) (((PT_REGS_S390 *)(x))->gprs[6])
+#define PT_REGS_RET(x) (((PT_REGS_S390 *)(x))->gprs[14])
+/* Works only with CONFIG_FRAME_POINTER */
+#define PT_REGS_FP(x) (((PT_REGS_S390 *)(x))->gprs[11])
+#define PT_REGS_RC(x) (((PT_REGS_S390 *)(x))->gprs[2])
+#define PT_REGS_SP(x) (((PT_REGS_S390 *)(x))->gprs[15])
+#define PT_REGS_IP(x) (((PT_REGS_S390 *)(x))->psw.addr)
+
+#elif defined(bpf_target_arm)
+
+#define PT_REGS_PARM1(x) ((x)->uregs[0])
+#define PT_REGS_PARM2(x) ((x)->uregs[1])
+#define PT_REGS_PARM3(x) ((x)->uregs[2])
+#define PT_REGS_PARM4(x) ((x)->uregs[3])
+#define PT_REGS_PARM5(x) ((x)->uregs[4])
+#define PT_REGS_RET(x) ((x)->uregs[14])
+#define PT_REGS_FP(x) ((x)->uregs[11]) /* Works only with CONFIG_FRAME_POINTER */
+#define PT_REGS_RC(x) ((x)->uregs[0])
+#define PT_REGS_SP(x) ((x)->uregs[13])
+#define PT_REGS_IP(x) ((x)->uregs[12])
+
+#elif defined(bpf_target_arm64)
+
+/* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */
+struct pt_regs;
+#define PT_REGS_ARM64 const volatile struct user_pt_regs
+#define PT_REGS_PARM1(x) (((PT_REGS_ARM64 *)(x))->regs[0])
+#define PT_REGS_PARM2(x) (((PT_REGS_ARM64 *)(x))->regs[1])
+#define PT_REGS_PARM3(x) (((PT_REGS_ARM64 *)(x))->regs[2])
+#define PT_REGS_PARM4(x) (((PT_REGS_ARM64 *)(x))->regs[3])
+#define PT_REGS_PARM5(x) (((PT_REGS_ARM64 *)(x))->regs[4])
+#define PT_REGS_RET(x) (((PT_REGS_ARM64 *)(x))->regs[30])
+/* Works only with CONFIG_FRAME_POINTER */
+#define PT_REGS_FP(x) (((PT_REGS_ARM64 *)(x))->regs[29])
+#define PT_REGS_RC(x) (((PT_REGS_ARM64 *)(x))->regs[0])
+#define PT_REGS_SP(x) (((PT_REGS_ARM64 *)(x))->sp)
+#define PT_REGS_IP(x) (((PT_REGS_ARM64 *)(x))->pc)
+
+#elif defined(bpf_target_mips)
+
+#define PT_REGS_PARM1(x) ((x)->regs[4])
+#define PT_REGS_PARM2(x) ((x)->regs[5])
+#define PT_REGS_PARM3(x) ((x)->regs[6])
+#define PT_REGS_PARM4(x) ((x)->regs[7])
+#define PT_REGS_PARM5(x) ((x)->regs[8])
+#define PT_REGS_RET(x) ((x)->regs[31])
+#define PT_REGS_FP(x) ((x)->regs[30]) /* Works only with CONFIG_FRAME_POINTER */
+#define PT_REGS_RC(x) ((x)->regs[1])
+#define PT_REGS_SP(x) ((x)->regs[29])
+#define PT_REGS_IP(x) ((x)->cp0_epc)
+
+#elif defined(bpf_target_powerpc)
+
+#define PT_REGS_PARM1(x) ((x)->gpr[3])
+#define PT_REGS_PARM2(x) ((x)->gpr[4])
+#define PT_REGS_PARM3(x) ((x)->gpr[5])
+#define PT_REGS_PARM4(x) ((x)->gpr[6])
+#define PT_REGS_PARM5(x) ((x)->gpr[7])
+#define PT_REGS_RC(x) ((x)->gpr[3])
+#define PT_REGS_SP(x) ((x)->sp)
+#define PT_REGS_IP(x) ((x)->nip)
+
+#elif defined(bpf_target_sparc)
+
+#define PT_REGS_PARM1(x) ((x)->u_regs[UREG_I0])
+#define PT_REGS_PARM2(x) ((x)->u_regs[UREG_I1])
+#define PT_REGS_PARM3(x) ((x)->u_regs[UREG_I2])
+#define PT_REGS_PARM4(x) ((x)->u_regs[UREG_I3])
+#define PT_REGS_PARM5(x) ((x)->u_regs[UREG_I4])
+#define PT_REGS_RET(x) ((x)->u_regs[UREG_I7])
+#define PT_REGS_RC(x) ((x)->u_regs[UREG_I0])
+#define PT_REGS_SP(x) ((x)->u_regs[UREG_FP])
+
+/* Should this also be a bpf_target check for the sparc case? */
+#if defined(__arch64__)
+#define PT_REGS_IP(x) ((x)->tpc)
+#else
+#define PT_REGS_IP(x) ((x)->pc)
+#endif
+
+#endif
+
+#if defined(bpf_target_powerpc)
+#define BPF_KPROBE_READ_RET_IP(ip, ctx)		({ (ip) = (ctx)->link; })
+#define BPF_KRETPROBE_READ_RET_IP		BPF_KPROBE_READ_RET_IP
+#elif defined(bpf_target_sparc)
+#define BPF_KPROBE_READ_RET_IP(ip, ctx)		({ (ip) = PT_REGS_RET(ctx); })
+#define BPF_KRETPROBE_READ_RET_IP		BPF_KPROBE_READ_RET_IP
+#else
+#define BPF_KPROBE_READ_RET_IP(ip, ctx)		({				\
+		bpf_probe_read(&(ip), sizeof(ip), (void *)PT_REGS_RET(ctx)); })
+#define BPF_KRETPROBE_READ_RET_IP(ip, ctx)	({				\
+		bpf_probe_read(&(ip), sizeof(ip),				\
+				(void *)(PT_REGS_FP(ctx) + sizeof(ip))); })
+#endif
+
+/*
+ * BPF_CORE_READ abstracts away bpf_probe_read() call and captures offset
+ * relocation for source address using __builtin_preserve_access_index()
+ * built-in, provided by Clang.
+ *
+ * __builtin_preserve_access_index() takes as an argument an expression of
+ * taking an address of a field within struct/union. It makes compiler emit
+ * a relocation, which records BTF type ID describing root struct/union and an
+ * accessor string which describes exact embedded field that was used to take
+ * an address. See detailed description of this relocation format and
+ * semantics in comments to struct bpf_offset_reloc in libbpf_internal.h.
+ *
+ * This relocation allows libbpf to adjust BPF instruction to use correct
+ * actual field offset, based on target kernel BTF type that matches original
+ * (local) BTF, used to record relocation.
+ */
+#define BPF_CORE_READ(dst, src)						\
+	bpf_probe_read((dst), sizeof(*(src)),				\
+		       __builtin_preserve_access_index(src))
+
+#endif
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH bpf-next 3/6] selftests/bpf: switch test to use libbpf's helpers
  2019-09-30 18:58 [PATCH bpf-next 0/6] Move bpf_helpers and add BPF_CORE_READ macros Andrii Nakryiko
  2019-09-30 18:58 ` [PATCH bpf-next 1/6] selftests/bpf: undo GCC-specific bpf_helpers.h changes Andrii Nakryiko
  2019-09-30 18:58 ` [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf Andrii Nakryiko
@ 2019-09-30 18:58 ` Andrii Nakryiko
  2019-09-30 18:58 ` [PATCH bpf-next 4/6] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers Andrii Nakryiko
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 47+ messages in thread
From: Andrii Nakryiko @ 2019-09-30 18:58 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Adjust selftest to use bpf_helpers.h and bpf_endian.h from libbpf.
Delete bpf_helpers.h/bpf_endian.h, that are still part of selftests.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/testing/selftests/bpf/Makefile      |   2 +-
 tools/testing/selftests/bpf/bpf_endian.h  |  72 ---
 tools/testing/selftests/bpf/bpf_helpers.h | 527 ----------------------
 3 files changed, 1 insertion(+), 600 deletions(-)
 delete mode 100644 tools/testing/selftests/bpf/bpf_endian.h
 delete mode 100644 tools/testing/selftests/bpf/bpf_helpers.h

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 6889c19a628c..b00a5d8046c7 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -148,7 +148,7 @@ $(shell $(1) -v -E - </dev/null 2>&1 \
 endef
 CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG))
 BPF_CFLAGS = -I. -I./include/uapi -I../../../include/uapi \
-	     -I$(OUTPUT)/../usr/include -D__TARGET_ARCH_$(SRCARCH)
+	     -I$(BPFDIR) -I$(OUTPUT)/../usr/include -D__TARGET_ARCH_$(SRCARCH)
 
 CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \
 	       -Wno-compare-distinct-pointer-types
diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
deleted file mode 100644
index fbe28008450f..000000000000
--- a/tools/testing/selftests/bpf/bpf_endian.h
+++ /dev/null
@@ -1,72 +0,0 @@
-/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
-#ifndef __BPF_ENDIAN__
-#define __BPF_ENDIAN__
-
-#include <linux/stddef.h>
-#include <linux/swab.h>
-
-/* LLVM's BPF target selects the endianness of the CPU
- * it compiles on, or the user specifies (bpfel/bpfeb),
- * respectively. The used __BYTE_ORDER__ is defined by
- * the compiler, we cannot rely on __BYTE_ORDER from
- * libc headers, since it doesn't reflect the actual
- * requested byte order.
- *
- * Note, LLVM's BPF target has different __builtin_bswapX()
- * semantics. It does map to BPF_ALU | BPF_END | BPF_TO_BE
- * in bpfel and bpfeb case, which means below, that we map
- * to cpu_to_be16(). We could use it unconditionally in BPF
- * case, but better not rely on it, so that this header here
- * can be used from application and BPF program side, which
- * use different targets.
- */
-#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
-# define __bpf_ntohs(x)			__builtin_bswap16(x)
-# define __bpf_htons(x)			__builtin_bswap16(x)
-# define __bpf_constant_ntohs(x)	___constant_swab16(x)
-# define __bpf_constant_htons(x)	___constant_swab16(x)
-# define __bpf_ntohl(x)			__builtin_bswap32(x)
-# define __bpf_htonl(x)			__builtin_bswap32(x)
-# define __bpf_constant_ntohl(x)	___constant_swab32(x)
-# define __bpf_constant_htonl(x)	___constant_swab32(x)
-# define __bpf_be64_to_cpu(x)		__builtin_bswap64(x)
-# define __bpf_cpu_to_be64(x)		__builtin_bswap64(x)
-# define __bpf_constant_be64_to_cpu(x)	___constant_swab64(x)
-# define __bpf_constant_cpu_to_be64(x)	___constant_swab64(x)
-#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
-# define __bpf_ntohs(x)			(x)
-# define __bpf_htons(x)			(x)
-# define __bpf_constant_ntohs(x)	(x)
-# define __bpf_constant_htons(x)	(x)
-# define __bpf_ntohl(x)			(x)
-# define __bpf_htonl(x)			(x)
-# define __bpf_constant_ntohl(x)	(x)
-# define __bpf_constant_htonl(x)	(x)
-# define __bpf_be64_to_cpu(x)		(x)
-# define __bpf_cpu_to_be64(x)		(x)
-# define __bpf_constant_be64_to_cpu(x)  (x)
-# define __bpf_constant_cpu_to_be64(x)  (x)
-#else
-# error "Fix your compiler's __BYTE_ORDER__?!"
-#endif
-
-#define bpf_htons(x)				\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_htons(x) : __bpf_htons(x))
-#define bpf_ntohs(x)				\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_ntohs(x) : __bpf_ntohs(x))
-#define bpf_htonl(x)				\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_htonl(x) : __bpf_htonl(x))
-#define bpf_ntohl(x)				\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_ntohl(x) : __bpf_ntohl(x))
-#define bpf_cpu_to_be64(x)			\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_cpu_to_be64(x) : __bpf_cpu_to_be64(x))
-#define bpf_be64_to_cpu(x)			\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_be64_to_cpu(x) : __bpf_be64_to_cpu(x))
-
-#endif /* __BPF_ENDIAN__ */
diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
deleted file mode 100644
index a1d9b97b8e15..000000000000
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ /dev/null
@@ -1,527 +0,0 @@
-/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
-#ifndef __BPF_HELPERS__
-#define __BPF_HELPERS__
-
-#define __uint(name, val) int (*name)[val]
-#define __type(name, val) val *name
-
-/* helper macro to print out debug messages */
-#define bpf_printk(fmt, ...)				\
-({							\
-	char ____fmt[] = fmt;				\
-	bpf_trace_printk(____fmt, sizeof(____fmt),	\
-			 ##__VA_ARGS__);		\
-})
-
-/* helper macro to place programs, maps, license in
- * different sections in elf_bpf file. Section names
- * are interpreted by elf_bpf loader
- */
-#define SEC(NAME) __attribute__((section(NAME), used))
-
-/* helper functions called from eBPF programs written in C */
-static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
-	(void *) BPF_FUNC_map_lookup_elem;
-static int (*bpf_map_update_elem)(void *map, const void *key, const void *value,
-				  unsigned long long flags) =
-	(void *) BPF_FUNC_map_update_elem;
-static int (*bpf_map_delete_elem)(void *map, const void *key) =
-	(void *) BPF_FUNC_map_delete_elem;
-static int (*bpf_map_push_elem)(void *map, const void *value,
-				unsigned long long flags) =
-	(void *) BPF_FUNC_map_push_elem;
-static int (*bpf_map_pop_elem)(void *map, void *value) =
-	(void *) BPF_FUNC_map_pop_elem;
-static int (*bpf_map_peek_elem)(void *map, void *value) =
-	(void *) BPF_FUNC_map_peek_elem;
-static int (*bpf_probe_read)(void *dst, int size, const void *unsafe_ptr) =
-	(void *) BPF_FUNC_probe_read;
-static unsigned long long (*bpf_ktime_get_ns)(void) =
-	(void *) BPF_FUNC_ktime_get_ns;
-static int (*bpf_trace_printk)(const char *fmt, int fmt_size, ...) =
-	(void *) BPF_FUNC_trace_printk;
-static void (*bpf_tail_call)(void *ctx, void *map, int index) =
-	(void *) BPF_FUNC_tail_call;
-static unsigned long long (*bpf_get_smp_processor_id)(void) =
-	(void *) BPF_FUNC_get_smp_processor_id;
-static unsigned long long (*bpf_get_current_pid_tgid)(void) =
-	(void *) BPF_FUNC_get_current_pid_tgid;
-static unsigned long long (*bpf_get_current_uid_gid)(void) =
-	(void *) BPF_FUNC_get_current_uid_gid;
-static int (*bpf_get_current_comm)(void *buf, int buf_size) =
-	(void *) BPF_FUNC_get_current_comm;
-static unsigned long long (*bpf_perf_event_read)(void *map,
-						 unsigned long long flags) =
-	(void *) BPF_FUNC_perf_event_read;
-static int (*bpf_clone_redirect)(void *ctx, int ifindex, int flags) =
-	(void *) BPF_FUNC_clone_redirect;
-static int (*bpf_redirect)(int ifindex, int flags) =
-	(void *) BPF_FUNC_redirect;
-static int (*bpf_redirect_map)(void *map, int key, int flags) =
-	(void *) BPF_FUNC_redirect_map;
-static int (*bpf_perf_event_output)(void *ctx, void *map,
-				    unsigned long long flags, void *data,
-				    int size) =
-	(void *) BPF_FUNC_perf_event_output;
-static int (*bpf_get_stackid)(void *ctx, void *map, int flags) =
-	(void *) BPF_FUNC_get_stackid;
-static int (*bpf_probe_write_user)(void *dst, const void *src, int size) =
-	(void *) BPF_FUNC_probe_write_user;
-static int (*bpf_current_task_under_cgroup)(void *map, int index) =
-	(void *) BPF_FUNC_current_task_under_cgroup;
-static int (*bpf_skb_get_tunnel_key)(void *ctx, void *key, int size, int flags) =
-	(void *) BPF_FUNC_skb_get_tunnel_key;
-static int (*bpf_skb_set_tunnel_key)(void *ctx, void *key, int size, int flags) =
-	(void *) BPF_FUNC_skb_set_tunnel_key;
-static int (*bpf_skb_get_tunnel_opt)(void *ctx, void *md, int size) =
-	(void *) BPF_FUNC_skb_get_tunnel_opt;
-static int (*bpf_skb_set_tunnel_opt)(void *ctx, void *md, int size) =
-	(void *) BPF_FUNC_skb_set_tunnel_opt;
-static unsigned long long (*bpf_get_prandom_u32)(void) =
-	(void *) BPF_FUNC_get_prandom_u32;
-static int (*bpf_xdp_adjust_head)(void *ctx, int offset) =
-	(void *) BPF_FUNC_xdp_adjust_head;
-static int (*bpf_xdp_adjust_meta)(void *ctx, int offset) =
-	(void *) BPF_FUNC_xdp_adjust_meta;
-static int (*bpf_get_socket_cookie)(void *ctx) =
-	(void *) BPF_FUNC_get_socket_cookie;
-static int (*bpf_setsockopt)(void *ctx, int level, int optname, void *optval,
-			     int optlen) =
-	(void *) BPF_FUNC_setsockopt;
-static int (*bpf_getsockopt)(void *ctx, int level, int optname, void *optval,
-			     int optlen) =
-	(void *) BPF_FUNC_getsockopt;
-static int (*bpf_sock_ops_cb_flags_set)(void *ctx, int flags) =
-	(void *) BPF_FUNC_sock_ops_cb_flags_set;
-static int (*bpf_sk_redirect_map)(void *ctx, void *map, int key, int flags) =
-	(void *) BPF_FUNC_sk_redirect_map;
-static int (*bpf_sk_redirect_hash)(void *ctx, void *map, void *key, int flags) =
-	(void *) BPF_FUNC_sk_redirect_hash;
-static int (*bpf_sock_map_update)(void *map, void *key, void *value,
-				  unsigned long long flags) =
-	(void *) BPF_FUNC_sock_map_update;
-static int (*bpf_sock_hash_update)(void *map, void *key, void *value,
-				   unsigned long long flags) =
-	(void *) BPF_FUNC_sock_hash_update;
-static int (*bpf_perf_event_read_value)(void *map, unsigned long long flags,
-					void *buf, unsigned int buf_size) =
-	(void *) BPF_FUNC_perf_event_read_value;
-static int (*bpf_perf_prog_read_value)(void *ctx, void *buf,
-				       unsigned int buf_size) =
-	(void *) BPF_FUNC_perf_prog_read_value;
-static int (*bpf_override_return)(void *ctx, unsigned long rc) =
-	(void *) BPF_FUNC_override_return;
-static int (*bpf_msg_redirect_map)(void *ctx, void *map, int key, int flags) =
-	(void *) BPF_FUNC_msg_redirect_map;
-static int (*bpf_msg_redirect_hash)(void *ctx,
-				    void *map, void *key, int flags) =
-	(void *) BPF_FUNC_msg_redirect_hash;
-static int (*bpf_msg_apply_bytes)(void *ctx, int len) =
-	(void *) BPF_FUNC_msg_apply_bytes;
-static int (*bpf_msg_cork_bytes)(void *ctx, int len) =
-	(void *) BPF_FUNC_msg_cork_bytes;
-static int (*bpf_msg_pull_data)(void *ctx, int start, int end, int flags) =
-	(void *) BPF_FUNC_msg_pull_data;
-static int (*bpf_msg_push_data)(void *ctx, int start, int end, int flags) =
-	(void *) BPF_FUNC_msg_push_data;
-static int (*bpf_msg_pop_data)(void *ctx, int start, int cut, int flags) =
-	(void *) BPF_FUNC_msg_pop_data;
-static int (*bpf_bind)(void *ctx, void *addr, int addr_len) =
-	(void *) BPF_FUNC_bind;
-static int (*bpf_xdp_adjust_tail)(void *ctx, int offset) =
-	(void *) BPF_FUNC_xdp_adjust_tail;
-static int (*bpf_skb_get_xfrm_state)(void *ctx, int index, void *state,
-				     int size, int flags) =
-	(void *) BPF_FUNC_skb_get_xfrm_state;
-static int (*bpf_sk_select_reuseport)(void *ctx, void *map, void *key, __u32 flags) =
-	(void *) BPF_FUNC_sk_select_reuseport;
-static int (*bpf_get_stack)(void *ctx, void *buf, int size, int flags) =
-	(void *) BPF_FUNC_get_stack;
-static int (*bpf_fib_lookup)(void *ctx, struct bpf_fib_lookup *params,
-			     int plen, __u32 flags) =
-	(void *) BPF_FUNC_fib_lookup;
-static int (*bpf_lwt_push_encap)(void *ctx, unsigned int type, void *hdr,
-				 unsigned int len) =
-	(void *) BPF_FUNC_lwt_push_encap;
-static int (*bpf_lwt_seg6_store_bytes)(void *ctx, unsigned int offset,
-				       void *from, unsigned int len) =
-	(void *) BPF_FUNC_lwt_seg6_store_bytes;
-static int (*bpf_lwt_seg6_action)(void *ctx, unsigned int action, void *param,
-				  unsigned int param_len) =
-	(void *) BPF_FUNC_lwt_seg6_action;
-static int (*bpf_lwt_seg6_adjust_srh)(void *ctx, unsigned int offset,
-				      unsigned int len) =
-	(void *) BPF_FUNC_lwt_seg6_adjust_srh;
-static int (*bpf_rc_repeat)(void *ctx) =
-	(void *) BPF_FUNC_rc_repeat;
-static int (*bpf_rc_keydown)(void *ctx, unsigned int protocol,
-			     unsigned long long scancode, unsigned int toggle) =
-	(void *) BPF_FUNC_rc_keydown;
-static unsigned long long (*bpf_get_current_cgroup_id)(void) =
-	(void *) BPF_FUNC_get_current_cgroup_id;
-static void *(*bpf_get_local_storage)(void *map, unsigned long long flags) =
-	(void *) BPF_FUNC_get_local_storage;
-static unsigned long long (*bpf_skb_cgroup_id)(void *ctx) =
-	(void *) BPF_FUNC_skb_cgroup_id;
-static unsigned long long (*bpf_skb_ancestor_cgroup_id)(void *ctx, int level) =
-	(void *) BPF_FUNC_skb_ancestor_cgroup_id;
-static struct bpf_sock *(*bpf_sk_lookup_tcp)(void *ctx,
-					     struct bpf_sock_tuple *tuple,
-					     int size, unsigned long long netns_id,
-					     unsigned long long flags) =
-	(void *) BPF_FUNC_sk_lookup_tcp;
-static struct bpf_sock *(*bpf_skc_lookup_tcp)(void *ctx,
-					     struct bpf_sock_tuple *tuple,
-					     int size, unsigned long long netns_id,
-					     unsigned long long flags) =
-	(void *) BPF_FUNC_skc_lookup_tcp;
-static struct bpf_sock *(*bpf_sk_lookup_udp)(void *ctx,
-					     struct bpf_sock_tuple *tuple,
-					     int size, unsigned long long netns_id,
-					     unsigned long long flags) =
-	(void *) BPF_FUNC_sk_lookup_udp;
-static int (*bpf_sk_release)(struct bpf_sock *sk) =
-	(void *) BPF_FUNC_sk_release;
-static int (*bpf_skb_vlan_push)(void *ctx, __be16 vlan_proto, __u16 vlan_tci) =
-	(void *) BPF_FUNC_skb_vlan_push;
-static int (*bpf_skb_vlan_pop)(void *ctx) =
-	(void *) BPF_FUNC_skb_vlan_pop;
-static int (*bpf_rc_pointer_rel)(void *ctx, int rel_x, int rel_y) =
-	(void *) BPF_FUNC_rc_pointer_rel;
-static void (*bpf_spin_lock)(struct bpf_spin_lock *lock) =
-	(void *) BPF_FUNC_spin_lock;
-static void (*bpf_spin_unlock)(struct bpf_spin_lock *lock) =
-	(void *) BPF_FUNC_spin_unlock;
-static struct bpf_sock *(*bpf_sk_fullsock)(struct bpf_sock *sk) =
-	(void *) BPF_FUNC_sk_fullsock;
-static struct bpf_tcp_sock *(*bpf_tcp_sock)(struct bpf_sock *sk) =
-	(void *) BPF_FUNC_tcp_sock;
-static struct bpf_sock *(*bpf_get_listener_sock)(struct bpf_sock *sk) =
-	(void *) BPF_FUNC_get_listener_sock;
-static int (*bpf_skb_ecn_set_ce)(void *ctx) =
-	(void *) BPF_FUNC_skb_ecn_set_ce;
-static int (*bpf_tcp_check_syncookie)(struct bpf_sock *sk,
-	    void *ip, int ip_len, void *tcp, int tcp_len) =
-	(void *) BPF_FUNC_tcp_check_syncookie;
-static int (*bpf_sysctl_get_name)(void *ctx, char *buf,
-				  unsigned long long buf_len,
-				  unsigned long long flags) =
-	(void *) BPF_FUNC_sysctl_get_name;
-static int (*bpf_sysctl_get_current_value)(void *ctx, char *buf,
-					   unsigned long long buf_len) =
-	(void *) BPF_FUNC_sysctl_get_current_value;
-static int (*bpf_sysctl_get_new_value)(void *ctx, char *buf,
-				       unsigned long long buf_len) =
-	(void *) BPF_FUNC_sysctl_get_new_value;
-static int (*bpf_sysctl_set_new_value)(void *ctx, const char *buf,
-				       unsigned long long buf_len) =
-	(void *) BPF_FUNC_sysctl_set_new_value;
-static int (*bpf_strtol)(const char *buf, unsigned long long buf_len,
-			 unsigned long long flags, long *res) =
-	(void *) BPF_FUNC_strtol;
-static int (*bpf_strtoul)(const char *buf, unsigned long long buf_len,
-			  unsigned long long flags, unsigned long *res) =
-	(void *) BPF_FUNC_strtoul;
-static void *(*bpf_sk_storage_get)(void *map, struct bpf_sock *sk,
-				   void *value, __u64 flags) =
-	(void *) BPF_FUNC_sk_storage_get;
-static int (*bpf_sk_storage_delete)(void *map, struct bpf_sock *sk) =
-	(void *)BPF_FUNC_sk_storage_delete;
-static int (*bpf_send_signal)(unsigned sig) = (void *)BPF_FUNC_send_signal;
-static long long (*bpf_tcp_gen_syncookie)(struct bpf_sock *sk, void *ip,
-					  int ip_len, void *tcp, int tcp_len) =
-	(void *) BPF_FUNC_tcp_gen_syncookie;
-
-/* llvm builtin functions that eBPF C program may use to
- * emit BPF_LD_ABS and BPF_LD_IND instructions
- */
-struct sk_buff;
-unsigned long long load_byte(void *skb,
-			     unsigned long long off) asm("llvm.bpf.load.byte");
-unsigned long long load_half(void *skb,
-			     unsigned long long off) asm("llvm.bpf.load.half");
-unsigned long long load_word(void *skb,
-			     unsigned long long off) asm("llvm.bpf.load.word");
-
-/* a helper structure used by eBPF C program
- * to describe map attributes to elf_bpf loader
- */
-struct bpf_map_def {
-	unsigned int type;
-	unsigned int key_size;
-	unsigned int value_size;
-	unsigned int max_entries;
-	unsigned int map_flags;
-	unsigned int inner_map_idx;
-	unsigned int numa_node;
-};
-
-#define BPF_ANNOTATE_KV_PAIR(name, type_key, type_val)		\
-	struct ____btf_map_##name {				\
-		type_key key;					\
-		type_val value;					\
-	};							\
-	struct ____btf_map_##name				\
-	__attribute__ ((section(".maps." #name), used))		\
-		____btf_map_##name = { }
-
-static int (*bpf_skb_load_bytes)(void *ctx, int off, void *to, int len) =
-	(void *) BPF_FUNC_skb_load_bytes;
-static int (*bpf_skb_load_bytes_relative)(void *ctx, int off, void *to, int len, __u32 start_header) =
-	(void *) BPF_FUNC_skb_load_bytes_relative;
-static int (*bpf_skb_store_bytes)(void *ctx, int off, void *from, int len, int flags) =
-	(void *) BPF_FUNC_skb_store_bytes;
-static int (*bpf_l3_csum_replace)(void *ctx, int off, int from, int to, int flags) =
-	(void *) BPF_FUNC_l3_csum_replace;
-static int (*bpf_l4_csum_replace)(void *ctx, int off, int from, int to, int flags) =
-	(void *) BPF_FUNC_l4_csum_replace;
-static int (*bpf_csum_diff)(void *from, int from_size, void *to, int to_size, int seed) =
-	(void *) BPF_FUNC_csum_diff;
-static int (*bpf_skb_under_cgroup)(void *ctx, void *map, int index) =
-	(void *) BPF_FUNC_skb_under_cgroup;
-static int (*bpf_skb_change_head)(void *, int len, int flags) =
-	(void *) BPF_FUNC_skb_change_head;
-static int (*bpf_skb_pull_data)(void *, int len) =
-	(void *) BPF_FUNC_skb_pull_data;
-static unsigned int (*bpf_get_cgroup_classid)(void *ctx) =
-	(void *) BPF_FUNC_get_cgroup_classid;
-static unsigned int (*bpf_get_route_realm)(void *ctx) =
-	(void *) BPF_FUNC_get_route_realm;
-static int (*bpf_skb_change_proto)(void *ctx, __be16 proto, __u64 flags) =
-	(void *) BPF_FUNC_skb_change_proto;
-static int (*bpf_skb_change_type)(void *ctx, __u32 type) =
-	(void *) BPF_FUNC_skb_change_type;
-static unsigned int (*bpf_get_hash_recalc)(void *ctx) =
-	(void *) BPF_FUNC_get_hash_recalc;
-static unsigned long long (*bpf_get_current_task)(void) =
-	(void *) BPF_FUNC_get_current_task;
-static int (*bpf_skb_change_tail)(void *ctx, __u32 len, __u64 flags) =
-	(void *) BPF_FUNC_skb_change_tail;
-static long long (*bpf_csum_update)(void *ctx, __u32 csum) =
-	(void *) BPF_FUNC_csum_update;
-static void (*bpf_set_hash_invalid)(void *ctx) =
-	(void *) BPF_FUNC_set_hash_invalid;
-static int (*bpf_get_numa_node_id)(void) =
-	(void *) BPF_FUNC_get_numa_node_id;
-static int (*bpf_probe_read_str)(void *ctx, __u32 size,
-				 const void *unsafe_ptr) =
-	(void *) BPF_FUNC_probe_read_str;
-static unsigned int (*bpf_get_socket_uid)(void *ctx) =
-	(void *) BPF_FUNC_get_socket_uid;
-static unsigned int (*bpf_set_hash)(void *ctx, __u32 hash) =
-	(void *) BPF_FUNC_set_hash;
-static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
-				  unsigned long long flags) =
-	(void *) BPF_FUNC_skb_adjust_room;
-
-/* Scan the ARCH passed in from ARCH env variable (see Makefile) */
-#if defined(__TARGET_ARCH_x86)
-	#define bpf_target_x86
-	#define bpf_target_defined
-#elif defined(__TARGET_ARCH_s390)
-	#define bpf_target_s390
-	#define bpf_target_defined
-#elif defined(__TARGET_ARCH_arm)
-	#define bpf_target_arm
-	#define bpf_target_defined
-#elif defined(__TARGET_ARCH_arm64)
-	#define bpf_target_arm64
-	#define bpf_target_defined
-#elif defined(__TARGET_ARCH_mips)
-	#define bpf_target_mips
-	#define bpf_target_defined
-#elif defined(__TARGET_ARCH_powerpc)
-	#define bpf_target_powerpc
-	#define bpf_target_defined
-#elif defined(__TARGET_ARCH_sparc)
-	#define bpf_target_sparc
-	#define bpf_target_defined
-#else
-	#undef bpf_target_defined
-#endif
-
-/* Fall back to what the compiler says */
-#ifndef bpf_target_defined
-#if defined(__x86_64__)
-	#define bpf_target_x86
-#elif defined(__s390__)
-	#define bpf_target_s390
-#elif defined(__arm__)
-	#define bpf_target_arm
-#elif defined(__aarch64__)
-	#define bpf_target_arm64
-#elif defined(__mips__)
-	#define bpf_target_mips
-#elif defined(__powerpc__)
-	#define bpf_target_powerpc
-#elif defined(__sparc__)
-	#define bpf_target_sparc
-#endif
-#endif
-
-#if defined(bpf_target_x86)
-
-#ifdef __KERNEL__
-#define PT_REGS_PARM1(x) ((x)->di)
-#define PT_REGS_PARM2(x) ((x)->si)
-#define PT_REGS_PARM3(x) ((x)->dx)
-#define PT_REGS_PARM4(x) ((x)->cx)
-#define PT_REGS_PARM5(x) ((x)->r8)
-#define PT_REGS_RET(x) ((x)->sp)
-#define PT_REGS_FP(x) ((x)->bp)
-#define PT_REGS_RC(x) ((x)->ax)
-#define PT_REGS_SP(x) ((x)->sp)
-#define PT_REGS_IP(x) ((x)->ip)
-#else
-#ifdef __i386__
-/* i386 kernel is built with -mregparm=3 */
-#define PT_REGS_PARM1(x) ((x)->eax)
-#define PT_REGS_PARM2(x) ((x)->edx)
-#define PT_REGS_PARM3(x) ((x)->ecx)
-#define PT_REGS_PARM4(x) 0
-#define PT_REGS_PARM5(x) 0
-#define PT_REGS_RET(x) ((x)->esp)
-#define PT_REGS_FP(x) ((x)->ebp)
-#define PT_REGS_RC(x) ((x)->eax)
-#define PT_REGS_SP(x) ((x)->esp)
-#define PT_REGS_IP(x) ((x)->eip)
-#else
-#define PT_REGS_PARM1(x) ((x)->rdi)
-#define PT_REGS_PARM2(x) ((x)->rsi)
-#define PT_REGS_PARM3(x) ((x)->rdx)
-#define PT_REGS_PARM4(x) ((x)->rcx)
-#define PT_REGS_PARM5(x) ((x)->r8)
-#define PT_REGS_RET(x) ((x)->rsp)
-#define PT_REGS_FP(x) ((x)->rbp)
-#define PT_REGS_RC(x) ((x)->rax)
-#define PT_REGS_SP(x) ((x)->rsp)
-#define PT_REGS_IP(x) ((x)->rip)
-#endif
-#endif
-
-#elif defined(bpf_target_s390)
-
-/* s390 provides user_pt_regs instead of struct pt_regs to userspace */
-struct pt_regs;
-#define PT_REGS_S390 const volatile user_pt_regs
-#define PT_REGS_PARM1(x) (((PT_REGS_S390 *)(x))->gprs[2])
-#define PT_REGS_PARM2(x) (((PT_REGS_S390 *)(x))->gprs[3])
-#define PT_REGS_PARM3(x) (((PT_REGS_S390 *)(x))->gprs[4])
-#define PT_REGS_PARM4(x) (((PT_REGS_S390 *)(x))->gprs[5])
-#define PT_REGS_PARM5(x) (((PT_REGS_S390 *)(x))->gprs[6])
-#define PT_REGS_RET(x) (((PT_REGS_S390 *)(x))->gprs[14])
-/* Works only with CONFIG_FRAME_POINTER */
-#define PT_REGS_FP(x) (((PT_REGS_S390 *)(x))->gprs[11])
-#define PT_REGS_RC(x) (((PT_REGS_S390 *)(x))->gprs[2])
-#define PT_REGS_SP(x) (((PT_REGS_S390 *)(x))->gprs[15])
-#define PT_REGS_IP(x) (((PT_REGS_S390 *)(x))->psw.addr)
-
-#elif defined(bpf_target_arm)
-
-#define PT_REGS_PARM1(x) ((x)->uregs[0])
-#define PT_REGS_PARM2(x) ((x)->uregs[1])
-#define PT_REGS_PARM3(x) ((x)->uregs[2])
-#define PT_REGS_PARM4(x) ((x)->uregs[3])
-#define PT_REGS_PARM5(x) ((x)->uregs[4])
-#define PT_REGS_RET(x) ((x)->uregs[14])
-#define PT_REGS_FP(x) ((x)->uregs[11]) /* Works only with CONFIG_FRAME_POINTER */
-#define PT_REGS_RC(x) ((x)->uregs[0])
-#define PT_REGS_SP(x) ((x)->uregs[13])
-#define PT_REGS_IP(x) ((x)->uregs[12])
-
-#elif defined(bpf_target_arm64)
-
-/* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */
-struct pt_regs;
-#define PT_REGS_ARM64 const volatile struct user_pt_regs
-#define PT_REGS_PARM1(x) (((PT_REGS_ARM64 *)(x))->regs[0])
-#define PT_REGS_PARM2(x) (((PT_REGS_ARM64 *)(x))->regs[1])
-#define PT_REGS_PARM3(x) (((PT_REGS_ARM64 *)(x))->regs[2])
-#define PT_REGS_PARM4(x) (((PT_REGS_ARM64 *)(x))->regs[3])
-#define PT_REGS_PARM5(x) (((PT_REGS_ARM64 *)(x))->regs[4])
-#define PT_REGS_RET(x) (((PT_REGS_ARM64 *)(x))->regs[30])
-/* Works only with CONFIG_FRAME_POINTER */
-#define PT_REGS_FP(x) (((PT_REGS_ARM64 *)(x))->regs[29])
-#define PT_REGS_RC(x) (((PT_REGS_ARM64 *)(x))->regs[0])
-#define PT_REGS_SP(x) (((PT_REGS_ARM64 *)(x))->sp)
-#define PT_REGS_IP(x) (((PT_REGS_ARM64 *)(x))->pc)
-
-#elif defined(bpf_target_mips)
-
-#define PT_REGS_PARM1(x) ((x)->regs[4])
-#define PT_REGS_PARM2(x) ((x)->regs[5])
-#define PT_REGS_PARM3(x) ((x)->regs[6])
-#define PT_REGS_PARM4(x) ((x)->regs[7])
-#define PT_REGS_PARM5(x) ((x)->regs[8])
-#define PT_REGS_RET(x) ((x)->regs[31])
-#define PT_REGS_FP(x) ((x)->regs[30]) /* Works only with CONFIG_FRAME_POINTER */
-#define PT_REGS_RC(x) ((x)->regs[1])
-#define PT_REGS_SP(x) ((x)->regs[29])
-#define PT_REGS_IP(x) ((x)->cp0_epc)
-
-#elif defined(bpf_target_powerpc)
-
-#define PT_REGS_PARM1(x) ((x)->gpr[3])
-#define PT_REGS_PARM2(x) ((x)->gpr[4])
-#define PT_REGS_PARM3(x) ((x)->gpr[5])
-#define PT_REGS_PARM4(x) ((x)->gpr[6])
-#define PT_REGS_PARM5(x) ((x)->gpr[7])
-#define PT_REGS_RC(x) ((x)->gpr[3])
-#define PT_REGS_SP(x) ((x)->sp)
-#define PT_REGS_IP(x) ((x)->nip)
-
-#elif defined(bpf_target_sparc)
-
-#define PT_REGS_PARM1(x) ((x)->u_regs[UREG_I0])
-#define PT_REGS_PARM2(x) ((x)->u_regs[UREG_I1])
-#define PT_REGS_PARM3(x) ((x)->u_regs[UREG_I2])
-#define PT_REGS_PARM4(x) ((x)->u_regs[UREG_I3])
-#define PT_REGS_PARM5(x) ((x)->u_regs[UREG_I4])
-#define PT_REGS_RET(x) ((x)->u_regs[UREG_I7])
-#define PT_REGS_RC(x) ((x)->u_regs[UREG_I0])
-#define PT_REGS_SP(x) ((x)->u_regs[UREG_FP])
-
-/* Should this also be a bpf_target check for the sparc case? */
-#if defined(__arch64__)
-#define PT_REGS_IP(x) ((x)->tpc)
-#else
-#define PT_REGS_IP(x) ((x)->pc)
-#endif
-
-#endif
-
-#if defined(bpf_target_powerpc)
-#define BPF_KPROBE_READ_RET_IP(ip, ctx)		({ (ip) = (ctx)->link; })
-#define BPF_KRETPROBE_READ_RET_IP		BPF_KPROBE_READ_RET_IP
-#elif defined(bpf_target_sparc)
-#define BPF_KPROBE_READ_RET_IP(ip, ctx)		({ (ip) = PT_REGS_RET(ctx); })
-#define BPF_KRETPROBE_READ_RET_IP		BPF_KPROBE_READ_RET_IP
-#else
-#define BPF_KPROBE_READ_RET_IP(ip, ctx)		({				\
-		bpf_probe_read(&(ip), sizeof(ip), (void *)PT_REGS_RET(ctx)); })
-#define BPF_KRETPROBE_READ_RET_IP(ip, ctx)	({				\
-		bpf_probe_read(&(ip), sizeof(ip),				\
-				(void *)(PT_REGS_FP(ctx) + sizeof(ip))); })
-#endif
-
-/*
- * BPF_CORE_READ abstracts away bpf_probe_read() call and captures offset
- * relocation for source address using __builtin_preserve_access_index()
- * built-in, provided by Clang.
- *
- * __builtin_preserve_access_index() takes as an argument an expression of
- * taking an address of a field within struct/union. It makes compiler emit
- * a relocation, which records BTF type ID describing root struct/union and an
- * accessor string which describes exact embedded field that was used to take
- * an address. See detailed description of this relocation format and
- * semantics in comments to struct bpf_offset_reloc in libbpf_internal.h.
- *
- * This relocation allows libbpf to adjust BPF instruction to use correct
- * actual field offset, based on target kernel BTF type that matches original
- * (local) BTF, used to record relocation.
- */
-#define BPF_CORE_READ(dst, src)						\
-	bpf_probe_read((dst), sizeof(*(src)),				\
-		       __builtin_preserve_access_index(src))
-
-#endif
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH bpf-next 4/6] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers
  2019-09-30 18:58 [PATCH bpf-next 0/6] Move bpf_helpers and add BPF_CORE_READ macros Andrii Nakryiko
                   ` (2 preceding siblings ...)
  2019-09-30 18:58 ` [PATCH bpf-next 3/6] selftests/bpf: switch test to use libbpf's helpers Andrii Nakryiko
@ 2019-09-30 18:58 ` Andrii Nakryiko
  2019-10-01 19:07   ` John Fastabend
  2019-10-01 21:14   ` Song Liu
  2019-09-30 18:58 ` [PATCH bpf-next 5/6] selftests/bpf: adjust CO-RE reloc tests for new BPF_CORE_READ macro Andrii Nakryiko
  2019-09-30 18:58 ` [PATCH bpf-next 6/6] selftests/bpf: add BPF_CORE_READ and BPF_CORE_READ_STR_INTO macro tests Andrii Nakryiko
  5 siblings, 2 replies; 47+ messages in thread
From: Andrii Nakryiko @ 2019-09-30 18:58 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Add few macros simplifying BCC-like multi-level probe reads, while also
emitting CO-RE relocations for each read.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/bpf_helpers.h | 151 +++++++++++++++++++++++++++++++++++-
 1 file changed, 147 insertions(+), 4 deletions(-)

diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
index a1d9b97b8e15..51e7b11d53e8 100644
--- a/tools/lib/bpf/bpf_helpers.h
+++ b/tools/lib/bpf/bpf_helpers.h
@@ -19,6 +19,10 @@
  */
 #define SEC(NAME) __attribute__((section(NAME), used))
 
+#ifndef __always_inline
+#define __always_inline __attribute__((always_inline))
+#endif
+
 /* helper functions called from eBPF programs written in C */
 static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
 	(void *) BPF_FUNC_map_lookup_elem;
@@ -505,7 +509,7 @@ struct pt_regs;
 #endif
 
 /*
- * BPF_CORE_READ abstracts away bpf_probe_read() call and captures offset
+ * bpf_core_read() abstracts away bpf_probe_read() call and captures field
  * relocation for source address using __builtin_preserve_access_index()
  * built-in, provided by Clang.
  *
@@ -520,8 +524,147 @@ struct pt_regs;
  * actual field offset, based on target kernel BTF type that matches original
  * (local) BTF, used to record relocation.
  */
-#define BPF_CORE_READ(dst, src)						\
-	bpf_probe_read((dst), sizeof(*(src)),				\
-		       __builtin_preserve_access_index(src))
+#define bpf_core_read(dst, sz, src)					    \
+	bpf_probe_read(dst, sz,						    \
+		       (const void *)__builtin_preserve_access_index(src))
+
+/*
+ * bpf_core_read_str() is a thin wrapper around bpf_probe_read_str()
+ * additionally emitting BPF CO-RE field relocation for specified source
+ * argument.
+ */
+#define bpf_core_read_str(dst, sz, src)					    \
+	bpf_probe_read_str(dst, sz,					    \
+			   (const void *)__builtin_preserve_access_index(src))
+
+#define ___concat(a, b) a ## b
+#define ___apply(fn, n) ___concat(fn, n)
+#define ___nth(_1, _2, _3, _4, _5, _6, _7, _8, _9, _10, __11, N, ...) N
+
+/* return number of provided arguments; used for switch-based variadic macro
+ * definitions (see ___last, ___arrow, etc below)
+ */
+#define ___narg(...) ___nth(_, ##__VA_ARGS__, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
+/* return 0 if no arguments are passed, N - otherwise; used for
+ * recursively-defined macros to specify termination (0) case, and generic
+ * (N) case (e.g., ___read_ptrs, ___core_read)
+ */
+#define ___empty(...) ___nth(_, ##__VA_ARGS__, N, N, N, N, N, N, N, N, N, N, 0)
+
+#define ___last1(x) x
+#define ___last2(a, x) x
+#define ___last3(a, b, x) x
+#define ___last4(a, b, c, x) x
+#define ___last5(a, b, c, d, x) x
+#define ___last6(a, b, c, d, e, x) x
+#define ___last7(a, b, c, d, e, f, x) x
+#define ___last8(a, b, c, d, e, f, g, x) x
+#define ___last9(a, b, c, d, e, f, g, h, x) x
+#define ___last10(a, b, c, d, e, f, g, h, i, x) x
+#define ___last(...) ___apply(___last, ___narg(__VA_ARGS__))(__VA_ARGS__)
+
+#define ___nolast2(a, _) a
+#define ___nolast3(a, b, _) a, b
+#define ___nolast4(a, b, c, _) a, b, c
+#define ___nolast5(a, b, c, d, _) a, b, c, d
+#define ___nolast6(a, b, c, d, e, _) a, b, c, d, e
+#define ___nolast7(a, b, c, d, e, f, _) a, b, c, d, e, f
+#define ___nolast8(a, b, c, d, e, f, g, _) a, b, c, d, e, f, g
+#define ___nolast9(a, b, c, d, e, f, g, h, _) a, b, c, d, e, f, g, h
+#define ___nolast10(a, b, c, d, e, f, g, h, i, _) a, b, c, d, e, f, g, h, i
+#define ___nolast(...) ___apply(___nolast, ___narg(__VA_ARGS__))(__VA_ARGS__)
+
+#define ___arrow1(a) a
+#define ___arrow2(a, b) a->b
+#define ___arrow3(a, b, c) a->b->c
+#define ___arrow4(a, b, c, d) a->b->c->d
+#define ___arrow5(a, b, c, d, e) a->b->c->d->e
+#define ___arrow6(a, b, c, d, e, f) a->b->c->d->e->f
+#define ___arrow7(a, b, c, d, e, f, g) a->b->c->d->e->f->g
+#define ___arrow8(a, b, c, d, e, f, g, h) a->b->c->d->e->f->g->h
+#define ___arrow9(a, b, c, d, e, f, g, h, i) a->b->c->d->e->f->g->h->i
+#define ___arrow10(a, b, c, d, e, f, g, h, i, j) a->b->c->d->e->f->g->h->i->j
+#define ___arrow(...) ___apply(___arrow, ___narg(__VA_ARGS__))(__VA_ARGS__)
+
+#define ___type(...) typeof(___arrow(__VA_ARGS__))
+
+#define ___read(read_fn, dst, src_type, src, accessor)			    \
+	read_fn((void *)(dst), sizeof(*(dst)), &((src_type)(src))->accessor)
+
+/* "recursively" read a sequence of inner pointers using local __t var */
+#define ___rd_last(...)							    \
+	___read(bpf_core_read, &__t,					    \
+		___type(___nolast(__VA_ARGS__)), __t, ___last(__VA_ARGS__));
+#define ___rd_p0(src) const void *__t = src;
+#define ___rd_p1(...) ___rd_p0(___nolast(__VA_ARGS__)) ___rd_last(__VA_ARGS__)
+#define ___rd_p2(...) ___rd_p1(___nolast(__VA_ARGS__)) ___rd_last(__VA_ARGS__)
+#define ___rd_p3(...) ___rd_p2(___nolast(__VA_ARGS__)) ___rd_last(__VA_ARGS__)
+#define ___rd_p4(...) ___rd_p3(___nolast(__VA_ARGS__)) ___rd_last(__VA_ARGS__)
+#define ___rd_p5(...) ___rd_p4(___nolast(__VA_ARGS__)) ___rd_last(__VA_ARGS__)
+#define ___rd_p6(...) ___rd_p5(___nolast(__VA_ARGS__)) ___rd_last(__VA_ARGS__)
+#define ___rd_p7(...) ___rd_p6(___nolast(__VA_ARGS__)) ___rd_last(__VA_ARGS__)
+#define ___rd_p8(...) ___rd_p7(___nolast(__VA_ARGS__)) ___rd_last(__VA_ARGS__)
+#define ___rd_p9(...) ___rd_p8(___nolast(__VA_ARGS__)) ___rd_last(__VA_ARGS__)
+#define ___read_ptrs(src, ...)						    \
+	___apply(___rd_p, ___narg(__VA_ARGS__))(src, __VA_ARGS__)
+
+#define ___core_read0(fn, dst, src, a)					    \
+	___read(fn, dst, ___type(src), src, a);
+#define ___core_readN(fn, dst, src, ...)				    \
+	___read_ptrs(src, ___nolast(__VA_ARGS__))			    \
+	___read(fn, dst, ___type(src, ___nolast(__VA_ARGS__)), __t,	    \
+		___last(__VA_ARGS__));
+#define ___core_read(fn, dst, src, a, ...)				    \
+	___apply(___core_read, ___empty(__VA_ARGS__))(fn, dst,		    \
+						      src, a, ##__VA_ARGS__)
+
+/*
+ * BPF_CORE_READ_INTO() is a more performance-conscious variant of
+ * BPF_CORE_READ(), in which final field is read into user-provided storage.
+ * See BPF_CORE_READ() below for more details on general usage.
+ */
+#define BPF_CORE_READ_INTO(dst, src, a, ...)				    \
+	({								    \
+		___core_read(bpf_core_read, dst, src, a, ##__VA_ARGS__)	    \
+	})
+
+/*
+ * BPF_CORE_READ_STR_INTO() does same "pointer chasing" as
+ * BPF_CORE_READ() for intermediate pointers, but then executes (and returns
+ * corresponding error code) bpf_core_read_str() for final string read.
+ */
+#define BPF_CORE_READ_STR_INTO(dst, src, a, ...)			    \
+	({								    \
+		___core_read(bpf_core_read_str, dst, src, a, ##__VA_ARGS__) \
+	})
+
+/*
+ * BPF_CORE_READ() is used to simplify BPF CO-RE relocatable read, especially
+ * when there are few pointer chasing steps.
+ * E.g., what in non-BPF world (or in BPF w/ BCC) would be something like:
+ *	int x = s->a.b.c->d.e->f->g;
+ * can be succinctly achieved using BPF_CORE_READ as:
+ *	int x = BPF_CORE_READ(s, a.b.c, d.e, f, g);
+ *
+ * BPF_CORE_READ will decompose above statement into 4 bpf_core_read (BPF
+ * CO-RE relocatable bpf_probe_read() wrapper) calls, logically equivalent to:
+ * 1. const void *__t = s->a.b.c;
+ * 2. __t = __t->d.e;
+ * 3. __t = __t->f;
+ * 4. return __t->g;
+ *
+ * Equivalence is logical, because there is a heavy type casting/preservation
+ * involved, as well as all the reads are happening through bpf_probe_read()
+ * calls using __builtin_preserve_access_index() to emit CO-RE relocations.
+ *
+ * N.B. Only up to 9 "field accessors" are supported, which should be more
+ * than enough for any practical purpose.
+ */
+#define BPF_CORE_READ(src, a, ...)					    \
+	({								    \
+		___type(src, a, ##__VA_ARGS__) __r;			    \
+		BPF_CORE_READ_INTO(&__r, src, a, ##__VA_ARGS__);	    \
+		__r;							    \
+	})
 
 #endif
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH bpf-next 5/6] selftests/bpf: adjust CO-RE reloc tests for new BPF_CORE_READ macro
  2019-09-30 18:58 [PATCH bpf-next 0/6] Move bpf_helpers and add BPF_CORE_READ macros Andrii Nakryiko
                   ` (3 preceding siblings ...)
  2019-09-30 18:58 ` [PATCH bpf-next 4/6] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers Andrii Nakryiko
@ 2019-09-30 18:58 ` Andrii Nakryiko
  2019-10-01 19:14   ` John Fastabend
  2019-10-01 21:47   ` Song Liu
  2019-09-30 18:58 ` [PATCH bpf-next 6/6] selftests/bpf: add BPF_CORE_READ and BPF_CORE_READ_STR_INTO macro tests Andrii Nakryiko
  5 siblings, 2 replies; 47+ messages in thread
From: Andrii Nakryiko @ 2019-09-30 18:58 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Given introduction of variadic BPF_CORE_READ with slightly different
syntax and semantics, define CORE_READ, which is a thin wrapper around
low-level bpf_core_read() macro, which in turn is just a wrapper around
bpf_probe_read(). BPF_CORE_READ is higher-level variadic macro
supporting multi-pointer reads and are tested separately.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 .../bpf/progs/test_core_reloc_arrays.c         | 10 ++++++----
 .../bpf/progs/test_core_reloc_flavors.c        |  8 +++++---
 .../selftests/bpf/progs/test_core_reloc_ints.c | 18 ++++++++++--------
 .../bpf/progs/test_core_reloc_kernel.c         |  6 ++++--
 .../selftests/bpf/progs/test_core_reloc_misc.c |  8 +++++---
 .../selftests/bpf/progs/test_core_reloc_mods.c | 18 ++++++++++--------
 .../bpf/progs/test_core_reloc_nesting.c        |  6 ++++--
 .../bpf/progs/test_core_reloc_primitives.c     | 12 +++++++-----
 .../bpf/progs/test_core_reloc_ptr_as_arr.c     |  4 +++-
 9 files changed, 54 insertions(+), 36 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_arrays.c b/tools/testing/selftests/bpf/progs/test_core_reloc_arrays.c
index bf67f0fdf743..58efe4944594 100644
--- a/tools/testing/selftests/bpf/progs/test_core_reloc_arrays.c
+++ b/tools/testing/selftests/bpf/progs/test_core_reloc_arrays.c
@@ -31,6 +31,8 @@ struct core_reloc_arrays {
 	struct core_reloc_arrays_substruct d[1][2];
 };
 
+#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src)
+
 SEC("raw_tracepoint/sys_enter")
 int test_core_arrays(void *ctx)
 {
@@ -38,16 +40,16 @@ int test_core_arrays(void *ctx)
 	struct core_reloc_arrays_output *out = (void *)&data.out;
 
 	/* in->a[2] */
-	if (BPF_CORE_READ(&out->a2, &in->a[2]))
+	if (CORE_READ(&out->a2, &in->a[2]))
 		return 1;
 	/* in->b[1][2][3] */
-	if (BPF_CORE_READ(&out->b123, &in->b[1][2][3]))
+	if (CORE_READ(&out->b123, &in->b[1][2][3]))
 		return 1;
 	/* in->c[1].c */
-	if (BPF_CORE_READ(&out->c1c, &in->c[1].c))
+	if (CORE_READ(&out->c1c, &in->c[1].c))
 		return 1;
 	/* in->d[0][0].d */
-	if (BPF_CORE_READ(&out->d00d, &in->d[0][0].d))
+	if (CORE_READ(&out->d00d, &in->d[0][0].d))
 		return 1;
 
 	return 0;
diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c b/tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c
index 9fda73e87972..3348acc7e50b 100644
--- a/tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c
+++ b/tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c
@@ -39,6 +39,8 @@ struct core_reloc_flavors___weird {
 	};
 };
 
+#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src)
+
 SEC("raw_tracepoint/sys_enter")
 int test_core_flavors(void *ctx)
 {
@@ -48,13 +50,13 @@ int test_core_flavors(void *ctx)
 	struct core_reloc_flavors *out = (void *)&data.out;
 
 	/* read a using weird layout */
-	if (BPF_CORE_READ(&out->a, &in_weird->a))
+	if (CORE_READ(&out->a, &in_weird->a))
 		return 1;
 	/* read b using reversed layout */
-	if (BPF_CORE_READ(&out->b, &in_rev->b))
+	if (CORE_READ(&out->b, &in_rev->b))
 		return 1;
 	/* read c using original layout */
-	if (BPF_CORE_READ(&out->c, &in_orig->c))
+	if (CORE_READ(&out->c, &in_orig->c))
 		return 1;
 
 	return 0;
diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_ints.c b/tools/testing/selftests/bpf/progs/test_core_reloc_ints.c
index d99233c8008a..cfe16ede48dd 100644
--- a/tools/testing/selftests/bpf/progs/test_core_reloc_ints.c
+++ b/tools/testing/selftests/bpf/progs/test_core_reloc_ints.c
@@ -23,20 +23,22 @@ struct core_reloc_ints {
 	int64_t		s64_field;
 };
 
+#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src)
+
 SEC("raw_tracepoint/sys_enter")
 int test_core_ints(void *ctx)
 {
 	struct core_reloc_ints *in = (void *)&data.in;
 	struct core_reloc_ints *out = (void *)&data.out;
 
-	if (BPF_CORE_READ(&out->u8_field, &in->u8_field) ||
-	    BPF_CORE_READ(&out->s8_field, &in->s8_field) ||
-	    BPF_CORE_READ(&out->u16_field, &in->u16_field) ||
-	    BPF_CORE_READ(&out->s16_field, &in->s16_field) ||
-	    BPF_CORE_READ(&out->u32_field, &in->u32_field) ||
-	    BPF_CORE_READ(&out->s32_field, &in->s32_field) ||
-	    BPF_CORE_READ(&out->u64_field, &in->u64_field) ||
-	    BPF_CORE_READ(&out->s64_field, &in->s64_field))
+	if (CORE_READ(&out->u8_field, &in->u8_field) ||
+	    CORE_READ(&out->s8_field, &in->s8_field) ||
+	    CORE_READ(&out->u16_field, &in->u16_field) ||
+	    CORE_READ(&out->s16_field, &in->s16_field) ||
+	    CORE_READ(&out->u32_field, &in->u32_field) ||
+	    CORE_READ(&out->s32_field, &in->s32_field) ||
+	    CORE_READ(&out->u64_field, &in->u64_field) ||
+	    CORE_READ(&out->s64_field, &in->s64_field))
 		return 1;
 
 	return 0;
diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c
index 37e02aa3f0c8..e5308026cfda 100644
--- a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c
+++ b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c
@@ -17,6 +17,8 @@ struct task_struct {
 	int tgid;
 };
 
+#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src)
+
 SEC("raw_tracepoint/sys_enter")
 int test_core_kernel(void *ctx)
 {
@@ -24,8 +26,8 @@ int test_core_kernel(void *ctx)
 	uint64_t pid_tgid = bpf_get_current_pid_tgid();
 	int pid, tgid;
 
-	if (BPF_CORE_READ(&pid, &task->pid) ||
-	    BPF_CORE_READ(&tgid, &task->tgid))
+	if (CORE_READ(&pid, &task->pid) ||
+	    CORE_READ(&tgid, &task->tgid))
 		return 1;
 
 	/* validate pid + tgid matches */
diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_misc.c b/tools/testing/selftests/bpf/progs/test_core_reloc_misc.c
index c59984bd3e23..40c4638ab5cc 100644
--- a/tools/testing/selftests/bpf/progs/test_core_reloc_misc.c
+++ b/tools/testing/selftests/bpf/progs/test_core_reloc_misc.c
@@ -32,6 +32,8 @@ struct core_reloc_misc_extensible {
 	int b;
 };
 
+#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src)
+
 SEC("raw_tracepoint/sys_enter")
 int test_core_misc(void *ctx)
 {
@@ -41,15 +43,15 @@ int test_core_misc(void *ctx)
 	struct core_reloc_misc_output *out = (void *)&data.out;
 
 	/* record two different relocations with the same accessor string */
-	if (BPF_CORE_READ(&out->a, &in_a->a1) ||	/* accessor: 0:0 */
-	    BPF_CORE_READ(&out->b, &in_b->b1))		/* accessor: 0:0 */
+	if (CORE_READ(&out->a, &in_a->a1) ||		/* accessor: 0:0 */
+	    CORE_READ(&out->b, &in_b->b1))		/* accessor: 0:0 */
 		return 1;
 
 	/* Validate relocations capture array-only accesses for structs with
 	 * fixed header, but with potentially extendable tail. This will read
 	 * first 4 bytes of 2nd element of in_ext array of potentially
 	 * variably sized struct core_reloc_misc_extensible. */ 
-	if (BPF_CORE_READ(&out->c, &in_ext[2]))		/* accessor: 2 */
+	if (CORE_READ(&out->c, &in_ext[2]))		/* accessor: 2 */
 		return 1;
 
 	return 0;
diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_mods.c b/tools/testing/selftests/bpf/progs/test_core_reloc_mods.c
index f98b942c062b..99fc9ee21895 100644
--- a/tools/testing/selftests/bpf/progs/test_core_reloc_mods.c
+++ b/tools/testing/selftests/bpf/progs/test_core_reloc_mods.c
@@ -41,20 +41,22 @@ struct core_reloc_mods {
 	core_reloc_mods_substruct_t h;
 };
 
+#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src)
+
 SEC("raw_tracepoint/sys_enter")
 int test_core_mods(void *ctx)
 {
 	struct core_reloc_mods *in = (void *)&data.in;
 	struct core_reloc_mods_output *out = (void *)&data.out;
 
-	if (BPF_CORE_READ(&out->a, &in->a) ||
-	    BPF_CORE_READ(&out->b, &in->b) ||
-	    BPF_CORE_READ(&out->c, &in->c) ||
-	    BPF_CORE_READ(&out->d, &in->d) ||
-	    BPF_CORE_READ(&out->e, &in->e[2]) ||
-	    BPF_CORE_READ(&out->f, &in->f[1]) ||
-	    BPF_CORE_READ(&out->g, &in->g.x) ||
-	    BPF_CORE_READ(&out->h, &in->h.y))
+	if (CORE_READ(&out->a, &in->a) ||
+	    CORE_READ(&out->b, &in->b) ||
+	    CORE_READ(&out->c, &in->c) ||
+	    CORE_READ(&out->d, &in->d) ||
+	    CORE_READ(&out->e, &in->e[2]) ||
+	    CORE_READ(&out->f, &in->f[1]) ||
+	    CORE_READ(&out->g, &in->g.x) ||
+	    CORE_READ(&out->h, &in->h.y))
 		return 1;
 
 	return 0;
diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_nesting.c b/tools/testing/selftests/bpf/progs/test_core_reloc_nesting.c
index 3ca30cec2b39..c84fff3bdd72 100644
--- a/tools/testing/selftests/bpf/progs/test_core_reloc_nesting.c
+++ b/tools/testing/selftests/bpf/progs/test_core_reloc_nesting.c
@@ -30,15 +30,17 @@ struct core_reloc_nesting {
 	} b;
 };
 
+#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src)
+
 SEC("raw_tracepoint/sys_enter")
 int test_core_nesting(void *ctx)
 {
 	struct core_reloc_nesting *in = (void *)&data.in;
 	struct core_reloc_nesting *out = (void *)&data.out;
 
-	if (BPF_CORE_READ(&out->a.a.a, &in->a.a.a))
+	if (CORE_READ(&out->a.a.a, &in->a.a.a))
 		return 1;
-	if (BPF_CORE_READ(&out->b.b.b, &in->b.b.b))
+	if (CORE_READ(&out->b.b.b, &in->b.b.b))
 		return 1;
 
 	return 0;
diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_primitives.c b/tools/testing/selftests/bpf/progs/test_core_reloc_primitives.c
index add52f23ab35..4038e9907162 100644
--- a/tools/testing/selftests/bpf/progs/test_core_reloc_primitives.c
+++ b/tools/testing/selftests/bpf/progs/test_core_reloc_primitives.c
@@ -25,17 +25,19 @@ struct core_reloc_primitives {
 	int (*f)(const char *);
 };
 
+#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src)
+
 SEC("raw_tracepoint/sys_enter")
 int test_core_primitives(void *ctx)
 {
 	struct core_reloc_primitives *in = (void *)&data.in;
 	struct core_reloc_primitives *out = (void *)&data.out;
 
-	if (BPF_CORE_READ(&out->a, &in->a) ||
-	    BPF_CORE_READ(&out->b, &in->b) ||
-	    BPF_CORE_READ(&out->c, &in->c) ||
-	    BPF_CORE_READ(&out->d, &in->d) ||
-	    BPF_CORE_READ(&out->f, &in->f))
+	if (CORE_READ(&out->a, &in->a) ||
+	    CORE_READ(&out->b, &in->b) ||
+	    CORE_READ(&out->c, &in->c) ||
+	    CORE_READ(&out->d, &in->d) ||
+	    CORE_READ(&out->f, &in->f))
 		return 1;
 
 	return 0;
diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_ptr_as_arr.c b/tools/testing/selftests/bpf/progs/test_core_reloc_ptr_as_arr.c
index 526b7ddc7ea1..414d44620258 100644
--- a/tools/testing/selftests/bpf/progs/test_core_reloc_ptr_as_arr.c
+++ b/tools/testing/selftests/bpf/progs/test_core_reloc_ptr_as_arr.c
@@ -16,13 +16,15 @@ struct core_reloc_ptr_as_arr {
 	int a;
 };
 
+#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src)
+
 SEC("raw_tracepoint/sys_enter")
 int test_core_ptr_as_arr(void *ctx)
 {
 	struct core_reloc_ptr_as_arr *in = (void *)&data.in;
 	struct core_reloc_ptr_as_arr *out = (void *)&data.out;
 
-	if (BPF_CORE_READ(&out->a, &in[2].a))
+	if (CORE_READ(&out->a, &in[2].a))
 		return 1;
 
 	return 0;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH bpf-next 6/6] selftests/bpf: add BPF_CORE_READ and BPF_CORE_READ_STR_INTO macro tests
  2019-09-30 18:58 [PATCH bpf-next 0/6] Move bpf_helpers and add BPF_CORE_READ macros Andrii Nakryiko
                   ` (4 preceding siblings ...)
  2019-09-30 18:58 ` [PATCH bpf-next 5/6] selftests/bpf: adjust CO-RE reloc tests for new BPF_CORE_READ macro Andrii Nakryiko
@ 2019-09-30 18:58 ` Andrii Nakryiko
  2019-10-01 19:19   ` John Fastabend
  5 siblings, 1 reply; 47+ messages in thread
From: Andrii Nakryiko @ 2019-09-30 18:58 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Validate BPF_CORE_READ correctness and handling of up to 9 levels of
nestedness using cyclic task->(group_leader->)*->tgid chains.

Also add a test of maximum-dpeth BPF_CORE_READ_STR_INTO() macro.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 .../selftests/bpf/prog_tests/core_reloc.c     |  8 ++-
 .../selftests/bpf/progs/core_reloc_types.h    |  9 ++++
 .../bpf/progs/test_core_reloc_kernel.c        | 54 ++++++++++++++++++-
 3 files changed, 68 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
index f3863f976a48..21a0dff66241 100644
--- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c
+++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
@@ -193,8 +193,12 @@ static struct core_reloc_test_case test_cases[] = {
 		.btf_src_file = NULL, /* load from /lib/modules/$(uname -r) */
 		.input = "",
 		.input_len = 0,
-		.output = "\1", /* true */
-		.output_len = 1,
+		.output = STRUCT_TO_CHAR_PTR(core_reloc_kernel_output) {
+			.valid = { 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, },
+			.comm = "test_progs\0\0\0\0\0",
+			.comm_len = 11,
+		},
+		.output_len = sizeof(struct core_reloc_kernel_output),
 	},
 
 	/* validate BPF program can use multiple flavors to match against
diff --git a/tools/testing/selftests/bpf/progs/core_reloc_types.h b/tools/testing/selftests/bpf/progs/core_reloc_types.h
index f686a8138d90..9a6bdeb4894c 100644
--- a/tools/testing/selftests/bpf/progs/core_reloc_types.h
+++ b/tools/testing/selftests/bpf/progs/core_reloc_types.h
@@ -1,5 +1,14 @@
 #include <stdint.h>
 #include <stdbool.h>
+/*
+ * KERNEL
+ */
+
+struct core_reloc_kernel_output {
+	int valid[10];
+	char comm[16];
+	int comm_len;
+};
 
 /*
  * FLAVORS
diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c
index e5308026cfda..f318d39623b5 100644
--- a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c
+++ b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c
@@ -12,9 +12,17 @@ static volatile struct data {
 	char out[256];
 } data;
 
+struct core_reloc_kernel_output {
+	int valid[10];
+	char comm[16];
+	int comm_len;
+};
+
 struct task_struct {
 	int pid;
 	int tgid;
+	char comm[16];
+	struct task_struct *group_leader;
 };
 
 #define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*dst), src)
@@ -23,7 +31,9 @@ SEC("raw_tracepoint/sys_enter")
 int test_core_kernel(void *ctx)
 {
 	struct task_struct *task = (void *)bpf_get_current_task();
+	struct core_reloc_kernel_output *out = (void *)&data.out;
 	uint64_t pid_tgid = bpf_get_current_pid_tgid();
+	uint32_t real_tgid = (uint32_t)pid_tgid;
 	int pid, tgid;
 
 	if (CORE_READ(&pid, &task->pid) ||
@@ -31,7 +41,49 @@ int test_core_kernel(void *ctx)
 		return 1;
 
 	/* validate pid + tgid matches */
-	data.out[0] = (((uint64_t)pid << 32) | tgid) == pid_tgid;
+	out->valid[0] = (((uint64_t)pid << 32) | tgid) == pid_tgid;
+
+	/* test variadic BPF_CORE_READ macros */
+	out->valid[1] = BPF_CORE_READ(task,
+				      tgid) == real_tgid;
+	out->valid[2] = BPF_CORE_READ(task,
+				      group_leader,
+				      tgid) == real_tgid;
+	out->valid[3] = BPF_CORE_READ(task,
+				      group_leader, group_leader,
+				      tgid) == real_tgid;
+	out->valid[4] = BPF_CORE_READ(task,
+				      group_leader, group_leader, group_leader,
+				      tgid) == real_tgid;
+	out->valid[5] = BPF_CORE_READ(task,
+				      group_leader, group_leader, group_leader,
+				      group_leader,
+				      tgid) == real_tgid;
+	out->valid[6] = BPF_CORE_READ(task,
+				      group_leader, group_leader, group_leader,
+				      group_leader, group_leader,
+				      tgid) == real_tgid;
+	out->valid[7] = BPF_CORE_READ(task,
+				      group_leader, group_leader, group_leader,
+				      group_leader, group_leader, group_leader,
+				      tgid) == real_tgid;
+	out->valid[8] = BPF_CORE_READ(task,
+				      group_leader, group_leader, group_leader,
+				      group_leader, group_leader, group_leader,
+				      group_leader,
+				      tgid) == real_tgid;
+	out->valid[9] = BPF_CORE_READ(task,
+				      group_leader, group_leader, group_leader,
+				      group_leader, group_leader, group_leader,
+				      group_leader, group_leader,
+				      tgid) == real_tgid;
+
+	/* test BPF_CORE_READ_STR_INTO() returns correct code and contents */
+	out->comm_len = BPF_CORE_READ_STR_INTO(
+		&out->comm, task,
+		group_leader, group_leader, group_leader, group_leader,
+		group_leader, group_leader, group_leader, group_leader,
+		comm);
 
 	return 0;
 }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 1/6] selftests/bpf: undo GCC-specific bpf_helpers.h changes
  2019-09-30 18:58 ` [PATCH bpf-next 1/6] selftests/bpf: undo GCC-specific bpf_helpers.h changes Andrii Nakryiko
@ 2019-09-30 22:53   ` Song Liu
  2019-10-01 10:25   ` Ilya Leoshkevich
  2019-10-01 19:10   ` John Fastabend
  2 siblings, 0 replies; 47+ messages in thread
From: Song Liu @ 2019-09-30 22:53 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Kernel Team

On Mon, Sep 30, 2019 at 1:42 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Having GCC provide its own bpf-helper.h is not the right approach and is
> going to be changed. Undo bpf_helpers.h change before moving
> bpf_helpers.h into libbpf.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

Acked-by: Song Liu <songliubraving@fb.com>

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-09-30 18:58 ` [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf Andrii Nakryiko
@ 2019-09-30 22:55   ` Song Liu
  2019-09-30 22:58     ` Andrii Nakryiko
  2019-10-01  7:10   ` Toke Høiland-Jørgensen
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 47+ messages in thread
From: Song Liu @ 2019-09-30 22:55 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Kernel Team

On Mon, Sep 30, 2019 at 1:43 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
> are installed along the other libbpf headers.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

Can we merge/rearrange 2/6 and 3/6, so they is a git-rename instead of
many +++ and ---?

Thanks,
Song

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-09-30 22:55   ` Song Liu
@ 2019-09-30 22:58     ` Andrii Nakryiko
  2019-09-30 23:18       ` Jakub Kicinski
  2019-09-30 23:23       ` Song Liu
  0 siblings, 2 replies; 47+ messages in thread
From: Andrii Nakryiko @ 2019-09-30 22:58 UTC (permalink / raw)
  To: Song Liu
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team

On Mon, Sep 30, 2019 at 3:55 PM Song Liu <liu.song.a23@gmail.com> wrote:
>
> On Mon, Sep 30, 2019 at 1:43 PM Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
> > are installed along the other libbpf headers.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> Can we merge/rearrange 2/6 and 3/6, so they is a git-rename instead of
> many +++ and ---?

I arranged them that way because of Github sync. We don't sync
selftests/bpf changes to Github, and it causes more churn if commits
have a mix of libbpf and selftests changes.

I didn't modify bpf_helpers.h/bpf_endian.h between those patches, so
don't worry about reviewing contents ;)

>
> Thanks,
> Song

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-09-30 22:58     ` Andrii Nakryiko
@ 2019-09-30 23:18       ` Jakub Kicinski
  2019-09-30 23:25         ` Song Liu
  2019-09-30 23:30         ` Andrii Nakryiko
  2019-09-30 23:23       ` Song Liu
  1 sibling, 2 replies; 47+ messages in thread
From: Jakub Kicinski @ 2019-09-30 23:18 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Song Liu, Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team

On Mon, 30 Sep 2019 15:58:35 -0700, Andrii Nakryiko wrote:
> On Mon, Sep 30, 2019 at 3:55 PM Song Liu <liu.song.a23@gmail.com> wrote:
> >
> > On Mon, Sep 30, 2019 at 1:43 PM Andrii Nakryiko <andriin@fb.com> wrote:  
> > >
> > > Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
> > > are installed along the other libbpf headers.
> > >
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>  
> >
> > Can we merge/rearrange 2/6 and 3/6, so they is a git-rename instead of
> > many +++ and ---?  
> 
> I arranged them that way because of Github sync. We don't sync
> selftests/bpf changes to Github, and it causes more churn if commits
> have a mix of libbpf and selftests changes.
> 
> I didn't modify bpf_helpers.h/bpf_endian.h between those patches, so
> don't worry about reviewing contents ;)

I thought we were over this :/ Please merge the patches.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-09-30 22:58     ` Andrii Nakryiko
  2019-09-30 23:18       ` Jakub Kicinski
@ 2019-09-30 23:23       ` Song Liu
  2019-09-30 23:27         ` Andrii Nakryiko
  1 sibling, 1 reply; 47+ messages in thread
From: Song Liu @ 2019-09-30 23:23 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Song Liu, Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team



> On Sep 30, 2019, at 3:58 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> On Mon, Sep 30, 2019 at 3:55 PM Song Liu <liu.song.a23@gmail.com> wrote:
>> 
>> On Mon, Sep 30, 2019 at 1:43 PM Andrii Nakryiko <andriin@fb.com> wrote:
>>> 
>>> Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
>>> are installed along the other libbpf headers.
>>> 
>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>> 
>> Can we merge/rearrange 2/6 and 3/6, so they is a git-rename instead of
>> many +++ and ---?
> 
> I arranged them that way because of Github sync. We don't sync
> selftests/bpf changes to Github, and it causes more churn if commits
> have a mix of libbpf and selftests changes.

Aha, I missed this point. 

> I didn't modify bpf_helpers.h/bpf_endian.h between those patches, so
> don't worry about reviewing contents ;)

Well, we need to be careful here. As headers in a library should be 
more stable than headers shipped with the code. 

Here, I am a little concerned with the fact that we added BPF_CORE_READ()
to libbpf, and then changed its syntax. This is within one release, so
it is mostly OK.

Thanks,
Song


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-09-30 23:18       ` Jakub Kicinski
@ 2019-09-30 23:25         ` Song Liu
  2019-09-30 23:36           ` Jakub Kicinski
  2019-09-30 23:30         ` Andrii Nakryiko
  1 sibling, 1 reply; 47+ messages in thread
From: Song Liu @ 2019-09-30 23:25 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Andrii Nakryiko, Song Liu, Andrii Nakryiko, bpf, Networking,
	Alexei Starovoitov, Daniel Borkmann, Kernel Team



> On Sep 30, 2019, at 4:18 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> 
> On Mon, 30 Sep 2019 15:58:35 -0700, Andrii Nakryiko wrote:
>> On Mon, Sep 30, 2019 at 3:55 PM Song Liu <liu.song.a23@gmail.com> wrote:
>>> 
>>> On Mon, Sep 30, 2019 at 1:43 PM Andrii Nakryiko <andriin@fb.com> wrote:  
>>>> 
>>>> Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
>>>> are installed along the other libbpf headers.
>>>> 
>>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>  
>>> 
>>> Can we merge/rearrange 2/6 and 3/6, so they is a git-rename instead of
>>> many +++ and ---?  
>> 
>> I arranged them that way because of Github sync. We don't sync
>> selftests/bpf changes to Github, and it causes more churn if commits
>> have a mix of libbpf and selftests changes.
>> 
>> I didn't modify bpf_helpers.h/bpf_endian.h between those patches, so
>> don't worry about reviewing contents ;)
> 
> I thought we were over this :/ Please merge the patches.

Andrii changed syntax for BPF_CORE_READ(). I guess that is new?

Thanks, 
Song

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-09-30 23:23       ` Song Liu
@ 2019-09-30 23:27         ` Andrii Nakryiko
  2019-09-30 23:35           ` Song Liu
  0 siblings, 1 reply; 47+ messages in thread
From: Andrii Nakryiko @ 2019-09-30 23:27 UTC (permalink / raw)
  To: Song Liu
  Cc: Song Liu, Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team

On Mon, Sep 30, 2019 at 4:23 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Sep 30, 2019, at 3:58 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Sep 30, 2019 at 3:55 PM Song Liu <liu.song.a23@gmail.com> wrote:
> >>
> >> On Mon, Sep 30, 2019 at 1:43 PM Andrii Nakryiko <andriin@fb.com> wrote:
> >>>
> >>> Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
> >>> are installed along the other libbpf headers.
> >>>
> >>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> >>
> >> Can we merge/rearrange 2/6 and 3/6, so they is a git-rename instead of
> >> many +++ and ---?
> >
> > I arranged them that way because of Github sync. We don't sync
> > selftests/bpf changes to Github, and it causes more churn if commits
> > have a mix of libbpf and selftests changes.
>
> Aha, I missed this point.
>
> > I didn't modify bpf_helpers.h/bpf_endian.h between those patches, so
> > don't worry about reviewing contents ;)
>
> Well, we need to be careful here. As headers in a library should be
> more stable than headers shipped with the code.
>
> Here, I am a little concerned with the fact that we added BPF_CORE_READ()
> to libbpf, and then changed its syntax. This is within one release, so
> it is mostly OK.

Well, I could bundle bpf_helpers move and fixing up selftests in one
commit, but I think it just makes commit unnecessarily big and
convoluted. BPF_CORE_READ in previous form was ever only used by
selftests, so it was never "released" per se, so it seems fine to do
it this way, but let me know if you disagree.

>
> Thanks,
> Song
>

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-09-30 23:18       ` Jakub Kicinski
  2019-09-30 23:25         ` Song Liu
@ 2019-09-30 23:30         ` Andrii Nakryiko
  2019-09-30 23:43           ` Jakub Kicinski
  1 sibling, 1 reply; 47+ messages in thread
From: Andrii Nakryiko @ 2019-09-30 23:30 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Song Liu, Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team

On Mon, Sep 30, 2019 at 4:18 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Mon, 30 Sep 2019 15:58:35 -0700, Andrii Nakryiko wrote:
> > On Mon, Sep 30, 2019 at 3:55 PM Song Liu <liu.song.a23@gmail.com> wrote:
> > >
> > > On Mon, Sep 30, 2019 at 1:43 PM Andrii Nakryiko <andriin@fb.com> wrote:
> > > >
> > > > Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
> > > > are installed along the other libbpf headers.
> > > >
> > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > >
> > > Can we merge/rearrange 2/6 and 3/6, so they is a git-rename instead of
> > > many +++ and ---?
> >
> > I arranged them that way because of Github sync. We don't sync
> > selftests/bpf changes to Github, and it causes more churn if commits
> > have a mix of libbpf and selftests changes.
> >
> > I didn't modify bpf_helpers.h/bpf_endian.h between those patches, so
> > don't worry about reviewing contents ;)
>
> I thought we were over this :/ Please merge the patches.

I'll merge those two patches, our sync script can handle that now,
though with a bit of human input. I'm not exactly sure on the "why"
though, I think generally splitting libbpf changes and selftests
changes is a good thing, no?

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-09-30 23:27         ` Andrii Nakryiko
@ 2019-09-30 23:35           ` Song Liu
  0 siblings, 0 replies; 47+ messages in thread
From: Song Liu @ 2019-09-30 23:35 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Song Liu, Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team



> On Sep 30, 2019, at 4:27 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> On Mon, Sep 30, 2019 at 4:23 PM Song Liu <songliubraving@fb.com> wrote:
>> 
>> 
>> 
>>> On Sep 30, 2019, at 3:58 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>>> 
>>> On Mon, Sep 30, 2019 at 3:55 PM Song Liu <liu.song.a23@gmail.com> wrote:
>>>> 
>>>> On Mon, Sep 30, 2019 at 1:43 PM Andrii Nakryiko <andriin@fb.com> wrote:
>>>>> 
>>>>> Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
>>>>> are installed along the other libbpf headers.
>>>>> 
>>>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>>> 
>>>> Can we merge/rearrange 2/6 and 3/6, so they is a git-rename instead of
>>>> many +++ and ---?
>>> 
>>> I arranged them that way because of Github sync. We don't sync
>>> selftests/bpf changes to Github, and it causes more churn if commits
>>> have a mix of libbpf and selftests changes.
>> 
>> Aha, I missed this point.
>> 
>>> I didn't modify bpf_helpers.h/bpf_endian.h between those patches, so
>>> don't worry about reviewing contents ;)
>> 
>> Well, we need to be careful here. As headers in a library should be
>> more stable than headers shipped with the code.
>> 
>> Here, I am a little concerned with the fact that we added BPF_CORE_READ()
>> to libbpf, and then changed its syntax. This is within one release, so
>> it is mostly OK.
> 
> Well, I could bundle bpf_helpers move and fixing up selftests in one
> commit, but I think it just makes commit unnecessarily big and
> convoluted. BPF_CORE_READ in previous form was ever only used by
> selftests, so it was never "released" per se, so it seems fine to do
> it this way, but let me know if you disagree.

A better approach is to modify BPF_CORE_READ in selftests before moving
it to libbpf. But I am ok with current approach as-is. 

Song



^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-09-30 23:25         ` Song Liu
@ 2019-09-30 23:36           ` Jakub Kicinski
  2019-09-30 23:39             ` Song Liu
  0 siblings, 1 reply; 47+ messages in thread
From: Jakub Kicinski @ 2019-09-30 23:36 UTC (permalink / raw)
  To: Song Liu
  Cc: Andrii Nakryiko, Song Liu, Andrii Nakryiko, bpf, Networking,
	Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Mon, 30 Sep 2019 23:25:33 +0000, Song Liu wrote:
> > On Sep 30, 2019, at 4:18 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> > 
> > On Mon, 30 Sep 2019 15:58:35 -0700, Andrii Nakryiko wrote:  
> >> On Mon, Sep 30, 2019 at 3:55 PM Song Liu <liu.song.a23@gmail.com> wrote:  
> >>> 
> >>> On Mon, Sep 30, 2019 at 1:43 PM Andrii Nakryiko <andriin@fb.com> wrote:    
> >>>> 
> >>>> Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
> >>>> are installed along the other libbpf headers.
> >>>> 
> >>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>    
> >>> 
> >>> Can we merge/rearrange 2/6 and 3/6, so they is a git-rename instead of
> >>> many +++ and ---?    
> >> 
> >> I arranged them that way because of Github sync. We don't sync
> >> selftests/bpf changes to Github, and it causes more churn if commits
> >> have a mix of libbpf and selftests changes.
> >> 
> >> I didn't modify bpf_helpers.h/bpf_endian.h between those patches, so
> >> don't worry about reviewing contents ;)  
> > 
> > I thought we were over this :/ Please merge the patches.  
> 
> Andrii changed syntax for BPF_CORE_READ(). I guess that is new?

I meant the battle to not split changes into harder to review, and less
logical form based on what some random, never review upstream script
can or cannot do :)

I was responding to the "don't worry about reviewing contents" - as you
pointed out git would just generate a move..

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-09-30 23:36           ` Jakub Kicinski
@ 2019-09-30 23:39             ` Song Liu
  0 siblings, 0 replies; 47+ messages in thread
From: Song Liu @ 2019-09-30 23:39 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Andrii Nakryiko, Song Liu, Andrii Nakryiko, bpf, Networking,
	Alexei Starovoitov, Daniel Borkmann, Kernel Team



> On Sep 30, 2019, at 4:36 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> 
> On Mon, 30 Sep 2019 23:25:33 +0000, Song Liu wrote:
>>> On Sep 30, 2019, at 4:18 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
>>> 
>>> On Mon, 30 Sep 2019 15:58:35 -0700, Andrii Nakryiko wrote:  
>>>> On Mon, Sep 30, 2019 at 3:55 PM Song Liu <liu.song.a23@gmail.com> wrote:  
>>>>> 
>>>>> On Mon, Sep 30, 2019 at 1:43 PM Andrii Nakryiko <andriin@fb.com> wrote:    
>>>>>> 
>>>>>> Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
>>>>>> are installed along the other libbpf headers.
>>>>>> 
>>>>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>    
>>>>> 
>>>>> Can we merge/rearrange 2/6 and 3/6, so they is a git-rename instead of
>>>>> many +++ and ---?    
>>>> 
>>>> I arranged them that way because of Github sync. We don't sync
>>>> selftests/bpf changes to Github, and it causes more churn if commits
>>>> have a mix of libbpf and selftests changes.
>>>> 
>>>> I didn't modify bpf_helpers.h/bpf_endian.h between those patches, so
>>>> don't worry about reviewing contents ;)  
>>> 
>>> I thought we were over this :/ Please merge the patches.  
>> 
>> Andrii changed syntax for BPF_CORE_READ(). I guess that is new?
> 
> I meant the battle to not split changes into harder to review, and less
> logical form based on what some random, never review upstream script
> can or cannot do :)
> 
> I was responding to the "don't worry about reviewing contents" - as you
> pointed out git would just generate a move..

Yeah, I got your actual point a few minutes later. 

Thanks for the clarification. :)
Song

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-09-30 23:30         ` Andrii Nakryiko
@ 2019-09-30 23:43           ` Jakub Kicinski
  0 siblings, 0 replies; 47+ messages in thread
From: Jakub Kicinski @ 2019-09-30 23:43 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Song Liu, Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team

On Mon, 30 Sep 2019 16:30:02 -0700, Andrii Nakryiko wrote:
> On Mon, Sep 30, 2019 at 4:18 PM Jakub Kicinski wrote:
> > On Mon, 30 Sep 2019 15:58:35 -0700, Andrii Nakryiko wrote:  
> > > On Mon, Sep 30, 2019 at 3:55 PM Song Liu <liu.song.a23@gmail.com> wrote:  
> > > >
> > > > On Mon, Sep 30, 2019 at 1:43 PM Andrii Nakryiko <andriin@fb.com> wrote:  
> > > > >
> > > > > Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
> > > > > are installed along the other libbpf headers.
> > > > >
> > > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>  
> > > >
> > > > Can we merge/rearrange 2/6 and 3/6, so they is a git-rename instead of
> > > > many +++ and ---?  
> > >
> > > I arranged them that way because of Github sync. We don't sync
> > > selftests/bpf changes to Github, and it causes more churn if commits
> > > have a mix of libbpf and selftests changes.
> > >
> > > I didn't modify bpf_helpers.h/bpf_endian.h between those patches, so
> > > don't worry about reviewing contents ;)  
> >
> > I thought we were over this :/ Please merge the patches.  
> 
> I'll merge those two patches, our sync script can handle that now,
> though with a bit of human input. I'm not exactly sure on the "why"
> though

Awesome, thank you!

> I think generally splitting libbpf changes and selftests
> changes is a good thing, no?

I'm not sure, here headers are moved to a better location. To me it
seems like the logical change is move, rather than add X, remove X.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-09-30 18:58 ` [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf Andrii Nakryiko
  2019-09-30 22:55   ` Song Liu
@ 2019-10-01  7:10   ` Toke Høiland-Jørgensen
  2019-10-01 19:18     ` John Fastabend
  2019-10-01 19:42     ` Andrii Nakryiko
  2019-10-01 21:19   ` Song Liu
  2019-10-01 22:45   ` Daniel Borkmann
  3 siblings, 2 replies; 47+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-01  7:10 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, netdev, ast, daniel
  Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko


> +struct bpf_map_def {
> +	unsigned int type;
> +	unsigned int key_size;
> +	unsigned int value_size;
> +	unsigned int max_entries;
> +	unsigned int map_flags;
> +	unsigned int inner_map_idx;
> +	unsigned int numa_node;
> +};

Didn't we agree on no new bpf_map_def ABI in libbpf, and that all
additions should be BTF-based?

-Toke


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 1/6] selftests/bpf: undo GCC-specific bpf_helpers.h changes
  2019-09-30 18:58 ` [PATCH bpf-next 1/6] selftests/bpf: undo GCC-specific bpf_helpers.h changes Andrii Nakryiko
  2019-09-30 22:53   ` Song Liu
@ 2019-10-01 10:25   ` Ilya Leoshkevich
  2019-10-01 19:10   ` John Fastabend
  2 siblings, 0 replies; 47+ messages in thread
From: Ilya Leoshkevich @ 2019-10-01 10:25 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: bpf, netdev, ast, daniel, andrii.nakryiko, kernel-team



> Am 30.09.2019 um 20:58 schrieb Andrii Nakryiko <andriin@fb.com>:
> 
> Having GCC provide its own bpf-helper.h is not the right approach and is
> going to be changed. Undo bpf_helpers.h change before moving
> bpf_helpers.h into libbpf.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> tools/testing/selftests/bpf/bpf_helpers.h | 8 --------
> 1 file changed, 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> index 54a50699bbfd..a1d9b97b8e15 100644
> --- a/tools/testing/selftests/bpf/bpf_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> @@ -13,8 +13,6 @@
> 			 ##__VA_ARGS__);		\
> })
> 
> -#ifdef __clang__
> -
> /* helper macro to place programs, maps, license in
>  * different sections in elf_bpf file. Section names
>  * are interpreted by elf_bpf loader
> @@ -258,12 +256,6 @@ struct bpf_map_def {
> 	unsigned int numa_node;
> };
> 
> -#else
> -
> -#include <bpf-helpers.h>
> -
> -#endif
> -
> #define BPF_ANNOTATE_KV_PAIR(name, type_key, type_val)		\
> 	struct ____btf_map_##name {				\
> 		type_key key;					\
> -- 
> 2.17.1
> 

Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>

^ permalink raw reply	[flat|nested] 47+ messages in thread

* RE: [PATCH bpf-next 4/6] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers
  2019-09-30 18:58 ` [PATCH bpf-next 4/6] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers Andrii Nakryiko
@ 2019-10-01 19:07   ` John Fastabend
  2019-10-01 21:14   ` Song Liu
  1 sibling, 0 replies; 47+ messages in thread
From: John Fastabend @ 2019-10-01 19:07 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, netdev, ast, daniel
  Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Andrii Nakryiko wrote:
> Add few macros simplifying BCC-like multi-level probe reads, while also
> emitting CO-RE relocations for each read.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

LGTM.

Acked-by: John Fastabend <john.fastabend@gmail.com>

^ permalink raw reply	[flat|nested] 47+ messages in thread

* RE: [PATCH bpf-next 1/6] selftests/bpf: undo GCC-specific bpf_helpers.h changes
  2019-09-30 18:58 ` [PATCH bpf-next 1/6] selftests/bpf: undo GCC-specific bpf_helpers.h changes Andrii Nakryiko
  2019-09-30 22:53   ` Song Liu
  2019-10-01 10:25   ` Ilya Leoshkevich
@ 2019-10-01 19:10   ` John Fastabend
  2 siblings, 0 replies; 47+ messages in thread
From: John Fastabend @ 2019-10-01 19:10 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, netdev, ast, daniel
  Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Andrii Nakryiko wrote:
> Having GCC provide its own bpf-helper.h is not the right approach and is
> going to be changed. Undo bpf_helpers.h change before moving
> bpf_helpers.h into libbpf.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
>  tools/testing/selftests/bpf/bpf_helpers.h | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> index 54a50699bbfd..a1d9b97b8e15 100644
> --- a/tools/testing/selftests/bpf/bpf_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> @@ -13,8 +13,6 @@
>  			 ##__VA_ARGS__);		\
>  })
>  
> -#ifdef __clang__
> -
>  /* helper macro to place programs, maps, license in
>   * different sections in elf_bpf file. Section names
>   * are interpreted by elf_bpf loader
> @@ -258,12 +256,6 @@ struct bpf_map_def {
>  	unsigned int numa_node;
>  };
>  
> -#else
> -
> -#include <bpf-helpers.h>
> -
> -#endif
> -
>  #define BPF_ANNOTATE_KV_PAIR(name, type_key, type_val)		\
>  	struct ____btf_map_##name {				\
>  		type_key key;					\
> -- 
> 2.17.1
> 

Acked-by: John Fastabend <john.fastabend@gmail.com>

^ permalink raw reply	[flat|nested] 47+ messages in thread

* RE: [PATCH bpf-next 5/6] selftests/bpf: adjust CO-RE reloc tests for new BPF_CORE_READ macro
  2019-09-30 18:58 ` [PATCH bpf-next 5/6] selftests/bpf: adjust CO-RE reloc tests for new BPF_CORE_READ macro Andrii Nakryiko
@ 2019-10-01 19:14   ` John Fastabend
  2019-10-01 21:31     ` Andrii Nakryiko
  2019-10-01 21:47   ` Song Liu
  1 sibling, 1 reply; 47+ messages in thread
From: John Fastabend @ 2019-10-01 19:14 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, netdev, ast, daniel
  Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Andrii Nakryiko wrote:
> Given introduction of variadic BPF_CORE_READ with slightly different
> syntax and semantics, define CORE_READ, which is a thin wrapper around
> low-level bpf_core_read() macro, which in turn is just a wrapper around
> bpf_probe_read(). BPF_CORE_READ is higher-level variadic macro
> supporting multi-pointer reads and are tested separately.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
>  .../bpf/progs/test_core_reloc_arrays.c         | 10 ++++++----
>  .../bpf/progs/test_core_reloc_flavors.c        |  8 +++++---
>  .../selftests/bpf/progs/test_core_reloc_ints.c | 18 ++++++++++--------
>  .../bpf/progs/test_core_reloc_kernel.c         |  6 ++++--
>  .../selftests/bpf/progs/test_core_reloc_misc.c |  8 +++++---
>  .../selftests/bpf/progs/test_core_reloc_mods.c | 18 ++++++++++--------
>  .../bpf/progs/test_core_reloc_nesting.c        |  6 ++++--
>  .../bpf/progs/test_core_reloc_primitives.c     | 12 +++++++-----
>  .../bpf/progs/test_core_reloc_ptr_as_arr.c     |  4 +++-
>  9 files changed, 54 insertions(+), 36 deletions(-)
> 

Starting to get many layers of macros here but makes sense here.

Acked-by: John Fastabend <john.fastabend@gmail.com>

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-10-01  7:10   ` Toke Høiland-Jørgensen
@ 2019-10-01 19:18     ` John Fastabend
  2019-10-01 19:44       ` Andrii Nakryiko
  2019-10-01 19:42     ` Andrii Nakryiko
  1 sibling, 1 reply; 47+ messages in thread
From: John Fastabend @ 2019-10-01 19:18 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen, Andrii Nakryiko, bpf, netdev,
	ast, daniel
  Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Toke Høiland-Jørgensen wrote:
> 
> > +struct bpf_map_def {
> > +	unsigned int type;
> > +	unsigned int key_size;
> > +	unsigned int value_size;
> > +	unsigned int max_entries;
> > +	unsigned int map_flags;
> > +	unsigned int inner_map_idx;
> > +	unsigned int numa_node;
> > +};
> 
> Didn't we agree on no new bpf_map_def ABI in libbpf, and that all
> additions should be BTF-based?
> 
> -Toke
> 

We use libbpf on pre BTF kernels so in this case I think it makes
sense to add these fields. Having inner_map_idx there should allow
us to remove some other things we have sitting around.

.John

^ permalink raw reply	[flat|nested] 47+ messages in thread

* RE: [PATCH bpf-next 6/6] selftests/bpf: add BPF_CORE_READ and BPF_CORE_READ_STR_INTO macro tests
  2019-09-30 18:58 ` [PATCH bpf-next 6/6] selftests/bpf: add BPF_CORE_READ and BPF_CORE_READ_STR_INTO macro tests Andrii Nakryiko
@ 2019-10-01 19:19   ` John Fastabend
  2019-10-01 21:22     ` Song Liu
  0 siblings, 1 reply; 47+ messages in thread
From: John Fastabend @ 2019-10-01 19:19 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, netdev, ast, daniel
  Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Andrii Nakryiko wrote:
> Validate BPF_CORE_READ correctness and handling of up to 9 levels of
> nestedness using cyclic task->(group_leader->)*->tgid chains.
> 
> Also add a test of maximum-dpeth BPF_CORE_READ_STR_INTO() macro.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---

Acked-by: John Fastabend <john.fastabend@gmail.com>

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-10-01  7:10   ` Toke Høiland-Jørgensen
  2019-10-01 19:18     ` John Fastabend
@ 2019-10-01 19:42     ` Andrii Nakryiko
  2019-10-02  7:21       ` Toke Høiland-Jørgensen
  1 sibling, 1 reply; 47+ messages in thread
From: Andrii Nakryiko @ 2019-10-01 19:42 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team

On Tue, Oct 1, 2019 at 12:10 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
>
> > +struct bpf_map_def {
> > +     unsigned int type;
> > +     unsigned int key_size;
> > +     unsigned int value_size;
> > +     unsigned int max_entries;
> > +     unsigned int map_flags;
> > +     unsigned int inner_map_idx;
> > +     unsigned int numa_node;
> > +};
>
> Didn't we agree on no new bpf_map_def ABI in libbpf, and that all
> additions should be BTF-based?

Oh yes, we did, sorry, this is an oversight. I really appreciate you
pointing this out ;)
I'll go over bpf_helpers.h carefully and will double-check if we don't
have any other custom selftests-only stuff left there.

>
> -Toke
>

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-10-01 19:18     ` John Fastabend
@ 2019-10-01 19:44       ` Andrii Nakryiko
  2019-10-01 22:00         ` John Fastabend
  2019-10-02  7:25         ` Toke Høiland-Jørgensen
  0 siblings, 2 replies; 47+ messages in thread
From: Andrii Nakryiko @ 2019-10-01 19:44 UTC (permalink / raw)
  To: John Fastabend
  Cc: Toke Høiland-Jørgensen, Andrii Nakryiko, bpf,
	Networking, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Tue, Oct 1, 2019 at 12:18 PM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Toke Høiland-Jørgensen wrote:
> >
> > > +struct bpf_map_def {
> > > +   unsigned int type;
> > > +   unsigned int key_size;
> > > +   unsigned int value_size;
> > > +   unsigned int max_entries;
> > > +   unsigned int map_flags;
> > > +   unsigned int inner_map_idx;
> > > +   unsigned int numa_node;
> > > +};
> >
> > Didn't we agree on no new bpf_map_def ABI in libbpf, and that all
> > additions should be BTF-based?
> >
> > -Toke
> >
>
> We use libbpf on pre BTF kernels so in this case I think it makes
> sense to add these fields. Having inner_map_idx there should allow
> us to remove some other things we have sitting around.

We had a discussion about supporting non-BTF and non-standard BPF map
definition before and it's still on my TODO list to go and do a proof
of concept how that can look like and what libbpf changes we need to
make. Right now libbpf doesn't support those new fields anyway, so we
shouldn't add them to public API.

>
> .John

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 4/6] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers
  2019-09-30 18:58 ` [PATCH bpf-next 4/6] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers Andrii Nakryiko
  2019-10-01 19:07   ` John Fastabend
@ 2019-10-01 21:14   ` Song Liu
  2019-10-01 21:25     ` Andrii Nakryiko
  1 sibling, 1 reply; 47+ messages in thread
From: Song Liu @ 2019-10-01 21:14 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: bpf, netdev, Alexei Starovoitov, daniel, andrii.nakryiko, Kernel Team


> On Sep 30, 2019, at 11:58 AM, Andrii Nakryiko <andriin@fb.com> wrote:
> 
> Add few macros simplifying BCC-like multi-level probe reads, while also
> emitting CO-RE relocations for each read.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> tools/lib/bpf/bpf_helpers.h | 151 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 147 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index a1d9b97b8e15..51e7b11d53e8 100644
> --- a/tools/lib/bpf/bpf_helpers.h
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -19,6 +19,10 @@
>  */
> #define SEC(NAME) __attribute__((section(NAME), used))
> 
> +#ifndef __always_inline
> +#define __always_inline __attribute__((always_inline))
> +#endif
> +
> /* helper functions called from eBPF programs written in C */
> static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
> 	(void *) BPF_FUNC_map_lookup_elem;
> @@ -505,7 +509,7 @@ struct pt_regs;
> #endif
> 
> /*
> - * BPF_CORE_READ abstracts away bpf_probe_read() call and captures offset
> + * bpf_core_read() abstracts away bpf_probe_read() call and captures field
>  * relocation for source address using __builtin_preserve_access_index()
>  * built-in, provided by Clang.
>  *
> @@ -520,8 +524,147 @@ struct pt_regs;
>  * actual field offset, based on target kernel BTF type that matches original
>  * (local) BTF, used to record relocation.
>  */
> -#define BPF_CORE_READ(dst, src)						\
> -	bpf_probe_read((dst), sizeof(*(src)),				\
> -		       __builtin_preserve_access_index(src))
> +#define bpf_core_read(dst, sz, src)					    \
> +	bpf_probe_read(dst, sz,						    \
> +		       (const void *)__builtin_preserve_access_index(src))
> +
> +/*
> + * bpf_core_read_str() is a thin wrapper around bpf_probe_read_str()
> + * additionally emitting BPF CO-RE field relocation for specified source
> + * argument.
> + */
> +#define bpf_core_read_str(dst, sz, src)					    \
> +	bpf_probe_read_str(dst, sz,					    \
> +			   (const void *)__builtin_preserve_access_index(src))
> +
> +#define ___concat(a, b) a ## b
> +#define ___apply(fn, n) ___concat(fn, n)
> +#define ___nth(_1, _2, _3, _4, _5, _6, _7, _8, _9, _10, __11, N, ...) N

We are adding many marcos with simple names: ___apply(), ___nth. So I worry
they may conflict with macro definitions from other libraries. Shall we hide
them in .c files or prefix/postfix them with _libbpf or something?

Thanks,
Song


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-09-30 18:58 ` [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf Andrii Nakryiko
  2019-09-30 22:55   ` Song Liu
  2019-10-01  7:10   ` Toke Høiland-Jørgensen
@ 2019-10-01 21:19   ` Song Liu
  2019-10-01 21:28     ` Andrii Nakryiko
  2019-10-01 22:45   ` Daniel Borkmann
  3 siblings, 1 reply; 47+ messages in thread
From: Song Liu @ 2019-10-01 21:19 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: bpf, netdev, Alexei Starovoitov, daniel, andrii.nakryiko, Kernel Team



> On Sep 30, 2019, at 11:58 AM, Andrii Nakryiko <andriin@fb.com> wrote:
> 
> Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
> are installed along the other libbpf headers.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> tools/lib/bpf/Makefile      |   4 +-
> tools/lib/bpf/bpf_endian.h  |  72 +++++
> tools/lib/bpf/bpf_helpers.h | 527 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 602 insertions(+), 1 deletion(-)
> create mode 100644 tools/lib/bpf/bpf_endian.h
> create mode 100644 tools/lib/bpf/bpf_helpers.h
> 
> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> index c6f94cffe06e..2ff345981803 100644
> --- a/tools/lib/bpf/Makefile
> +++ b/tools/lib/bpf/Makefile
> @@ -240,7 +240,9 @@ install_headers:
> 		$(call do_install,libbpf.h,$(prefix)/include/bpf,644); \
> 		$(call do_install,btf.h,$(prefix)/include/bpf,644); \
> 		$(call do_install,libbpf_util.h,$(prefix)/include/bpf,644); \
> -		$(call do_install,xsk.h,$(prefix)/include/bpf,644);
> +		$(call do_install,xsk.h,$(prefix)/include/bpf,644); \
> +		$(call do_install,bpf_helpers.h,$(prefix)/include/bpf,644); \
> +		$(call do_install,bpf_endian.h,$(prefix)/include/bpf,644);
> 
> install_pkgconfig: $(PC_FILE)
> 	$(call QUIET_INSTALL, $(PC_FILE)) \
> diff --git a/tools/lib/bpf/bpf_endian.h b/tools/lib/bpf/bpf_endian.h
> new file mode 100644
> index 000000000000..fbe28008450f
> --- /dev/null
> +++ b/tools/lib/bpf/bpf_endian.h
> @@ -0,0 +1,72 @@
> +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> +#ifndef __BPF_ENDIAN__
> +#define __BPF_ENDIAN__
> +
> +#include <linux/stddef.h>
> +#include <linux/swab.h>
> +
> +/* LLVM's BPF target selects the endianness of the CPU
> + * it compiles on, or the user specifies (bpfel/bpfeb),
> + * respectively. The used __BYTE_ORDER__ is defined by
> + * the compiler, we cannot rely on __BYTE_ORDER from
> + * libc headers, since it doesn't reflect the actual
> + * requested byte order.
> + *
> + * Note, LLVM's BPF target has different __builtin_bswapX()
> + * semantics. It does map to BPF_ALU | BPF_END | BPF_TO_BE
> + * in bpfel and bpfeb case, which means below, that we map
> + * to cpu_to_be16(). We could use it unconditionally in BPF
> + * case, but better not rely on it, so that this header here
> + * can be used from application and BPF program side, which
> + * use different targets.
> + */
> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +# define __bpf_ntohs(x)			__builtin_bswap16(x)
> +# define __bpf_htons(x)			__builtin_bswap16(x)
> +# define __bpf_constant_ntohs(x)	___constant_swab16(x)
> +# define __bpf_constant_htons(x)	___constant_swab16(x)
> +# define __bpf_ntohl(x)			__builtin_bswap32(x)
> +# define __bpf_htonl(x)			__builtin_bswap32(x)
> +# define __bpf_constant_ntohl(x)	___constant_swab32(x)
> +# define __bpf_constant_htonl(x)	___constant_swab32(x)
> +# define __bpf_be64_to_cpu(x)		__builtin_bswap64(x)
> +# define __bpf_cpu_to_be64(x)		__builtin_bswap64(x)
> +# define __bpf_constant_be64_to_cpu(x)	___constant_swab64(x)
> +# define __bpf_constant_cpu_to_be64(x)	___constant_swab64(x)
> +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> +# define __bpf_ntohs(x)			(x)
> +# define __bpf_htons(x)			(x)
> +# define __bpf_constant_ntohs(x)	(x)
> +# define __bpf_constant_htons(x)	(x)
> +# define __bpf_ntohl(x)			(x)
> +# define __bpf_htonl(x)			(x)
> +# define __bpf_constant_ntohl(x)	(x)
> +# define __bpf_constant_htonl(x)	(x)
> +# define __bpf_be64_to_cpu(x)		(x)
> +# define __bpf_cpu_to_be64(x)		(x)
> +# define __bpf_constant_be64_to_cpu(x)  (x)
> +# define __bpf_constant_cpu_to_be64(x)  (x)
> +#else
> +# error "Fix your compiler's __BYTE_ORDER__?!"
> +#endif
> +
> +#define bpf_htons(x)				\
> +	(__builtin_constant_p(x) ?		\
> +	 __bpf_constant_htons(x) : __bpf_htons(x))
> +#define bpf_ntohs(x)				\
> +	(__builtin_constant_p(x) ?		\
> +	 __bpf_constant_ntohs(x) : __bpf_ntohs(x))
> +#define bpf_htonl(x)				\
> +	(__builtin_constant_p(x) ?		\
> +	 __bpf_constant_htonl(x) : __bpf_htonl(x))
> +#define bpf_ntohl(x)				\
> +	(__builtin_constant_p(x) ?		\
> +	 __bpf_constant_ntohl(x) : __bpf_ntohl(x))
> +#define bpf_cpu_to_be64(x)			\
> +	(__builtin_constant_p(x) ?		\
> +	 __bpf_constant_cpu_to_be64(x) : __bpf_cpu_to_be64(x))
> +#define bpf_be64_to_cpu(x)			\
> +	(__builtin_constant_p(x) ?		\
> +	 __bpf_constant_be64_to_cpu(x) : __bpf_be64_to_cpu(x))
> +
> +#endif /* __BPF_ENDIAN__ */
> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> new file mode 100644
> index 000000000000..a1d9b97b8e15
> --- /dev/null
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -0,0 +1,527 @@
> +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> +#ifndef __BPF_HELPERS__
> +#define __BPF_HELPERS__
> +
> +#define __uint(name, val) int (*name)[val]
> +#define __type(name, val) val *name

Similar to the concern with 4/6, maybe we should rename/prefix/postfix
these two macros?

Thanks,
Song


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 6/6] selftests/bpf: add BPF_CORE_READ and BPF_CORE_READ_STR_INTO macro tests
  2019-10-01 19:19   ` John Fastabend
@ 2019-10-01 21:22     ` Song Liu
  0 siblings, 0 replies; 47+ messages in thread
From: Song Liu @ 2019-10-01 21:22 UTC (permalink / raw)
  To: John Fastabend
  Cc: Andrii Nakryiko, bpf, netdev, Alexei Starovoitov, daniel,
	andrii.nakryiko, Kernel Team



> On Oct 1, 2019, at 12:19 PM, John Fastabend <john.fastabend@gmail.com> wrote:
> 
> Andrii Nakryiko wrote:
>> Validate BPF_CORE_READ correctness and handling of up to 9 levels of
>> nestedness using cyclic task->(group_leader->)*->tgid chains.
>> 
>> Also add a test of maximum-dpeth BPF_CORE_READ_STR_INTO() macro.
>> 
>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>> ---
> 
> Acked-by: John Fastabend <john.fastabend@gmail.com>

Acked-by: Song Liu <songliubraving@fb.com> 


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 4/6] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers
  2019-10-01 21:14   ` Song Liu
@ 2019-10-01 21:25     ` Andrii Nakryiko
  2019-10-01 21:46       ` Song Liu
  0 siblings, 1 reply; 47+ messages in thread
From: Andrii Nakryiko @ 2019-10-01 21:25 UTC (permalink / raw)
  To: Song Liu
  Cc: Andrii Nakryiko, bpf, netdev, Alexei Starovoitov, daniel, Kernel Team

On Tue, Oct 1, 2019 at 2:14 PM Song Liu <songliubraving@fb.com> wrote:
>
>
> > On Sep 30, 2019, at 11:58 AM, Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > Add few macros simplifying BCC-like multi-level probe reads, while also
> > emitting CO-RE relocations for each read.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > tools/lib/bpf/bpf_helpers.h | 151 +++++++++++++++++++++++++++++++++++-
> > 1 file changed, 147 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > index a1d9b97b8e15..51e7b11d53e8 100644
> > --- a/tools/lib/bpf/bpf_helpers.h
> > +++ b/tools/lib/bpf/bpf_helpers.h
> > @@ -19,6 +19,10 @@
> >  */
> > #define SEC(NAME) __attribute__((section(NAME), used))
> >
> > +#ifndef __always_inline
> > +#define __always_inline __attribute__((always_inline))
> > +#endif
> > +
> > /* helper functions called from eBPF programs written in C */
> > static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
> >       (void *) BPF_FUNC_map_lookup_elem;
> > @@ -505,7 +509,7 @@ struct pt_regs;
> > #endif
> >
> > /*
> > - * BPF_CORE_READ abstracts away bpf_probe_read() call and captures offset
> > + * bpf_core_read() abstracts away bpf_probe_read() call and captures field
> >  * relocation for source address using __builtin_preserve_access_index()
> >  * built-in, provided by Clang.
> >  *
> > @@ -520,8 +524,147 @@ struct pt_regs;
> >  * actual field offset, based on target kernel BTF type that matches original
> >  * (local) BTF, used to record relocation.
> >  */
> > -#define BPF_CORE_READ(dst, src)                                              \
> > -     bpf_probe_read((dst), sizeof(*(src)),                           \
> > -                    __builtin_preserve_access_index(src))
> > +#define bpf_core_read(dst, sz, src)                                      \
> > +     bpf_probe_read(dst, sz,                                             \
> > +                    (const void *)__builtin_preserve_access_index(src))
> > +
> > +/*
> > + * bpf_core_read_str() is a thin wrapper around bpf_probe_read_str()
> > + * additionally emitting BPF CO-RE field relocation for specified source
> > + * argument.
> > + */
> > +#define bpf_core_read_str(dst, sz, src)                                          \
> > +     bpf_probe_read_str(dst, sz,                                         \
> > +                        (const void *)__builtin_preserve_access_index(src))
> > +
> > +#define ___concat(a, b) a ## b
> > +#define ___apply(fn, n) ___concat(fn, n)
> > +#define ___nth(_1, _2, _3, _4, _5, _6, _7, _8, _9, _10, __11, N, ...) N
>
> We are adding many marcos with simple names: ___apply(), ___nth. So I worry
> they may conflict with macro definitions from other libraries. Shall we hide
> them in .c files or prefix/postfix them with _libbpf or something?

Keep in mind, this is the header that's included from BPF code.

They are prefixed with three underscores, I was hoping it's good
enough to avoid accidental conflicts. It's unlikely someone will have
macros with the same names **in BPF-side code**.
Prefixing with _libbpf is an option, but it will make it super ugly
and hard to follow (I've spent a bunch of time to even get it to the
current state), so I'd like to avoid that.

>
> Thanks,
> Song
>

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-10-01 21:19   ` Song Liu
@ 2019-10-01 21:28     ` Andrii Nakryiko
  0 siblings, 0 replies; 47+ messages in thread
From: Andrii Nakryiko @ 2019-10-01 21:28 UTC (permalink / raw)
  To: Song Liu
  Cc: Andrii Nakryiko, bpf, netdev, Alexei Starovoitov, daniel, Kernel Team

On Tue, Oct 1, 2019 at 2:19 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Sep 30, 2019, at 11:58 AM, Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
> > are installed along the other libbpf headers.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > tools/lib/bpf/Makefile      |   4 +-
> > tools/lib/bpf/bpf_endian.h  |  72 +++++
> > tools/lib/bpf/bpf_helpers.h | 527 ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 602 insertions(+), 1 deletion(-)
> > create mode 100644 tools/lib/bpf/bpf_endian.h
> > create mode 100644 tools/lib/bpf/bpf_helpers.h
> >

[...]

> > +#endif /* __BPF_ENDIAN__ */
> > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > new file mode 100644
> > index 000000000000..a1d9b97b8e15
> > --- /dev/null
> > +++ b/tools/lib/bpf/bpf_helpers.h
> > @@ -0,0 +1,527 @@
> > +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> > +#ifndef __BPF_HELPERS__
> > +#define __BPF_HELPERS__
> > +
> > +#define __uint(name, val) int (*name)[val]
> > +#define __type(name, val) val *name
>
> Similar to the concern with 4/6, maybe we should rename/prefix/postfix
> these two macros?

Those were specifically named so they are as clear and clean in user
code as possible, it was an explicit goal. Ideally they would be
"uint" and "type", but that's pushing it too far :)

>
> Thanks,
> Song
>

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 5/6] selftests/bpf: adjust CO-RE reloc tests for new BPF_CORE_READ macro
  2019-10-01 19:14   ` John Fastabend
@ 2019-10-01 21:31     ` Andrii Nakryiko
  0 siblings, 0 replies; 47+ messages in thread
From: Andrii Nakryiko @ 2019-10-01 21:31 UTC (permalink / raw)
  To: John Fastabend
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team

On Tue, Oct 1, 2019 at 12:14 PM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Andrii Nakryiko wrote:
> > Given introduction of variadic BPF_CORE_READ with slightly different
> > syntax and semantics, define CORE_READ, which is a thin wrapper around
> > low-level bpf_core_read() macro, which in turn is just a wrapper around
> > bpf_probe_read(). BPF_CORE_READ is higher-level variadic macro
> > supporting multi-pointer reads and are tested separately.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >  .../bpf/progs/test_core_reloc_arrays.c         | 10 ++++++----
> >  .../bpf/progs/test_core_reloc_flavors.c        |  8 +++++---
> >  .../selftests/bpf/progs/test_core_reloc_ints.c | 18 ++++++++++--------
> >  .../bpf/progs/test_core_reloc_kernel.c         |  6 ++++--
> >  .../selftests/bpf/progs/test_core_reloc_misc.c |  8 +++++---
> >  .../selftests/bpf/progs/test_core_reloc_mods.c | 18 ++++++++++--------
> >  .../bpf/progs/test_core_reloc_nesting.c        |  6 ++++--
> >  .../bpf/progs/test_core_reloc_primitives.c     | 12 +++++++-----
> >  .../bpf/progs/test_core_reloc_ptr_as_arr.c     |  4 +++-
> >  9 files changed, 54 insertions(+), 36 deletions(-)
> >
>
> Starting to get many layers of macros here but makes sense here.

Yeah, a bit. I was considering to either switch to bpf_core_read()
with explicit sizeof or making bpf_core_read() deriving sizeof(), but
didn't because:

1. wanted to keep bpf_core_read() a direct "substitute" for bpf_probe_read()
2. figured one copy-pasted #define for each of few files is small
enough price for much more readable tests

>
> Acked-by: John Fastabend <john.fastabend@gmail.com>

Thanks for review!

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 4/6] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers
  2019-10-01 21:25     ` Andrii Nakryiko
@ 2019-10-01 21:46       ` Song Liu
  2019-10-01 22:42         ` Andrii Nakryiko
  0 siblings, 1 reply; 47+ messages in thread
From: Song Liu @ 2019-10-01 21:46 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, bpf, netdev, Alexei Starovoitov, daniel, Kernel Team



> On Oct 1, 2019, at 2:25 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> On Tue, Oct 1, 2019 at 2:14 PM Song Liu <songliubraving@fb.com> wrote:
>> 
>> 
>>> On Sep 30, 2019, at 11:58 AM, Andrii Nakryiko <andriin@fb.com> wrote:
>>> 
>>> Add few macros simplifying BCC-like multi-level probe reads, while also
>>> emitting CO-RE relocations for each read.
>>> 
>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>> ---
>>> tools/lib/bpf/bpf_helpers.h | 151 +++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 147 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
>>> index a1d9b97b8e15..51e7b11d53e8 100644
>>> --- a/tools/lib/bpf/bpf_helpers.h
>>> +++ b/tools/lib/bpf/bpf_helpers.h
>>> @@ -19,6 +19,10 @@
>>> */
>>> #define SEC(NAME) __attribute__((section(NAME), used))
>>> 
>>> +#ifndef __always_inline
>>> +#define __always_inline __attribute__((always_inline))
>>> +#endif
>>> +
>>> /* helper functions called from eBPF programs written in C */
>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
>>>      (void *) BPF_FUNC_map_lookup_elem;
>>> @@ -505,7 +509,7 @@ struct pt_regs;
>>> #endif
>>> 
>>> /*
>>> - * BPF_CORE_READ abstracts away bpf_probe_read() call and captures offset
>>> + * bpf_core_read() abstracts away bpf_probe_read() call and captures field
>>> * relocation for source address using __builtin_preserve_access_index()
>>> * built-in, provided by Clang.
>>> *
>>> @@ -520,8 +524,147 @@ struct pt_regs;
>>> * actual field offset, based on target kernel BTF type that matches original
>>> * (local) BTF, used to record relocation.
>>> */
>>> -#define BPF_CORE_READ(dst, src)                                              \
>>> -     bpf_probe_read((dst), sizeof(*(src)),                           \
>>> -                    __builtin_preserve_access_index(src))
>>> +#define bpf_core_read(dst, sz, src)                                      \
>>> +     bpf_probe_read(dst, sz,                                             \
>>> +                    (const void *)__builtin_preserve_access_index(src))
>>> +
>>> +/*
>>> + * bpf_core_read_str() is a thin wrapper around bpf_probe_read_str()
>>> + * additionally emitting BPF CO-RE field relocation for specified source
>>> + * argument.
>>> + */
>>> +#define bpf_core_read_str(dst, sz, src)                                          \
>>> +     bpf_probe_read_str(dst, sz,                                         \
>>> +                        (const void *)__builtin_preserve_access_index(src))
>>> +
>>> +#define ___concat(a, b) a ## b
>>> +#define ___apply(fn, n) ___concat(fn, n)
>>> +#define ___nth(_1, _2, _3, _4, _5, _6, _7, _8, _9, _10, __11, N, ...) N
>> 
>> We are adding many marcos with simple names: ___apply(), ___nth. So I worry
>> they may conflict with macro definitions from other libraries. Shall we hide
>> them in .c files or prefix/postfix them with _libbpf or something?
> 
> Keep in mind, this is the header that's included from BPF code.
> 
> They are prefixed with three underscores, I was hoping it's good
> enough to avoid accidental conflicts. It's unlikely someone will have
> macros with the same names **in BPF-side code**.

BPF side code would include kernel headers. So there are many headers
to conflict with. And we won't know until somebody want to trace certain
kernel structure. 

> Prefixing with _libbpf is an option, but it will make it super ugly
> and hard to follow (I've spent a bunch of time to even get it to the
> current state), so I'd like to avoid that.

BPF programs will not use these marcos directly, so I feel it is OK to 
pay the pain of _libbpf prefix, as it is contained within this file. 

Thanks,
Song 




^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 5/6] selftests/bpf: adjust CO-RE reloc tests for new BPF_CORE_READ macro
  2019-09-30 18:58 ` [PATCH bpf-next 5/6] selftests/bpf: adjust CO-RE reloc tests for new BPF_CORE_READ macro Andrii Nakryiko
  2019-10-01 19:14   ` John Fastabend
@ 2019-10-01 21:47   ` Song Liu
  1 sibling, 0 replies; 47+ messages in thread
From: Song Liu @ 2019-10-01 21:47 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: bpf, netdev, Alexei Starovoitov, daniel, andrii.nakryiko, Kernel Team



> On Sep 30, 2019, at 11:58 AM, Andrii Nakryiko <andriin@fb.com> wrote:
> 
> Given introduction of variadic BPF_CORE_READ with slightly different
> syntax and semantics, define CORE_READ, which is a thin wrapper around
> low-level bpf_core_read() macro, which in turn is just a wrapper around
> bpf_probe_read(). BPF_CORE_READ is higher-level variadic macro
> supporting multi-pointer reads and are tested separately.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> 

Acked-by: Song Liu <songliubraving@fb.com> 



^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-10-01 19:44       ` Andrii Nakryiko
@ 2019-10-01 22:00         ` John Fastabend
  2019-10-02  7:25         ` Toke Høiland-Jørgensen
  1 sibling, 0 replies; 47+ messages in thread
From: John Fastabend @ 2019-10-01 22:00 UTC (permalink / raw)
  To: Andrii Nakryiko, John Fastabend
  Cc: Toke Høiland-Jørgensen, Andrii Nakryiko, bpf,
	Networking, Alexei Starovoitov, Daniel Borkmann, Kernel Team

Andrii Nakryiko wrote:
> On Tue, Oct 1, 2019 at 12:18 PM John Fastabend <john.fastabend@gmail.com> wrote:
> >
> > Toke Høiland-Jørgensen wrote:
> > >
> > > > +struct bpf_map_def {
> > > > +   unsigned int type;
> > > > +   unsigned int key_size;
> > > > +   unsigned int value_size;
> > > > +   unsigned int max_entries;
> > > > +   unsigned int map_flags;
> > > > +   unsigned int inner_map_idx;
> > > > +   unsigned int numa_node;
> > > > +};
> > >
> > > Didn't we agree on no new bpf_map_def ABI in libbpf, and that all
> > > additions should be BTF-based?
> > >
> > > -Toke
> > >
> >
> > We use libbpf on pre BTF kernels so in this case I think it makes
> > sense to add these fields. Having inner_map_idx there should allow
> > us to remove some other things we have sitting around.
> 
> We had a discussion about supporting non-BTF and non-standard BPF map
> definition before and it's still on my TODO list to go and do a proof
> of concept how that can look like and what libbpf changes we need to
> make. Right now libbpf doesn't support those new fields anyway, so we
> shouldn't add them to public API.

I'm carrying around a patch for perf_event_open_probe() to get it working
without BTF I'll send that out today/tomorrow. It seems enough to get
all the basic prog load, maps reused/pinned, etc at least for my use
case.

> 
> >
> > .John



^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 4/6] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers
  2019-10-01 21:46       ` Song Liu
@ 2019-10-01 22:42         ` Andrii Nakryiko
  2019-10-01 23:44           ` Song Liu
  0 siblings, 1 reply; 47+ messages in thread
From: Andrii Nakryiko @ 2019-10-01 22:42 UTC (permalink / raw)
  To: Song Liu
  Cc: Andrii Nakryiko, bpf, netdev, Alexei Starovoitov, daniel, Kernel Team

On Tue, Oct 1, 2019 at 2:46 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Oct 1, 2019, at 2:25 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Oct 1, 2019 at 2:14 PM Song Liu <songliubraving@fb.com> wrote:
> >>
> >>
> >>> On Sep 30, 2019, at 11:58 AM, Andrii Nakryiko <andriin@fb.com> wrote:
> >>>
> >>> Add few macros simplifying BCC-like multi-level probe reads, while also
> >>> emitting CO-RE relocations for each read.
> >>>
> >>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> >>> ---
> >>> tools/lib/bpf/bpf_helpers.h | 151 +++++++++++++++++++++++++++++++++++-
> >>> 1 file changed, 147 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> >>> index a1d9b97b8e15..51e7b11d53e8 100644
> >>> --- a/tools/lib/bpf/bpf_helpers.h
> >>> +++ b/tools/lib/bpf/bpf_helpers.h
> >>> @@ -19,6 +19,10 @@
> >>> */
> >>> #define SEC(NAME) __attribute__((section(NAME), used))
> >>>
> >>> +#ifndef __always_inline
> >>> +#define __always_inline __attribute__((always_inline))
> >>> +#endif
> >>> +
> >>> /* helper functions called from eBPF programs written in C */
> >>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
> >>>      (void *) BPF_FUNC_map_lookup_elem;
> >>> @@ -505,7 +509,7 @@ struct pt_regs;
> >>> #endif
> >>>
> >>> /*
> >>> - * BPF_CORE_READ abstracts away bpf_probe_read() call and captures offset
> >>> + * bpf_core_read() abstracts away bpf_probe_read() call and captures field
> >>> * relocation for source address using __builtin_preserve_access_index()
> >>> * built-in, provided by Clang.
> >>> *
> >>> @@ -520,8 +524,147 @@ struct pt_regs;
> >>> * actual field offset, based on target kernel BTF type that matches original
> >>> * (local) BTF, used to record relocation.
> >>> */
> >>> -#define BPF_CORE_READ(dst, src)                                              \
> >>> -     bpf_probe_read((dst), sizeof(*(src)),                           \
> >>> -                    __builtin_preserve_access_index(src))
> >>> +#define bpf_core_read(dst, sz, src)                                      \
> >>> +     bpf_probe_read(dst, sz,                                             \
> >>> +                    (const void *)__builtin_preserve_access_index(src))
> >>> +
> >>> +/*
> >>> + * bpf_core_read_str() is a thin wrapper around bpf_probe_read_str()
> >>> + * additionally emitting BPF CO-RE field relocation for specified source
> >>> + * argument.
> >>> + */
> >>> +#define bpf_core_read_str(dst, sz, src)                                          \
> >>> +     bpf_probe_read_str(dst, sz,                                         \
> >>> +                        (const void *)__builtin_preserve_access_index(src))
> >>> +
> >>> +#define ___concat(a, b) a ## b
> >>> +#define ___apply(fn, n) ___concat(fn, n)
> >>> +#define ___nth(_1, _2, _3, _4, _5, _6, _7, _8, _9, _10, __11, N, ...) N
> >>
> >> We are adding many marcos with simple names: ___apply(), ___nth. So I worry
> >> they may conflict with macro definitions from other libraries. Shall we hide
> >> them in .c files or prefix/postfix them with _libbpf or something?
> >
> > Keep in mind, this is the header that's included from BPF code.
> >
> > They are prefixed with three underscores, I was hoping it's good
> > enough to avoid accidental conflicts. It's unlikely someone will have
> > macros with the same names **in BPF-side code**.
>
> BPF side code would include kernel headers. So there are many headers
> to conflict with. And we won't know until somebody want to trace certain
> kernel structure.

We have all the kernel sources at our disposal, there's no need to
guess :) There is no instance of ___apply, ___concat, ___nth,
___arrow, ___last, ___nolast, or ___type, not even speaking about
other more specific names. There are currently two instances of
"____last_____" used in a string. And I'm certainly not afraid that
user code can use triple-underscored identifier with exact those names
and complain about bpf_helpers.h :)

>
> > Prefixing with _libbpf is an option, but it will make it super ugly
> > and hard to follow (I've spent a bunch of time to even get it to the
> > current state), so I'd like to avoid that.
>
> BPF programs will not use these marcos directly, so I feel it is OK to
> pay the pain of _libbpf prefix, as it is contained within this file.

I disagree, having more convoluted multi-line macros will eventually
lead to a stupid mistake or typo, which could have been spotted
otherwise. Plus, if user screws up (which is inevitable anyway) usage
of these macro, he will be presented with long and incomprehensible
chain of macro with more obscure names than necessary. If you think
that internal names don't matter, ask anyone who had to decipher
compilation errors involving C++ standard library templates. I'd be OK
with that if there was a real risk of name conflict, but I disagree
with the premise.

>
> Thanks,
> Song
>
>
>

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-09-30 18:58 ` [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf Andrii Nakryiko
                     ` (2 preceding siblings ...)
  2019-10-01 21:19   ` Song Liu
@ 2019-10-01 22:45   ` Daniel Borkmann
  2019-10-01 23:31     ` Andrii Nakryiko
  3 siblings, 1 reply; 47+ messages in thread
From: Daniel Borkmann @ 2019-10-01 22:45 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: bpf, netdev, ast, andrii.nakryiko, kernel-team

On Mon, Sep 30, 2019 at 11:58:51AM -0700, Andrii Nakryiko wrote:
> Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
> are installed along the other libbpf headers.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

[...]
> new file mode 100644
> index 000000000000..fbe28008450f
> --- /dev/null
> +++ b/tools/lib/bpf/bpf_endian.h
> @@ -0,0 +1,72 @@
> +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> +#ifndef __BPF_ENDIAN__
> +#define __BPF_ENDIAN__
> +
> +#include <linux/stddef.h>
> +#include <linux/swab.h>
> +
> +/* LLVM's BPF target selects the endianness of the CPU
> + * it compiles on, or the user specifies (bpfel/bpfeb),
> + * respectively. The used __BYTE_ORDER__ is defined by
> + * the compiler, we cannot rely on __BYTE_ORDER from
> + * libc headers, since it doesn't reflect the actual
> + * requested byte order.
> + *
> + * Note, LLVM's BPF target has different __builtin_bswapX()
> + * semantics. It does map to BPF_ALU | BPF_END | BPF_TO_BE
> + * in bpfel and bpfeb case, which means below, that we map
> + * to cpu_to_be16(). We could use it unconditionally in BPF
> + * case, but better not rely on it, so that this header here
> + * can be used from application and BPF program side, which
> + * use different targets.
> + */
> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +# define __bpf_ntohs(x)			__builtin_bswap16(x)
> +# define __bpf_htons(x)			__builtin_bswap16(x)
> +# define __bpf_constant_ntohs(x)	___constant_swab16(x)
> +# define __bpf_constant_htons(x)	___constant_swab16(x)
> +# define __bpf_ntohl(x)			__builtin_bswap32(x)
> +# define __bpf_htonl(x)			__builtin_bswap32(x)
> +# define __bpf_constant_ntohl(x)	___constant_swab32(x)
> +# define __bpf_constant_htonl(x)	___constant_swab32(x)
> +# define __bpf_be64_to_cpu(x)		__builtin_bswap64(x)
> +# define __bpf_cpu_to_be64(x)		__builtin_bswap64(x)
> +# define __bpf_constant_be64_to_cpu(x)	___constant_swab64(x)
> +# define __bpf_constant_cpu_to_be64(x)	___constant_swab64(x)
> +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> +# define __bpf_ntohs(x)			(x)
> +# define __bpf_htons(x)			(x)
> +# define __bpf_constant_ntohs(x)	(x)
> +# define __bpf_constant_htons(x)	(x)
> +# define __bpf_ntohl(x)			(x)
> +# define __bpf_htonl(x)			(x)
> +# define __bpf_constant_ntohl(x)	(x)
> +# define __bpf_constant_htonl(x)	(x)
> +# define __bpf_be64_to_cpu(x)		(x)
> +# define __bpf_cpu_to_be64(x)		(x)
> +# define __bpf_constant_be64_to_cpu(x)  (x)
> +# define __bpf_constant_cpu_to_be64(x)  (x)
> +#else
> +# error "Fix your compiler's __BYTE_ORDER__?!"
> +#endif
> +
> +#define bpf_htons(x)				\
> +	(__builtin_constant_p(x) ?		\
> +	 __bpf_constant_htons(x) : __bpf_htons(x))
> +#define bpf_ntohs(x)				\
> +	(__builtin_constant_p(x) ?		\
> +	 __bpf_constant_ntohs(x) : __bpf_ntohs(x))
> +#define bpf_htonl(x)				\
> +	(__builtin_constant_p(x) ?		\
> +	 __bpf_constant_htonl(x) : __bpf_htonl(x))
> +#define bpf_ntohl(x)				\
> +	(__builtin_constant_p(x) ?		\
> +	 __bpf_constant_ntohl(x) : __bpf_ntohl(x))
> +#define bpf_cpu_to_be64(x)			\
> +	(__builtin_constant_p(x) ?		\
> +	 __bpf_constant_cpu_to_be64(x) : __bpf_cpu_to_be64(x))
> +#define bpf_be64_to_cpu(x)			\
> +	(__builtin_constant_p(x) ?		\
> +	 __bpf_constant_be64_to_cpu(x) : __bpf_be64_to_cpu(x))

Nit: if we move this into a public API for libbpf, we should probably
also define for sake of consistency a bpf_cpu_to_be{64,32,16}() and
bpf_be{64,32,16}_to_cpu() and have the latter two point to the existing
bpf_hton{l,s}() and bpf_ntoh{l,s}() macros.

> +#endif /* __BPF_ENDIAN__ */

> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> new file mode 100644
> index 000000000000..a1d9b97b8e15
> --- /dev/null
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -0,0 +1,527 @@
> +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> +#ifndef __BPF_HELPERS__
> +#define __BPF_HELPERS__
> +
> +#define __uint(name, val) int (*name)[val]
> +#define __type(name, val) val *name
> +
> +/* helper macro to print out debug messages */
> +#define bpf_printk(fmt, ...)				\
> +({							\
> +	char ____fmt[] = fmt;				\
> +	bpf_trace_printk(____fmt, sizeof(____fmt),	\
> +			 ##__VA_ARGS__);		\
> +})

Did you double check if by now via .rodata we can have the fmt as
const char * and add __attribute__ ((format (printf, 1, 2))) to it?
If that works we should avoid having to copy the string as above in
the API.

> +/* helper macro to place programs, maps, license in
> + * different sections in elf_bpf file. Section names
> + * are interpreted by elf_bpf loader
> + */
> +#define SEC(NAME) __attribute__((section(NAME), used))
> +
> +/* helper functions called from eBPF programs written in C */

As mentioned earlier, the whole helper function description below
should get a big cleanup in here when moved into libbpf API.

> +static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
> +	(void *) BPF_FUNC_map_lookup_elem;
> +static int (*bpf_map_update_elem)(void *map, const void *key, const void *value,
> +				  unsigned long long flags) =
[...]
> +
> +/* llvm builtin functions that eBPF C program may use to
> + * emit BPF_LD_ABS and BPF_LD_IND instructions
> + */
> +struct sk_buff;
> +unsigned long long load_byte(void *skb,
> +			     unsigned long long off) asm("llvm.bpf.load.byte");
> +unsigned long long load_half(void *skb,
> +			     unsigned long long off) asm("llvm.bpf.load.half");
> +unsigned long long load_word(void *skb,
> +			     unsigned long long off) asm("llvm.bpf.load.word");

These should be removed from the public API, no point in adding legacy/
deprecated stuff in there.

> +/* a helper structure used by eBPF C program
> + * to describe map attributes to elf_bpf loader
> + */
> +struct bpf_map_def {
> +	unsigned int type;
> +	unsigned int key_size;
> +	unsigned int value_size;
> +	unsigned int max_entries;
> +	unsigned int map_flags;
> +	unsigned int inner_map_idx;
> +	unsigned int numa_node;
> +};
> +
> +#define BPF_ANNOTATE_KV_PAIR(name, type_key, type_val)		\
> +	struct ____btf_map_##name {				\
> +		type_key key;					\
> +		type_val value;					\
> +	};							\
> +	struct ____btf_map_##name				\
> +	__attribute__ ((section(".maps." #name), used))		\
> +		____btf_map_##name = { }

Same here.

> +static int (*bpf_skb_load_bytes)(void *ctx, int off, void *to, int len) =
> +	(void *) BPF_FUNC_skb_load_bytes;
[...]

Given we already move bpf_endian.h to a separate header, I'd do the
same for all tracing specifics as well, e.g. bpf_tracing.h.

> +/* Scan the ARCH passed in from ARCH env variable (see Makefile) */
> +#if defined(__TARGET_ARCH_x86)
> +	#define bpf_target_x86
> +	#define bpf_target_defined
> +#elif defined(__TARGET_ARCH_s390)
> +	#define bpf_target_s390
> +	#define bpf_target_defined
> +#elif defined(__TARGET_ARCH_arm)
> +	#define bpf_target_arm
> +	#define bpf_target_defined
> +#elif defined(__TARGET_ARCH_arm64)
> +	#define bpf_target_arm64
> +	#define bpf_target_defined
> +#elif defined(__TARGET_ARCH_mips)
> +	#define bpf_target_mips
> +	#define bpf_target_defined
> +#elif defined(__TARGET_ARCH_powerpc)
> +	#define bpf_target_powerpc
> +	#define bpf_target_defined
> +#elif defined(__TARGET_ARCH_sparc)
> +	#define bpf_target_sparc
> +	#define bpf_target_defined
> +#else
> +	#undef bpf_target_defined
> +#endif
> +
> +/* Fall back to what the compiler says */
> +#ifndef bpf_target_defined
> +#if defined(__x86_64__)
> +	#define bpf_target_x86
> +#elif defined(__s390__)
> +	#define bpf_target_s390
> +#elif defined(__arm__)
> +	#define bpf_target_arm
> +#elif defined(__aarch64__)
> +	#define bpf_target_arm64
> +#elif defined(__mips__)
> +	#define bpf_target_mips
> +#elif defined(__powerpc__)
> +	#define bpf_target_powerpc
> +#elif defined(__sparc__)
> +	#define bpf_target_sparc
> +#endif
> +#endif
> +
> +#if defined(bpf_target_x86)
> +
> +#ifdef __KERNEL__
> +#define PT_REGS_PARM1(x) ((x)->di)
> +#define PT_REGS_PARM2(x) ((x)->si)
> +#define PT_REGS_PARM3(x) ((x)->dx)
> +#define PT_REGS_PARM4(x) ((x)->cx)
> +#define PT_REGS_PARM5(x) ((x)->r8)
> +#define PT_REGS_RET(x) ((x)->sp)
> +#define PT_REGS_FP(x) ((x)->bp)
> +#define PT_REGS_RC(x) ((x)->ax)
> +#define PT_REGS_SP(x) ((x)->sp)
> +#define PT_REGS_IP(x) ((x)->ip)
> +#else
> +#ifdef __i386__
> +/* i386 kernel is built with -mregparm=3 */
> +#define PT_REGS_PARM1(x) ((x)->eax)
> +#define PT_REGS_PARM2(x) ((x)->edx)
> +#define PT_REGS_PARM3(x) ((x)->ecx)
> +#define PT_REGS_PARM4(x) 0
> +#define PT_REGS_PARM5(x) 0
> +#define PT_REGS_RET(x) ((x)->esp)
> +#define PT_REGS_FP(x) ((x)->ebp)
> +#define PT_REGS_RC(x) ((x)->eax)
> +#define PT_REGS_SP(x) ((x)->esp)
> +#define PT_REGS_IP(x) ((x)->eip)
> +#else
> +#define PT_REGS_PARM1(x) ((x)->rdi)
> +#define PT_REGS_PARM2(x) ((x)->rsi)
> +#define PT_REGS_PARM3(x) ((x)->rdx)
> +#define PT_REGS_PARM4(x) ((x)->rcx)
> +#define PT_REGS_PARM5(x) ((x)->r8)
> +#define PT_REGS_RET(x) ((x)->rsp)
> +#define PT_REGS_FP(x) ((x)->rbp)
> +#define PT_REGS_RC(x) ((x)->rax)
> +#define PT_REGS_SP(x) ((x)->rsp)
> +#define PT_REGS_IP(x) ((x)->rip)
> +#endif
> +#endif
> +
> +#elif defined(bpf_target_s390)
> +
> +/* s390 provides user_pt_regs instead of struct pt_regs to userspace */
> +struct pt_regs;
> +#define PT_REGS_S390 const volatile user_pt_regs
> +#define PT_REGS_PARM1(x) (((PT_REGS_S390 *)(x))->gprs[2])
> +#define PT_REGS_PARM2(x) (((PT_REGS_S390 *)(x))->gprs[3])
> +#define PT_REGS_PARM3(x) (((PT_REGS_S390 *)(x))->gprs[4])
> +#define PT_REGS_PARM4(x) (((PT_REGS_S390 *)(x))->gprs[5])
> +#define PT_REGS_PARM5(x) (((PT_REGS_S390 *)(x))->gprs[6])
> +#define PT_REGS_RET(x) (((PT_REGS_S390 *)(x))->gprs[14])
> +/* Works only with CONFIG_FRAME_POINTER */
> +#define PT_REGS_FP(x) (((PT_REGS_S390 *)(x))->gprs[11])
> +#define PT_REGS_RC(x) (((PT_REGS_S390 *)(x))->gprs[2])
> +#define PT_REGS_SP(x) (((PT_REGS_S390 *)(x))->gprs[15])
> +#define PT_REGS_IP(x) (((PT_REGS_S390 *)(x))->psw.addr)
> +
> +#elif defined(bpf_target_arm)
> +
> +#define PT_REGS_PARM1(x) ((x)->uregs[0])
> +#define PT_REGS_PARM2(x) ((x)->uregs[1])
> +#define PT_REGS_PARM3(x) ((x)->uregs[2])
> +#define PT_REGS_PARM4(x) ((x)->uregs[3])
> +#define PT_REGS_PARM5(x) ((x)->uregs[4])
> +#define PT_REGS_RET(x) ((x)->uregs[14])
> +#define PT_REGS_FP(x) ((x)->uregs[11]) /* Works only with CONFIG_FRAME_POINTER */
> +#define PT_REGS_RC(x) ((x)->uregs[0])
> +#define PT_REGS_SP(x) ((x)->uregs[13])
> +#define PT_REGS_IP(x) ((x)->uregs[12])
> +
> +#elif defined(bpf_target_arm64)
> +
> +/* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */
> +struct pt_regs;
> +#define PT_REGS_ARM64 const volatile struct user_pt_regs
> +#define PT_REGS_PARM1(x) (((PT_REGS_ARM64 *)(x))->regs[0])
> +#define PT_REGS_PARM2(x) (((PT_REGS_ARM64 *)(x))->regs[1])
> +#define PT_REGS_PARM3(x) (((PT_REGS_ARM64 *)(x))->regs[2])
> +#define PT_REGS_PARM4(x) (((PT_REGS_ARM64 *)(x))->regs[3])
> +#define PT_REGS_PARM5(x) (((PT_REGS_ARM64 *)(x))->regs[4])
> +#define PT_REGS_RET(x) (((PT_REGS_ARM64 *)(x))->regs[30])
> +/* Works only with CONFIG_FRAME_POINTER */
> +#define PT_REGS_FP(x) (((PT_REGS_ARM64 *)(x))->regs[29])
> +#define PT_REGS_RC(x) (((PT_REGS_ARM64 *)(x))->regs[0])
> +#define PT_REGS_SP(x) (((PT_REGS_ARM64 *)(x))->sp)
> +#define PT_REGS_IP(x) (((PT_REGS_ARM64 *)(x))->pc)
> +
> +#elif defined(bpf_target_mips)
> +
> +#define PT_REGS_PARM1(x) ((x)->regs[4])
> +#define PT_REGS_PARM2(x) ((x)->regs[5])
> +#define PT_REGS_PARM3(x) ((x)->regs[6])
> +#define PT_REGS_PARM4(x) ((x)->regs[7])
> +#define PT_REGS_PARM5(x) ((x)->regs[8])
> +#define PT_REGS_RET(x) ((x)->regs[31])
> +#define PT_REGS_FP(x) ((x)->regs[30]) /* Works only with CONFIG_FRAME_POINTER */
> +#define PT_REGS_RC(x) ((x)->regs[1])
> +#define PT_REGS_SP(x) ((x)->regs[29])
> +#define PT_REGS_IP(x) ((x)->cp0_epc)
> +
> +#elif defined(bpf_target_powerpc)
> +
> +#define PT_REGS_PARM1(x) ((x)->gpr[3])
> +#define PT_REGS_PARM2(x) ((x)->gpr[4])
> +#define PT_REGS_PARM3(x) ((x)->gpr[5])
> +#define PT_REGS_PARM4(x) ((x)->gpr[6])
> +#define PT_REGS_PARM5(x) ((x)->gpr[7])
> +#define PT_REGS_RC(x) ((x)->gpr[3])
> +#define PT_REGS_SP(x) ((x)->sp)
> +#define PT_REGS_IP(x) ((x)->nip)
> +
> +#elif defined(bpf_target_sparc)
> +
> +#define PT_REGS_PARM1(x) ((x)->u_regs[UREG_I0])
> +#define PT_REGS_PARM2(x) ((x)->u_regs[UREG_I1])
> +#define PT_REGS_PARM3(x) ((x)->u_regs[UREG_I2])
> +#define PT_REGS_PARM4(x) ((x)->u_regs[UREG_I3])
> +#define PT_REGS_PARM5(x) ((x)->u_regs[UREG_I4])
> +#define PT_REGS_RET(x) ((x)->u_regs[UREG_I7])
> +#define PT_REGS_RC(x) ((x)->u_regs[UREG_I0])
> +#define PT_REGS_SP(x) ((x)->u_regs[UREG_FP])
> +
> +/* Should this also be a bpf_target check for the sparc case? */
> +#if defined(__arch64__)
> +#define PT_REGS_IP(x) ((x)->tpc)
> +#else
> +#define PT_REGS_IP(x) ((x)->pc)
> +#endif
> +
> +#endif
> +
> +#if defined(bpf_target_powerpc)
> +#define BPF_KPROBE_READ_RET_IP(ip, ctx)		({ (ip) = (ctx)->link; })
> +#define BPF_KRETPROBE_READ_RET_IP		BPF_KPROBE_READ_RET_IP
> +#elif defined(bpf_target_sparc)
> +#define BPF_KPROBE_READ_RET_IP(ip, ctx)		({ (ip) = PT_REGS_RET(ctx); })
> +#define BPF_KRETPROBE_READ_RET_IP		BPF_KPROBE_READ_RET_IP
> +#else
> +#define BPF_KPROBE_READ_RET_IP(ip, ctx)		({				\
> +		bpf_probe_read(&(ip), sizeof(ip), (void *)PT_REGS_RET(ctx)); })
> +#define BPF_KRETPROBE_READ_RET_IP(ip, ctx)	({				\
> +		bpf_probe_read(&(ip), sizeof(ip),				\
> +				(void *)(PT_REGS_FP(ctx) + sizeof(ip))); })
> +#endif
> +
> +/*
> + * BPF_CORE_READ abstracts away bpf_probe_read() call and captures offset
> + * relocation for source address using __builtin_preserve_access_index()
> + * built-in, provided by Clang.
> + *
> + * __builtin_preserve_access_index() takes as an argument an expression of
> + * taking an address of a field within struct/union. It makes compiler emit
> + * a relocation, which records BTF type ID describing root struct/union and an
> + * accessor string which describes exact embedded field that was used to take
> + * an address. See detailed description of this relocation format and
> + * semantics in comments to struct bpf_offset_reloc in libbpf_internal.h.
> + *
> + * This relocation allows libbpf to adjust BPF instruction to use correct
> + * actual field offset, based on target kernel BTF type that matches original
> + * (local) BTF, used to record relocation.
> + */
> +#define BPF_CORE_READ(dst, src)						\
> +	bpf_probe_read((dst), sizeof(*(src)),				\
> +		       __builtin_preserve_access_index(src))
> +
> +#endif
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-10-01 22:45   ` Daniel Borkmann
@ 2019-10-01 23:31     ` Andrii Nakryiko
  2019-10-01 23:40       ` Andrii Nakryiko
  2019-10-02  9:51       ` Daniel Borkmann
  0 siblings, 2 replies; 47+ messages in thread
From: Andrii Nakryiko @ 2019-10-01 23:31 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov, Kernel Team

On Tue, Oct 1, 2019 at 3:45 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On Mon, Sep 30, 2019 at 11:58:51AM -0700, Andrii Nakryiko wrote:
> > Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
> > are installed along the other libbpf headers.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> [...]
> > new file mode 100644
> > index 000000000000..fbe28008450f
> > --- /dev/null
> > +++ b/tools/lib/bpf/bpf_endian.h
> > @@ -0,0 +1,72 @@
> > +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> > +#ifndef __BPF_ENDIAN__
> > +#define __BPF_ENDIAN__
> > +

[...]

> > +#define bpf_cpu_to_be64(x)                   \
> > +     (__builtin_constant_p(x) ?              \
> > +      __bpf_constant_cpu_to_be64(x) : __bpf_cpu_to_be64(x))
> > +#define bpf_be64_to_cpu(x)                   \
> > +     (__builtin_constant_p(x) ?              \
> > +      __bpf_constant_be64_to_cpu(x) : __bpf_be64_to_cpu(x))
>
> Nit: if we move this into a public API for libbpf, we should probably
> also define for sake of consistency a bpf_cpu_to_be{64,32,16}() and
> bpf_be{64,32,16}_to_cpu() and have the latter two point to the existing
> bpf_hton{l,s}() and bpf_ntoh{l,s}() macros.

I think these deserve better tests before we add more stuff, both from
BPF-side and userland-side (as they are supposed to be includeable
from both, right)? I'm hesitant adding new unfamiliar macro/builtins
without tests, but I don't want to get distracted with that right now,
especially this patch set already becomes bigger than I'd hope.

Given we are talking about adding new stuff which is not breaking
change, we can add it later after we move this as is, agree?

>
> > +#endif /* __BPF_ENDIAN__ */
>
> > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > new file mode 100644
> > index 000000000000..a1d9b97b8e15
> > --- /dev/null
> > +++ b/tools/lib/bpf/bpf_helpers.h
> > @@ -0,0 +1,527 @@
> > +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> > +#ifndef __BPF_HELPERS__
> > +#define __BPF_HELPERS__
> > +
> > +#define __uint(name, val) int (*name)[val]
> > +#define __type(name, val) val *name
> > +
> > +/* helper macro to print out debug messages */
> > +#define bpf_printk(fmt, ...)                         \
> > +({                                                   \
> > +     char ____fmt[] = fmt;                           \
> > +     bpf_trace_printk(____fmt, sizeof(____fmt),      \
> > +                      ##__VA_ARGS__);                \
> > +})
>
> Did you double check if by now via .rodata we can have the fmt as
> const char * and add __attribute__ ((format (printf, 1, 2))) to it?
> If that works we should avoid having to copy the string as above in
> the API.

This doesn't work still, unfortunately. Eventually, though, we'll be
able to essentially nop it out with direct call to bpf_trace_printk,
so I'm not concerned about future API burden.

>
> > +/* helper macro to place programs, maps, license in
> > + * different sections in elf_bpf file. Section names
> > + * are interpreted by elf_bpf loader
> > + */
> > +#define SEC(NAME) __attribute__((section(NAME), used))
> > +
> > +/* helper functions called from eBPF programs written in C */
>
> As mentioned earlier, the whole helper function description below
> should get a big cleanup in here when moved into libbpf API.

Right, I just recalled that today, sorry I didn't do it for this version.

There were two things you mentioned on that thread that you wanted to clean up:
1. using __u32 instead int and stuff like that
2. using macro to hide some of the ugliness of (void *) = BPF_FUNC_xxx

So with 1) I'm concerned that we can't just assume that __u32 is
always going to be defined. Also we need bpf_helpers.h to be usable
both with including system headers, as well as auto-generated
vmlinux.h. In first case, I don't think we can assume that typedef is
always defined, in latter case we can't really define it on our own.
So I think we should just keep it as int, unsigned long long, etc.
Thoughts?

For 2), I'm doing that right now, but it's not that much cleaner, let's see.

Am I missing something else?

>
> > +static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
> > +     (void *) BPF_FUNC_map_lookup_elem;
> > +static int (*bpf_map_update_elem)(void *map, const void *key, const void *value,
> > +                               unsigned long long flags) =
> [...]
> > +
> > +/* llvm builtin functions that eBPF C program may use to
> > + * emit BPF_LD_ABS and BPF_LD_IND instructions
> > + */
> > +struct sk_buff;
> > +unsigned long long load_byte(void *skb,
> > +                          unsigned long long off) asm("llvm.bpf.load.byte");
> > +unsigned long long load_half(void *skb,
> > +                          unsigned long long off) asm("llvm.bpf.load.half");
> > +unsigned long long load_word(void *skb,
> > +                          unsigned long long off) asm("llvm.bpf.load.word");
>
> These should be removed from the public API, no point in adding legacy/
> deprecated stuff in there.

Oh, cool, never knew what that stuff is anyway :)

>
> > +/* a helper structure used by eBPF C program
> > + * to describe map attributes to elf_bpf loader
> > + */
> > +struct bpf_map_def {
> > +     unsigned int type;
> > +     unsigned int key_size;
> > +     unsigned int value_size;
> > +     unsigned int max_entries;
> > +     unsigned int map_flags;
> > +     unsigned int inner_map_idx;
> > +     unsigned int numa_node;
> > +};
> > +
> > +#define BPF_ANNOTATE_KV_PAIR(name, type_key, type_val)               \
> > +     struct ____btf_map_##name {                             \
> > +             type_key key;                                   \
> > +             type_val value;                                 \
> > +     };                                                      \
> > +     struct ____btf_map_##name                               \
> > +     __attribute__ ((section(".maps." #name), used))         \
> > +             ____btf_map_##name = { }
>
> Same here.

We still use it in few selftests, I'll move it into selftests-only header.

>
> > +static int (*bpf_skb_load_bytes)(void *ctx, int off, void *to, int len) =
> > +     (void *) BPF_FUNC_skb_load_bytes;
> [...]
>
> Given we already move bpf_endian.h to a separate header, I'd do the
> same for all tracing specifics as well, e.g. bpf_tracing.h.

You mean all the stuff below, right? I'll extract it into separate
header, no problem.

What about CO-RE stuff. It's not strictly for tracing, so does it make
sense to keep it here?

>
> > +/* Scan the ARCH passed in from ARCH env variable (see Makefile) */
> > +#if defined(__TARGET_ARCH_x86)
> > +     #define bpf_target_x86
> > +     #define bpf_target_defined
> > +#elif defined(__TARGET_ARCH_s390)

[...]

> > --
> > 2.17.1
> >

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-10-01 23:31     ` Andrii Nakryiko
@ 2019-10-01 23:40       ` Andrii Nakryiko
  2019-10-02  9:51       ` Daniel Borkmann
  1 sibling, 0 replies; 47+ messages in thread
From: Andrii Nakryiko @ 2019-10-01 23:40 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov, Kernel Team

On Tue, Oct 1, 2019 at 4:31 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Oct 1, 2019 at 3:45 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On Mon, Sep 30, 2019 at 11:58:51AM -0700, Andrii Nakryiko wrote:
> > > Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
> > > are installed along the other libbpf headers.
> > >
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> >
> > [...]

[...]

> >
> > As mentioned earlier, the whole helper function description below
> > should get a big cleanup in here when moved into libbpf API.
>
> Right, I just recalled that today, sorry I didn't do it for this version.
>
> There were two things you mentioned on that thread that you wanted to clean up:
> 1. using __u32 instead int and stuff like that
> 2. using macro to hide some of the ugliness of (void *) = BPF_FUNC_xxx
>
> So with 1) I'm concerned that we can't just assume that __u32 is
> always going to be defined. Also we need bpf_helpers.h to be usable
> both with including system headers, as well as auto-generated
> vmlinux.h. In first case, I don't think we can assume that typedef is
> always defined, in latter case we can't really define it on our own.
> So I think we should just keep it as int, unsigned long long, etc.
> Thoughts?
>
> For 2), I'm doing that right now, but it's not that much cleaner, let's see.
>
> Am I missing something else?

Ok, so this doesn't work with just

#define BPF_FUNC(NAME, ...) (*NAME)(__VA_ARGS__)
__attribute__((unused)) = (void *) BPF_FUNC_##NAME

because helper is called bpf_map_update_elem(), but enum value is
BPF_FUNC_map_update_elem (without bpf_ prefix). So one way to do this
would be to have bpf_ prepended to function name by macro:

static int BPF_FUNC(map_update_elem, ....);

But this is super confusing, because this definition becomes basically
un-greppable. I think we should just keep it as is, or go all the way
in for super-verbose

static int BPF_FUNC(BPF_FUNC_map_update_elem, bpf_map_elem, ...);

I hate both, honestly.

>
> >
> > > +static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
> > > +     (void *) BPF_FUNC_map_lookup_elem;
> > > +static int (*bpf_map_update_elem)(void *map, const void *key, const void *value,
> > > +                               unsigned long long flags) =
> > [...]
> > > +

[...]

> > > --
> > > 2.17.1
> > >

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 4/6] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers
  2019-10-01 22:42         ` Andrii Nakryiko
@ 2019-10-01 23:44           ` Song Liu
  2019-10-02  3:36             ` Andrii Nakryiko
  0 siblings, 1 reply; 47+ messages in thread
From: Song Liu @ 2019-10-01 23:44 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, bpf, netdev, Alexei Starovoitov, daniel, Kernel Team



> On Oct 1, 2019, at 3:42 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> On Tue, Oct 1, 2019 at 2:46 PM Song Liu <songliubraving@fb.com> wrote:
>> 
>> 
>> 
>>> On Oct 1, 2019, at 2:25 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>>> 
>>> On Tue, Oct 1, 2019 at 2:14 PM Song Liu <songliubraving@fb.com> wrote:
>>>> 
>>>> 
>>>>> On Sep 30, 2019, at 11:58 AM, Andrii Nakryiko <andriin@fb.com> wrote:
>>>>> 
>>>>> Add few macros simplifying BCC-like multi-level probe reads, while also
>>>>> emitting CO-RE relocations for each read.
>>>>> 
>>>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>>>> ---
>>>>> tools/lib/bpf/bpf_helpers.h | 151 +++++++++++++++++++++++++++++++++++-
>>>>> 1 file changed, 147 insertions(+), 4 deletions(-)
>>>>> 
>>>>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
>>>>> index a1d9b97b8e15..51e7b11d53e8 100644
>>>>> --- a/tools/lib/bpf/bpf_helpers.h
>>>>> +++ b/tools/lib/bpf/bpf_helpers.h
>>>>> @@ -19,6 +19,10 @@
>>>>> */
>>>>> #define SEC(NAME) __attribute__((section(NAME), used))
>>>>> 
>>>>> +#ifndef __always_inline
>>>>> +#define __always_inline __attribute__((always_inline))
>>>>> +#endif
>>>>> +
>>>>> /* helper functions called from eBPF programs written in C */
>>>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
>>>>>     (void *) BPF_FUNC_map_lookup_elem;
>>>>> @@ -505,7 +509,7 @@ struct pt_regs;
>>>>> #endif
>>>>> 
>>>>> /*
>>>>> - * BPF_CORE_READ abstracts away bpf_probe_read() call and captures offset
>>>>> + * bpf_core_read() abstracts away bpf_probe_read() call and captures field
>>>>> * relocation for source address using __builtin_preserve_access_index()
>>>>> * built-in, provided by Clang.
>>>>> *
>>>>> @@ -520,8 +524,147 @@ struct pt_regs;
>>>>> * actual field offset, based on target kernel BTF type that matches original
>>>>> * (local) BTF, used to record relocation.
>>>>> */
>>>>> -#define BPF_CORE_READ(dst, src)                                              \
>>>>> -     bpf_probe_read((dst), sizeof(*(src)),                           \
>>>>> -                    __builtin_preserve_access_index(src))
>>>>> +#define bpf_core_read(dst, sz, src)                                      \
>>>>> +     bpf_probe_read(dst, sz,                                             \
>>>>> +                    (const void *)__builtin_preserve_access_index(src))
>>>>> +
>>>>> +/*
>>>>> + * bpf_core_read_str() is a thin wrapper around bpf_probe_read_str()
>>>>> + * additionally emitting BPF CO-RE field relocation for specified source
>>>>> + * argument.
>>>>> + */
>>>>> +#define bpf_core_read_str(dst, sz, src)                                          \
>>>>> +     bpf_probe_read_str(dst, sz,                                         \
>>>>> +                        (const void *)__builtin_preserve_access_index(src))
>>>>> +
>>>>> +#define ___concat(a, b) a ## b
>>>>> +#define ___apply(fn, n) ___concat(fn, n)
>>>>> +#define ___nth(_1, _2, _3, _4, _5, _6, _7, _8, _9, _10, __11, N, ...) N
>>>> 
>>>> We are adding many marcos with simple names: ___apply(), ___nth. So I worry
>>>> they may conflict with macro definitions from other libraries. Shall we hide
>>>> them in .c files or prefix/postfix them with _libbpf or something?
>>> 
>>> Keep in mind, this is the header that's included from BPF code.
>>> 
>>> They are prefixed with three underscores, I was hoping it's good
>>> enough to avoid accidental conflicts. It's unlikely someone will have
>>> macros with the same names **in BPF-side code**.
>> 
>> BPF side code would include kernel headers. So there are many headers
>> to conflict with. And we won't know until somebody want to trace certain
>> kernel structure.
> 
> We have all the kernel sources at our disposal, there's no need to
> guess :) There is no instance of ___apply, ___concat, ___nth,
> ___arrow, ___last, ___nolast, or ___type, not even speaking about
> other more specific names. There are currently two instances of
> "____last_____" used in a string. And I'm certainly not afraid that
> user code can use triple-underscored identifier with exact those names
> and complain about bpf_helpers.h :)

I worry more about _future_ conflicts, that someone may add ___apply to 
some kernel header file and break some BPF programs. Since these BPF
programs are not in-tree, it is very difficult to test them properly.
We have had name conflicts from other libraries, so I hope we don't create 
more ourselves. 

Thanks,
Song

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 4/6] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers
  2019-10-01 23:44           ` Song Liu
@ 2019-10-02  3:36             ` Andrii Nakryiko
  2019-10-02 16:25               ` Song Liu
  0 siblings, 1 reply; 47+ messages in thread
From: Andrii Nakryiko @ 2019-10-02  3:36 UTC (permalink / raw)
  To: Song Liu
  Cc: Andrii Nakryiko, bpf, netdev, Alexei Starovoitov, daniel, Kernel Team

On Tue, Oct 1, 2019 at 4:44 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Oct 1, 2019, at 3:42 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Oct 1, 2019 at 2:46 PM Song Liu <songliubraving@fb.com> wrote:
> >>
> >>
> >>
> >>> On Oct 1, 2019, at 2:25 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >>>
> >>> On Tue, Oct 1, 2019 at 2:14 PM Song Liu <songliubraving@fb.com> wrote:
> >>>>
> >>>>
> >>>>> On Sep 30, 2019, at 11:58 AM, Andrii Nakryiko <andriin@fb.com> wrote:
> >>>>>
> >>>>> Add few macros simplifying BCC-like multi-level probe reads, while also
> >>>>> emitting CO-RE relocations for each read.
> >>>>>
> >>>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> >>>>> ---
> >>>>> tools/lib/bpf/bpf_helpers.h | 151 +++++++++++++++++++++++++++++++++++-
> >>>>> 1 file changed, 147 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> >>>>> index a1d9b97b8e15..51e7b11d53e8 100644
> >>>>> --- a/tools/lib/bpf/bpf_helpers.h
> >>>>> +++ b/tools/lib/bpf/bpf_helpers.h
> >>>>> @@ -19,6 +19,10 @@
> >>>>> */
> >>>>> #define SEC(NAME) __attribute__((section(NAME), used))
> >>>>>
> >>>>> +#ifndef __always_inline
> >>>>> +#define __always_inline __attribute__((always_inline))
> >>>>> +#endif
> >>>>> +
> >>>>> /* helper functions called from eBPF programs written in C */
> >>>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
> >>>>>     (void *) BPF_FUNC_map_lookup_elem;
> >>>>> @@ -505,7 +509,7 @@ struct pt_regs;
> >>>>> #endif
> >>>>>
> >>>>> /*
> >>>>> - * BPF_CORE_READ abstracts away bpf_probe_read() call and captures offset
> >>>>> + * bpf_core_read() abstracts away bpf_probe_read() call and captures field
> >>>>> * relocation for source address using __builtin_preserve_access_index()
> >>>>> * built-in, provided by Clang.
> >>>>> *
> >>>>> @@ -520,8 +524,147 @@ struct pt_regs;
> >>>>> * actual field offset, based on target kernel BTF type that matches original
> >>>>> * (local) BTF, used to record relocation.
> >>>>> */
> >>>>> -#define BPF_CORE_READ(dst, src)                                              \
> >>>>> -     bpf_probe_read((dst), sizeof(*(src)),                           \
> >>>>> -                    __builtin_preserve_access_index(src))
> >>>>> +#define bpf_core_read(dst, sz, src)                                      \
> >>>>> +     bpf_probe_read(dst, sz,                                             \
> >>>>> +                    (const void *)__builtin_preserve_access_index(src))
> >>>>> +
> >>>>> +/*
> >>>>> + * bpf_core_read_str() is a thin wrapper around bpf_probe_read_str()
> >>>>> + * additionally emitting BPF CO-RE field relocation for specified source
> >>>>> + * argument.
> >>>>> + */
> >>>>> +#define bpf_core_read_str(dst, sz, src)                                          \
> >>>>> +     bpf_probe_read_str(dst, sz,                                         \
> >>>>> +                        (const void *)__builtin_preserve_access_index(src))
> >>>>> +
> >>>>> +#define ___concat(a, b) a ## b
> >>>>> +#define ___apply(fn, n) ___concat(fn, n)
> >>>>> +#define ___nth(_1, _2, _3, _4, _5, _6, _7, _8, _9, _10, __11, N, ...) N
> >>>>
> >>>> We are adding many marcos with simple names: ___apply(), ___nth. So I worry
> >>>> they may conflict with macro definitions from other libraries. Shall we hide
> >>>> them in .c files or prefix/postfix them with _libbpf or something?
> >>>
> >>> Keep in mind, this is the header that's included from BPF code.
> >>>
> >>> They are prefixed with three underscores, I was hoping it's good
> >>> enough to avoid accidental conflicts. It's unlikely someone will have
> >>> macros with the same names **in BPF-side code**.
> >>
> >> BPF side code would include kernel headers. So there are many headers
> >> to conflict with. And we won't know until somebody want to trace certain
> >> kernel structure.
> >
> > We have all the kernel sources at our disposal, there's no need to
> > guess :) There is no instance of ___apply, ___concat, ___nth,
> > ___arrow, ___last, ___nolast, or ___type, not even speaking about
> > other more specific names. There are currently two instances of
> > "____last_____" used in a string. And I'm certainly not afraid that
> > user code can use triple-underscored identifier with exact those names
> > and complain about bpf_helpers.h :)
>
> I worry more about _future_ conflicts, that someone may add ___apply to

You can say the same about pretty much any name that user might use,
that's just the fact of life with C language without namespaces. I
don't think that justifies usage of obscure names.

Look at SEC macro, for instance. It's also an enum value in
drivers/sbus/char/oradax.c, but it might some day end up in one of
driver's headers. This is probably not a reason to rename it, though.

> some kernel header file and break some BPF programs. Since these BPF
> programs are not in-tree, it is very difficult to test them properly.
> We have had name conflicts from other libraries, so I hope we don't create
> more ourselves.

Let's agree to come back to this problem when and if we ever encounter
it. All those ___xxx macro are internal and users shouldn't rely on
them, which means if we ever get a real conflict, we'll be able to
rename them to avoid the conflict.

>
> Thanks,
> Song

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-10-01 19:42     ` Andrii Nakryiko
@ 2019-10-02  7:21       ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 47+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-02  7:21 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Tue, Oct 1, 2019 at 12:10 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>>
>> > +struct bpf_map_def {
>> > +     unsigned int type;
>> > +     unsigned int key_size;
>> > +     unsigned int value_size;
>> > +     unsigned int max_entries;
>> > +     unsigned int map_flags;
>> > +     unsigned int inner_map_idx;
>> > +     unsigned int numa_node;
>> > +};
>>
>> Didn't we agree on no new bpf_map_def ABI in libbpf, and that all
>> additions should be BTF-based?
>
> Oh yes, we did, sorry, this is an oversight. I really appreciate you
> pointing this out ;)
> I'll go over bpf_helpers.h carefully and will double-check if we don't
> have any other custom selftests-only stuff left there.

Great, thanks!

-Toke


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-10-01 19:44       ` Andrii Nakryiko
  2019-10-01 22:00         ` John Fastabend
@ 2019-10-02  7:25         ` Toke Høiland-Jørgensen
  1 sibling, 0 replies; 47+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-02  7:25 UTC (permalink / raw)
  To: Andrii Nakryiko, John Fastabend
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Tue, Oct 1, 2019 at 12:18 PM John Fastabend <john.fastabend@gmail.com> wrote:
>>
>> Toke Høiland-Jørgensen wrote:
>> >
>> > > +struct bpf_map_def {
>> > > +   unsigned int type;
>> > > +   unsigned int key_size;
>> > > +   unsigned int value_size;
>> > > +   unsigned int max_entries;
>> > > +   unsigned int map_flags;
>> > > +   unsigned int inner_map_idx;
>> > > +   unsigned int numa_node;
>> > > +};
>> >
>> > Didn't we agree on no new bpf_map_def ABI in libbpf, and that all
>> > additions should be BTF-based?
>> >
>> > -Toke
>> >
>>
>> We use libbpf on pre BTF kernels so in this case I think it makes
>> sense to add these fields. Having inner_map_idx there should allow
>> us to remove some other things we have sitting around.
>
> We had a discussion about supporting non-BTF and non-standard BPF map
> definition before and it's still on my TODO list to go and do a proof
> of concept how that can look like and what libbpf changes we need to
> make. Right now libbpf doesn't support those new fields anyway, so we
> shouldn't add them to public API.

This was the thread; the context was libbpf support in iproute2:
https://lore.kernel.org/netdev/20190820114706.18546-1-toke@redhat.com/

Basically, we agreed that rather than adding more fields to bpf_map_def
in libbpf itself, we'd support BTF definitions natively, and provide
applications the right callbacks to support custom formats as they see
fit.

-Toke


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf
  2019-10-01 23:31     ` Andrii Nakryiko
  2019-10-01 23:40       ` Andrii Nakryiko
@ 2019-10-02  9:51       ` Daniel Borkmann
  1 sibling, 0 replies; 47+ messages in thread
From: Daniel Borkmann @ 2019-10-02  9:51 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov, Kernel Team

On Tue, Oct 01, 2019 at 04:31:27PM -0700, Andrii Nakryiko wrote:
> On Tue, Oct 1, 2019 at 3:45 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > On Mon, Sep 30, 2019 at 11:58:51AM -0700, Andrii Nakryiko wrote:
> > > Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
> > > are installed along the other libbpf headers.
> > >
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> >
> > [...]
> > > new file mode 100644
> > > index 000000000000..fbe28008450f
> > > --- /dev/null
> > > +++ b/tools/lib/bpf/bpf_endian.h
> > > @@ -0,0 +1,72 @@
> > > +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> > > +#ifndef __BPF_ENDIAN__
> > > +#define __BPF_ENDIAN__
> > > +
> 
> [...]
> 
> > > +#define bpf_cpu_to_be64(x)                   \
> > > +     (__builtin_constant_p(x) ?              \
> > > +      __bpf_constant_cpu_to_be64(x) : __bpf_cpu_to_be64(x))
> > > +#define bpf_be64_to_cpu(x)                   \
> > > +     (__builtin_constant_p(x) ?              \
> > > +      __bpf_constant_be64_to_cpu(x) : __bpf_be64_to_cpu(x))
> >
> > Nit: if we move this into a public API for libbpf, we should probably
> > also define for sake of consistency a bpf_cpu_to_be{64,32,16}() and
> > bpf_be{64,32,16}_to_cpu() and have the latter two point to the existing
> > bpf_hton{l,s}() and bpf_ntoh{l,s}() macros.
> 
> I think these deserve better tests before we add more stuff, both from
> BPF-side and userland-side (as they are supposed to be includeable
> from both, right)? I'm hesitant adding new unfamiliar macro/builtins
> without tests, but I don't want to get distracted with that right now,
> especially this patch set already becomes bigger than I'd hope.
> 
> Given we are talking about adding new stuff which is not breaking
> change, we can add it later after we move this as is, agree?

Sure, we can add it later, I only meant to say adding things like
define bpf_cpu_to_be32 pointing to bpf_htonl and so on, so no new
extra helpers, but just to make it more consistent API wise and
from usability side.

> > > +#endif /* __BPF_ENDIAN__ */
> >
> > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > > new file mode 100644
> > > index 000000000000..a1d9b97b8e15
> > > --- /dev/null
> > > +++ b/tools/lib/bpf/bpf_helpers.h
> > > @@ -0,0 +1,527 @@
> > > +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> > > +#ifndef __BPF_HELPERS__
> > > +#define __BPF_HELPERS__
> > > +
> > > +#define __uint(name, val) int (*name)[val]
> > > +#define __type(name, val) val *name
> > > +
> > > +/* helper macro to print out debug messages */
> > > +#define bpf_printk(fmt, ...)                         \
> > > +({                                                   \
> > > +     char ____fmt[] = fmt;                           \
> > > +     bpf_trace_printk(____fmt, sizeof(____fmt),      \
> > > +                      ##__VA_ARGS__);                \
> > > +})
> >
> > Did you double check if by now via .rodata we can have the fmt as
> > const char * and add __attribute__ ((format (printf, 1, 2))) to it?
> > If that works we should avoid having to copy the string as above in
> > the API.
> 
> This doesn't work still, unfortunately. Eventually, though, we'll be
> able to essentially nop it out with direct call to bpf_trace_printk,
> so I'm not concerned about future API burden.
> 
> > > +/* helper macro to place programs, maps, license in
> > > + * different sections in elf_bpf file. Section names
> > > + * are interpreted by elf_bpf loader
> > > + */
> > > +#define SEC(NAME) __attribute__((section(NAME), used))
> > > +
> > > +/* helper functions called from eBPF programs written in C */
> >
> > As mentioned earlier, the whole helper function description below
> > should get a big cleanup in here when moved into libbpf API.
> 
> Right, I just recalled that today, sorry I didn't do it for this version.
> 
> There were two things you mentioned on that thread that you wanted to clean up:
> 1. using __u32 instead int and stuff like that
> 2. using macro to hide some of the ugliness of (void *) = BPF_FUNC_xxx
> 
> So with 1) I'm concerned that we can't just assume that __u32 is
> always going to be defined. Also we need bpf_helpers.h to be usable
> both with including system headers, as well as auto-generated
> vmlinux.h. In first case, I don't think we can assume that typedef is
> always defined, in latter case we can't really define it on our own.
> So I think we should just keep it as int, unsigned long long, etc.
> Thoughts?

Long time ago in Cilium I tried to clean up the mess a bit [0] for most
of the helpers we're using. I was using types from stdint.h actually,
haven't heard of any issues so far.

The __u* types which you mentioned is shipped in linux/types.h, these
are typically used in uapi headers like linux/bpf.h and match kernel
side u* counterpart, so they should generally be available as well as
an alternative.

  [0] https://github.com/cilium/cilium/blob/master/bpf/include/bpf/api.h

> For 2), I'm doing that right now, but it's not that much cleaner, let's see.
> 
> Am I missing something else?
> 
> > > +static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
> > > +     (void *) BPF_FUNC_map_lookup_elem;
> > > +static int (*bpf_map_update_elem)(void *map, const void *key, const void *value,
> > > +                               unsigned long long flags) =
> > [...]
> > > +
> > > +/* llvm builtin functions that eBPF C program may use to
> > > + * emit BPF_LD_ABS and BPF_LD_IND instructions
> > > + */
> > > +struct sk_buff;
> > > +unsigned long long load_byte(void *skb,
> > > +                          unsigned long long off) asm("llvm.bpf.load.byte");
> > > +unsigned long long load_half(void *skb,
> > > +                          unsigned long long off) asm("llvm.bpf.load.half");
> > > +unsigned long long load_word(void *skb,
> > > +                          unsigned long long off) asm("llvm.bpf.load.word");
> >
> > These should be removed from the public API, no point in adding legacy/
> > deprecated stuff in there.
> 
> Oh, cool, never knew what that stuff is anyway :)
> 
> > > +/* a helper structure used by eBPF C program
> > > + * to describe map attributes to elf_bpf loader
> > > + */
> > > +struct bpf_map_def {
> > > +     unsigned int type;
> > > +     unsigned int key_size;
> > > +     unsigned int value_size;
> > > +     unsigned int max_entries;
> > > +     unsigned int map_flags;
> > > +     unsigned int inner_map_idx;
> > > +     unsigned int numa_node;
> > > +};
> > > +
> > > +#define BPF_ANNOTATE_KV_PAIR(name, type_key, type_val)               \
> > > +     struct ____btf_map_##name {                             \
> > > +             type_key key;                                   \
> > > +             type_val value;                                 \
> > > +     };                                                      \
> > > +     struct ____btf_map_##name                               \
> > > +     __attribute__ ((section(".maps." #name), used))         \
> > > +             ____btf_map_##name = { }
> >
> > Same here.
> 
> We still use it in few selftests, I'll move it into selftests-only header.

Yes, all the legacy stuff we don't want exposed in libbpf as API should
be kept in samples or selftests only header until it's fully converted
one day where we can remove that temporary header entirely.

> > > +static int (*bpf_skb_load_bytes)(void *ctx, int off, void *to, int len) =
> > > +     (void *) BPF_FUNC_skb_load_bytes;
> > [...]
> >
> > Given we already move bpf_endian.h to a separate header, I'd do the
> > same for all tracing specifics as well, e.g. bpf_tracing.h.
> 
> You mean all the stuff below, right? I'll extract it into separate
> header, no problem.

Sounds good. We should probably get this more in order as well if it
becomes API. My worry is that potentially PT_REGS_* names may clash
in future, but this cleanup can be done in a second step as long as
its done in this development cycle and not officially released yet.

> What about CO-RE stuff. It's not strictly for tracing, so does it make
> sense to keep it here?

Yes, as it's core and has a use-case beyond tracing, I'd keep it here.

> > > +/* Scan the ARCH passed in from ARCH env variable (see Makefile) */
> > > +#if defined(__TARGET_ARCH_x86)
> > > +     #define bpf_target_x86
> > > +     #define bpf_target_defined
> > > +#elif defined(__TARGET_ARCH_s390)
> 
> [...]

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH bpf-next 4/6] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers
  2019-10-02  3:36             ` Andrii Nakryiko
@ 2019-10-02 16:25               ` Song Liu
  0 siblings, 0 replies; 47+ messages in thread
From: Song Liu @ 2019-10-02 16:25 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, bpf, netdev, Alexei Starovoitov, daniel, Kernel Team



> On Oct 1, 2019, at 8:36 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> On Tue, Oct 1, 2019 at 4:44 PM Song Liu <songliubraving@fb.com> wrote:
>> 
>> 
>> 
>>> On Oct 1, 2019, at 3:42 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>>> 
>>> On Tue, Oct 1, 2019 at 2:46 PM Song Liu <songliubraving@fb.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Oct 1, 2019, at 2:25 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>>>>> 
>>>>> On Tue, Oct 1, 2019 at 2:14 PM Song Liu <songliubraving@fb.com> wrote:
>>>>>> 
>>>>>> 
>>>>>>> On Sep 30, 2019, at 11:58 AM, Andrii Nakryiko <andriin@fb.com> wrote:
>>>>>>> 
>>>>>>> Add few macros simplifying BCC-like multi-level probe reads, while also
>>>>>>> emitting CO-RE relocations for each read.
>>>>>>> 
>>>>>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>>>>>> ---
>>>>>>> tools/lib/bpf/bpf_helpers.h | 151 +++++++++++++++++++++++++++++++++++-
>>>>>>> 1 file changed, 147 insertions(+), 4 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
>>>>>>> index a1d9b97b8e15..51e7b11d53e8 100644
>>>>>>> --- a/tools/lib/bpf/bpf_helpers.h
>>>>>>> +++ b/tools/lib/bpf/bpf_helpers.h
>>>>>>> @@ -19,6 +19,10 @@
>>>>>>> */
>>>>>>> #define SEC(NAME) __attribute__((section(NAME), used))
>>>>>>> 
>>>>>>> +#ifndef __always_inline
>>>>>>> +#define __always_inline __attribute__((always_inline))
>>>>>>> +#endif
>>>>>>> +
>>>>>>> /* helper functions called from eBPF programs written in C */
>>>>>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
>>>>>>>    (void *) BPF_FUNC_map_lookup_elem;
>>>>>>> @@ -505,7 +509,7 @@ struct pt_regs;
>>>>>>> #endif
>>>>>>> 
>>>>>>> /*
>>>>>>> - * BPF_CORE_READ abstracts away bpf_probe_read() call and captures offset
>>>>>>> + * bpf_core_read() abstracts away bpf_probe_read() call and captures field
>>>>>>> * relocation for source address using __builtin_preserve_access_index()
>>>>>>> * built-in, provided by Clang.
>>>>>>> *
>>>>>>> @@ -520,8 +524,147 @@ struct pt_regs;
>>>>>>> * actual field offset, based on target kernel BTF type that matches original
>>>>>>> * (local) BTF, used to record relocation.
>>>>>>> */
>>>>>>> -#define BPF_CORE_READ(dst, src)                                              \
>>>>>>> -     bpf_probe_read((dst), sizeof(*(src)),                           \
>>>>>>> -                    __builtin_preserve_access_index(src))
>>>>>>> +#define bpf_core_read(dst, sz, src)                                      \
>>>>>>> +     bpf_probe_read(dst, sz,                                             \
>>>>>>> +                    (const void *)__builtin_preserve_access_index(src))
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * bpf_core_read_str() is a thin wrapper around bpf_probe_read_str()
>>>>>>> + * additionally emitting BPF CO-RE field relocation for specified source
>>>>>>> + * argument.
>>>>>>> + */
>>>>>>> +#define bpf_core_read_str(dst, sz, src)                                          \
>>>>>>> +     bpf_probe_read_str(dst, sz,                                         \
>>>>>>> +                        (const void *)__builtin_preserve_access_index(src))
>>>>>>> +
>>>>>>> +#define ___concat(a, b) a ## b
>>>>>>> +#define ___apply(fn, n) ___concat(fn, n)
>>>>>>> +#define ___nth(_1, _2, _3, _4, _5, _6, _7, _8, _9, _10, __11, N, ...) N
>>>>>> 
>>>>>> We are adding many marcos with simple names: ___apply(), ___nth. So I worry
>>>>>> they may conflict with macro definitions from other libraries. Shall we hide
>>>>>> them in .c files or prefix/postfix them with _libbpf or something?
>>>>> 
>>>>> Keep in mind, this is the header that's included from BPF code.
>>>>> 
>>>>> They are prefixed with three underscores, I was hoping it's good
>>>>> enough to avoid accidental conflicts. It's unlikely someone will have
>>>>> macros with the same names **in BPF-side code**.
>>>> 
>>>> BPF side code would include kernel headers. So there are many headers
>>>> to conflict with. And we won't know until somebody want to trace certain
>>>> kernel structure.
>>> 
>>> We have all the kernel sources at our disposal, there's no need to
>>> guess :) There is no instance of ___apply, ___concat, ___nth,
>>> ___arrow, ___last, ___nolast, or ___type, not even speaking about
>>> other more specific names. There are currently two instances of
>>> "____last_____" used in a string. And I'm certainly not afraid that
>>> user code can use triple-underscored identifier with exact those names
>>> and complain about bpf_helpers.h :)
>> 
>> I worry more about _future_ conflicts, that someone may add ___apply to
> 
> You can say the same about pretty much any name that user might use,
> that's just the fact of life with C language without namespaces. I
> don't think that justifies usage of obscure names.
> 
> Look at SEC macro, for instance. It's also an enum value in
> drivers/sbus/char/oradax.c, but it might some day end up in one of
> driver's headers. This is probably not a reason to rename it, though.
> 
>> some kernel header file and break some BPF programs. Since these BPF
>> programs are not in-tree, it is very difficult to test them properly.
>> We have had name conflicts from other libraries, so I hope we don't create
>> more ourselves.
> 
> Let's agree to come back to this problem when and if we ever encounter
> it. All those ___xxx macro are internal and users shouldn't rely on
> them, which means if we ever get a real conflict, we'll be able to
> rename them to avoid the conflict.

Well, if this really happens, we will have to fix them. 

I won't block this set just for this. If you insist, let's keep these 
as-is. 

Thanks,
Song


^ permalink raw reply	[flat|nested] 47+ messages in thread

end of thread, other threads:[~2019-10-02 16:25 UTC | newest]

Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-30 18:58 [PATCH bpf-next 0/6] Move bpf_helpers and add BPF_CORE_READ macros Andrii Nakryiko
2019-09-30 18:58 ` [PATCH bpf-next 1/6] selftests/bpf: undo GCC-specific bpf_helpers.h changes Andrii Nakryiko
2019-09-30 22:53   ` Song Liu
2019-10-01 10:25   ` Ilya Leoshkevich
2019-10-01 19:10   ` John Fastabend
2019-09-30 18:58 ` [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf Andrii Nakryiko
2019-09-30 22:55   ` Song Liu
2019-09-30 22:58     ` Andrii Nakryiko
2019-09-30 23:18       ` Jakub Kicinski
2019-09-30 23:25         ` Song Liu
2019-09-30 23:36           ` Jakub Kicinski
2019-09-30 23:39             ` Song Liu
2019-09-30 23:30         ` Andrii Nakryiko
2019-09-30 23:43           ` Jakub Kicinski
2019-09-30 23:23       ` Song Liu
2019-09-30 23:27         ` Andrii Nakryiko
2019-09-30 23:35           ` Song Liu
2019-10-01  7:10   ` Toke Høiland-Jørgensen
2019-10-01 19:18     ` John Fastabend
2019-10-01 19:44       ` Andrii Nakryiko
2019-10-01 22:00         ` John Fastabend
2019-10-02  7:25         ` Toke Høiland-Jørgensen
2019-10-01 19:42     ` Andrii Nakryiko
2019-10-02  7:21       ` Toke Høiland-Jørgensen
2019-10-01 21:19   ` Song Liu
2019-10-01 21:28     ` Andrii Nakryiko
2019-10-01 22:45   ` Daniel Borkmann
2019-10-01 23:31     ` Andrii Nakryiko
2019-10-01 23:40       ` Andrii Nakryiko
2019-10-02  9:51       ` Daniel Borkmann
2019-09-30 18:58 ` [PATCH bpf-next 3/6] selftests/bpf: switch test to use libbpf's helpers Andrii Nakryiko
2019-09-30 18:58 ` [PATCH bpf-next 4/6] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers Andrii Nakryiko
2019-10-01 19:07   ` John Fastabend
2019-10-01 21:14   ` Song Liu
2019-10-01 21:25     ` Andrii Nakryiko
2019-10-01 21:46       ` Song Liu
2019-10-01 22:42         ` Andrii Nakryiko
2019-10-01 23:44           ` Song Liu
2019-10-02  3:36             ` Andrii Nakryiko
2019-10-02 16:25               ` Song Liu
2019-09-30 18:58 ` [PATCH bpf-next 5/6] selftests/bpf: adjust CO-RE reloc tests for new BPF_CORE_READ macro Andrii Nakryiko
2019-10-01 19:14   ` John Fastabend
2019-10-01 21:31     ` Andrii Nakryiko
2019-10-01 21:47   ` Song Liu
2019-09-30 18:58 ` [PATCH bpf-next 6/6] selftests/bpf: add BPF_CORE_READ and BPF_CORE_READ_STR_INTO macro tests Andrii Nakryiko
2019-10-01 19:19   ` John Fastabend
2019-10-01 21:22     ` Song Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).