mbox series

[bpf-next,0/3] samples: bpf: Refactor XDP programs with libbpf

Message ID 20201009160353.1529-1-danieltimlee@gmail.com (mailing list archive)
Headers show
Series samples: bpf: Refactor XDP programs with libbpf | expand

Message

Daniel T. Lee Oct. 9, 2020, 4:03 p.m. UTC
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(-)

Comments

Andrii Nakryiko Oct. 9, 2020, 6:29 p.m. UTC | #1
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!
Daniel T. Lee Oct. 10, 2020, 10:41 a.m. UTC | #2
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.