* [PATCH net-next v2 0/2] bpf/tracing: allow user space to query prog array on the same tp
@ 2017-12-06 6:31 Yonghong Song
2017-12-06 6:31 ` [PATCH net-next v2 1/2] " Yonghong Song
2017-12-06 6:31 ` [PATCH net-next v2 2/2] bpf/tracing: add a bpf test for new ioctl query interface Yonghong Song
0 siblings, 2 replies; 6+ messages in thread
From: Yonghong Song @ 2017-12-06 6:31 UTC (permalink / raw)
To: peterz, rostedt, ast, daniel, kafai, netdev; +Cc: kernel-team
Commit e87c6bc3852b ("bpf: permit multiple bpf attachments
for a single perf event") added support to attach multiple
bpf programs to a single perf event. Given a perf event
(kprobe, uprobe, or kernel tracepoint), the perf ioctl interface
is used to query bpf programs attached to the same trace event.
There already exists a BPF_PROG_QUERY command for introspection
currently used by cgroup+bpf. We did have an implementation for
querying tracepoint+bpf through the same interface. However, it
looks cleaner to use ioctl() style of api here, since attaching
bpf prog to tracepoint/kuprobe is also done via ioctl.
Patch #1 had the core implementation and patch #2 added
a test case in tools bpf selftests suite.
Changelogs:
v1-> v2:
- Rebase on top of net-next.
- Use existing bpf_prog_array_length function instead of
implementing the same functionality in function
bpf_prog_array_copy_info.
Yonghong Song (2):
bpf/tracing: allow user space to query prog array on the same tp
bpf/tracing: add a bpf test for new ioctl query interface
include/linux/bpf.h | 4 +
include/uapi/linux/perf_event.h | 6 +
kernel/bpf/core.c | 21 ++++
kernel/events/core.c | 3 +
kernel/trace/bpf_trace.c | 23 ++++
tools/include/uapi/linux/perf_event.h | 6 +
tools/testing/selftests/bpf/Makefile | 2 +-
tools/testing/selftests/bpf/test_progs.c | 155 ++++++++++++++++++++++++++
tools/testing/selftests/bpf/test_tracepoint.c | 26 +++++
9 files changed, 245 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/test_tracepoint.c
--
2.9.5
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net-next v2 1/2] bpf/tracing: allow user space to query prog array on the same tp
2017-12-06 6:31 [PATCH net-next v2 0/2] bpf/tracing: allow user space to query prog array on the same tp Yonghong Song
@ 2017-12-06 6:31 ` Yonghong Song
2017-12-06 11:56 ` Peter Zijlstra
2017-12-06 6:31 ` [PATCH net-next v2 2/2] bpf/tracing: add a bpf test for new ioctl query interface Yonghong Song
1 sibling, 1 reply; 6+ messages in thread
From: Yonghong Song @ 2017-12-06 6:31 UTC (permalink / raw)
To: peterz, rostedt, ast, daniel, kafai, netdev; +Cc: kernel-team
Commit e87c6bc3852b ("bpf: permit multiple bpf attachments
for a single perf event") added support to attach multiple
bpf programs to a single perf event.
Commit 2541517c32be ("tracing, perf: Implement BPF programs
attached to kprobes") utilized the existing perf ioctl
interface and added the command PERF_EVENT_IOC_SET_BPF
to attach a bpf program to a tracepoint.
This patch adds a new ioctl
command, given a perf event fd, to query the bpf program array
attached to the same perf tracepoint event.
The new uapi ioctl command:
PERF_EVENT_IOC_QUERY_BPF
The new uapi/linux/perf_event.h structure:
struct perf_event_query_bpf {
__u64 prog_ids;
__u32 prog_cnt;
};
The usage:
struct perf_event_query_bpf query;
query.prog_ids = (__u64)usr_prog_ids_buf;
query.prog_cnt = usr_prog_ids_buf_len;
err = ioctl(pmu_efd, PERF_EVENT_IOC_QUERY_BPF, &query);
Signed-off-by: Yonghong Song <yhs@fb.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
include/linux/bpf.h | 4 ++++
include/uapi/linux/perf_event.h | 6 ++++++
kernel/bpf/core.c | 21 +++++++++++++++++++++
kernel/events/core.c | 3 +++
kernel/trace/bpf_trace.c | 23 +++++++++++++++++++++++
5 files changed, 57 insertions(+)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index e55e425..f812ac5 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -254,6 +254,7 @@ typedef unsigned long (*bpf_ctx_copy_t)(void *dst, const void *src,
u64 bpf_event_output(struct bpf_map *map, u64 flags, void *meta, u64 meta_size,
void *ctx, u64 ctx_size, bpf_ctx_copy_t ctx_copy);
+int bpf_event_query_prog_array(struct perf_event *event, void __user *info);
int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
union bpf_attr __user *uattr);
@@ -285,6 +286,9 @@ int bpf_prog_array_copy_to_user(struct bpf_prog_array __rcu *progs,
void bpf_prog_array_delete_safe(struct bpf_prog_array __rcu *progs,
struct bpf_prog *old_prog);
+int bpf_prog_array_copy_info(struct bpf_prog_array __rcu *array,
+ __u32 __user *prog_ids, u32 request_cnt,
+ __u32 __user *prog_cnt);
int bpf_prog_array_copy(struct bpf_prog_array __rcu *old_array,
struct bpf_prog *exclude_prog,
struct bpf_prog *include_prog,
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index b9a4953..fee0b43 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -418,6 +418,11 @@ struct perf_event_attr {
__u16 __reserved_2; /* align to __u64 */
};
+struct perf_event_query_bpf {
+ __u64 prog_ids;
+ __u32 prog_cnt;
+};
+
#define perf_flags(attr) (*(&(attr)->read_format + 1))
/*
@@ -433,6 +438,7 @@ struct perf_event_attr {
#define PERF_EVENT_IOC_ID _IOR('$', 7, __u64 *)
#define PERF_EVENT_IOC_SET_BPF _IOW('$', 8, __u32)
#define PERF_EVENT_IOC_PAUSE_OUTPUT _IOW('$', 9, __u32)
+#define PERF_EVENT_IOC_QUERY_BPF _IOWR('$', 10, struct perf_event_query_bpf *)
enum perf_event_ioc_flags {
PERF_IOC_FLAG_GROUP = 1U << 0,
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 86b50aa..35b427aa 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1462,6 +1462,8 @@ int bpf_prog_array_copy_to_user(struct bpf_prog_array __rcu *progs,
rcu_read_lock();
prog = rcu_dereference(progs)->progs;
for (; *prog; prog++) {
+ if (*prog == &dummy_bpf_prog.prog)
+ continue;
id = (*prog)->aux->id;
if (copy_to_user(prog_ids + i, &id, sizeof(id))) {
rcu_read_unlock();
@@ -1545,6 +1547,25 @@ int bpf_prog_array_copy(struct bpf_prog_array __rcu *old_array,
return 0;
}
+int bpf_prog_array_copy_info(struct bpf_prog_array __rcu *array,
+ __u32 __user *prog_ids, u32 request_cnt,
+ __u32 __user *prog_cnt)
+{
+ u32 cnt = 0;
+
+ if (array)
+ cnt = bpf_prog_array_length(array);
+
+ if (copy_to_user(prog_cnt, &cnt, sizeof(cnt)))
+ return -EFAULT;
+
+ /* return early if user requested only program count or nothing to copy */
+ if (!request_cnt || !prog_ids || !cnt)
+ return 0;
+
+ return bpf_prog_array_copy_to_user(array, prog_ids, request_cnt);
+}
+
static void bpf_prog_free_deferred(struct work_struct *work)
{
struct bpf_prog_aux *aux;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 16beab4..f10609e 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4723,6 +4723,9 @@ static long _perf_ioctl(struct perf_event *event, unsigned int cmd, unsigned lon
rcu_read_unlock();
return 0;
}
+
+ case PERF_EVENT_IOC_QUERY_BPF:
+ return bpf_event_query_prog_array(event, (void __user *)arg);
default:
return -ENOTTY;
}
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 0ce99c3..81eedb2 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -820,3 +820,26 @@ void perf_event_detach_bpf_prog(struct perf_event *event)
unlock:
mutex_unlock(&bpf_event_mutex);
}
+
+int bpf_event_query_prog_array(struct perf_event *event, void __user *info)
+{
+ struct perf_event_query_bpf __user *uquery = info;
+ struct perf_event_query_bpf query = {};
+ int ret;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+ if (event->attr.type != PERF_TYPE_TRACEPOINT)
+ return -EINVAL;
+ if (copy_from_user(&query, uquery, sizeof(query)))
+ return -EFAULT;
+
+ mutex_lock(&bpf_event_mutex);
+ ret = bpf_prog_array_copy_info(event->tp_event->prog_array,
+ u64_to_user_ptr(query.prog_ids),
+ query.prog_cnt,
+ &uquery->prog_cnt);
+ mutex_unlock(&bpf_event_mutex);
+
+ return ret;
+}
--
2.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net-next v2 2/2] bpf/tracing: add a bpf test for new ioctl query interface
2017-12-06 6:31 [PATCH net-next v2 0/2] bpf/tracing: allow user space to query prog array on the same tp Yonghong Song
2017-12-06 6:31 ` [PATCH net-next v2 1/2] " Yonghong Song
@ 2017-12-06 6:31 ` Yonghong Song
1 sibling, 0 replies; 6+ messages in thread
From: Yonghong Song @ 2017-12-06 6:31 UTC (permalink / raw)
To: peterz, rostedt, ast, daniel, kafai, netdev; +Cc: kernel-team
Added a subtest in test_progs. The tracepoint is
sched/sched_switch. Multiple bpf programs are attached to
this tracepoint and the query interface is exercised.
Signed-off-by: Yonghong Song <yhs@fb.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
tools/include/uapi/linux/perf_event.h | 6 +
tools/testing/selftests/bpf/Makefile | 2 +-
tools/testing/selftests/bpf/test_progs.c | 155 ++++++++++++++++++++++++++
tools/testing/selftests/bpf/test_tracepoint.c | 26 +++++
4 files changed, 188 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/test_tracepoint.c
diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
index 362493a..8523db0 100644
--- a/tools/include/uapi/linux/perf_event.h
+++ b/tools/include/uapi/linux/perf_event.h
@@ -418,6 +418,11 @@ struct perf_event_attr {
__u16 __reserved_2; /* align to __u64 */
};
+struct perf_event_query_bpf {
+ __u64 prog_ids;
+ __u32 prog_cnt;
+};
+
#define perf_flags(attr) (*(&(attr)->read_format + 1))
/*
@@ -433,6 +438,7 @@ struct perf_event_attr {
#define PERF_EVENT_IOC_ID _IOR('$', 7, __u64 *)
#define PERF_EVENT_IOC_SET_BPF _IOW('$', 8, __u32)
#define PERF_EVENT_IOC_PAUSE_OUTPUT _IOW('$', 9, __u32)
+#define PERF_EVENT_IOC_QUERY_BPF _IOWR('$', 10, struct perf_event_query_bpf *)
enum perf_event_ioc_flags {
PERF_IOC_FLAG_GROUP = 1U << 0,
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 2c9d8c6..255fb1f 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -17,7 +17,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test_obj_id.o \
test_pkt_md_access.o test_xdp_redirect.o test_xdp_meta.o sockmap_parse_prog.o \
- sockmap_verdict_prog.o dev_cgroup.o sample_ret0.o
+ sockmap_verdict_prog.o dev_cgroup.o sample_ret0.o test_tracepoint.o
TEST_PROGS := test_kmod.sh test_xdp_redirect.sh test_xdp_meta.sh \
test_offload.py
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 6942753..dde23ed 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -21,8 +21,10 @@ typedef __u16 __sum16;
#include <linux/ipv6.h>
#include <linux/tcp.h>
#include <linux/filter.h>
+#include <linux/perf_event.h>
#include <linux/unistd.h>
+#include <sys/ioctl.h>
#include <sys/wait.h>
#include <sys/resource.h>
#include <sys/types.h>
@@ -617,6 +619,158 @@ static void test_obj_name(void)
}
}
+static void test_tp_attach_query(void)
+{
+ const int num_progs = 3;
+ __u32 duration = 0, info_len, prog_ids[num_progs], saved_prog_ids[num_progs];
+ int i, j, bytes, efd, err, prog_fd[num_progs], pmu_fd[num_progs];
+ const char *file = "./test_tracepoint.o";
+ struct perf_event_query_bpf query = {};
+ struct perf_event_attr attr = {};
+ struct bpf_object *obj[num_progs];
+ struct bpf_prog_info prog_info;
+ char buf[256];
+
+ snprintf(buf, sizeof(buf),
+ "/sys/kernel/debug/tracing/events/sched/sched_switch/id");
+ efd = open(buf, O_RDONLY, 0);
+ if (CHECK(efd < 0, "open", "err %d errno %d\n", efd, errno))
+ return;
+ bytes = read(efd, buf, sizeof(buf));
+ close(efd);
+ if (CHECK(bytes <= 0 || bytes >= sizeof(buf),
+ "read", "bytes %d errno %d\n", bytes, errno))
+ return;
+
+ attr.config = strtol(buf, NULL, 0);
+ attr.type = PERF_TYPE_TRACEPOINT;
+ attr.sample_type = PERF_SAMPLE_RAW | PERF_SAMPLE_CALLCHAIN;
+ attr.sample_period = 1;
+ attr.wakeup_events = 1;
+
+ for (i = 0; i < num_progs; i++) {
+ err = bpf_prog_load(file, BPF_PROG_TYPE_TRACEPOINT, &obj[i],
+ &prog_fd[i]);
+ if (CHECK(err, "prog_load", "err %d errno %d\n", err, errno))
+ goto cleanup1;
+
+ bzero(&prog_info, sizeof(prog_info));
+ prog_info.jited_prog_len = 0;
+ prog_info.xlated_prog_len = 0;
+ prog_info.nr_map_ids = 0;
+ info_len = sizeof(prog_info);
+ err = bpf_obj_get_info_by_fd(prog_fd[i], &prog_info, &info_len);
+ if (CHECK(err, "bpf_obj_get_info_by_fd", "err %d errno %d\n",
+ err, errno))
+ goto cleanup1;
+ saved_prog_ids[i] = prog_info.id;
+
+ pmu_fd[i] = syscall(__NR_perf_event_open, &attr, -1 /* pid */,
+ 0 /* cpu 0 */, -1 /* group id */,
+ 0 /* flags */);
+ if (CHECK(pmu_fd[i] < 0, "perf_event_open", "err %d errno %d\n",
+ pmu_fd[i], errno))
+ goto cleanup2;
+ err = ioctl(pmu_fd[i], PERF_EVENT_IOC_ENABLE, 0);
+ if (CHECK(err, "perf_event_ioc_enable", "err %d errno %d\n",
+ err, errno))
+ goto cleanup3;
+
+ if (i == 0) {
+ /* check NULL prog array query */
+ query.prog_ids = (__u64)prog_ids;
+ query.prog_cnt = num_progs;
+ err = ioctl(pmu_fd[i], PERF_EVENT_IOC_QUERY_BPF, &query);
+ if (CHECK(err || query.prog_cnt != 0,
+ "perf_event_ioc_query_bpf",
+ "err %d errno %d query.prog_cnt %u\n",
+ err, errno, query.prog_cnt))
+ goto cleanup3;
+ }
+
+ err = ioctl(pmu_fd[i], PERF_EVENT_IOC_SET_BPF, prog_fd[i]);
+ if (CHECK(err, "perf_event_ioc_set_bpf", "err %d errno %d\n",
+ err, errno))
+ goto cleanup3;
+
+ if (i == 1) {
+ /* try to get # of programs only: prog_cnt == 0 */
+ query.prog_cnt = 0;
+ err = ioctl(pmu_fd[i], PERF_EVENT_IOC_QUERY_BPF, &query);
+ if (CHECK(err || query.prog_cnt != 2,
+ "perf_event_ioc_query_bpf",
+ "err %d errno %d query.prog_cnt %u\n",
+ err, errno, query.prog_cnt))
+ goto cleanup3;
+
+ /* try to get # of programs only: prog_ids == 0 */
+ query.prog_ids = 0;
+ query.prog_cnt = num_progs;
+ err = ioctl(pmu_fd[i], PERF_EVENT_IOC_QUERY_BPF, &query);
+ if (CHECK(err || query.prog_cnt != 2,
+ "perf_event_ioc_query_bpf",
+ "err %d errno %d query.prog_cnt %u\n",
+ err, errno, query.prog_cnt))
+ goto cleanup3;
+
+ /* try a few negative tests */
+ /* invalid query pointer */
+ err = ioctl(pmu_fd[i], PERF_EVENT_IOC_QUERY_BPF,
+ (struct perf_event_query_bpf *)0x1);
+ if (CHECK(!err || errno != EFAULT,
+ "perf_event_ioc_query_bpf",
+ "err %d errno %d query.prog_cnt %u\n",
+ err, errno, query.prog_cnt))
+ goto cleanup3;
+
+ /* invalid prog_ids pointer */
+ query.prog_ids = 0x1;
+ query.prog_cnt = 1;
+ err = ioctl(pmu_fd[i], PERF_EVENT_IOC_QUERY_BPF, &query);
+ if (CHECK(!err || errno != EFAULT,
+ "perf_event_ioc_query_bpf",
+ "err %d errno %d query.prog_cnt %u\n",
+ err, errno, query.prog_cnt))
+ goto cleanup3;
+
+ /* no enough space */
+ query.prog_ids = (__u64)prog_ids;
+ query.prog_cnt = 1;
+ err = ioctl(pmu_fd[i], PERF_EVENT_IOC_QUERY_BPF, &query);
+ if (CHECK(!err || errno != ENOSPC,
+ "perf_event_ioc_query_bpf",
+ "err %d errno %d query.prog_cnt %u\n",
+ err, errno, query.prog_cnt))
+ goto cleanup3;
+ }
+
+ query.prog_ids = (__u64)prog_ids;
+ query.prog_cnt = num_progs;
+ err = ioctl(pmu_fd[i], PERF_EVENT_IOC_QUERY_BPF, &query);
+ if (CHECK(err || query.prog_cnt != (i + 1),
+ "perf_event_ioc_query_bpf",
+ "err %d errno %d query.prog_cnt %u\n",
+ err, errno, query.prog_cnt))
+ goto cleanup3;
+ for (j = 0; j < i + 1; j++)
+ if (CHECK(saved_prog_ids[j] != prog_ids[j],
+ "perf_event_ioc_query_bpf",
+ "#%d saved_prog_id %x query prog_id %x\n",
+ j, saved_prog_ids[j], prog_ids[j]))
+ goto cleanup3;
+ }
+
+ i = num_progs - 1;
+ for (; i >= 0; i--) {
+ cleanup3:
+ ioctl(pmu_fd[i], PERF_EVENT_IOC_DISABLE);
+ cleanup2:
+ close(pmu_fd[i]);
+ cleanup1:
+ bpf_object__close(obj[i]);
+ }
+}
+
int main(void)
{
struct rlimit rinf = { RLIM_INFINITY, RLIM_INFINITY };
@@ -630,6 +784,7 @@ int main(void)
test_bpf_obj_id();
test_pkt_md_access();
test_obj_name();
+ test_tp_attach_query();
printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt);
return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
diff --git a/tools/testing/selftests/bpf/test_tracepoint.c b/tools/testing/selftests/bpf/test_tracepoint.c
new file mode 100644
index 0000000..04bf084
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_tracepoint.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2017 Facebook
+
+#include <linux/bpf.h>
+#include "bpf_helpers.h"
+
+/* taken from /sys/kernel/debug/tracing/events/sched/sched_switch/format */
+struct sched_switch_args {
+ unsigned long long pad;
+ char prev_comm[16];
+ int prev_pid;
+ int prev_prio;
+ long long prev_state;
+ char next_comm[16];
+ int next_pid;
+ int next_prio;
+};
+
+SEC("tracepoint/sched/sched_switch")
+int oncpu(struct sched_switch_args *ctx)
+{
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
+__u32 _version SEC("version") = 1; /* ignored by tracepoints, required by libbpf.a */
--
2.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v2 1/2] bpf/tracing: allow user space to query prog array on the same tp
2017-12-06 6:31 ` [PATCH net-next v2 1/2] " Yonghong Song
@ 2017-12-06 11:56 ` Peter Zijlstra
2017-12-06 13:16 ` Peter Zijlstra
0 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2017-12-06 11:56 UTC (permalink / raw)
To: Yonghong Song; +Cc: rostedt, ast, daniel, kafai, netdev, kernel-team
On Tue, Dec 05, 2017 at 10:31:28PM -0800, Yonghong Song wrote:
> Commit e87c6bc3852b ("bpf: permit multiple bpf attachments
> for a single perf event") added support to attach multiple
> bpf programs to a single perf event.
> Commit 2541517c32be ("tracing, perf: Implement BPF programs
> attached to kprobes") utilized the existing perf ioctl
> interface and added the command PERF_EVENT_IOC_SET_BPF
> to attach a bpf program to a tracepoint.
>
> This patch adds a new ioctl
> command, given a perf event fd, to query the bpf program array
> attached to the same perf tracepoint event.
>
> The new uapi ioctl command:
> PERF_EVENT_IOC_QUERY_BPF
>
> The new uapi/linux/perf_event.h structure:
> struct perf_event_query_bpf {
> __u64 prog_ids;
> __u32 prog_cnt;
> };
>
> The usage:
> struct perf_event_query_bpf query;
> query.prog_ids = (__u64)usr_prog_ids_buf;
> query.prog_cnt = usr_prog_ids_buf_len;
> err = ioctl(pmu_efd, PERF_EVENT_IOC_QUERY_BPF, &query);
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> Acked-by: Alexei Starovoitov <ast@kernel.org>
Can you please fix that example to make it clear that prog_ids is in
fact a pointer to an array of size prog_cnt. Ideally also describing
what the type of array is.
In fact, would not something like:
struct perf_event_query_bpf {
__u32 len;
__u32 __reserved;
__u64 ids[0];
};
be a much clearer interface?
Also, you forgot to tell us why we need this interface at all.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v2 1/2] bpf/tracing: allow user space to query prog array on the same tp
2017-12-06 11:56 ` Peter Zijlstra
@ 2017-12-06 13:16 ` Peter Zijlstra
2017-12-06 18:37 ` Yonghong Song
0 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2017-12-06 13:16 UTC (permalink / raw)
To: Yonghong Song; +Cc: rostedt, ast, daniel, kafai, netdev, kernel-team
On Wed, Dec 06, 2017 at 12:56:36PM +0100, Peter Zijlstra wrote:
> On Tue, Dec 05, 2017 at 10:31:28PM -0800, Yonghong Song wrote:
> > Commit e87c6bc3852b ("bpf: permit multiple bpf attachments
> > for a single perf event") added support to attach multiple
> > bpf programs to a single perf event.
> > Commit 2541517c32be ("tracing, perf: Implement BPF programs
> > attached to kprobes") utilized the existing perf ioctl
> > interface and added the command PERF_EVENT_IOC_SET_BPF
> > to attach a bpf program to a tracepoint.
> >
> > This patch adds a new ioctl
> > command, given a perf event fd, to query the bpf program array
> > attached to the same perf tracepoint event.
> >
> > The new uapi ioctl command:
> > PERF_EVENT_IOC_QUERY_BPF
> >
> > The new uapi/linux/perf_event.h structure:
> > struct perf_event_query_bpf {
> > __u64 prog_ids;
> > __u32 prog_cnt;
> > };
> >
> > The usage:
> > struct perf_event_query_bpf query;
> > query.prog_ids = (__u64)usr_prog_ids_buf;
> > query.prog_cnt = usr_prog_ids_buf_len;
> > err = ioctl(pmu_efd, PERF_EVENT_IOC_QUERY_BPF, &query);
> >
> > Signed-off-by: Yonghong Song <yhs@fb.com>
> > Acked-by: Alexei Starovoitov <ast@kernel.org>
>
> Can you please fix that example to make it clear that prog_ids is in
> fact a pointer to an array of size prog_cnt. Ideally also describing
> what the type of array is.
>
> In fact, would not something like:
>
> struct perf_event_query_bpf {
> __u32 len;
> __u32 __reserved;
I suppose we could use this field to store the number of entries
returned, retaining the len to indicate how large the structure is.
> __u64 ids[0];
> };
>
> be a much clearer interface?
>
> Also, you forgot to tell us why we need this interface at all.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v2 1/2] bpf/tracing: allow user space to query prog array on the same tp
2017-12-06 13:16 ` Peter Zijlstra
@ 2017-12-06 18:37 ` Yonghong Song
0 siblings, 0 replies; 6+ messages in thread
From: Yonghong Song @ 2017-12-06 18:37 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: rostedt, ast, daniel, kafai, netdev, kernel-team
On 12/6/17 5:16 AM, Peter Zijlstra wrote:
> On Wed, Dec 06, 2017 at 12:56:36PM +0100, Peter Zijlstra wrote:
>> On Tue, Dec 05, 2017 at 10:31:28PM -0800, Yonghong Song wrote:
>>> Commit e87c6bc3852b ("bpf: permit multiple bpf attachments
>>> for a single perf event") added support to attach multiple
>>> bpf programs to a single perf event.
>>> Commit 2541517c32be ("tracing, perf: Implement BPF programs
>>> attached to kprobes") utilized the existing perf ioctl
>>> interface and added the command PERF_EVENT_IOC_SET_BPF
>>> to attach a bpf program to a tracepoint.
>>>
>>> This patch adds a new ioctl
>>> command, given a perf event fd, to query the bpf program array
>>> attached to the same perf tracepoint event.
>>>
>>> The new uapi ioctl command:
>>> PERF_EVENT_IOC_QUERY_BPF
>>>
>>> The new uapi/linux/perf_event.h structure:
>>> struct perf_event_query_bpf {
>>> __u64 prog_ids;
>>> __u32 prog_cnt;
>>> };
>>>
>>> The usage:
>>> struct perf_event_query_bpf query;
>>> query.prog_ids = (__u64)usr_prog_ids_buf;
>>> query.prog_cnt = usr_prog_ids_buf_len;
>>> err = ioctl(pmu_efd, PERF_EVENT_IOC_QUERY_BPF, &query);
>>>
>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>> Acked-by: Alexei Starovoitov <ast@kernel.org>
>>
>> Can you please fix that example to make it clear that prog_ids is in
>> fact a pointer to an array of size prog_cnt. Ideally also describing
>> what the type of array is.
Right. Will address this with more descriptions in the commit message
and also add some comments in the perf_event.h.
>>
>> In fact, would not something like:
>>
>> struct perf_event_query_bpf {
>> __u32 len;
>> __u32 __reserved;
>
> I suppose we could use this field to store the number of entries
> returned, retaining the len to indicate how large the structure is.
>
>> __u64 ids[0];
>> };
>>
>> be a much clearer interface?
Yes, this is clearer and may be consistent with perf interface.
FYI, my old interface is similar to the BPF query interface below:
struct { /* anonymous struct used by BPF_PROG_QUERY command */
__u32 target_fd; /* container object to
query */
__u32 attach_type;
__u32 query_flags;
__u32 attach_flags;
__aligned_u64 prog_ids;
__u32 prog_cnt;
} query;
>>
>> Also, you forgot to tell us why we need this interface at all.
Right. Will add some descriptions for this.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-06 18:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-06 6:31 [PATCH net-next v2 0/2] bpf/tracing: allow user space to query prog array on the same tp Yonghong Song
2017-12-06 6:31 ` [PATCH net-next v2 1/2] " Yonghong Song
2017-12-06 11:56 ` Peter Zijlstra
2017-12-06 13:16 ` Peter Zijlstra
2017-12-06 18:37 ` Yonghong Song
2017-12-06 6:31 ` [PATCH net-next v2 2/2] bpf/tracing: add a bpf test for new ioctl query interface Yonghong Song
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.