| Message ID | 20201009160353.1529-1-danieltimlee@gmail.com |
|---|---|
| Headers | show |
| Series | samples: bpf: Refactor XDP programs with libbpf | expand |
On Fri, Oct 9, 2020 at 9:04 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > From commit d7a18ea7e8b6 ("libbpf: Add generic bpf_program__attach()"), > for some BPF programs, it is now possible to attach BPF programs > with __attach() instead of explicitly calling __attach_<type>(). > > This commit refactors the __attach_tracepoint() with libbpf's generic > __attach() method. In addition, this refactors the logic of setting > the map FD to simplify the code. Also, the missing removal of > bpf_load.o in Makefile has been fixed. > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > --- > samples/bpf/Makefile | 2 +- > samples/bpf/xdp_redirect_cpu_user.c | 138 +++++++++++++--------------- > 2 files changed, 67 insertions(+), 73 deletions(-) > [...] > #define NUM_TP 5 > +#define NUM_MAP 9 > struct bpf_link *tp_links[NUM_TP] = { 0 }; = {} > +static int map_fds[NUM_MAP]; > static int tp_cnt = 0; > > /* Exit return codes */ [...] > -static struct bpf_link * attach_tp(struct bpf_object *obj, > - const char *tp_category, > - const char* tp_name) > +static int init_tracepoints(struct bpf_object *obj) > { > + char *tp_section = "tracepoint/"; > struct bpf_program *prog; > - struct bpf_link *link; > - char sec_name[PATH_MAX]; > - int len; > + const char *section; > > - len = snprintf(sec_name, PATH_MAX, "tracepoint/%s/%s", > - tp_category, tp_name); > - if (len < 0) > - exit(EXIT_FAIL); > + bpf_object__for_each_program(prog, obj) { > + section = bpf_program__section_name(prog); > + if (strncmp(section, tp_section, strlen(tp_section)) != 0) > + continue; that's a convoluted and error-prone way (you can also use "tp/bla/bla" for tracepoint programs, for example). Use bpf_program__is_tracepoint() check. > > - prog = bpf_object__find_program_by_title(obj, sec_name); > - if (!prog) { > - fprintf(stderr, "ERR: finding progsec: %s\n", sec_name); > - exit(EXIT_FAIL_BPF); > + tp_links[tp_cnt] = bpf_program__attach(prog); > + if (libbpf_get_error(tp_links[tp_cnt])) { > + tp_links[tp_cnt] = NULL; > + return -EINVAL; > + } > + tp_cnt++; > } > [...]
On Fri, Oct 9, 2020 at 9:04 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > To avoid confusion caused by the increasing fragmentation of the BPF > Loader program, this commit would like to convert the previous bpf_load > loader with the libbpf loader. > > Thanks to libbpf's bpf_link interface, managing the tracepoint BPF > program is much easier. bpf_program__attach_tracepoint manages the > enable of tracepoint event and attach of BPF programs to it with a > single interface bpf_link, so there is no need to manage event_fd and > prog_fd separately. > > And due to addition of generic bpf_program__attach() to libbpf, it is > now possible to attach BPF programs with __attach() instead of > explicitly calling __attach_<type>(). > > This patchset refactors xdp_monitor with using this libbpf API, and the > bpf_load is removed and migrated to libbpf. Also, attach_tracepoint() > is replaced with the generic __attach() method in xdp_redirect_cpu. > Moreover, maps in kern program have been converted to BTF-defined map. > > Daniel T. Lee (3): > samples: bpf: Refactor xdp_monitor with libbpf > samples: bpf: Replace attach_tracepoint() to attach() in > xdp_redirect_cpu > samples: bpf: refactor XDP kern program maps with BTF-defined map > > samples/bpf/Makefile | 4 +- > samples/bpf/xdp_monitor_kern.c | 60 ++++++------ > samples/bpf/xdp_monitor_user.c | 144 +++++++++++++++++++++------- > samples/bpf/xdp_redirect_cpu_user.c | 138 +++++++++++++------------- > samples/bpf/xdp_sample_pkts_kern.c | 14 ++- > samples/bpf/xdp_sample_pkts_user.c | 1 - > 6 files changed, 211 insertions(+), 150 deletions(-) > > -- > 2.25.1 > Thanks for this clean up, Daniel! It's great! I left a few nits here and there in the appropriate patches. There still seem to be a bunch of users of bpf_load.c, which would be nice to get rid of completely. But before you go do that, consider integrating BPF skeleton into samples/bpf Makefile. That way instead of all those look ups of maps/programs by name, you'd be writing a straightforward skel->maps.my_map and similar short and non-failing code. This should make the overall time spent on conversion much smaller (and more pleasant, IMO). You've dealt with a lot of samples/bpf reworking, so it should be too hard for you to figure out the best way to do this, but check selftests/bpf's Makefile, if you need some ideas. Or just ask for help. Thanks!
On Sat, Oct 10, 2020 at 3:23 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Oct 9, 2020 at 9:04 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > > > From commit d7a18ea7e8b6 ("libbpf: Add generic bpf_program__attach()"), > > for some BPF programs, it is now possible to attach BPF programs > > with __attach() instead of explicitly calling __attach_<type>(). > > > > This commit refactors the __attach_tracepoint() with libbpf's generic > > __attach() method. In addition, this refactors the logic of setting > > the map FD to simplify the code. Also, the missing removal of > > bpf_load.o in Makefile has been fixed. > > > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > > --- > > samples/bpf/Makefile | 2 +- > > samples/bpf/xdp_redirect_cpu_user.c | 138 +++++++++++++--------------- > > 2 files changed, 67 insertions(+), 73 deletions(-) > > > > [...] > > > #define NUM_TP 5 > > +#define NUM_MAP 9 > > struct bpf_link *tp_links[NUM_TP] = { 0 }; > > = {} > > > +static int map_fds[NUM_MAP]; > > static int tp_cnt = 0; > > > > /* Exit return codes */ > > [...] > > > -static struct bpf_link * attach_tp(struct bpf_object *obj, > > - const char *tp_category, > > - const char* tp_name) > > +static int init_tracepoints(struct bpf_object *obj) > > { > > + char *tp_section = "tracepoint/"; > > struct bpf_program *prog; > > - struct bpf_link *link; > > - char sec_name[PATH_MAX]; > > - int len; > > + const char *section; > > > > - len = snprintf(sec_name, PATH_MAX, "tracepoint/%s/%s", > > - tp_category, tp_name); > > - if (len < 0) > > - exit(EXIT_FAIL); > > + bpf_object__for_each_program(prog, obj) { > > + section = bpf_program__section_name(prog); > > + if (strncmp(section, tp_section, strlen(tp_section)) != 0) > > + continue; > > that's a convoluted and error-prone way (you can also use "tp/bla/bla" > for tracepoint programs, for example). Use > bpf_program__is_tracepoint() check. > Thanks for the review! I think that's a much better way. I will send the next patch with applying that method. > > > > - prog = bpf_object__find_program_by_title(obj, sec_name); > > - if (!prog) { > > - fprintf(stderr, "ERR: finding progsec: %s\n", sec_name); > > - exit(EXIT_FAIL_BPF); > > + tp_links[tp_cnt] = bpf_program__attach(prog); > > + if (libbpf_get_error(tp_links[tp_cnt])) { > > + tp_links[tp_cnt] = NULL; > > + return -EINVAL; > > + } > > + tp_cnt++; > > } > > > > [...]
On Sat, Oct 10, 2020 at 3:30 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Oct 9, 2020 at 9:04 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > > > To avoid confusion caused by the increasing fragmentation of the BPF > > Loader program, this commit would like to convert the previous bpf_load > > loader with the libbpf loader. > > > > Thanks to libbpf's bpf_link interface, managing the tracepoint BPF > > program is much easier. bpf_program__attach_tracepoint manages the > > enable of tracepoint event and attach of BPF programs to it with a > > single interface bpf_link, so there is no need to manage event_fd and > > prog_fd separately. > > > > And due to addition of generic bpf_program__attach() to libbpf, it is > > now possible to attach BPF programs with __attach() instead of > > explicitly calling __attach_<type>(). > > > > This patchset refactors xdp_monitor with using this libbpf API, and the > > bpf_load is removed and migrated to libbpf. Also, attach_tracepoint() > > is replaced with the generic __attach() method in xdp_redirect_cpu. > > Moreover, maps in kern program have been converted to BTF-defined map. > > > > Daniel T. Lee (3): > > samples: bpf: Refactor xdp_monitor with libbpf > > samples: bpf: Replace attach_tracepoint() to attach() in > > xdp_redirect_cpu > > samples: bpf: refactor XDP kern program maps with BTF-defined map > > > > samples/bpf/Makefile | 4 +- > > samples/bpf/xdp_monitor_kern.c | 60 ++++++------ > > samples/bpf/xdp_monitor_user.c | 144 +++++++++++++++++++++------- > > samples/bpf/xdp_redirect_cpu_user.c | 138 +++++++++++++------------- > > samples/bpf/xdp_sample_pkts_kern.c | 14 ++- > > samples/bpf/xdp_sample_pkts_user.c | 1 - > > 6 files changed, 211 insertions(+), 150 deletions(-) > > > > -- > > 2.25.1 > > > > Thanks for this clean up, Daniel! It's great! I left a few nits here > and there in the appropriate patches. > > There still seem to be a bunch of users of bpf_load.c, which would be > nice to get rid of completely. But before you go do that, consider > integrating BPF skeleton into samples/bpf Makefile. That way instead > of all those look ups of maps/programs by name, you'd be writing a > straightforward skel->maps.my_map and similar short and non-failing > code. This should make the overall time spent on conversion much > smaller (and more pleasant, IMO). > > You've dealt with a lot of samples/bpf reworking, so it should be too > hard for you to figure out the best way to do this, but check > selftests/bpf's Makefile, if you need some ideas. Or just ask for > help. Thanks! Thanks for the great feedback! Thank you for letting me know about the BPF features that I can apply. Currently, I'm not familiar with the BPF skeleton yet, but I'll take a good look at the BPF skeleton to apply it in a more advanced form. Thank you for your time and effort for the review. -- Best, Daniel T. Lee
To avoid confusion caused by the increasing fragmentation of the BPF Loader program, this commit would like to convert the previous bpf_load loader with the libbpf loader. Thanks to libbpf's bpf_link interface, managing the tracepoint BPF program is much easier. bpf_program__attach_tracepoint manages the enable of tracepoint event and attach of BPF programs to it with a single interface bpf_link, so there is no need to manage event_fd and prog_fd separately. And due to addition of generic bpf_program__attach() to libbpf, it is now possible to attach BPF programs with __attach() instead of explicitly calling __attach_<type>(). This patchset refactors xdp_monitor with using this libbpf API, and the bpf_load is removed and migrated to libbpf. Also, attach_tracepoint() is replaced with the generic __attach() method in xdp_redirect_cpu. Moreover, maps in kern program have been converted to BTF-defined map. Daniel T. Lee (3): samples: bpf: Refactor xdp_monitor with libbpf samples: bpf: Replace attach_tracepoint() to attach() in xdp_redirect_cpu samples: bpf: refactor XDP kern program maps with BTF-defined map samples/bpf/Makefile | 4 +- samples/bpf/xdp_monitor_kern.c | 60 ++++++------ samples/bpf/xdp_monitor_user.c | 144 +++++++++++++++++++++------- samples/bpf/xdp_redirect_cpu_user.c | 138 +++++++++++++------------- samples/bpf/xdp_sample_pkts_kern.c | 14 ++- samples/bpf/xdp_sample_pkts_user.c | 1 - 6 files changed, 211 insertions(+), 150 deletions(-)