* [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 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).