Re: [PATCH bpf-next v2 2/8] samples: bpf: Add common infrastructure for XDP samples

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Wed, Jul 21, 2021 at 2:28 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> This file implements some common helpers to consolidate differences in
> features and functionality between the various XDP samples and give them
> a consistent look, feel, and reporting capabilities.
>
> Some of the key features are:
>  * A concise output format accompanied by helpful text explaining its
>    fields.
>  * An elaborate output format building upon the concise one, and folding
>    out details in case of errors and staying out of view otherwise.
>  * Extended reporting of redirect errors by capturing hits for each
>    errno and displaying them inline (ENETDOWN, EINVAL, ENOSPC, etc.)
>    to aid debugging.
>  * Reporting of each xdp_exception action for all samples that use these
>    helpers (XDP_ABORTED, etc.) to aid debugging.
>  * Capturing per ifindex pair devmap_xmit counts for decomposing the
>    total TX count per devmap redirection.
>  * Ability to jump to source locations invoking tracepoints.
>  * Faster retrieval of stats per polling interval using mmap'd eBPF
>    array map (through .bss).
>  * Printing driver names for devices redirecting packets.
>  * Printing summarized total statistics for the entire session.
>  * Ability to dynamically switch between concise and verbose mode, using
>    SIGQUIT (Ctrl + \).
>
> The goal is sharing these helpers that most of the XDP samples implement
> in some form but differently for each, lacking in some respect compared
> to one another.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  samples/bpf/Makefile            |    6 +-
>  samples/bpf/xdp_sample_shared.h |   53 ++
>  samples/bpf/xdp_sample_user.c   | 1380 +++++++++++++++++++++++++++++++
>  samples/bpf/xdp_sample_user.h   |  202 +++++
>  4 files changed, 1640 insertions(+), 1 deletion(-)
>  create mode 100644 samples/bpf/xdp_sample_shared.h
>  create mode 100644 samples/bpf/xdp_sample_user.c
>  create mode 100644 samples/bpf/xdp_sample_user.h
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 036998d11ded..57ccff5ccac4 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -62,6 +62,7 @@ LIBBPF = $(TOOLS_PATH)/lib/bpf/libbpf.a
>
>  CGROUP_HELPERS := ../../tools/testing/selftests/bpf/cgroup_helpers.o
>  TRACE_HELPERS := ../../tools/testing/selftests/bpf/trace_helpers.o
> +XDP_SAMPLE := xdp_sample_user.o
>
>  fds_example-objs := fds_example.o
>  sockex1-objs := sockex1_user.o
> @@ -116,6 +117,8 @@ xdp_sample_pkts-objs := xdp_sample_pkts_user.o
>  ibumad-objs := ibumad_user.o
>  hbm-objs := hbm.o $(CGROUP_HELPERS)
>
> +xdp_sample_user-objs := xdp_sample_user.o $(LIBBPFDIR)/hashmap.o
> +

nope, hashmap is an internal libbpf API, no using it from samples, please

>  # Tell kbuild to always build the programs
>  always-y := $(tprogs-y)
>  always-y += sockex1_kern.o
> @@ -201,6 +204,7 @@ TPROGS_CFLAGS += -Wstrict-prototypes
>  TPROGS_CFLAGS += -I$(objtree)/usr/include
>  TPROGS_CFLAGS += -I$(srctree)/tools/testing/selftests/bpf/
>  TPROGS_CFLAGS += -I$(srctree)/tools/lib/
> +TPROGS_CFLAGS += -I$(srctree)/tools/lib/bpf
>  TPROGS_CFLAGS += -I$(srctree)/tools/include
>  TPROGS_CFLAGS += -I$(srctree)/tools/perf
>  TPROGS_CFLAGS += -DHAVE_ATTR_TEST=0
> @@ -210,7 +214,7 @@ TPROGS_CFLAGS += --sysroot=$(SYSROOT)
>  TPROGS_LDFLAGS := -L$(SYSROOT)/usr/lib
>  endif
>
> -TPROGS_LDLIBS                  += $(LIBBPF) -lelf -lz
> +TPROGS_LDLIBS                  += $(LIBBPF) -lelf -lz -lm
>  TPROGLDLIBS_tracex4            += -lrt
>  TPROGLDLIBS_trace_output       += -lrt
>  TPROGLDLIBS_map_perf_test      += -lrt
> diff --git a/samples/bpf/xdp_sample_shared.h b/samples/bpf/xdp_sample_shared.h
> new file mode 100644
> index 000000000000..b211dca233d9
> --- /dev/null
> +++ b/samples/bpf/xdp_sample_shared.h
> @@ -0,0 +1,53 @@
> +#ifndef _XDP_SAMPLE_SHARED_H
> +#define _XDP_SAMPLE_SHARED_H
> +
> +/*
> + * Best-effort relaxed load/store
> + * __atomic_load_n/__atomic_store_n built-in is not supported for BPF target
> + */
> +#define ATOMIC_LOAD(var) ({ (*(volatile typeof(var) *)&(var)); })
> +#define ATOMIC_STORE(var, val) ({ *((volatile typeof(var) *)&(var)) = (val); })
> +/* This does a load + store instead of the expensive atomic fetch add, but store
> + * is still atomic so that userspace reading the value reads the old or the new
> + * one, but not a partial store.
> + */
> +#define ATOMIC_ADD_NORMW(var, val)                                             \

it's rather some sort of NO_TEAR_ADD, but definitely not an atomic
add? Wouldn't this ATOMIC_ADD name be too misleading?

> +       ({                                                                     \
> +               typeof(val) __val = (val);                                     \
> +               if (__val)                                                     \
> +                       ATOMIC_STORE((var), (var) + __val);                    \
> +       })
> +
> +#define ATOMIC_INC_NORMW(var) ATOMIC_ADD_NORMW((var), 1)
> +
> +#define MAX_CPUS 64
> +
> +/* Values being read/stored must be word sized */
> +#if __LP64__
> +typedef __u64 datarec_t;
> +#else
> +typedef __u32 datarec_t;
> +#endif

why not use size_t instead of typedefs?

> +
> +struct datarec {
> +       datarec_t processed;
> +       datarec_t dropped;
> +       datarec_t issue;
> +       union {
> +               datarec_t xdp_pass;
> +               datarec_t info;
> +       };
> +       datarec_t xdp_drop;
> +       datarec_t xdp_redirect;
> +} __attribute__((aligned(64)));
> +

[...]

> +#define __attach_tp(name)                                                      \
> +       ({                                                                     \
> +               if (!bpf_program__is_tracing(skel->progs.name))                \
> +                       return -EINVAL;                                        \
> +               skel->links.name = bpf_program__attach(skel->progs.name);      \
> +               if (!skel->links.name)                                         \

you need to turn on libbpf 1.0 mode for errors to use NULL check, see
libbpf_set_strict_mode(); otherwise need to use libbpf_get_error() to
check

> +                       return -errno;                                         \
> +       })
> +

[...]