On Sun, Dec 29, 2019 at 2:51 PM Eric Sage <eric@xxxxxxxx> 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@xxxxxxxx>
> ---
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;
> + }
> +
[...]
