| Message ID | 20201117145644.1166255-1-danieltimlee@gmail.com |
|---|---|
| Headers | show |
| Series | bpf: remove bpf_load loader completely | expand |
On Tue, Nov 17, 2020 at 02:56:36PM +0000, Daniel T. Lee wrote: > Under the samples/bpf directory, similar tracing helpers are > fragmented around. To keep consistent of tracing programs, this commit > moves the helper and define locations to increase the reuse of each > helper function. > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > > --- > samples/bpf/Makefile | 2 +- > samples/bpf/hbm.c | 51 ++++----------------- > tools/testing/selftests/bpf/trace_helpers.c | 33 ++++++++++++- > tools/testing/selftests/bpf/trace_helpers.h | 3 ++ > 4 files changed, 45 insertions(+), 44 deletions(-) > > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile > index aeebf5d12f32..3e83cd5ca1c2 100644 > --- a/samples/bpf/Makefile > +++ b/samples/bpf/Makefile > @@ -110,7 +110,7 @@ xdp_fwd-objs := xdp_fwd_user.o > task_fd_query-objs := bpf_load.o task_fd_query_user.o $(TRACE_HELPERS) > xdp_sample_pkts-objs := xdp_sample_pkts_user.o $(TRACE_HELPERS) > ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS) > -hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS) > +hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS) $(TRACE_HELPERS) > > # Tell kbuild to always build the programs > always-y := $(tprogs-y) > diff --git a/samples/bpf/hbm.c b/samples/bpf/hbm.c > index 400e741a56eb..b9f9f771dd81 100644 > --- a/samples/bpf/hbm.c > +++ b/samples/bpf/hbm.c > @@ -48,6 +48,7 @@ > > #include "bpf_load.h" > #include "bpf_rlimit.h" > +#include "trace_helpers.h" > #include "cgroup_helpers.h" > #include "hbm.h" > #include "bpf_util.h" > @@ -65,51 +66,12 @@ bool no_cn_flag; > bool edt_flag; > > static void Usage(void); > -static void read_trace_pipe2(void); > static void do_error(char *msg, bool errno_flag); > > -#define DEBUGFS "/sys/kernel/debug/tracing/" > - > struct bpf_object *obj; > int bpfprog_fd; > int cgroup_storage_fd; > > -static void read_trace_pipe2(void) > -{ > - int trace_fd; > - FILE *outf; > - char *outFname = "hbm_out.log"; > - > - trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0); > - if (trace_fd < 0) { > - printf("Error opening trace_pipe\n"); > - return; > - } > - > -// Future support of ingress > -// if (!outFlag) > -// outFname = "hbm_in.log"; > - outf = fopen(outFname, "w"); > - > - if (outf == NULL) > - printf("Error creating %s\n", outFname); > - > - while (1) { > - static char buf[4097]; > - ssize_t sz; > - > - sz = read(trace_fd, buf, sizeof(buf) - 1); > - if (sz > 0) { > - buf[sz] = 0; > - puts(buf); > - if (outf != NULL) { > - fprintf(outf, "%s\n", buf); > - fflush(outf); > - } > - } > - } > -} > - > static void do_error(char *msg, bool errno_flag) > { > if (errno_flag) > @@ -392,8 +354,15 @@ static int run_bpf_prog(char *prog, int cg_id) > fclose(fout); > } > > - if (debugFlag) > - read_trace_pipe2(); > + if (debugFlag) { > + char *out_fname = "hbm_out.log"; > + /* Future support of ingress */ > + // if (!outFlag) > + // out_fname = "hbm_in.log"; > + > + read_trace_pipe2(out_fname); > + } > + > return rc; > err: > rc = 1; > diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c > index 1bbd1d9830c8..b7c184e109e8 100644 > --- a/tools/testing/selftests/bpf/trace_helpers.c > +++ b/tools/testing/selftests/bpf/trace_helpers.c > @@ -11,8 +11,6 @@ > #include <sys/mman.h> > #include "trace_helpers.h" > > -#define DEBUGFS "/sys/kernel/debug/tracing/" Is this change needed? > - > #define MAX_SYMS 300000 > static struct ksym syms[MAX_SYMS]; > static int sym_cnt; > @@ -136,3 +134,34 @@ void read_trace_pipe(void) > } > } > } > + > +void read_trace_pipe2(char *filename) > +{ > + int trace_fd; > + FILE *outf; > + > + trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0); > + if (trace_fd < 0) { > + printf("Error opening trace_pipe\n"); > + return; > + } > + > + outf = fopen(filename, "w"); > + if (!outf) > + printf("Error creating %s\n", filename); > + > + while (1) { > + static char buf[4096]; > + ssize_t sz; > + > + sz = read(trace_fd, buf, sizeof(buf) - 1); > + if (sz > 0) { > + buf[sz] = 0; > + puts(buf); > + if (outf) { > + fprintf(outf, "%s\n", buf); > + fflush(outf); > + } > + } > + } It needs a fclose(). IIUC, this function will never return. I am not sure this is something that should be made available to selftests.
On Wed, Nov 18, 2020 at 10:19 AM Martin KaFai Lau <kafai@fb.com> wrote: > > On Tue, Nov 17, 2020 at 02:56:36PM +0000, Daniel T. Lee wrote: > > Under the samples/bpf directory, similar tracing helpers are > > fragmented around. To keep consistent of tracing programs, this commit > > moves the helper and define locations to increase the reuse of each > > helper function. > > > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > > > > --- > > samples/bpf/Makefile | 2 +- > > samples/bpf/hbm.c | 51 ++++----------------- > > tools/testing/selftests/bpf/trace_helpers.c | 33 ++++++++++++- > > tools/testing/selftests/bpf/trace_helpers.h | 3 ++ > > 4 files changed, 45 insertions(+), 44 deletions(-) > > > > [...] > > > > -#define DEBUGFS "/sys/kernel/debug/tracing/" > Is this change needed? This macro can be used in other samples such as the 4th' patch of this patchset, task_fd_query. > > - > > #define MAX_SYMS 300000 > > static struct ksym syms[MAX_SYMS]; > > static int sym_cnt; > > @@ -136,3 +134,34 @@ void read_trace_pipe(void) > > } > > } > > } > > + > > +void read_trace_pipe2(char *filename) > > +{ > > + int trace_fd; > > + FILE *outf; > > + > > + trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0); > > + if (trace_fd < 0) { > > + printf("Error opening trace_pipe\n"); > > + return; > > + } > > + > > + outf = fopen(filename, "w"); > > + if (!outf) > > + printf("Error creating %s\n", filename); > > + > > + while (1) { > > + static char buf[4096]; > > + ssize_t sz; > > + > > + sz = read(trace_fd, buf, sizeof(buf) - 1); > > + if (sz > 0) { > > + buf[sz] = 0; > > + puts(buf); > > + if (outf) { > > + fprintf(outf, "%s\n", buf); > > + fflush(outf); > > + } > > + } > > + } > It needs a fclose(). > > IIUC, this function will never return. I am not sure > this is something that should be made available to selftests. Actually, read_trace_pipe and read_trace_pipe2 are helpers that are only used under samples directory. Since these helpers are not used in selftests, What do you think about moving these helpers to something like samples/bpf/trace_pipe.h? Thanks for your time and effort for the review.
On Tue, Nov 17, 2020 at 6:57 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > This commit refactors the existing ibumad program with libbpf bpf > loader. Attach/detach of Tracepoint bpf programs has been managed > with the generic bpf_program__attach() and bpf_link__destroy() from > the libbpf. > > Also, instead of using the previous BPF MAP definition, this commit > refactors ibumad MAP definition with the new BTF-defined MAP format. > > To verify that this bpf program works without an infiniband device, > try loading ib_umad kernel module and test the program as follows: > > # modprobe ib_umad > # ./ibumad > > Moreover, TRACE_HELPERS has been removed from the Makefile since it is > not used on this program. > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > --- > samples/bpf/Makefile | 2 +- > samples/bpf/ibumad_kern.c | 26 +++++++-------- > samples/bpf/ibumad_user.c | 66 ++++++++++++++++++++++++++++++--------- > 3 files changed, 65 insertions(+), 29 deletions(-) > > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile > index 36b261c7afc7..bfa595379493 100644 > --- a/samples/bpf/Makefile > +++ b/samples/bpf/Makefile > @@ -109,7 +109,7 @@ xsk_fwd-objs := xsk_fwd.o > xdp_fwd-objs := xdp_fwd_user.o > task_fd_query-objs := task_fd_query_user.o $(TRACE_HELPERS) > xdp_sample_pkts-objs := xdp_sample_pkts_user.o $(TRACE_HELPERS) > -ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS) > +ibumad-objs := ibumad_user.o > hbm-objs := hbm.o $(CGROUP_HELPERS) $(TRACE_HELPERS) > > # Tell kbuild to always build the programs > diff --git a/samples/bpf/ibumad_kern.c b/samples/bpf/ibumad_kern.c > index 3a91b4c1989a..26dcd4dde946 100644 > --- a/samples/bpf/ibumad_kern.c > +++ b/samples/bpf/ibumad_kern.c > @@ -16,19 +16,19 @@ > #include <bpf/bpf_helpers.h> > > > -struct bpf_map_def SEC("maps") read_count = { > - .type = BPF_MAP_TYPE_ARRAY, > - .key_size = sizeof(u32), /* class; u32 required */ > - .value_size = sizeof(u64), /* count of mads read */ > - .max_entries = 256, /* Room for all Classes */ > -}; > - > -struct bpf_map_def SEC("maps") write_count = { > - .type = BPF_MAP_TYPE_ARRAY, > - .key_size = sizeof(u32), /* class; u32 required */ > - .value_size = sizeof(u64), /* count of mads written */ > - .max_entries = 256, /* Room for all Classes */ > -}; > +struct { > + __uint(type, BPF_MAP_TYPE_ARRAY); > + __type(key, u32); /* class; u32 required */ > + __type(value, u64); /* count of mads read */ > + __uint(max_entries, 256); /* Room for all Classes */ > +} read_count SEC(".maps"); > + > +struct { > + __uint(type, BPF_MAP_TYPE_ARRAY); > + __type(key, u32); /* class; u32 required */ > + __type(value, u64); /* count of mads written */ > + __uint(max_entries, 256); /* Room for all Classes */ > +} write_count SEC(".maps"); > > #undef DEBUG > #ifndef DEBUG > diff --git a/samples/bpf/ibumad_user.c b/samples/bpf/ibumad_user.c > index fa06eef31a84..66a06272f242 100644 > --- a/samples/bpf/ibumad_user.c > +++ b/samples/bpf/ibumad_user.c > @@ -23,10 +23,15 @@ > #include <getopt.h> > #include <net/if.h> > > -#include "bpf_load.h" > +#include <bpf/bpf.h> > #include "bpf_util.h" > #include <bpf/libbpf.h> > > +struct bpf_link *tp_links[3] = {}; > +struct bpf_object *obj; statics and you can drop = {} part. > +static int map_fd[2]; > +static int tp_cnt; > + > static void dump_counts(int fd) > { > __u32 key; > @@ -53,6 +58,11 @@ static void dump_all_counts(void) > static void dump_exit(int sig) > { > dump_all_counts(); > + /* Detach tracepoints */ > + while (tp_cnt) > + bpf_link__destroy(tp_links[--tp_cnt]); > + > + bpf_object__close(obj); > exit(0); > } > > @@ -73,19 +83,11 @@ static void usage(char *cmd) > > int main(int argc, char **argv) > { > + struct bpf_program *prog; > unsigned long delay = 5; > + char filename[256]; > int longindex = 0; > int opt; > - char bpf_file[256]; > - > - /* Create the eBPF kernel code path name. > - * This follows the pattern of all of the other bpf samples > - */ > - snprintf(bpf_file, sizeof(bpf_file), "%s_kern.o", argv[0]); > - > - /* Do one final dump when exiting */ > - signal(SIGINT, dump_exit); > - signal(SIGTERM, dump_exit); > > while ((opt = getopt_long(argc, argv, "hd:rSw", > long_options, &longindex)) != -1) { > @@ -107,10 +109,38 @@ int main(int argc, char **argv) > } > } > > - if (load_bpf_file(bpf_file)) { > - fprintf(stderr, "ERROR: failed to load eBPF from file : %s\n", > - bpf_file); > - return 1; > + /* Do one final dump when exiting */ > + signal(SIGINT, dump_exit); > + signal(SIGTERM, dump_exit); > + > + snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]); > + obj = bpf_object__open_file(filename, NULL); > + if (libbpf_get_error(obj)) { > + fprintf(stderr, "ERROR: opening BPF object file failed\n"); > + return 0; not really a success, no? > + } > + > + /* load BPF program */ > + if (bpf_object__load(obj)) { > + fprintf(stderr, "ERROR: loading BPF object file failed\n"); > + goto cleanup; > + } > + > + map_fd[0] = bpf_object__find_map_fd_by_name(obj, "read_count"); > + map_fd[1] = bpf_object__find_map_fd_by_name(obj, "write_count"); > + if (map_fd[0] < 0 || map_fd[1] < 0) { > + fprintf(stderr, "ERROR: finding a map in obj file failed\n"); > + goto cleanup; > + } > + > + bpf_object__for_each_program(prog, obj) { > + tp_links[tp_cnt] = bpf_program__attach(prog); > + if (libbpf_get_error(tp_links[tp_cnt])) { > + fprintf(stderr, "ERROR: bpf_program__attach failed\n"); > + tp_links[tp_cnt] = NULL; > + goto cleanup; > + } > + tp_cnt++; > } This cries for the BPF skeleton... But one step at a time :) > > while (1) { > @@ -118,5 +148,11 @@ int main(int argc, char **argv) > dump_all_counts(); > } > > +cleanup: > + /* Detach tracepoints */ > + while (tp_cnt) > + bpf_link__destroy(tp_links[--tp_cnt]); > + > + bpf_object__close(obj); > return 0; same, in a lot of cases it's not a success, probably need int err variable somewhere. > } > -- > 2.25.1 >
On Tue, Nov 17, 2020 at 6:55 PM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > On Wed, Nov 18, 2020 at 10:58 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Nov 17, 2020 at 6:57 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > > > > > Under the samples/bpf directory, similar tracing helpers are > > > fragmented around. To keep consistent of tracing programs, this commit > > > moves the helper and define locations to increase the reuse of each > > > helper function. > > > > > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > > > > > > --- > > > [...] > > > -static void read_trace_pipe2(void) > > > > This is used only in hbm.c, why move it into trace_helpers.c? > > > > I think this function can be made into a helper that can be used in > other programs. Which is basically same as 'read_trace_pipe' and > also writes the content of that pipe to file either. Well, it's not used > anywhere else, but I moved this function for the potential of reuse. Let's not make premature extraction of helpers. We generally add helpers when we have a repeated need for them. It's not currently the case for read_trace_pipe2(). > > Since these 'read_trace_pipe's are helpers that are only used > under samples directory, what do you think about moving these > helpers to something like samples/bpf/trace_pipe.h? Nope, not yet. I'd just drop this patch for now (see my comments on another patch regarding DEBUGFS). > > Thanks for your time and effort for the review. > > -- > Best, > Daniel T. Lee
On Tue, Nov 17, 2020 at 7:05 PM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > On Wed, Nov 18, 2020 at 11:52 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Nov 17, 2020 at 6:57 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > > > > > This commit refactors the existing ibumad program with libbpf bpf > > > loader. Attach/detach of Tracepoint bpf programs has been managed > > > with the generic bpf_program__attach() and bpf_link__destroy() from > > > the libbpf. > > > > > > Also, instead of using the previous BPF MAP definition, this commit > > > refactors ibumad MAP definition with the new BTF-defined MAP format. > > > > > > To verify that this bpf program works without an infiniband device, > > > try loading ib_umad kernel module and test the program as follows: > > > > > > # modprobe ib_umad > > > # ./ibumad > > > > > > Moreover, TRACE_HELPERS has been removed from the Makefile since it is > > > not used on this program. > > > > > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > > > --- > > > samples/bpf/Makefile | 2 +- > > > samples/bpf/ibumad_kern.c | 26 +++++++-------- > > > samples/bpf/ibumad_user.c | 66 ++++++++++++++++++++++++++++++--------- > > > 3 files changed, 65 insertions(+), 29 deletions(-) > > > > > > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile > > > index 36b261c7afc7..bfa595379493 100644 > > > --- a/samples/bpf/Makefile > > > +++ b/samples/bpf/Makefile > > > @@ -109,7 +109,7 @@ xsk_fwd-objs := xsk_fwd.o > > > xdp_fwd-objs := xdp_fwd_user.o > > > task_fd_query-objs := task_fd_query_user.o $(TRACE_HELPERS) > > > xdp_sample_pkts-objs := xdp_sample_pkts_user.o $(TRACE_HELPERS) > > > -ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS) > > > +ibumad-objs := ibumad_user.o > > > hbm-objs := hbm.o $(CGROUP_HELPERS) $(TRACE_HELPERS) > > > > > > # Tell kbuild to always build the programs > > > diff --git a/samples/bpf/ibumad_kern.c b/samples/bpf/ibumad_kern.c > > > index 3a91b4c1989a..26dcd4dde946 100644 > > > --- a/samples/bpf/ibumad_kern.c > > > +++ b/samples/bpf/ibumad_kern.c > > > @@ -16,19 +16,19 @@ > > > #include <bpf/bpf_helpers.h> > > > > > > > > > -struct bpf_map_def SEC("maps") read_count = { > > > - .type = BPF_MAP_TYPE_ARRAY, > > > - .key_size = sizeof(u32), /* class; u32 required */ > > > - .value_size = sizeof(u64), /* count of mads read */ > > > - .max_entries = 256, /* Room for all Classes */ > > > -}; > > > - > > > -struct bpf_map_def SEC("maps") write_count = { > > > - .type = BPF_MAP_TYPE_ARRAY, > > > - .key_size = sizeof(u32), /* class; u32 required */ > > > - .value_size = sizeof(u64), /* count of mads written */ > > > - .max_entries = 256, /* Room for all Classes */ > > > -}; > > > +struct { > > > + __uint(type, BPF_MAP_TYPE_ARRAY); > > > + __type(key, u32); /* class; u32 required */ > > > + __type(value, u64); /* count of mads read */ > > > + __uint(max_entries, 256); /* Room for all Classes */ > > > +} read_count SEC(".maps"); > > > + > > > +struct { > > > + __uint(type, BPF_MAP_TYPE_ARRAY); > > > + __type(key, u32); /* class; u32 required */ > > > + __type(value, u64); /* count of mads written */ > > > + __uint(max_entries, 256); /* Room for all Classes */ > > > +} write_count SEC(".maps"); > > > > > > #undef DEBUG > > > #ifndef DEBUG > > > diff --git a/samples/bpf/ibumad_user.c b/samples/bpf/ibumad_user.c > > > index fa06eef31a84..66a06272f242 100644 > > > --- a/samples/bpf/ibumad_user.c > > > +++ b/samples/bpf/ibumad_user.c > > > @@ -23,10 +23,15 @@ > > > #include <getopt.h> > > > #include <net/if.h> > > > > > > -#include "bpf_load.h" > > > +#include <bpf/bpf.h> > > > #include "bpf_util.h" > > > #include <bpf/libbpf.h> > > > > > > +struct bpf_link *tp_links[3] = {}; > > > +struct bpf_object *obj; > > > > statics and you can drop = {} part. > > > > > +static int map_fd[2]; > > > +static int tp_cnt; > > > + > > > static void dump_counts(int fd) > > > { > > > __u32 key; > > > @@ -53,6 +58,11 @@ static void dump_all_counts(void) > > > static void dump_exit(int sig) > > > { > > > dump_all_counts(); > > > + /* Detach tracepoints */ > > > + while (tp_cnt) > > > + bpf_link__destroy(tp_links[--tp_cnt]); > > > + > > > + bpf_object__close(obj); > > > exit(0); > > > } > > > > > > @@ -73,19 +83,11 @@ static void usage(char *cmd) > > > > > > int main(int argc, char **argv) > > > { > > > + struct bpf_program *prog; > > > unsigned long delay = 5; > > > + char filename[256]; > > > int longindex = 0; > > > int opt; > > > - char bpf_file[256]; > > > - > > > - /* Create the eBPF kernel code path name. > > > - * This follows the pattern of all of the other bpf samples > > > - */ > > > - snprintf(bpf_file, sizeof(bpf_file), "%s_kern.o", argv[0]); > > > - > > > - /* Do one final dump when exiting */ > > > - signal(SIGINT, dump_exit); > > > - signal(SIGTERM, dump_exit); > > > > > > while ((opt = getopt_long(argc, argv, "hd:rSw", > > > long_options, &longindex)) != -1) { > > > @@ -107,10 +109,38 @@ int main(int argc, char **argv) > > > } > > > } > > > > > > - if (load_bpf_file(bpf_file)) { > > > - fprintf(stderr, "ERROR: failed to load eBPF from file : %s\n", > > > - bpf_file); > > > - return 1; > > > + /* Do one final dump when exiting */ > > > + signal(SIGINT, dump_exit); > > > + signal(SIGTERM, dump_exit); > > > + > > > + snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]); > > > + obj = bpf_object__open_file(filename, NULL); > > > + if (libbpf_get_error(obj)) { > > > + fprintf(stderr, "ERROR: opening BPF object file failed\n"); > > > + return 0; > > > > not really a success, no? > > > > > + } > > > + > > > + /* load BPF program */ > > > + if (bpf_object__load(obj)) { > > > + fprintf(stderr, "ERROR: loading BPF object file failed\n"); > > > + goto cleanup; > > > + } > > > + > > > + map_fd[0] = bpf_object__find_map_fd_by_name(obj, "read_count"); > > > + map_fd[1] = bpf_object__find_map_fd_by_name(obj, "write_count"); > > > + if (map_fd[0] < 0 || map_fd[1] < 0) { > > > + fprintf(stderr, "ERROR: finding a map in obj file failed\n"); > > > + goto cleanup; > > > + } > > > + > > > + bpf_object__for_each_program(prog, obj) { > > > + tp_links[tp_cnt] = bpf_program__attach(prog); > > > + if (libbpf_get_error(tp_links[tp_cnt])) { > > > + fprintf(stderr, "ERROR: bpf_program__attach failed\n"); > > > + tp_links[tp_cnt] = NULL; > > > + goto cleanup; > > > + } > > > + tp_cnt++; > > > } > > > > This cries for the BPF skeleton... But one step at a time :) > > > > I will make sure it'll be updated by using skeleton next time. :D > > > > > > > while (1) { > > > @@ -118,5 +148,11 @@ int main(int argc, char **argv) > > > dump_all_counts(); > > > } > > > > > > +cleanup: > > > + /* Detach tracepoints */ > > > + while (tp_cnt) > > > + bpf_link__destroy(tp_links[--tp_cnt]); > > > + > > > + bpf_object__close(obj); > > > return 0; > > > > same, in a lot of cases it's not a success, probably need int err > > variable somewhere. > > > > I will correct the return code and send the next version of patch. > > Thanks for your time and effort for the review. You don't have to write that every time :) It's an implicit contract: you contribute your time to prepare, test, and submit a patch. Maintainers and reviewers contribute theirs to review, maybe improve, and eventually apply it. Together as a community we move the needle. > > -- > Best, > Daniel T. Lee
On Wed, Nov 18, 2020 at 12:02 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Nov 17, 2020 at 6:57 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > > > This commit refactors the existing cgroup program with libbpf bpf > > loader. The original test_cgrp2_sock2 has keeped the bpf program > > attached to the cgroup hierarchy even after the exit of user program. > > To implement the same functionality with libbpf, this commit uses the > > BPF_LINK_PINNING to pin the link attachment even after it is closed. > > > > Since this uses LINK instead of ATTACH, detach of bpf program from > > cgroup with 'test_cgrp2_sock' is not used anymore. > > > > The code to mount the bpf was added to the .sh file in case the bpff > > was not mounted on /sys/fs/bpf. Additionally, to fix the problem that > > shell script cannot find the binary object from the current path, > > relative path './' has been added in front of binary. > > > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > > --- > > samples/bpf/Makefile | 2 +- > > samples/bpf/test_cgrp2_sock2.c | 63 ++++++++++++++++++++++++--------- > > samples/bpf/test_cgrp2_sock2.sh | 21 ++++++++--- > > 3 files changed, 64 insertions(+), 22 deletions(-) > > > > [...] > > > > > - return EXIT_SUCCESS; > > + err = bpf_link__pin(link, link_pin_path); > > + if (err < 0) { > > + printf("err : %d\n", err); > > more meaningful error message would be helpful > Thanks for pointing out, I will fix it directly! > > + goto cleanup; > > + } > > + > > + ret = EXIT_SUCCESS; > > + > > +cleanup: > > + if (ret != EXIT_SUCCESS) > > + bpf_link__destroy(link); > > + > > + bpf_object__close(obj); > > + return ret; > > } > > [...] > > > > > function attach_bpf { > > - test_cgrp2_sock2 /tmp/cgroupv2/foo sock_flags_kern.o $1 > > + ./test_cgrp2_sock2 /tmp/cgroupv2/foo sock_flags_kern.o $1 > > Can you please add Fixes: tag for this? > Will add it in the next version of patch :) Thanks for your time and effort for the review.
On Tue, Nov 17, 2020 at 02:56:38PM +0000, Daniel T. Lee wrote: [ ... ] > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile > index 01449d767122..7a643595ac6c 100644 > --- a/samples/bpf/Makefile > +++ b/samples/bpf/Makefile > @@ -82,7 +82,7 @@ test_overhead-objs := bpf_load.o test_overhead_user.o > test_cgrp2_array_pin-objs := test_cgrp2_array_pin.o > test_cgrp2_attach-objs := test_cgrp2_attach.o > test_cgrp2_sock-objs := test_cgrp2_sock.o > -test_cgrp2_sock2-objs := bpf_load.o test_cgrp2_sock2.o > +test_cgrp2_sock2-objs := test_cgrp2_sock2.o > xdp1-objs := xdp1_user.o > # reuse xdp1 source intentionally > xdp2-objs := xdp1_user.o > diff --git a/samples/bpf/test_cgrp2_sock2.c b/samples/bpf/test_cgrp2_sock2.c > index a9277b118c33..518526c7ce16 100644 > --- a/samples/bpf/test_cgrp2_sock2.c > +++ b/samples/bpf/test_cgrp2_sock2.c > @@ -20,9 +20,9 @@ > #include <net/if.h> > #include <linux/bpf.h> > #include <bpf/bpf.h> > +#include <bpf/libbpf.h> > > #include "bpf_insn.h" > -#include "bpf_load.h" > > static int usage(const char *argv0) > { > @@ -32,37 +32,66 @@ static int usage(const char *argv0) > > int main(int argc, char **argv) > { > - int cg_fd, ret, filter_id = 0; > + int cg_fd, err, ret = EXIT_FAILURE, filter_id = 0, prog_cnt = 0; > + const char *link_pin_path = "/sys/fs/bpf/test_cgrp2_sock2"; > + struct bpf_link *link = NULL; > + struct bpf_program *progs[2]; > + struct bpf_program *prog; > + struct bpf_object *obj; > > if (argc < 3) > return usage(argv[0]); > > + if (argc > 3) > + filter_id = atoi(argv[3]); > + > cg_fd = open(argv[1], O_DIRECTORY | O_RDONLY); > if (cg_fd < 0) { > printf("Failed to open cgroup path: '%s'\n", strerror(errno)); > - return EXIT_FAILURE; > + return ret; > } > > - if (load_bpf_file(argv[2])) > - return EXIT_FAILURE; > - > - printf("Output from kernel verifier:\n%s\n-------\n", bpf_log_buf); > + obj = bpf_object__open_file(argv[2], NULL); > + if (libbpf_get_error(obj)) { > + printf("ERROR: opening BPF object file failed\n"); > + return ret; > + } > > - if (argc > 3) > - filter_id = atoi(argv[3]); > + bpf_object__for_each_program(prog, obj) { > + progs[prog_cnt] = prog; > + prog_cnt++; > + } > > if (filter_id >= prog_cnt) { > printf("Invalid program id; program not found in file\n"); > - return EXIT_FAILURE; > + goto cleanup; > + } > + > + /* load BPF program */ > + if (bpf_object__load(obj)) { > + printf("ERROR: loading BPF object file failed\n"); > + goto cleanup; > } > > - ret = bpf_prog_attach(prog_fd[filter_id], cg_fd, > - BPF_CGROUP_INET_SOCK_CREATE, 0); > - if (ret < 0) { > - printf("Failed to attach prog to cgroup: '%s'\n", > - strerror(errno)); > - return EXIT_FAILURE; > + link = bpf_program__attach_cgroup(progs[filter_id], cg_fd); > + if (libbpf_get_error(link)) { > + printf("ERROR: bpf_program__attach failed\n"); > + link = NULL; > + goto cleanup; > } > > - return EXIT_SUCCESS; > + err = bpf_link__pin(link, link_pin_path); > + if (err < 0) { > + printf("err : %d\n", err); > + goto cleanup; > + } > + > + ret = EXIT_SUCCESS; > + > +cleanup: > + if (ret != EXIT_SUCCESS) > + bpf_link__destroy(link); This looks wrong. cleanup should be done regardless. > + > + bpf_object__close(obj); > + return ret; > }