Re: [PATCH v3 bpf-next 4/4] tools/bpftool: switch map event_pipe to libbpf's perf_buffer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Re-sending as plain text, first time it somehow turned into HTML... :(


On Sat, Jun 29, 2019 at 11:24 AM Jakub Kicinski
<jakub.kicinski@xxxxxxxxxxxxx> wrote:
>
> On Fri, 28 Jun 2019 22:53:09 -0700, Andrii Nakryiko wrote:
> >       map_info_len = sizeof(map_info);
> >       map_fd = map_parse_fd_and_info(&argc, &argv, &map_info, &map_info_len);
> > -     if (map_fd < 0)
> > +     if (map_fd < 0) {
> > +             p_err("failed to get map info");
>
> Can't do, map_parse_fd_and_info() prints an error already, we can't
> have multiple errors in JSON.

Oh.. I need to be more careful with printing error in bpftool due to
this JSON business... Thanks!

>
> >               return -1;
> > +     }
> >
> >       if (map_info.type != BPF_MAP_TYPE_PERF_EVENT_ARRAY) {
> >               p_err("map is not a perf event array");
> > @@ -205,7 +157,7 @@ int do_event_pipe(int argc, char **argv)
> >                       char *endptr;
> >
> >                       NEXT_ARG();
> > -                     cpu = strtoul(*argv, &endptr, 0);
> > +                     ctx.cpu = strtoul(*argv, &endptr, 0);
> >                       if (*endptr) {
> >                               p_err("can't parse %s as CPU ID", **argv);
> >                               goto err_close_map;
> > @@ -216,7 +168,7 @@ int do_event_pipe(int argc, char **argv)
> >                       char *endptr;
> >
> >                       NEXT_ARG();
> > -                     index = strtoul(*argv, &endptr, 0);
> > +                     ctx.idx = strtoul(*argv, &endptr, 0);
> >                       if (*endptr) {
> >                               p_err("can't parse %s as index", **argv);
> >                               goto err_close_map;
> > @@ -228,45 +180,32 @@ int do_event_pipe(int argc, char **argv)
> >                       goto err_close_map;
> >               }
> >
> > -             do_all = false;
> > +             ctx.all_cpus = false;
> >       }
> >
> > -     if (!do_all) {
> > -             if (index == -1 || cpu == -1) {
> > +     if (!ctx.all_cpus) {
> > +             if (ctx.idx == -1 || ctx.cpu == -1) {
> >                       p_err("cpu and index must be specified together");
> >                       goto err_close_map;
>
> Now that you look at err looks like we're missing an err = -1 assignment
> here?  but...

No need, see below.

>
> >               }
> > -
> > -             nfds = 1;
> >       } else {
> > -             nfds = min(get_possible_cpus(), map_info.max_entries);
> > -             cpu = 0;
> > -             index = 0;
> > +             ctx.cpu = 0;
> > +             ctx.idx = 0;
> >       }
> >
> > -     rings = calloc(nfds, sizeof(rings[0]));
> > -     if (!rings)
> > +     opts.attr = &perf_attr;
> > +     opts.event_cb = print_bpf_output;
> > +     opts.ctx = &ctx;
> > +     opts.cpu_cnt = ctx.all_cpus ? 0 : 1;
> > +     opts.cpus = &ctx.cpu;
> > +     opts.map_keys = &ctx.idx;
> > +
> > +     pb = perf_buffer__new_raw(map_fd, MMAP_PAGE_CNT, &opts);
> > +     err = libbpf_get_error(pb);
> > +     if (err) {
> > +             p_err("failed to create perf buffer: %s (%d)",
> > +                   strerror(err), err);
> >               goto err_close_map;
> > -
> > -     pfds = calloc(nfds, sizeof(pfds[0]));
> > -     if (!pfds)
> > -             goto err_free_rings;
> > -
> > -     for (i = 0; i < nfds; i++) {
> > -             rings[i].cpu = cpu + i;
> > -             rings[i].key = index + i;
> > -
> > -             rings[i].fd = bpf_perf_event_open(map_fd, rings[i].key,
> > -                                               rings[i].cpu);
> > -             if (rings[i].fd < 0)
> > -                     goto err_close_fds_prev;
> > -
> > -             rings[i].mem = perf_event_mmap(rings[i].fd);
> > -             if (!rings[i].mem)
> > -                     goto err_close_fds_current;
> > -
> > -             pfds[i].fd = rings[i].fd;
> > -             pfds[i].events = POLLIN;
> >       }
> >
> >       signal(SIGINT, int_exit);
> > @@ -277,35 +216,25 @@ int do_event_pipe(int argc, char **argv)
> >               jsonw_start_array(json_wtr);
> >
> >       while (!stop) {
> > -             poll(pfds, nfds, 200);
> > -             for (i = 0; i < nfds; i++)
> > -                     perf_event_read(&rings[i], &tmp_buf, &tmp_buf_sz);
> > +             err = perf_buffer__poll(pb, 200);
> > +             if (err < 0 && err != -EINTR) {
> > +                     p_err("perf buffer polling failed: %s (%d)",
> > +                           strerror(err), err);
> > +                     goto err_close_pb;
> > +             }
> >       }
> > -     free(tmp_buf);
> >
> >       if (json_output)
> >               jsonw_end_array(json_wtr);
> >
> > -     for (i = 0; i < nfds; i++) {
> > -             perf_event_unmap(rings[i].mem);
> > -             close(rings[i].fd);
> > -     }
> > -     free(pfds);
> > -     free(rings);
> > +     perf_buffer__free(pb);
> >       close(map_fd);
> >
> >       return 0;
> >
> > -err_close_fds_prev:
> > -     while (i--) {
> > -             perf_event_unmap(rings[i].mem);
> > -err_close_fds_current:
> > -             close(rings[i].fd);
> > -     }
> > -     free(pfds);
> > -err_free_rings:
> > -     free(rings);
> > +err_close_pb:
> > +     perf_buffer__free(pb);
> >  err_close_map:
> >       close(map_fd);
> > -     return -1;
> > +     return err ? -1 : 0;
>
> ... how can we return 0 on the error path?

Good catch! I initially was going to merge success and error path, but
after seeing how many explicit `err = <something>` I had to do I
abandoned the effort, but forgot to undo this `return -1` change. I'll
return -1 unconditionally.

>
> >  }
>



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux