From: "Daniel T. Lee" <danieltimlee@gmail.com>
To: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>,
Alexei Starovoitov <ast@kernel.org>,
Andrii Nakryiko <andrii.nakryiko@gmail.com>,
netdev <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf-next 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link
Date: Wed, 11 Mar 2020 07:49:28 +0900 [thread overview]
Message-ID: <CAEKGpziMNonMiPa++zZT2G0QDUGCz_BcstPaboCXC9Jr8tqUWw@mail.gmail.com> (raw)
In-Reply-To: <5e6807c39b0b1_586d2b10f16785b8eb@john-XPS-13-9370.notmuch>
On Wed, Mar 11, 2020 at 6:34 AM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Daniel T. Lee wrote:
> > The bpf_program__attach of libbpf(using bpf_link) is much more intuitive
> > than the previous method using ioctl.
> >
> > bpf_program__attach_perf_event manages the enable of perf_event and
> > attach of BPF programs to it, so there's no neeed to do this
> > directly with ioctl.
> >
> > In addition, bpf_link provides consistency in the use of API because it
> > allows disable (detach, destroy) for multiple events to be treated as
> > one bpf_link__destroy.
> >
> > This commit refactors samples that attach the bpf program to perf_event
> > by using libbbpf instead of ioctl. Also the bpf_load in the samples were
> > removed and migrated to use libbbpf API.
> >
> > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > ---
>
> [...]
>
> >
> > int main(int argc, char **argv)
> > {
> > + int prog_fd, *pmu_fd, opt, freq = DEFAULT_FREQ, secs = DEFAULT_SECS;
> > + struct bpf_program *prog;
> > + struct bpf_object *obj;
> > + struct bpf_link **link;
> > char filename[256];
> > - int *pmu_fd, opt, freq = DEFAULT_FREQ, secs = DEFAULT_SECS;
> >
> > /* process arguments */
> > while ((opt = getopt(argc, argv, "F:h")) != -1) {
> > @@ -165,36 +170,47 @@ int main(int argc, char **argv)
> > /* create perf FDs for each CPU */
> > nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
> > pmu_fd = malloc(nr_cpus * sizeof(int));
> > - if (pmu_fd == NULL) {
> > - fprintf(stderr, "ERROR: malloc of pmu_fd\n");
> > + link = malloc(nr_cpus * sizeof(struct bpf_link *));
> > + if (pmu_fd == NULL || link == NULL) {
> > + fprintf(stderr, "ERROR: malloc of pmu_fd/link\n");
> > return 1;
> > }
> >
> > /* load BPF program */
> > snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> > - if (load_bpf_file(filename)) {
> > + if (bpf_prog_load(filename, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd)) {
> > fprintf(stderr, "ERROR: loading BPF program (errno %d):\n",
> > errno);
> > - if (strcmp(bpf_log_buf, "") == 0)
> > - fprintf(stderr, "Try: ulimit -l unlimited\n");
> > - else
> > - fprintf(stderr, "%s", bpf_log_buf);
> > return 1;
> > }
> > +
> > + prog = bpf_program__next(NULL, obj);
> > + if (!prog) {
> > + printf("finding a prog in obj file failed\n");
> > + return 1;
> > + }
> > +
> > + map_fd = bpf_object__find_map_fd_by_name(obj, "ip_map");
> > + if (map_fd < 0) {
> > + printf("finding a ip_map map in obj file failed\n");
> > + return 1;
> > + }
> > +
> > signal(SIGINT, int_exit);
> > signal(SIGTERM, int_exit);
> >
> > /* do sampling */
> > printf("Sampling at %d Hertz for %d seconds. Ctrl-C also ends.\n",
> > freq, secs);
> > - if (sampling_start(pmu_fd, freq) != 0)
> > + if (sampling_start(pmu_fd, freq, prog, link) != 0)
> > return 1;
> > sleep(secs);
> > - sampling_end(pmu_fd);
> > + sampling_end(link);
> > free(pmu_fd);
> > + free(link);
>
> Not really a problem with this patch but on error we don't free() memory but
> then on normal exit there is a free() its a bit inconsistent. How about adding
> free on errors as well?
I think you're right.
I'll add free() on errors to keep it consistent.
Will apply feedback right away!
>
> >
> > /* output sample counts */
> > - print_ip_map(map_fd[0]);
> > + print_ip_map(map_fd);
> >
> > return 0;
> > }
>
> [...]
>
> > static void print_ksym(__u64 addr)
> > @@ -137,6 +136,7 @@ static inline int generate_load(void)
> > static void test_perf_event_all_cpu(struct perf_event_attr *attr)
> > {
> > int nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
> > + struct bpf_link **link = malloc(nr_cpus * sizeof(struct bpf_link *));
>
> need to check if its null? Its not going to be very friendly to segfault
> later. Or maybe I'm missing the check.
>
Also, checking whether it is null will be more safe.
I'll apply and send next version patch.
> > int *pmu_fd = malloc(nr_cpus * sizeof(int));
> > int i, error = 0;
> >
> > @@ -151,8 +151,12 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
> > error = 1;
> > goto all_cpu_err;
> > }
> > - assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_SET_BPF, prog_fd[0]) == 0);
> > - assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_ENABLE) == 0);
> > + link[i] = bpf_program__attach_perf_event(prog, pmu_fd[i]);
> > + if (link[i] < 0) {
> > + printf("bpf_program__attach_perf_event failed\n");
> > + error = 1;
> > + goto all_cpu_err;
> > + }
> > }
> >
> > if (generate_load() < 0) {
> > @@ -161,11 +165,11 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
> > }
> > print_stacks();
> > all_cpu_err:
> > - for (i--; i >= 0; i--) {
> > - ioctl(pmu_fd[i], PERF_EVENT_IOC_DISABLE);
> > - close(pmu_fd[i]);
> > - }
> > + for (i--; i >= 0; i--)
> > + bpf_link__destroy(link[i]);
> > +
> > free(pmu_fd);
> > + free(link);
> > if (error)
> > int_exit(0);
> > }
>
> Thanks,
> John
Thank you for your time and effort for the review.
Best,
Daniel
prev parent reply other threads:[~2020-03-10 22:49 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-10 5:51 [PATCH bpf-next 0/2] samples: bpf: refactor perf_event user program with libbpf bpf_link Daniel T. Lee
2020-03-10 5:51 ` [PATCH bpf-next 1/2] samples: bpf: move read_trace_pipe to trace_helpers Daniel T. Lee
2020-03-10 21:11 ` John Fastabend
2020-03-10 5:51 ` [PATCH bpf-next 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link Daniel T. Lee
2020-03-10 21:33 ` John Fastabend
2020-03-10 22:49 ` Daniel T. Lee [this message]
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=CAEKGpziMNonMiPa++zZT2G0QDUGCz_BcstPaboCXC9Jr8tqUWw@mail.gmail.com \
--to=danieltimlee@gmail.com \
--cc=andrii.nakryiko@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=john.fastabend@gmail.com \
--cc=netdev@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).