Re: [PATCH] samples/bpf: Add xdp_stat sample program

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

On Sun, Dec 29, 2019 at 2:51 PM Eric Sage <eric@sage.org> wrote:
>
> At Facebook we use tail calls to jump between our firewall filters and
> our L4LB. This is a program I wrote to estimate per program performance
> by swapping out the entries in the program array with interceptors that
> take measurements and then jump to the original entries.
>
> I found the sample programs to be invaluable in understanding how to use
> the libbpf API (as well as the test env from the xdp-tutorial repo for
> testing), and want to return the favor. I am currently working on
> my next iteration that uses fentry/fexit to be less invasive,
> but I thought it was an interesting PoC of what you can do with program
> arrays.
>
> Signed-off-by: Eric Sage <eric@sage.org>
> ---

Hey Eric,

Thanks for contributing this to samples. Ideally this would use BPF
skeleton to simplify all the map and program look ups, but given
samples/bpf doesn't have a infrastructure set up for this, it might be
a bit too much effort at this point. So I think it's ok to do it this
way, even though it would be really nice to try.

But please update the old-style bpf_map_def to BTF-defined maps before
this gets merged (see below).

>  samples/bpf/Makefile          |   3 +
>  samples/bpf/xdp_stat_common.h |  28 ++
>  samples/bpf/xdp_stat_kern.c   | 169 ++++++++
>  samples/bpf/xdp_stat_user.c   | 746 ++++++++++++++++++++++++++++++++++
>  4 files changed, 946 insertions(+)
>  create mode 100644 samples/bpf/xdp_stat_common.h
>  create mode 100644 samples/bpf/xdp_stat_kern.c
>  create mode 100644 samples/bpf/xdp_stat_user.c
>

[...]

> +#include <uapi/linux/bpf.h>
> +#include "bpf_helpers.h"
> +
> +#include "xdp_stat_common.h"
> +
> +#define MAX_PROG_ARRAY 64
> +
> +/* NR is used to map interceptors to the programs that are being intercepted. */
> +#define INTERCEPTOR(INDEX)                                                     \
> +       SEC("xdp/interceptor_" #INDEX)                                         \
> +       int interceptor_##INDEX(struct xdp_md *ctx)                            \
> +       {                                                                      \
> +               return interceptor_impl(ctx, INDEX);                           \
> +       }
> +
> +       /* Required to use bpf_ktime_get_ns() */
> +       char _license[] SEC("license") = "GPL";

Wrong indentation?

> +
> +/* interception_info holds a single record per CPU to pass global state between
> + * interceptor programs.
> + */
> +struct bpf_map_def SEC("maps") interception_info = {
> +       .type = BPF_MAP_TYPE_PERCPU_ARRAY,
> +       .key_size = sizeof(__u32),
> +       .value_size = sizeof(struct interception_info_rec),
> +       .max_entries = 1,
> +};

With BTF-define maps this would be

struct {
    __uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
    __uint(max_entries, 1);
    __type(key, __u32);
    __type(value, struct interception_info_rec);
} interception_info SEC(".maps");

Can you please switch all the maps to this new syntax?

> +
> +/* interceptor_stats maps interceptor indexes to measurements of an intercepted
> + * BPF program. Index 0 maps the interceptor entrypoint to measurements of the
> + * original entrypoint.
> + */
> +struct bpf_map_def SEC("maps") prog_stats = {
> +       .type = BPF_MAP_TYPE_PERCPU_ARRAY,
> +       .key_size = sizeof(__u32),
> +       .value_size = sizeof(struct prog_stats_rec),
> +       .max_entries = MAX_PROG_ARRAY,
> +};
> +

[...]

> +static struct intercept_ctx *intercept__setup(char *mappath, int ifindex)
> +{
> +       struct intercept_ctx *ctx = malloc(sizeof(struct intercept_ctx));
> +
> +       ctx->ifindex = ifindex;
> +
> +       ctx->stats_enabled_oldval =
> +               _update_sysctl("/proc/sys/kernel/bpf_stats_enabled", 1);
> +       if (ctx->stats_enabled_oldval < 0)
> +               perror("ERR: set bpf_stats_enabled sysctl failed\n");
> +
> +       if (bpf_prog_load(FILENAME_XDP_STAT_KERN, BPF_PROG_TYPE_XDP,
> +                         &ctx->bpf_obj, &ctx->entry_fd)) {

bpf_object__open() + bpf_object__load() is preferred over
bpf_prog_load() way to open and load BPF object files, would be nice
to use that instead.

Alternative is BPF skeleton, but as I said, it will require a bit more
set up in Makefile (you'd need to generate skeleton with `bpftool gen
skeleton <your-bpf-object>.o`). Few benefits of BPF skeleton, though:
  - you wouldn't need to look up maps and programs by name, they would
be accessible as struct fields of skeleton (e.g.,
skel->maps.interception_info);
  - BPF object file would be embedded into userspace program, so when
distributing this you wouldn't need to drag along a separate .o file.


> +               fprintf(stderr, "ERR: failed to load %s\n",
> +                       FILENAME_XDP_STAT_KERN);
> +               return NULL;
> +       }
> +

[...]