From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 445BBC10DCE for ; Sat, 14 Mar 2020 01:48:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E8ED32074B for ; Sat, 14 Mar 2020 01:48:36 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="Ti3fPPn0" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726485AbgCNBsg (ORCPT ); Fri, 13 Mar 2020 21:48:36 -0400 Received: from mail-qk1-f193.google.com ([209.85.222.193]:44627 "EHLO mail-qk1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726462AbgCNBsg (ORCPT ); Fri, 13 Mar 2020 21:48:36 -0400 Received: by mail-qk1-f193.google.com with SMTP id f198so16104425qke.11; Fri, 13 Mar 2020 18:48:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=7ksM7MyfVTVqBxb3jtCpPXoEWEAu9MEcT4VPwq79enM=; b=Ti3fPPn0h6sQzOyZzrA08YdcFPGaCIfaT/ZaCfS4CJZCqJa8yF9XiULVDlen4SIwOn m08ptrGlYp5tiwumcemgmoFWYUYuRLUayvvie8SDaJyQnrxYk7CaJ8I4aHXHLlFFsuKb dJQS67TgET2MrRYobasEpqY0pHBbVnu+Ucxj8sOgNFfJeqawQsJ+R/uBrE26Zx8J0jCq 1Ty1TTCBjC4HizwU8S2vRvFdaHoNAiFlpc/HbfospiCZ2a71/YAPTLiaXnTIJQt7vUrU LJP/jSnJ4arIdlpwxSXb2BLNm3PGcMi8/6gOA3CMfVJGnSN0OYmkljTykDlpVZ9BsBXV Wrnw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=7ksM7MyfVTVqBxb3jtCpPXoEWEAu9MEcT4VPwq79enM=; b=DXA5R/WIyUHL7XBGAtfXdJYllnblX7OJMXwuDLRTRy0DhyQn9jtkPvPsk7tb4QDEPD O2M81RuucMyPmfuYrKdFJTN6SHniYG3swmUGPJ0o5XeelReahVUbCkCRjK8Bm+tL4bxk Zz8z0vb9IuwVoUkqYiv7gxo1dxpyuK9jvq8SsJ+TdnjWx3H2VmIYNokXCehoWUE3aTpw 4Ex7nYXNUjHuQJNBGLY5thMFlAuvaTSksx6iHdNV3kk07SlvqpZWEA4U9+tTBXkGn/n1 31P1MG4QzmTcoaBntGTDUV9bVIfUDG1jwJ7335OKGMaPRkD/WPUOolDFp1x1/zwzI8Gb Up0A== X-Gm-Message-State: ANhLgQ0mVdg4XmWsNeXIbXHL9gz1et14yhMCkoq1JZQUVoprZajEfqc5 qTZ9GYLM6i66HfJcAUNYA4Gp7ViOlkG3hGjryWZk2P2O X-Google-Smtp-Source: ADFU+vsjhJ+tyl5nfYL4JUZqjUcKwZNO8hg2S7WWMi3LPcezX1ufkkNOR2zDG7R8eRtd1UhnMNm+aAxHuSZkZBVlGcg= X-Received: by 2002:a37:992:: with SMTP id 140mr16575768qkj.36.1584150514029; Fri, 13 Mar 2020 18:48:34 -0700 (PDT) MIME-Version: 1.0 References: <20200313115220.29073-1-danieltimlee@gmail.com> <20200313115220.29073-3-danieltimlee@gmail.com> In-Reply-To: From: Andrii Nakryiko Date: Fri, 13 Mar 2020 18:48:22 -0700 Message-ID: Subject: Re: [PATCH bpf-next v3 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link To: "Daniel T. Lee" Cc: Daniel Borkmann , Alexei Starovoitov , John Fastabend , Networking , bpf Content-Type: text/plain; charset="UTF-8" Sender: bpf-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On Fri, Mar 13, 2020 at 6:46 PM Daniel T. Lee wrote: > > On Sat, Mar 14, 2020 at 3:48 AM Andrii Nakryiko > wrote: > > > > On Fri, Mar 13, 2020 at 4:52 AM 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. Also, bpf_link__destroy manages the close() of > > > perf_event fd. > > > > > > 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 > > > --- > > > Changes in v2: > > > - check memory allocation is successful > > > - clean up allocated memory on error > > > > > > Changes in v3: > > > - Improve pointer error check (IS_ERR()) > > > - change to calloc for easier destroy of bpf_link > > > - remove perf_event fd list since bpf_link handles fd > > > - use newer bpf_object__{open/load} API instead of bpf_prog_load > > > - perf_event for _SC_NPROCESSORS_ONLN instead of _SC_NPROCESSORS_CONF > > > - find program with name explicitly instead of bpf_program__next > > > - unconditional bpf_link__destroy() on cleanup > > > - return error code on int_exit > > > > > > samples/bpf/Makefile | 4 +- > > > samples/bpf/sampleip_user.c | 100 ++++++++++++++++++++++----------- > > > samples/bpf/trace_event_user.c | 89 ++++++++++++++++++++--------- > > > 3 files changed, 131 insertions(+), 62 deletions(-) > > > > > > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile > > > index ff0061467dd3..424f6fe7ce38 100644 > > > --- a/samples/bpf/Makefile > > > +++ b/samples/bpf/Makefile > > > @@ -88,8 +88,8 @@ xdp2-objs := xdp1_user.o > > > xdp_router_ipv4-objs := xdp_router_ipv4_user.o > > > test_current_task_under_cgroup-objs := bpf_load.o $(CGROUP_HELPERS) \ > > > test_current_task_under_cgroup_user.o > > > -trace_event-objs := bpf_load.o trace_event_user.o $(TRACE_HELPERS) > > > -sampleip-objs := bpf_load.o sampleip_user.o $(TRACE_HELPERS) > > > +trace_event-objs := trace_event_user.o $(TRACE_HELPERS) > > > +sampleip-objs := sampleip_user.o $(TRACE_HELPERS) > > > tc_l2_redirect-objs := bpf_load.o tc_l2_redirect_user.o > > > lwt_len_hist-objs := bpf_load.o lwt_len_hist_user.o > > > xdp_tx_iptunnel-objs := xdp_tx_iptunnel_user.o > > > diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c > > > index b0f115f938bc..05eca7b00e22 100644 > > > --- a/samples/bpf/sampleip_user.c > > > +++ b/samples/bpf/sampleip_user.c > > > @@ -10,21 +10,23 @@ > > > #include > > > #include > > > #include > > > -#include > > > #include > > > #include > > > #include > > > -#include > > > +#include > > > #include > > > -#include "bpf_load.h" > > > #include "perf-sys.h" > > > #include "trace_helpers.h" > > > > > > +#define __must_check > > > +#include > > > + > > > #define DEFAULT_FREQ 99 > > > #define DEFAULT_SECS 5 > > > #define MAX_IPS 8192 > > > #define PAGE_OFFSET 0xffff880000000000 > > > > > > +static int map_fd; > > > static int nr_cpus; > > > > > > static void usage(void) > > > @@ -34,9 +36,10 @@ static void usage(void) > > > printf(" duration # sampling duration (seconds), default 5\n"); > > > } > > > > > > -static int sampling_start(int *pmu_fd, int freq) > > > +static int sampling_start(int freq, struct bpf_program *prog, > > > + struct bpf_link *links[]) > > > { > > > - int i; > > > + int i, pmu_fd; > > > > > > struct perf_event_attr pe_sample_attr = { > > > .type = PERF_TYPE_SOFTWARE, > > > @@ -47,26 +50,29 @@ static int sampling_start(int *pmu_fd, int freq) > > > }; > > > > > > for (i = 0; i < nr_cpus; i++) { > > > - pmu_fd[i] = sys_perf_event_open(&pe_sample_attr, -1 /* pid */, i, > > > + pmu_fd = sys_perf_event_open(&pe_sample_attr, -1 /* pid */, i, > > > -1 /* group_fd */, 0 /* flags */); > > > - if (pmu_fd[i] < 0) { > > > + if (pmu_fd < 0) { > > > fprintf(stderr, "ERROR: Initializing perf sampling\n"); > > > return 1; > > > } > > > - assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_SET_BPF, > > > - prog_fd[0]) == 0); > > > - assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_ENABLE, 0) == 0); > > > + links[i] = bpf_program__attach_perf_event(prog, pmu_fd); > > > + if (IS_ERR(links[i])) { > > > > links[i] = NULL; > > > > Since bpf_program__attach_perf_event will return err code such > as ERR_PTR(-EINVAL), so it is natural to reset the pointer to NULL! > Thank you for the heads up! > > > > + fprintf(stderr, "ERROR: Attach perf event\n"); > > > + close(pmu_fd); > > > + return 1; > > > + } > > > } > > > > > > return 0; > > > } > > > > > > -static void sampling_end(int *pmu_fd) > > > +static void sampling_end(struct bpf_link *links[]) > > > { > > > int i; > > > > > > for (i = 0; i < nr_cpus; i++) > > > - close(pmu_fd[i]); > > > + bpf_link__destroy(links[i]); > > > } > > > > > > struct ipcount { > > > @@ -128,14 +134,17 @@ static void print_ip_map(int fd) > > > static void int_exit(int sig) > > > { > > > printf("\n"); > > > - print_ip_map(map_fd[0]); > > > + print_ip_map(map_fd); > > > exit(0); > > > } > > > > > > int main(int argc, char **argv) > > > { > > > + int opt, freq = DEFAULT_FREQ, secs = DEFAULT_SECS, error = 0; > > > + struct bpf_program *prog; > > > + struct bpf_object *obj; > > > > initialize to NULL here > > > > I'll apply this at next version patch. > > > > + struct bpf_link **links; > > > char filename[256]; > > > - int *pmu_fd, opt, freq = DEFAULT_FREQ, secs = DEFAULT_SECS; > > > > > > /* process arguments */ > > > while ((opt = getopt(argc, argv, "F:h")) != -1) { > > > @@ -163,38 +172,61 @@ 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"); > > > - return 1; > > > + nr_cpus = sysconf(_SC_NPROCESSORS_ONLN); > > > + links = calloc(nr_cpus, sizeof(struct bpf_link *)); > > > + if (!links) { > > > + fprintf(stderr, "ERROR: malloc of links\n"); > > > + error = 1; > > > + goto cleanup; > > > } > > > > > > - /* load BPF program */ > > > snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]); > > > - if (load_bpf_file(filename)) { > > > - 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; > > > + obj = bpf_object__open_file(filename, NULL); > > > + if (IS_ERR(obj)) { > > > > obj = NULL; > > > > Also again, bpf_object__open_file will return err code of pointer, > and the pointer needs to be set to NULL for bpf_object__close. > > > > + fprintf(stderr, "ERROR: opening BPF object file failed\n"); > > > + error = 1; > > > + goto cleanup; > > > + } > > > + > > > + prog = bpf_object__find_program_by_name(obj, "do_sample"); > > > + if (!prog) { > > > + fprintf(stderr, "ERROR: finding a prog in obj file failed\n"); > > > + error = 1; > > > + goto cleanup; > > > } > > > + > > > + /* load BPF program */ > > > + if (bpf_object__load(obj)) { > > > + fprintf(stderr, "ERROR: loading BPF object file failed\n"); > > > + error = 1; > > > + goto cleanup; > > > + } > > > + > > > + map_fd = bpf_object__find_map_fd_by_name(obj, "ip_map"); > > > + if (map_fd < 0) { > > > + fprintf(stderr, "ERROR: finding a map in obj file failed\n"); > > > + error = 1; > > > + goto cleanup; > > > + } > > > + > > > 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) > > > - return 1; > > > + if (sampling_start(freq, prog, links) != 0) { > > > + error = 1; > > > + goto cleanup; > > > + } > > > sleep(secs); > > > - sampling_end(pmu_fd); > > > - free(pmu_fd); > > > > > > +cleanup: > > > + sampling_end(links); > > > > bpf_object__destroy(obj) > > > > Sorry, but I might not following you, but is there bpf_object__destroy > exist in libbpf? I guess you're talking about bpf_object__close, right? Yeah, I meant bpf_object__close, sorry. > > > > /* output sample counts */ > > > - print_ip_map(map_fd[0]); > > > + if (!error) > > > + print_ip_map(map_fd); > > > > > > - return 0; > > > + free(links); > > > + return error; > > > } > > > diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c > > > index 356171bc392b..5f64ff524cc3 100644 > > > --- a/samples/bpf/trace_event_user.c > > > +++ b/samples/bpf/trace_event_user.c > > > @@ -6,22 +6,24 @@ > > > #include > > > #include > > > #include > > > -#include > > > -#include > > > -#include > > > #include > > > #include > > > #include > > > -#include > > > #include > > > #include > > > +#include > > > #include > > > -#include "bpf_load.h" > > > #include "perf-sys.h" > > > #include "trace_helpers.h" > > > > > > +#define __must_check > > > +#include > > > + > > > #define SAMPLE_FREQ 50 > > > > > > +/* counts, stackmap */ > > > +static int map_fd[2]; > > > +struct bpf_program *prog; > > > static bool sys_read_seen, sys_write_seen; > > > > > > static void print_ksym(__u64 addr) > > > @@ -136,23 +138,34 @@ 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); > > > - int *pmu_fd = malloc(nr_cpus * sizeof(int)); > > > - int i, error = 0; > > > + int nr_cpus = sysconf(_SC_NPROCESSORS_ONLN); > > > + struct bpf_link **links = calloc(nr_cpus, sizeof(struct bpf_link *)); > > > + int i, pmu_fd, error = 0; > > > + > > > + if (!links) { > > > + printf("malloc of links failed\n"); > > > + error = 1; > > > + goto err; > > > + } > > > > > > /* system wide perf event, no need to inherit */ > > > attr->inherit = 0; > > > > > > /* open perf_event on all cpus */ > > > for (i = 0; i < nr_cpus; i++) { > > > - pmu_fd[i] = sys_perf_event_open(attr, -1, i, -1, 0); > > > - if (pmu_fd[i] < 0) { > > > + pmu_fd = sys_perf_event_open(attr, -1, i, -1, 0); > > > + if (pmu_fd < 0) { > > > printf("sys_perf_event_open failed\n"); > > > 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); > > > + links[i] = bpf_program__attach_perf_event(prog, pmu_fd); > > > + if (IS_ERR(links[i])) { > > > + printf("bpf_program__attach_perf_event failed\n"); > > > + close(pmu_fd); > > > + error = 1; > > > + goto all_cpu_err; > > > + } > > > } > > > > > > if (generate_load() < 0) { > > > @@ -161,18 +174,18 @@ 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]); > > > - } > > > - free(pmu_fd); > > > + for (i--; i >= 0; i--) > > > + bpf_link__destroy(links[i]); > > > +err: > > > + free(links); > > > if (error) > > > - int_exit(0); > > > + int_exit(error); > > > } > > > > > > static void test_perf_event_task(struct perf_event_attr *attr) > > > { > > > int pmu_fd, error = 0; > > > + struct bpf_link *link; > > > > > > /* per task perf event, enable inherit so the "dd ..." command can be traced properly. > > > * Enabling inherit will cause bpf_perf_prog_read_time helper failure. > > > @@ -185,8 +198,12 @@ static void test_perf_event_task(struct perf_event_attr *attr) > > > printf("sys_perf_event_open failed\n"); > > > int_exit(0); > > > } > > > - assert(ioctl(pmu_fd, PERF_EVENT_IOC_SET_BPF, prog_fd[0]) == 0); > > > - assert(ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE) == 0); > > > + link = bpf_program__attach_perf_event(prog, pmu_fd); > > > + if (IS_ERR(link)) { > > > + printf("bpf_program__attach_perf_event failed\n"); > > > + close(pmu_fd); > > > + int_exit(0); > > > + } > > > > > > if (generate_load() < 0) { > > > error = 1; > > > @@ -194,10 +211,9 @@ static void test_perf_event_task(struct perf_event_attr *attr) > > > } > > > print_stacks(); > > > err: > > > - ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE); > > > - close(pmu_fd); > > > + bpf_link__destroy(link); > > > if (error) > > > - int_exit(0); > > > + int_exit(error); > > > } > > > > > > static void test_bpf_perf_event(void) > > > @@ -282,6 +298,7 @@ static void test_bpf_perf_event(void) > > > int main(int argc, char **argv) > > > { > > > struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY}; > > > + struct bpf_object *obj; > > > char filename[256]; > > > > > > snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]); > > > @@ -295,9 +312,29 @@ int main(int argc, char **argv) > > > return 1; > > > } > > > > > > - if (load_bpf_file(filename)) { > > > - printf("%s", bpf_log_buf); > > > - return 2; > > > + obj = bpf_object__open_file(filename, NULL); > > > + if (IS_ERR(obj)) { > > > + printf("opening BPF object file failed\n"); > > > + return 1; > > > + } > > > + > > > + prog = bpf_object__find_program_by_name(obj, "bpf_prog1"); > > > + if (!prog) { > > > + printf("finding a prog in obj file failed\n"); > > > > > > bpf_object__close(obj); > > > > Also, I'll apply this at next version patch. > > > > + return 1; > > > + } > > > + > > > + /* load BPF program */ > > > + if (bpf_object__load(obj)) { > > > > close bpf_object (better do goto clean approach, of course) > > > > I will modify this with goto cleanup approach. > > > > + printf("loading BPF object file failed\n"); > > > + return 1; > > > + } > > > + > > > + map_fd[0] = bpf_object__find_map_fd_by_name(obj, "counts"); > > > + map_fd[1] = bpf_object__find_map_fd_by_name(obj, "stackmap"); > > > + if (map_fd[0] < 0 || map_fd[1] < 0) { > > > + printf("finding a counts/stackmap map in obj file failed\n"); > > > + return 1; > > > } > > > > > > if (fork() == 0) { > > > -- > > > 2.25.1 > > > > > Thank you for your time and effort for the review :) > > Best, > Daniel