* [PATCH bpf-next v2 0/2] Refactor perf_event sample user program with libbpf bpf_link
@ 2020-03-10 23:26 Daniel T. Lee
2020-03-10 23:26 ` [PATCH bpf-next v2 1/2] samples: bpf: move read_trace_pipe to trace_helpers Daniel T. Lee
2020-03-10 23:26 ` [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link Daniel T. Lee
0 siblings, 2 replies; 7+ messages in thread
From: Daniel T. Lee @ 2020-03-10 23:26 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov
Cc: John Fastabend, Andrii Nakryiko, netdev, bpf
Currently, some samples are using ioctl for enabling perf_event and
attaching BPF programs to this event. However, 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.
To refactor samples with using this libbpf API, the bpf_load in the
samples were removed and migrated to libbbpf. Because read_trace_pipe
is used in bpf_load, multiple samples cannot be migrated to libbpf,
this function was moved to trace_helpers.
Changes in v2:
- check memory allocation is successful
- clean up allocated memory on error
Daniel T. Lee (2):
samples: bpf: move read_trace_pipe to trace_helpers
samples: bpf: refactor perf_event user program with libbpf bpf_link
samples/bpf/Makefile | 8 +--
samples/bpf/bpf_load.c | 20 ------
samples/bpf/bpf_load.h | 1 -
samples/bpf/sampleip_user.c | 76 ++++++++++++++-------
samples/bpf/trace_event_user.c | 63 ++++++++++++-----
samples/bpf/tracex1_user.c | 1 +
samples/bpf/tracex5_user.c | 1 +
tools/testing/selftests/bpf/trace_helpers.c | 23 +++++++
tools/testing/selftests/bpf/trace_helpers.h | 1 +
9 files changed, 125 insertions(+), 69 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH bpf-next v2 1/2] samples: bpf: move read_trace_pipe to trace_helpers
2020-03-10 23:26 [PATCH bpf-next v2 0/2] Refactor perf_event sample user program with libbpf bpf_link Daniel T. Lee
@ 2020-03-10 23:26 ` Daniel T. Lee
2020-03-10 23:26 ` [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link Daniel T. Lee
1 sibling, 0 replies; 7+ messages in thread
From: Daniel T. Lee @ 2020-03-10 23:26 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov
Cc: John Fastabend, Andrii Nakryiko, netdev, bpf
To reduce the reliance of trace samples (trace*_user) on bpf_load,
move read_trace_pipe to trace_helpers. By moving this bpf_loader helper
elsewhere, trace functions can be easily migrated to libbbpf.
Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
samples/bpf/Makefile | 4 ++--
samples/bpf/bpf_load.c | 20 ------------------
samples/bpf/bpf_load.h | 1 -
samples/bpf/tracex1_user.c | 1 +
samples/bpf/tracex5_user.c | 1 +
tools/testing/selftests/bpf/trace_helpers.c | 23 +++++++++++++++++++++
tools/testing/selftests/bpf/trace_helpers.h | 1 +
7 files changed, 28 insertions(+), 23 deletions(-)
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 79b0fee6943b..ff0061467dd3 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -64,11 +64,11 @@ fds_example-objs := fds_example.o
sockex1-objs := sockex1_user.o
sockex2-objs := sockex2_user.o
sockex3-objs := bpf_load.o sockex3_user.o
-tracex1-objs := bpf_load.o tracex1_user.o
+tracex1-objs := bpf_load.o tracex1_user.o $(TRACE_HELPERS)
tracex2-objs := bpf_load.o tracex2_user.o
tracex3-objs := bpf_load.o tracex3_user.o
tracex4-objs := bpf_load.o tracex4_user.o
-tracex5-objs := bpf_load.o tracex5_user.o
+tracex5-objs := bpf_load.o tracex5_user.o $(TRACE_HELPERS)
tracex6-objs := bpf_load.o tracex6_user.o
tracex7-objs := bpf_load.o tracex7_user.o
test_probe_write_user-objs := bpf_load.o test_probe_write_user_user.o
diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
index 4574b1939e49..c5ad528f046e 100644
--- a/samples/bpf/bpf_load.c
+++ b/samples/bpf/bpf_load.c
@@ -665,23 +665,3 @@ int load_bpf_file_fixup_map(const char *path, fixup_map_cb fixup_map)
{
return do_load_bpf_file(path, fixup_map);
}
-
-void read_trace_pipe(void)
-{
- int trace_fd;
-
- trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0);
- if (trace_fd < 0)
- return;
-
- while (1) {
- static char buf[4096];
- ssize_t sz;
-
- sz = read(trace_fd, buf, sizeof(buf) - 1);
- if (sz > 0) {
- buf[sz] = 0;
- puts(buf);
- }
- }
-}
diff --git a/samples/bpf/bpf_load.h b/samples/bpf/bpf_load.h
index 814894a12974..4fcd258c616f 100644
--- a/samples/bpf/bpf_load.h
+++ b/samples/bpf/bpf_load.h
@@ -53,6 +53,5 @@ extern int map_data_count;
int load_bpf_file(char *path);
int load_bpf_file_fixup_map(const char *path, fixup_map_cb fixup_map);
-void read_trace_pipe(void);
int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags);
#endif
diff --git a/samples/bpf/tracex1_user.c b/samples/bpf/tracex1_user.c
index af8c20608ab5..55fddbd08702 100644
--- a/samples/bpf/tracex1_user.c
+++ b/samples/bpf/tracex1_user.c
@@ -4,6 +4,7 @@
#include <unistd.h>
#include <bpf/bpf.h>
#include "bpf_load.h"
+#include "trace_helpers.h"
int main(int ac, char **argv)
{
diff --git a/samples/bpf/tracex5_user.c b/samples/bpf/tracex5_user.c
index c4ab91c89494..c2317b39e0d2 100644
--- a/samples/bpf/tracex5_user.c
+++ b/samples/bpf/tracex5_user.c
@@ -8,6 +8,7 @@
#include <bpf/bpf.h>
#include "bpf_load.h"
#include <sys/resource.h>
+#include "trace_helpers.h"
/* install fake seccomp program to enable seccomp code path inside the kernel,
* so that our kprobe attached to seccomp_phase1() can be triggered
diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
index 7f989b3e4e22..4d0e913bbb22 100644
--- a/tools/testing/selftests/bpf/trace_helpers.c
+++ b/tools/testing/selftests/bpf/trace_helpers.c
@@ -4,12 +4,15 @@
#include <string.h>
#include <assert.h>
#include <errno.h>
+#include <fcntl.h>
#include <poll.h>
#include <unistd.h>
#include <linux/perf_event.h>
#include <sys/mman.h>
#include "trace_helpers.h"
+#define DEBUGFS "/sys/kernel/debug/tracing/"
+
#define MAX_SYMS 300000
static struct ksym syms[MAX_SYMS];
static int sym_cnt;
@@ -86,3 +89,23 @@ long ksym_get_addr(const char *name)
return 0;
}
+
+void read_trace_pipe(void)
+{
+ int trace_fd;
+
+ trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0);
+ if (trace_fd < 0)
+ return;
+
+ while (1) {
+ static char buf[4096];
+ ssize_t sz;
+
+ sz = read(trace_fd, buf, sizeof(buf) - 1);
+ if (sz > 0) {
+ buf[sz] = 0;
+ puts(buf);
+ }
+ }
+}
diff --git a/tools/testing/selftests/bpf/trace_helpers.h b/tools/testing/selftests/bpf/trace_helpers.h
index 0383c9b8adc1..25ef597dd03f 100644
--- a/tools/testing/selftests/bpf/trace_helpers.h
+++ b/tools/testing/selftests/bpf/trace_helpers.h
@@ -12,5 +12,6 @@ struct ksym {
int load_kallsyms(void);
struct ksym *ksym_search(long key);
long ksym_get_addr(const char *name);
+void read_trace_pipe(void);
#endif
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link
2020-03-10 23:26 [PATCH bpf-next v2 0/2] Refactor perf_event sample user program with libbpf bpf_link Daniel T. Lee
2020-03-10 23:26 ` [PATCH bpf-next v2 1/2] samples: bpf: move read_trace_pipe to trace_helpers Daniel T. Lee
@ 2020-03-10 23:26 ` Daniel T. Lee
2020-03-11 4:55 ` Andrii Nakryiko
1 sibling, 1 reply; 7+ messages in thread
From: Daniel T. Lee @ 2020-03-10 23:26 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov
Cc: John Fastabend, Andrii Nakryiko, netdev, bpf
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>
---
Changes in v2:
- check memory allocation is successful
- clean up allocated memory on error
samples/bpf/Makefile | 4 +-
samples/bpf/sampleip_user.c | 76 ++++++++++++++++++++++------------
samples/bpf/trace_event_user.c | 63 ++++++++++++++++++++--------
3 files changed, 97 insertions(+), 46 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..fd763a49c913 100644
--- a/samples/bpf/sampleip_user.c
+++ b/samples/bpf/sampleip_user.c
@@ -10,13 +10,11 @@
#include <errno.h>
#include <signal.h>
#include <string.h>
-#include <assert.h>
#include <linux/perf_event.h>
#include <linux/ptrace.h>
#include <linux/bpf.h>
-#include <sys/ioctl.h>
+#include <bpf/bpf.h>
#include <bpf/libbpf.h>
-#include "bpf_load.h"
#include "perf-sys.h"
#include "trace_helpers.h"
@@ -25,6 +23,7 @@
#define MAX_IPS 8192
#define PAGE_OFFSET 0xffff880000000000
+static int map_fd;
static int nr_cpus;
static void usage(void)
@@ -34,7 +33,8 @@ 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 *pmu_fd, int freq, struct bpf_program *prog,
+ struct bpf_link **link)
{
int i;
@@ -53,20 +53,22 @@ static int sampling_start(int *pmu_fd, int freq)
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);
+ link[i] = bpf_program__attach_perf_event(prog, pmu_fd[i]);
+ if (link[i] < 0) {
+ fprintf(stderr, "ERROR: Attach perf event\n");
+ return 1;
+ }
}
return 0;
}
-static void sampling_end(int *pmu_fd)
+static void sampling_end(struct bpf_link **link)
{
int i;
for (i = 0; i < nr_cpus; i++)
- close(pmu_fd[i]);
+ bpf_link__destroy(link[i]);
}
struct ipcount {
@@ -128,14 +130,18 @@ 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 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;
+ int error = 0;
/* process arguments */
while ((opt = getopt(argc, argv, "F:h")) != -1) {
@@ -165,36 +171,54 @@ 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;
+ link = malloc(nr_cpus * sizeof(struct bpf_link *));
+ if (pmu_fd == NULL || link == NULL) {
+ fprintf(stderr, "ERROR: malloc of pmu_fd/link\n");
+ error = 1;
+ goto cleanup;
}
/* 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;
+ error = 1;
+ goto cleanup;
+ }
+
+ prog = bpf_program__next(NULL, obj);
+ if (!prog) {
+ fprintf(stderr, "ERROR: finding a prog in obj 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(pmu_fd, freq, prog, link) != 0) {
+ error = 1;
+ goto cleanup;
+ }
sleep(secs);
- sampling_end(pmu_fd);
- free(pmu_fd);
+ sampling_end(link);
/* output sample counts */
- print_ip_map(map_fd[0]);
+ print_ip_map(map_fd);
- return 0;
+cleanup:
+ free(pmu_fd);
+ free(link);
+ return error;
}
diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c
index 356171bc392b..30c25ef99fc5 100644
--- a/samples/bpf/trace_event_user.c
+++ b/samples/bpf/trace_event_user.c
@@ -6,22 +6,21 @@
#include <stdlib.h>
#include <stdbool.h>
#include <string.h>
-#include <fcntl.h>
-#include <poll.h>
-#include <sys/ioctl.h>
#include <linux/perf_event.h>
#include <linux/bpf.h>
#include <signal.h>
-#include <assert.h>
#include <errno.h>
#include <sys/resource.h>
+#include <bpf/bpf.h>
#include <bpf/libbpf.h>
-#include "bpf_load.h"
#include "perf-sys.h"
#include "trace_helpers.h"
#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)
@@ -137,9 +136,16 @@ 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 *));
int *pmu_fd = malloc(nr_cpus * sizeof(int));
int i, error = 0;
+ if (pmu_fd == NULL || link == NULL) {
+ printf("malloc of pmu_fd/link failed\n");
+ error = 1;
+ goto err;
+ }
+
/* system wide perf event, no need to inherit */
attr->inherit = 0;
@@ -151,8 +157,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 +171,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]);
+err:
free(pmu_fd);
+ free(link);
if (error)
int_exit(0);
}
@@ -173,6 +183,7 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
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 +196,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 (link < 0) {
+ printf("bpf_program__attach_perf_event failed\n");
+ close(pmu_fd);
+ int_exit(0);
+ }
if (generate_load() < 0) {
error = 1;
@@ -194,8 +209,7 @@ 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);
}
@@ -282,7 +296,9 @@ 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];
+ int prog_fd;
snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
setrlimit(RLIMIT_MEMLOCK, &r);
@@ -295,9 +311,20 @@ int main(int argc, char **argv)
return 1;
}
- if (load_bpf_file(filename)) {
- printf("%s", bpf_log_buf);
- return 2;
+ if (bpf_prog_load(filename, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd))
+ return 1;
+
+ prog = bpf_program__next(NULL, obj);
+ if (!prog) {
+ printf("finding a prog in obj 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link
2020-03-10 23:26 ` [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link Daniel T. Lee
@ 2020-03-11 4:55 ` Andrii Nakryiko
2020-03-12 6:15 ` Daniel T. Lee
0 siblings, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2020-03-11 4:55 UTC (permalink / raw)
To: Daniel T. Lee
Cc: Daniel Borkmann, Alexei Starovoitov, John Fastabend, Networking, bpf
On Tue, Mar 10, 2020 at 4:27 PM Daniel T. Lee <danieltimlee@gmail.com> 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>
> ---
Daniel, thanks for this clean up! It's good to see samples be
modernized a bit :)
> Changes in v2:
> - check memory allocation is successful
> - clean up allocated memory on error
>
> samples/bpf/Makefile | 4 +-
> samples/bpf/sampleip_user.c | 76 ++++++++++++++++++++++------------
> samples/bpf/trace_event_user.c | 63 ++++++++++++++++++++--------
> 3 files changed, 97 insertions(+), 46 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..fd763a49c913 100644
> --- a/samples/bpf/sampleip_user.c
> +++ b/samples/bpf/sampleip_user.c
> @@ -10,13 +10,11 @@
> #include <errno.h>
> #include <signal.h>
> #include <string.h>
> -#include <assert.h>
> #include <linux/perf_event.h>
> #include <linux/ptrace.h>
> #include <linux/bpf.h>
> -#include <sys/ioctl.h>
> +#include <bpf/bpf.h>
> #include <bpf/libbpf.h>
> -#include "bpf_load.h"
> #include "perf-sys.h"
> #include "trace_helpers.h"
>
> @@ -25,6 +23,7 @@
> #define MAX_IPS 8192
> #define PAGE_OFFSET 0xffff880000000000
>
> +static int map_fd;
> static int nr_cpus;
>
> static void usage(void)
> @@ -34,7 +33,8 @@ 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 *pmu_fd, int freq, struct bpf_program *prog,
> + struct bpf_link **link)
It's not apparent from looking at struct bpf_link **link whether it's
an output parameter (so sampling_start is supposed to assign *single*
link to return it to calling function) or it's an array of pointers.
Seems like it's the latter, so I'd prefer this written as
struct bpf_link *links[] (notice also plural name).
Please consider this.
> {
> int i;
>
> @@ -53,20 +53,22 @@ static int sampling_start(int *pmu_fd, int freq)
> 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);
> + link[i] = bpf_program__attach_perf_event(prog, pmu_fd[i]);
> + if (link[i] < 0) {
link is a pointer, < 0 doesn't make sense and is always going to be
false on x86. Use IS_ERR(link[i]). It's also a good idea to set it to
NULL, if link creation failed to prevent accidental
bpf_link__destroy(link[i]) later on, trying to free bogus pointer.
> + fprintf(stderr, "ERROR: Attach perf event\n");
> + return 1;
> + }
> }
>
> return 0;
> }
>
> -static void sampling_end(int *pmu_fd)
> +static void sampling_end(struct bpf_link **link)
same as above, struct bpf_link *links[] would be much better here, IMO.
> {
> int i;
>
> for (i = 0; i < nr_cpus; i++)
> - close(pmu_fd[i]);
> + bpf_link__destroy(link[i]);
> }
>
> struct ipcount {
> @@ -128,14 +130,18 @@ 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 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;
> + int error = 0;
>
> /* process arguments */
> while ((opt = getopt(argc, argv, "F:h")) != -1) {
> @@ -165,36 +171,54 @@ int main(int argc, char **argv)
> /* create perf FDs for each CPU */
> nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
While neither approach is ideal, using number of online CPUs
(_SC_NPROCESSORS_ONLN) will probably work in slightly more cases
(there are machines configured with, say, 256 possible CPUs, but only
32 available, for instance).
> pmu_fd = malloc(nr_cpus * sizeof(int));
similar naming nit: pmu_fds?
> - if (pmu_fd == NULL) {
> - fprintf(stderr, "ERROR: malloc of pmu_fd\n");
> - return 1;
> + link = malloc(nr_cpus * sizeof(struct bpf_link *));
Use calloc() to have those links initialized to NULL automatically.
Makes clean up so much easier.
> + if (pmu_fd == NULL || link == NULL) {
> + fprintf(stderr, "ERROR: malloc of pmu_fd/link\n");
> + error = 1;
> + goto cleanup;
> }
>
> /* 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)) {
Using skeleton would be best, but it's probably more appropriate for
another patch to integrate skeleton generation with samples/bpf. So
the next one would be bpf_object__open_file(), instead of legacy
bpf_prog_load().
> 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;
> + error = 1;
> + goto cleanup;
> + }
> +
> + prog = bpf_program__next(NULL, obj);
I'm a bit lazy here, sorry, but isn't the name of the program known?
bpf_object__find_program_by_title() is preferable.
> + if (!prog) {
> + fprintf(stderr, "ERROR: finding a prog in obj 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(pmu_fd, freq, prog, link) != 0) {
> + error = 1;
> + goto cleanup;
> + }
> sleep(secs);
> - sampling_end(pmu_fd);
> - free(pmu_fd);
> + sampling_end(link);
>
> /* output sample counts */
> - print_ip_map(map_fd[0]);
> + print_ip_map(map_fd);
>
> - return 0;
> +cleanup:
> + free(pmu_fd);
> + free(link);
Uhm... you are freeing this only on clean up. Also, you need to
bpf_link__destroy() first. And close all pmu_fds. Surely process exit
will ensure all this is cleaned up, but it's a good tone to clean up
all resources explicitly.
> + return error;
> }
> diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c
> index 356171bc392b..30c25ef99fc5 100644
> --- a/samples/bpf/trace_event_user.c
> +++ b/samples/bpf/trace_event_user.c
> @@ -6,22 +6,21 @@
> #include <stdlib.h>
> #include <stdbool.h>
> #include <string.h>
> -#include <fcntl.h>
> -#include <poll.h>
> -#include <sys/ioctl.h>
> #include <linux/perf_event.h>
> #include <linux/bpf.h>
> #include <signal.h>
> -#include <assert.h>
> #include <errno.h>
> #include <sys/resource.h>
> +#include <bpf/bpf.h>
> #include <bpf/libbpf.h>
> -#include "bpf_load.h"
> #include "perf-sys.h"
> #include "trace_helpers.h"
>
> #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)
> @@ -137,9 +136,16 @@ 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 *));
same as above, calloc() is better choice here
> int *pmu_fd = malloc(nr_cpus * sizeof(int));
> int i, error = 0;
>
> + if (pmu_fd == NULL || link == NULL) {
> + printf("malloc of pmu_fd/link failed\n");
> + error = 1;
> + goto err;
> + }
> +
> /* system wide perf event, no need to inherit */
> attr->inherit = 0;
>
> @@ -151,8 +157,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 +171,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]);
still need close(pmu_fd[i]);
> +err:
> free(pmu_fd);
> + free(link);
> if (error)
> int_exit(0);
> }
> @@ -173,6 +183,7 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
> 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 +196,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 (link < 0) {
> + printf("bpf_program__attach_perf_event failed\n");
> + close(pmu_fd);
> + int_exit(0);
> + }
>
> if (generate_load() < 0) {
> error = 1;
> @@ -194,8 +209,7 @@ 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);
This will exit with 0 error code and won't notify about error... Pass
through err?
> }
> @@ -282,7 +296,9 @@ 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];
> + int prog_fd;
>
> snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> setrlimit(RLIMIT_MEMLOCK, &r);
> @@ -295,9 +311,20 @@ int main(int argc, char **argv)
> return 1;
> }
>
> - if (load_bpf_file(filename)) {
> - printf("%s", bpf_log_buf);
> - return 2;
> + if (bpf_prog_load(filename, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd))
> + return 1;
> +
> + prog = bpf_program__next(NULL, obj);
> + if (!prog) {
> + printf("finding a prog in obj 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link
2020-03-11 4:55 ` Andrii Nakryiko
@ 2020-03-12 6:15 ` Daniel T. Lee
2020-03-12 6:27 ` Andrii Nakryiko
0 siblings, 1 reply; 7+ messages in thread
From: Daniel T. Lee @ 2020-03-12 6:15 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Daniel Borkmann, Alexei Starovoitov, John Fastabend, Networking, bpf
On Wed, Mar 11, 2020 at 1:55 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Mar 10, 2020 at 4:27 PM Daniel T. Lee <danieltimlee@gmail.com> 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>
> > ---
>
> Daniel, thanks for this clean up! It's good to see samples be
> modernized a bit :)
>
Thank you for your time and effort for the review :)
> > Changes in v2:
> > - check memory allocation is successful
> > - clean up allocated memory on error
> >
> > samples/bpf/Makefile | 4 +-
> > samples/bpf/sampleip_user.c | 76 ++++++++++++++++++++++------------
> > samples/bpf/trace_event_user.c | 63 ++++++++++++++++++++--------
> > 3 files changed, 97 insertions(+), 46 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..fd763a49c913 100644
> > --- a/samples/bpf/sampleip_user.c
> > +++ b/samples/bpf/sampleip_user.c
> > @@ -10,13 +10,11 @@
> > #include <errno.h>
> > #include <signal.h>
> > #include <string.h>
> > -#include <assert.h>
> > #include <linux/perf_event.h>
> > #include <linux/ptrace.h>
> > #include <linux/bpf.h>
> > -#include <sys/ioctl.h>
> > +#include <bpf/bpf.h>
> > #include <bpf/libbpf.h>
> > -#include "bpf_load.h"
> > #include "perf-sys.h"
> > #include "trace_helpers.h"
> >
> > @@ -25,6 +23,7 @@
> > #define MAX_IPS 8192
> > #define PAGE_OFFSET 0xffff880000000000
> >
> > +static int map_fd;
> > static int nr_cpus;
> >
> > static void usage(void)
> > @@ -34,7 +33,8 @@ 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 *pmu_fd, int freq, struct bpf_program *prog,
> > + struct bpf_link **link)
>
> It's not apparent from looking at struct bpf_link **link whether it's
> an output parameter (so sampling_start is supposed to assign *single*
> link to return it to calling function) or it's an array of pointers.
> Seems like it's the latter, so I'd prefer this written as
>
> struct bpf_link *links[] (notice also plural name).
>
> Please consider this.
>
This approach looks more apparent!
I'll update code using this way.
> > {
> > int i;
> >
> > @@ -53,20 +53,22 @@ static int sampling_start(int *pmu_fd, int freq)
> > 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);
> > + link[i] = bpf_program__attach_perf_event(prog, pmu_fd[i]);
> > + if (link[i] < 0) {
>
> link is a pointer, < 0 doesn't make sense and is always going to be
> false on x86. Use IS_ERR(link[i]). It's also a good idea to set it to
> NULL, if link creation failed to prevent accidental
> bpf_link__destroy(link[i]) later on, trying to free bogus pointer.
>
Failure on link creation is exactly what I was concerned about.
Thank you for giving me a clear solution!
> > + fprintf(stderr, "ERROR: Attach perf event\n");
> > + return 1;
> > + }
> > }
> >
> > return 0;
> > }
> >
> > -static void sampling_end(int *pmu_fd)
> > +static void sampling_end(struct bpf_link **link)
>
> same as above, struct bpf_link *links[] would be much better here, IMO.
>
Also, I'll apply this at next version patch.
> > {
> > int i;
> >
> > for (i = 0; i < nr_cpus; i++)
> > - close(pmu_fd[i]);
> > + bpf_link__destroy(link[i]);
> > }
> >
> > struct ipcount {
> > @@ -128,14 +130,18 @@ 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 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;
> > + int error = 0;
> >
> > /* process arguments */
> > while ((opt = getopt(argc, argv, "F:h")) != -1) {
> > @@ -165,36 +171,54 @@ int main(int argc, char **argv)
> > /* create perf FDs for each CPU */
> > nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
>
> While neither approach is ideal, using number of online CPUs
> (_SC_NPROCESSORS_ONLN) will probably work in slightly more cases
> (there are machines configured with, say, 256 possible CPUs, but only
> 32 available, for instance).
>
Thank you for pointing me out!
I've never thought about situation when processors may be offline.
>
> > pmu_fd = malloc(nr_cpus * sizeof(int));
>
> similar naming nit: pmu_fds?
>
Same again, apply this at next version patch.
> > - if (pmu_fd == NULL) {
> > - fprintf(stderr, "ERROR: malloc of pmu_fd\n");
> > - return 1;
> > + link = malloc(nr_cpus * sizeof(struct bpf_link *));
>
> Use calloc() to have those links initialized to NULL automatically.
> Makes clean up so much easier.
>
About NULL set, like you mentioned, calloc approach looks more neat.
> > + if (pmu_fd == NULL || link == NULL) {
> > + fprintf(stderr, "ERROR: malloc of pmu_fd/link\n");
> > + error = 1;
> > + goto cleanup;
> > }
> >
> > /* 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)) {
>
> Using skeleton would be best, but it's probably more appropriate for
> another patch to integrate skeleton generation with samples/bpf. So
> the next one would be bpf_object__open_file(), instead of legacy
> bpf_prog_load().
>
I'll try skeleton with other sample cleanup. For now, I'll stick with
bpf_object__{open/load}() instead of bpf_prog_load().
> > 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;
> > + error = 1;
> > + goto cleanup;
> > + }
> > +
> > + prog = bpf_program__next(NULL, obj);
>
> I'm a bit lazy here, sorry, but isn't the name of the program known?
> bpf_object__find_program_by_title() is preferable.
>
I also think it is good to specify the program title clearly.
> > + if (!prog) {
> > + fprintf(stderr, "ERROR: finding a prog in obj 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(pmu_fd, freq, prog, link) != 0) {
> > + error = 1;
> > + goto cleanup;
> > + }
> > sleep(secs);
> > - sampling_end(pmu_fd);
> > - free(pmu_fd);
> > + sampling_end(link);
> >
> > /* output sample counts */
> > - print_ip_map(map_fd[0]);
> > + print_ip_map(map_fd);
> >
> > - return 0;
> > +cleanup:
> > + free(pmu_fd);
> > + free(link);
>
>
> Uhm... you are freeing this only on clean up. Also, you need to
> bpf_link__destroy() first. And close all pmu_fds. Surely process exit
> will ensure all this is cleaned up, but it's a good tone to clean up
> all resources explicitly.
>
Well, cleanup: could cover link destroy (sampling_end), but I think
it is strange to clean up the link even though the bpf program is not
attached to the event. I think it is better to specify the link destroy
after the sampling starts.
And, I've missed the link destroy when sampling got error.
Since sampling_end will destroy the links, so I'll add this on error.
if (sampling_start(pmu_fd, freq, prog, link) != 0) {
error = 1;
+ sampling_end(links);
goto cleanup;
}
> > + return error;
> > }
> > diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c
> > index 356171bc392b..30c25ef99fc5 100644
> > --- a/samples/bpf/trace_event_user.c
> > +++ b/samples/bpf/trace_event_user.c
> > @@ -6,22 +6,21 @@
> > #include <stdlib.h>
> > #include <stdbool.h>
> > #include <string.h>
> > -#include <fcntl.h>
> > -#include <poll.h>
> > -#include <sys/ioctl.h>
> > #include <linux/perf_event.h>
> > #include <linux/bpf.h>
> > #include <signal.h>
> > -#include <assert.h>
> > #include <errno.h>
> > #include <sys/resource.h>
> > +#include <bpf/bpf.h>
> > #include <bpf/libbpf.h>
> > -#include "bpf_load.h"
> > #include "perf-sys.h"
> > #include "trace_helpers.h"
> >
> > #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)
> > @@ -137,9 +136,16 @@ 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 *));
>
> same as above, calloc() is better choice here
>
Will apply this at next version patch.
> > int *pmu_fd = malloc(nr_cpus * sizeof(int));
> > int i, error = 0;
> >
> > + if (pmu_fd == NULL || link == NULL) {
> > + printf("malloc of pmu_fd/link failed\n");
> > + error = 1;
> > + goto err;
> > + }
> > +
> > /* system wide perf event, no need to inherit */
> > attr->inherit = 0;
> >
> > @@ -151,8 +157,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 +171,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]);
>
> still need close(pmu_fd[i]);
>
AFAIK, bpf_link__detach_perf_event() closes the pmu_fd.
Am I missed something?
static int bpf_link__detach_perf_event(struct bpf_link *link)
// TRUNCATED
close(link->fd);
return err;
}
> > +err:
> > free(pmu_fd);
> > + free(link);
> > if (error)
> > int_exit(0);
>
>
> > }
> > @@ -173,6 +183,7 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
> > 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 +196,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 (link < 0) {
> > + printf("bpf_program__attach_perf_event failed\n");
> > + close(pmu_fd);
> > + int_exit(0);
> > + }
> >
> > if (generate_load() < 0) {
> > error = 1;
> > @@ -194,8 +209,7 @@ 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);
>
> This will exit with 0 error code and won't notify about error... Pass
> through err?
>
You're right. Missed the return code.
Will apply this at next version patch.
> > }
> > @@ -282,7 +296,9 @@ 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];
> > + int prog_fd;
> >
> > snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> > setrlimit(RLIMIT_MEMLOCK, &r);
> > @@ -295,9 +311,20 @@ int main(int argc, char **argv)
> > return 1;
> > }
> >
> > - if (load_bpf_file(filename)) {
> > - printf("%s", bpf_log_buf);
> > - return 2;
> > + if (bpf_prog_load(filename, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd))
> > + return 1;
> > +
> > + prog = bpf_program__next(NULL, obj);
> > + if (!prog) {
> > + printf("finding a prog in obj 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 detailed review!
Best,
Daniel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link
2020-03-12 6:15 ` Daniel T. Lee
@ 2020-03-12 6:27 ` Andrii Nakryiko
2020-03-12 6:35 ` Daniel T. Lee
0 siblings, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2020-03-12 6:27 UTC (permalink / raw)
To: Daniel T. Lee
Cc: Daniel Borkmann, Alexei Starovoitov, John Fastabend, Networking, bpf
On Wed, Mar 11, 2020 at 11:16 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> On Wed, Mar 11, 2020 at 1:55 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Mar 10, 2020 at 4:27 PM Daniel T. Lee <danieltimlee@gmail.com> 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>
> > > ---
> >
> > Daniel, thanks for this clean up! It's good to see samples be
> > modernized a bit :)
> >
>
> Thank you for your time and effort for the review :)
>
> > > Changes in v2:
> > > - check memory allocation is successful
> > > - clean up allocated memory on error
> > >
> > > samples/bpf/Makefile | 4 +-
> > > samples/bpf/sampleip_user.c | 76 ++++++++++++++++++++++------------
> > > samples/bpf/trace_event_user.c | 63 ++++++++++++++++++++--------
> > > 3 files changed, 97 insertions(+), 46 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..fd763a49c913 100644
> > > --- a/samples/bpf/sampleip_user.c
> > > +++ b/samples/bpf/sampleip_user.c
> > > @@ -10,13 +10,11 @@
> > > #include <errno.h>
> > > #include <signal.h>
> > > #include <string.h>
> > > -#include <assert.h>
> > > #include <linux/perf_event.h>
> > > #include <linux/ptrace.h>
> > > #include <linux/bpf.h>
> > > -#include <sys/ioctl.h>
> > > +#include <bpf/bpf.h>
> > > #include <bpf/libbpf.h>
> > > -#include "bpf_load.h"
> > > #include "perf-sys.h"
> > > #include "trace_helpers.h"
> > >
> > > @@ -25,6 +23,7 @@
> > > #define MAX_IPS 8192
> > > #define PAGE_OFFSET 0xffff880000000000
> > >
> > > +static int map_fd;
> > > static int nr_cpus;
> > >
> > > static void usage(void)
> > > @@ -34,7 +33,8 @@ 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 *pmu_fd, int freq, struct bpf_program *prog,
> > > + struct bpf_link **link)
> >
> > It's not apparent from looking at struct bpf_link **link whether it's
> > an output parameter (so sampling_start is supposed to assign *single*
> > link to return it to calling function) or it's an array of pointers.
> > Seems like it's the latter, so I'd prefer this written as
> >
> > struct bpf_link *links[] (notice also plural name).
> >
> > Please consider this.
> >
>
> This approach looks more apparent!
> I'll update code using this way.
>
> > > {
> > > int i;
> > >
> > > @@ -53,20 +53,22 @@ static int sampling_start(int *pmu_fd, int freq)
> > > 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);
> > > + link[i] = bpf_program__attach_perf_event(prog, pmu_fd[i]);
> > > + if (link[i] < 0) {
> >
> > link is a pointer, < 0 doesn't make sense and is always going to be
> > false on x86. Use IS_ERR(link[i]). It's also a good idea to set it to
> > NULL, if link creation failed to prevent accidental
> > bpf_link__destroy(link[i]) later on, trying to free bogus pointer.
> >
>
> Failure on link creation is exactly what I was concerned about.
> Thank you for giving me a clear solution!
>
> > > + fprintf(stderr, "ERROR: Attach perf event\n");
> > > + return 1;
> > > + }
> > > }
> > >
> > > return 0;
> > > }
> > >
> > > -static void sampling_end(int *pmu_fd)
> > > +static void sampling_end(struct bpf_link **link)
> >
> > same as above, struct bpf_link *links[] would be much better here, IMO.
> >
>
> Also, I'll apply this at next version patch.
>
> > > {
> > > int i;
> > >
> > > for (i = 0; i < nr_cpus; i++)
> > > - close(pmu_fd[i]);
> > > + bpf_link__destroy(link[i]);
> > > }
> > >
> > > struct ipcount {
> > > @@ -128,14 +130,18 @@ 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 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;
> > > + int error = 0;
> > >
> > > /* process arguments */
> > > while ((opt = getopt(argc, argv, "F:h")) != -1) {
> > > @@ -165,36 +171,54 @@ int main(int argc, char **argv)
> > > /* create perf FDs for each CPU */
> > > nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
> >
> > While neither approach is ideal, using number of online CPUs
> > (_SC_NPROCESSORS_ONLN) will probably work in slightly more cases
> > (there are machines configured with, say, 256 possible CPUs, but only
> > 32 available, for instance).
> >
>
> Thank you for pointing me out!
> I've never thought about situation when processors may be offline.
>
> >
> > > pmu_fd = malloc(nr_cpus * sizeof(int));
> >
> > similar naming nit: pmu_fds?
> >
>
> Same again, apply this at next version patch.
>
> > > - if (pmu_fd == NULL) {
> > > - fprintf(stderr, "ERROR: malloc of pmu_fd\n");
> > > - return 1;
> > > + link = malloc(nr_cpus * sizeof(struct bpf_link *));
> >
> > Use calloc() to have those links initialized to NULL automatically.
> > Makes clean up so much easier.
> >
>
> About NULL set, like you mentioned, calloc approach looks more neat.
>
> > > + if (pmu_fd == NULL || link == NULL) {
> > > + fprintf(stderr, "ERROR: malloc of pmu_fd/link\n");
> > > + error = 1;
> > > + goto cleanup;
> > > }
> > >
> > > /* 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)) {
> >
> > Using skeleton would be best, but it's probably more appropriate for
> > another patch to integrate skeleton generation with samples/bpf. So
> > the next one would be bpf_object__open_file(), instead of legacy
> > bpf_prog_load().
> >
>
> I'll try skeleton with other sample cleanup. For now, I'll stick with
> bpf_object__{open/load}() instead of bpf_prog_load().
>
> > > 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;
> > > + error = 1;
> > > + goto cleanup;
> > > + }
> > > +
> > > + prog = bpf_program__next(NULL, obj);
> >
> > I'm a bit lazy here, sorry, but isn't the name of the program known?
> > bpf_object__find_program_by_title() is preferable.
> >
>
> I also think it is good to specify the program title clearly.
>
> > > + if (!prog) {
> > > + fprintf(stderr, "ERROR: finding a prog in obj 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(pmu_fd, freq, prog, link) != 0) {
> > > + error = 1;
> > > + goto cleanup;
> > > + }
> > > sleep(secs);
> > > - sampling_end(pmu_fd);
> > > - free(pmu_fd);
> > > + sampling_end(link);
> > >
> > > /* output sample counts */
> > > - print_ip_map(map_fd[0]);
> > > + print_ip_map(map_fd);
> > >
> > > - return 0;
> > > +cleanup:
> > > + free(pmu_fd);
> > > + free(link);
> >
> >
> > Uhm... you are freeing this only on clean up. Also, you need to
> > bpf_link__destroy() first. And close all pmu_fds. Surely process exit
> > will ensure all this is cleaned up, but it's a good tone to clean up
> > all resources explicitly.
> >
>
> Well, cleanup: could cover link destroy (sampling_end), but I think
> it is strange to clean up the link even though the bpf program is not
> attached to the event. I think it is better to specify the link destroy
> after the sampling starts.
bpf_link__destroy() is designed in such a way that if passed NULL it
will do nothing. So doing unconditional clean up at the end is clean
and straightforward solution. You'll see it in a bunch of places in
selftests.
>
> And, I've missed the link destroy when sampling got error.
> Since sampling_end will destroy the links, so I'll add this on error.
>
> if (sampling_start(pmu_fd, freq, prog, link) != 0) {
> error = 1;
> + sampling_end(links);
> goto cleanup;
> }
>
> > > + return error;
> > > }
> > > diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c
> > > index 356171bc392b..30c25ef99fc5 100644
> > > --- a/samples/bpf/trace_event_user.c
> > > +++ b/samples/bpf/trace_event_user.c
> > > @@ -6,22 +6,21 @@
> > > #include <stdlib.h>
> > > #include <stdbool.h>
> > > #include <string.h>
> > > -#include <fcntl.h>
> > > -#include <poll.h>
> > > -#include <sys/ioctl.h>
> > > #include <linux/perf_event.h>
> > > #include <linux/bpf.h>
> > > #include <signal.h>
> > > -#include <assert.h>
> > > #include <errno.h>
> > > #include <sys/resource.h>
> > > +#include <bpf/bpf.h>
> > > #include <bpf/libbpf.h>
> > > -#include "bpf_load.h"
> > > #include "perf-sys.h"
> > > #include "trace_helpers.h"
> > >
> > > #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)
> > > @@ -137,9 +136,16 @@ 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 *));
> >
> > same as above, calloc() is better choice here
> >
>
> Will apply this at next version patch.
>
> > > int *pmu_fd = malloc(nr_cpus * sizeof(int));
> > > int i, error = 0;
> > >
> > > + if (pmu_fd == NULL || link == NULL) {
> > > + printf("malloc of pmu_fd/link failed\n");
> > > + error = 1;
> > > + goto err;
> > > + }
> > > +
> > > /* system wide perf event, no need to inherit */
> > > attr->inherit = 0;
> > >
> > > @@ -151,8 +157,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 +171,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]);
> >
> > still need close(pmu_fd[i]);
> >
>
> AFAIK, bpf_link__detach_perf_event() closes the pmu_fd.
> Am I missed something?
Ah, you are right, I missed that fact. But then you don't need pmu_fd
array at all. Do perf_event_open and then immediately
bpf_program__attach_perf_event().
>
> static int bpf_link__detach_perf_event(struct bpf_link *link)
> // TRUNCATED
> close(link->fd);
> return err;
> }
>
> > > +err:
> > > free(pmu_fd);
> > > + free(link);
> > > if (error)
> > > int_exit(0);
> >
> >
> > > }
> > > @@ -173,6 +183,7 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
> > > 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 +196,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 (link < 0) {
> > > + printf("bpf_program__attach_perf_event failed\n");
> > > + close(pmu_fd);
> > > + int_exit(0);
> > > + }
> > >
> > > if (generate_load() < 0) {
> > > error = 1;
> > > @@ -194,8 +209,7 @@ 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);
> >
> > This will exit with 0 error code and won't notify about error... Pass
> > through err?
> >
>
> You're right. Missed the return code.
> Will apply this at next version patch.
>
> > > }
> > > @@ -282,7 +296,9 @@ 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];
> > > + int prog_fd;
> > >
> > > snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> > > setrlimit(RLIMIT_MEMLOCK, &r);
> > > @@ -295,9 +311,20 @@ int main(int argc, char **argv)
> > > return 1;
> > > }
> > >
> > > - if (load_bpf_file(filename)) {
> > > - printf("%s", bpf_log_buf);
> > > - return 2;
> > > + if (bpf_prog_load(filename, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd))
> > > + return 1;
> > > +
> > > + prog = bpf_program__next(NULL, obj);
> > > + if (!prog) {
> > > + printf("finding a prog in obj 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 detailed review!
>
> Best,
> Daniel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link
2020-03-12 6:27 ` Andrii Nakryiko
@ 2020-03-12 6:35 ` Daniel T. Lee
0 siblings, 0 replies; 7+ messages in thread
From: Daniel T. Lee @ 2020-03-12 6:35 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Daniel Borkmann, Alexei Starovoitov, John Fastabend, Networking, bpf
On Thu, Mar 12, 2020 at 3:27 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Mar 11, 2020 at 11:16 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> >
> > On Wed, Mar 11, 2020 at 1:55 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Tue, Mar 10, 2020 at 4:27 PM Daniel T. Lee <danieltimlee@gmail.com> 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>
> > > > ---
> > >
> > > Daniel, thanks for this clean up! It's good to see samples be
> > > modernized a bit :)
> > >
> >
> > Thank you for your time and effort for the review :)
> >
> > > > Changes in v2:
> > > > - check memory allocation is successful
> > > > - clean up allocated memory on error
> > > >
> > > > samples/bpf/Makefile | 4 +-
> > > > samples/bpf/sampleip_user.c | 76 ++++++++++++++++++++++------------
> > > > samples/bpf/trace_event_user.c | 63 ++++++++++++++++++++--------
> > > > 3 files changed, 97 insertions(+), 46 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..fd763a49c913 100644
> > > > --- a/samples/bpf/sampleip_user.c
> > > > +++ b/samples/bpf/sampleip_user.c
> > > > @@ -10,13 +10,11 @@
> > > > #include <errno.h>
> > > > #include <signal.h>
> > > > #include <string.h>
> > > > -#include <assert.h>
> > > > #include <linux/perf_event.h>
> > > > #include <linux/ptrace.h>
> > > > #include <linux/bpf.h>
> > > > -#include <sys/ioctl.h>
> > > > +#include <bpf/bpf.h>
> > > > #include <bpf/libbpf.h>
> > > > -#include "bpf_load.h"
> > > > #include "perf-sys.h"
> > > > #include "trace_helpers.h"
> > > >
> > > > @@ -25,6 +23,7 @@
> > > > #define MAX_IPS 8192
> > > > #define PAGE_OFFSET 0xffff880000000000
> > > >
> > > > +static int map_fd;
> > > > static int nr_cpus;
> > > >
> > > > static void usage(void)
> > > > @@ -34,7 +33,8 @@ 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 *pmu_fd, int freq, struct bpf_program *prog,
> > > > + struct bpf_link **link)
> > >
> > > It's not apparent from looking at struct bpf_link **link whether it's
> > > an output parameter (so sampling_start is supposed to assign *single*
> > > link to return it to calling function) or it's an array of pointers.
> > > Seems like it's the latter, so I'd prefer this written as
> > >
> > > struct bpf_link *links[] (notice also plural name).
> > >
> > > Please consider this.
> > >
> >
> > This approach looks more apparent!
> > I'll update code using this way.
> >
> > > > {
> > > > int i;
> > > >
> > > > @@ -53,20 +53,22 @@ static int sampling_start(int *pmu_fd, int freq)
> > > > 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);
> > > > + link[i] = bpf_program__attach_perf_event(prog, pmu_fd[i]);
> > > > + if (link[i] < 0) {
> > >
> > > link is a pointer, < 0 doesn't make sense and is always going to be
> > > false on x86. Use IS_ERR(link[i]). It's also a good idea to set it to
> > > NULL, if link creation failed to prevent accidental
> > > bpf_link__destroy(link[i]) later on, trying to free bogus pointer.
> > >
> >
> > Failure on link creation is exactly what I was concerned about.
> > Thank you for giving me a clear solution!
> >
> > > > + fprintf(stderr, "ERROR: Attach perf event\n");
> > > > + return 1;
> > > > + }
> > > > }
> > > >
> > > > return 0;
> > > > }
> > > >
> > > > -static void sampling_end(int *pmu_fd)
> > > > +static void sampling_end(struct bpf_link **link)
> > >
> > > same as above, struct bpf_link *links[] would be much better here, IMO.
> > >
> >
> > Also, I'll apply this at next version patch.
> >
> > > > {
> > > > int i;
> > > >
> > > > for (i = 0; i < nr_cpus; i++)
> > > > - close(pmu_fd[i]);
> > > > + bpf_link__destroy(link[i]);
> > > > }
> > > >
> > > > struct ipcount {
> > > > @@ -128,14 +130,18 @@ 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 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;
> > > > + int error = 0;
> > > >
> > > > /* process arguments */
> > > > while ((opt = getopt(argc, argv, "F:h")) != -1) {
> > > > @@ -165,36 +171,54 @@ int main(int argc, char **argv)
> > > > /* create perf FDs for each CPU */
> > > > nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
> > >
> > > While neither approach is ideal, using number of online CPUs
> > > (_SC_NPROCESSORS_ONLN) will probably work in slightly more cases
> > > (there are machines configured with, say, 256 possible CPUs, but only
> > > 32 available, for instance).
> > >
> >
> > Thank you for pointing me out!
> > I've never thought about situation when processors may be offline.
> >
> > >
> > > > pmu_fd = malloc(nr_cpus * sizeof(int));
> > >
> > > similar naming nit: pmu_fds?
> > >
> >
> > Same again, apply this at next version patch.
> >
> > > > - if (pmu_fd == NULL) {
> > > > - fprintf(stderr, "ERROR: malloc of pmu_fd\n");
> > > > - return 1;
> > > > + link = malloc(nr_cpus * sizeof(struct bpf_link *));
> > >
> > > Use calloc() to have those links initialized to NULL automatically.
> > > Makes clean up so much easier.
> > >
> >
> > About NULL set, like you mentioned, calloc approach looks more neat.
> >
> > > > + if (pmu_fd == NULL || link == NULL) {
> > > > + fprintf(stderr, "ERROR: malloc of pmu_fd/link\n");
> > > > + error = 1;
> > > > + goto cleanup;
> > > > }
> > > >
> > > > /* 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)) {
> > >
> > > Using skeleton would be best, but it's probably more appropriate for
> > > another patch to integrate skeleton generation with samples/bpf. So
> > > the next one would be bpf_object__open_file(), instead of legacy
> > > bpf_prog_load().
> > >
> >
> > I'll try skeleton with other sample cleanup. For now, I'll stick with
> > bpf_object__{open/load}() instead of bpf_prog_load().
> >
> > > > 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;
> > > > + error = 1;
> > > > + goto cleanup;
> > > > + }
> > > > +
> > > > + prog = bpf_program__next(NULL, obj);
> > >
> > > I'm a bit lazy here, sorry, but isn't the name of the program known?
> > > bpf_object__find_program_by_title() is preferable.
> > >
> >
> > I also think it is good to specify the program title clearly.
> >
> > > > + if (!prog) {
> > > > + fprintf(stderr, "ERROR: finding a prog in obj 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(pmu_fd, freq, prog, link) != 0) {
> > > > + error = 1;
> > > > + goto cleanup;
> > > > + }
> > > > sleep(secs);
> > > > - sampling_end(pmu_fd);
> > > > - free(pmu_fd);
> > > > + sampling_end(link);
> > > >
> > > > /* output sample counts */
> > > > - print_ip_map(map_fd[0]);
> > > > + print_ip_map(map_fd);
> > > >
> > > > - return 0;
> > > > +cleanup:
> > > > + free(pmu_fd);
> > > > + free(link);
> > >
> > >
> > > Uhm... you are freeing this only on clean up. Also, you need to
> > > bpf_link__destroy() first. And close all pmu_fds. Surely process exit
> > > will ensure all this is cleaned up, but it's a good tone to clean up
> > > all resources explicitly.
> > >
> >
> > Well, cleanup: could cover link destroy (sampling_end), but I think
> > it is strange to clean up the link even though the bpf program is not
> > attached to the event. I think it is better to specify the link destroy
> > after the sampling starts.
>
> bpf_link__destroy() is designed in such a way that if passed NULL it
> will do nothing. So doing unconditional clean up at the end is clean
> and straightforward solution. You'll see it in a bunch of places in
> selftests.
>
I see. I will move this to cleanup: for an unconditional clean up at end.
> >
> > And, I've missed the link destroy when sampling got error.
> > Since sampling_end will destroy the links, so I'll add this on error.
> >
> > if (sampling_start(pmu_fd, freq, prog, link) != 0) {
> > error = 1;
> > + sampling_end(links);
> > goto cleanup;
> > }
> >
> > > > + return error;
> > > > }
> > > > diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c
> > > > index 356171bc392b..30c25ef99fc5 100644
> > > > --- a/samples/bpf/trace_event_user.c
> > > > +++ b/samples/bpf/trace_event_user.c
> > > > @@ -6,22 +6,21 @@
> > > > #include <stdlib.h>
> > > > #include <stdbool.h>
> > > > #include <string.h>
> > > > -#include <fcntl.h>
> > > > -#include <poll.h>
> > > > -#include <sys/ioctl.h>
> > > > #include <linux/perf_event.h>
> > > > #include <linux/bpf.h>
> > > > #include <signal.h>
> > > > -#include <assert.h>
> > > > #include <errno.h>
> > > > #include <sys/resource.h>
> > > > +#include <bpf/bpf.h>
> > > > #include <bpf/libbpf.h>
> > > > -#include "bpf_load.h"
> > > > #include "perf-sys.h"
> > > > #include "trace_helpers.h"
> > > >
> > > > #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)
> > > > @@ -137,9 +136,16 @@ 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 *));
> > >
> > > same as above, calloc() is better choice here
> > >
> >
> > Will apply this at next version patch.
> >
> > > > int *pmu_fd = malloc(nr_cpus * sizeof(int));
> > > > int i, error = 0;
> > > >
> > > > + if (pmu_fd == NULL || link == NULL) {
> > > > + printf("malloc of pmu_fd/link failed\n");
> > > > + error = 1;
> > > > + goto err;
> > > > + }
> > > > +
> > > > /* system wide perf event, no need to inherit */
> > > > attr->inherit = 0;
> > > >
> > > > @@ -151,8 +157,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 +171,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]);
> > >
> > > still need close(pmu_fd[i]);
> > >
> >
> > AFAIK, bpf_link__detach_perf_event() closes the pmu_fd.
> > Am I missed something?
>
> Ah, you are right, I missed that fact. But then you don't need pmu_fd
> array at all. Do perf_event_open and then immediately
> bpf_program__attach_perf_event().
>
Right. I won't need to handle pmu_fds, since bpf_link manages all of it.
Thanks for the tip!
> >
> > static int bpf_link__detach_perf_event(struct bpf_link *link)
> > // TRUNCATED
> > close(link->fd);
> > return err;
> > }
> >
> > > > +err:
> > > > free(pmu_fd);
> > > > + free(link);
> > > > if (error)
> > > > int_exit(0);
> > >
> > >
> > > > }
> > > > @@ -173,6 +183,7 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
> > > > 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 +196,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 (link < 0) {
> > > > + printf("bpf_program__attach_perf_event failed\n");
> > > > + close(pmu_fd);
> > > > + int_exit(0);
> > > > + }
> > > >
> > > > if (generate_load() < 0) {
> > > > error = 1;
> > > > @@ -194,8 +209,7 @@ 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);
> > >
> > > This will exit with 0 error code and won't notify about error... Pass
> > > through err?
> > >
> >
> > You're right. Missed the return code.
> > Will apply this at next version patch.
> >
> > > > }
> > > > @@ -282,7 +296,9 @@ 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];
> > > > + int prog_fd;
> > > >
> > > > snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> > > > setrlimit(RLIMIT_MEMLOCK, &r);
> > > > @@ -295,9 +311,20 @@ int main(int argc, char **argv)
> > > > return 1;
> > > > }
> > > >
> > > > - if (load_bpf_file(filename)) {
> > > > - printf("%s", bpf_log_buf);
> > > > - return 2;
> > > > + if (bpf_prog_load(filename, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd))
> > > > + return 1;
> > > > +
> > > > + prog = bpf_program__next(NULL, obj);
> > > > + if (!prog) {
> > > > + printf("finding a prog in obj 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 detailed review!
> >
> > Best,
> > Daniel
Thanks for the super fast response!
Best,
Daniel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-12 6:35 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-10 23:26 [PATCH bpf-next v2 0/2] Refactor perf_event sample user program with libbpf bpf_link Daniel T. Lee
2020-03-10 23:26 ` [PATCH bpf-next v2 1/2] samples: bpf: move read_trace_pipe to trace_helpers Daniel T. Lee
2020-03-10 23:26 ` [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link Daniel T. Lee
2020-03-11 4:55 ` Andrii Nakryiko
2020-03-12 6:15 ` Daniel T. Lee
2020-03-12 6:27 ` Andrii Nakryiko
2020-03-12 6:35 ` Daniel T. Lee
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).