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


On Sat, Jun 29, 2019 at 11:24 AM Jakub Kicinski
[off-list ref] wrote:

On Fri, 28 Jun 2019 22:53:09 -0700, Andrii Nakryiko wrote:

quoted

      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!

quoted

              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.

quoted

              }
-
-             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.

quoted

 }