From: "Daniel T. Lee" <danieltimlee@gmail.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>,
Alexei Starovoitov <ast@kernel.org>,
Jesper Dangaard Brouer <brouer@redhat.com>,
Lorenzo Bianconi <lorenzo@kernel.org>, bpf <bpf@vger.kernel.org>,
Networking <netdev@vger.kernel.org>,
Xdp <xdp-newbies@vger.kernel.org>
Subject: Re: [PATCH bpf-next 1/3] samples: bpf: Refactor xdp_monitor with libbpf
Date: Sat, 10 Oct 2020 19:08:45 +0900 [thread overview]
Message-ID: <CAEKGpzjcYCgOFCN2f4M-X-mnozTrcayp4jQVb6YB9cYE0M8F8A@mail.gmail.com> (raw)
In-Reply-To: <CAEf4BzYNF_BbwXM-HFFSk=ybJRdR=_P1OcVwxZ6dav6_b4BOWw@mail.gmail.com>
On Sat, Oct 10, 2020 at 3:17 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 change to the libbpf loader
> > instead of using the bpf_load.
> >
> > 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.
> >
> > This commit refactors xdp_monitor with using this libbpf API, and the
> > bpf_load is removed and migrated to libbpf.
> >
> > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > ---
> > samples/bpf/Makefile | 2 +-
> > samples/bpf/xdp_monitor_user.c | 144 ++++++++++++++++++++++++---------
> > 2 files changed, 108 insertions(+), 38 deletions(-)
> >
>
> [...]
>
> > +static int tp_cnt;
> > +static int map_cnt;
> > static int verbose = 1;
> > static bool debug = false;
> > +struct bpf_map *map_data[NUM_MAP] = { 0 };
> > +struct bpf_link *tp_links[NUM_TP] = { 0 };
>
> this syntax means "initialize *only the first element* to 0
> (explicitly) and the rest of elements to default (which is also 0)".
> So it's just misleading, use ` = {}`.
>
Thanks for the great review!
Come to think of it, it could be confusing as you mentioned. I will
remove the unnecessary initializer in the next patch and resend it.
> >
> > static const struct option long_options[] = {
> > {"help", no_argument, NULL, 'h' },
> > @@ -41,6 +65,15 @@ static const struct option long_options[] = {
> > {0, 0, NULL, 0 }
> > };
> >
> > +static void int_exit(int sig)
> > +{
> > + /* Detach tracepoints */
> > + while (tp_cnt)
> > + bpf_link__destroy(tp_links[--tp_cnt]);
> > +
>
> see below about proper cleanup
>
> > + exit(0);
> > +}
> > +
> > /* C standard specifies two constants, EXIT_SUCCESS(0) and EXIT_FAILURE(1) */
> > #define EXIT_FAIL_MEM 5
> >
>
> [...]
>
> >
> > -static void print_bpf_prog_info(void)
> > +static void print_bpf_prog_info(struct bpf_object *obj)
> > {
> > - int i;
> > + struct bpf_program *prog;
> > + struct bpf_map *map;
> > + int i = 0;
> >
> > /* Prog info */
> > - printf("Loaded BPF prog have %d bpf program(s)\n", prog_cnt);
> > - for (i = 0; i < prog_cnt; i++) {
> > - printf(" - prog_fd[%d] = fd(%d)\n", i, prog_fd[i]);
> > + printf("Loaded BPF prog have %d bpf program(s)\n", tp_cnt);
> > + bpf_object__for_each_program(prog, obj) {
> > + printf(" - prog_fd[%d] = fd(%d)\n", i++, bpf_program__fd(prog));
> > }
> >
> > + i = 0;
> > /* Maps info */
> > - printf("Loaded BPF prog have %d map(s)\n", map_data_count);
> > - for (i = 0; i < map_data_count; i++) {
> > - char *name = map_data[i].name;
> > - int fd = map_data[i].fd;
> > + printf("Loaded BPF prog have %d map(s)\n", map_cnt);
> > + bpf_object__for_each_map(map, obj) {
> > + const char *name = bpf_map__name(map);
> > + int fd = bpf_map__fd(map);
> >
> > - printf(" - map_data[%d] = fd(%d) name:%s\n", i, fd, name);
> > + printf(" - map_data[%d] = fd(%d) name:%s\n", i++, fd, name);
>
> please move out increment into a separate statement, no need to
> confuse readers unnecessarily
>
I will fix it at the following patch.
> > }
> >
> > /* Event info */
> > - printf("Searching for (max:%d) event file descriptor(s)\n", prog_cnt);
> > - for (i = 0; i < prog_cnt; i++) {
> > - if (event_fd[i] != -1)
> > - printf(" - event_fd[%d] = fd(%d)\n", i, event_fd[i]);
> > + printf("Searching for (max:%d) event file descriptor(s)\n", tp_cnt);
> > + for (i = 0; i < tp_cnt; i++) {
> > + int fd = bpf_link__fd(tp_links[i]);
> > +
> > + if (fd != -1)
> > + printf(" - event_fd[%d] = fd(%d)\n", i, fd);
> > }
> > }
> >
> > int main(int argc, char **argv)
> > {
>
> [...]
>
> > + obj = bpf_object__open_file(filename, NULL);
> > + if (libbpf_get_error(obj)) {
> > + printf("ERROR: opening BPF object file failed\n");
> > + obj = NULL;
> > return EXIT_FAILURE;
> > }
> > - if (!prog_fd[0]) {
> > - printf("ERROR - load_bpf_file: %s\n", strerror(errno));
> > +
> > + /* load BPF program */
> > + if (bpf_object__load(obj)) {
>
> would be still good to call bpf_object__close(obj) here, this will
> avoid warnings about memory leaks, if you run this program under ASAN
>
> > + printf("ERROR: loading BPF object file failed\n");
> > return EXIT_FAILURE;
> > }
> >
> > + for (type = 0; type < NUM_MAP; type++) {
> > + map_data[type] =
> > + bpf_object__find_map_by_name(obj, map_type_strings[type]);
> > +
> > + if (libbpf_get_error(map_data[type])) {
> > + printf("ERROR: finding a map in obj file failed\n");
>
> same about cleanup, goto into single cleanup place would be
> appropriate throughout this entire function, probably.
>
Jump to single cleanup will be much more intuitive.
I will update and send the next version of patch right away.
Thank you for your time and effort for the review.
Best,
Daniel
> > + return EXIT_FAILURE;
> > + }
> > + map_cnt++;
> > + }
> > +
>
> [...]
next prev parent reply other threads:[~2020-10-10 11:44 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-09 16:03 [PATCH bpf-next 0/3] samples: bpf: Refactor XDP programs with libbpf Daniel T. Lee
2020-10-09 16:03 ` [PATCH bpf-next 1/3] samples: bpf: Refactor xdp_monitor " Daniel T. Lee
2020-10-09 18:17 ` Andrii Nakryiko
2020-10-10 10:08 ` Daniel T. Lee [this message]
2020-10-09 16:03 ` [PATCH bpf-next 2/3] samples: bpf: Replace attach_tracepoint() to attach() in xdp_redirect_cpu Daniel T. Lee
2020-10-09 18:23 ` Andrii Nakryiko
2020-10-10 9:58 ` Daniel T. Lee
2020-10-09 16:03 ` [PATCH bpf-next 3/3] samples: bpf: refactor XDP kern program maps with BTF-defined map Daniel T. Lee
2020-10-09 18:25 ` Andrii Nakryiko
2020-10-10 9:55 ` Daniel T. Lee
2020-10-09 18:29 ` [PATCH bpf-next 0/3] samples: bpf: Refactor XDP programs with libbpf Andrii Nakryiko
2020-10-10 10:41 ` Daniel T. Lee
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAEKGpzjcYCgOFCN2f4M-X-mnozTrcayp4jQVb6YB9cYE0M8F8A@mail.gmail.com \
--to=danieltimlee@gmail.com \
--cc=andrii.nakryiko@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=brouer@redhat.com \
--cc=daniel@iogearbox.net \
--cc=lorenzo@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=xdp-newbies@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).