* [PATCH v2 bpf-next 0/3] enable BPF_PROG_TEST_RUN for raw_tp
@ 2020-09-23 16:53 Song Liu
2020-09-23 16:53 ` [PATCH v2 bpf-next 1/3] bpf: enable BPF_PROG_TEST_RUN for raw_tracepoint Song Liu
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Song Liu @ 2020-09-23 16:53 UTC (permalink / raw)
To: netdev, bpf; +Cc: kernel-team, ast, daniel, john.fastabend, kpsingh, Song Liu
This set enables BPF_PROG_TEST_RUN for raw_tracepoint type programs. This
set also enables running the raw_tp program on a specific CPU. This feature
can be used by user space to trigger programs that access percpu resources,
e.g. perf_event, percpu variables.
Changes v1 => v2:
1. More checks for retval in the selftest. (John)
2. Remove unnecessary goto in bpf_prog_test_run_raw_tp. (John)
Song Liu (3):
bpf: enable BPF_PROG_TEST_RUN for raw_tracepoint
libbpf: introduce bpf_prog_test_run_xattr_opts
selftests/bpf: add raw_tp_test_run
include/linux/bpf.h | 3 +
include/uapi/linux/bpf.h | 5 ++
kernel/bpf/syscall.c | 2 +-
kernel/trace/bpf_trace.c | 1 +
net/bpf/test_run.c | 88 +++++++++++++++++++
tools/include/uapi/linux/bpf.h | 5 ++
tools/lib/bpf/bpf.c | 13 ++-
tools/lib/bpf/bpf.h | 11 +++
tools/lib/bpf/libbpf.map | 1 +
.../bpf/prog_tests/raw_tp_test_run.c | 73 +++++++++++++++
.../bpf/progs/test_raw_tp_test_run.c | 26 ++++++
11 files changed, 226 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/raw_tp_test_run.c
create mode 100644 tools/testing/selftests/bpf/progs/test_raw_tp_test_run.c
--
2.24.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 bpf-next 1/3] bpf: enable BPF_PROG_TEST_RUN for raw_tracepoint
2020-09-23 16:53 [PATCH v2 bpf-next 0/3] enable BPF_PROG_TEST_RUN for raw_tp Song Liu
@ 2020-09-23 16:53 ` Song Liu
2020-09-23 19:36 ` Andrii Nakryiko
2020-09-23 16:54 ` [PATCH v2 bpf-next 2/3] libbpf: introduce bpf_prog_test_run_xattr_opts Song Liu
2020-09-23 16:54 ` [PATCH v2 bpf-next 3/3] selftests/bpf: add raw_tp_test_run Song Liu
2 siblings, 1 reply; 13+ messages in thread
From: Song Liu @ 2020-09-23 16:53 UTC (permalink / raw)
To: netdev, bpf; +Cc: kernel-team, ast, daniel, john.fastabend, kpsingh, Song Liu
Add .test_run for raw_tracepoint. Also, introduce a new feature that runs
the target program on a specific CPU. This is achieved by a new flag in
bpf_attr.test, cpu_plus. For compatibility, cpu_plus == 0 means run the
program on current cpu, cpu_plus > 0 means run the program on cpu with id
(cpu_plus - 1). This feature is needed for BPF programs that handle
perf_event and other percpu resources, as the program can access these
resource locally.
Acked-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Song Liu <songliubraving@fb.com>
---
include/linux/bpf.h | 3 ++
include/uapi/linux/bpf.h | 5 ++
kernel/bpf/syscall.c | 2 +-
kernel/trace/bpf_trace.c | 1 +
net/bpf/test_run.c | 88 ++++++++++++++++++++++++++++++++++
tools/include/uapi/linux/bpf.h | 5 ++
6 files changed, 103 insertions(+), 1 deletion(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d7c5a6ed87e30..23758c282eb4b 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1376,6 +1376,9 @@ int bpf_prog_test_run_tracing(struct bpf_prog *prog,
int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
const union bpf_attr *kattr,
union bpf_attr __user *uattr);
+int bpf_prog_test_run_raw_tp(struct bpf_prog *prog,
+ const union bpf_attr *kattr,
+ union bpf_attr __user *uattr);
bool btf_ctx_access(int off, int size, enum bpf_access_type type,
const struct bpf_prog *prog,
struct bpf_insn_access_aux *info);
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index a22812561064a..89acf41913e70 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -566,6 +566,11 @@ union bpf_attr {
*/
__aligned_u64 ctx_in;
__aligned_u64 ctx_out;
+ __u32 cpu_plus; /* run this program on cpu
+ * (cpu_plus - 1).
+ * If cpu_plus == 0, run on
+ * current cpu.
+ */
} test;
struct { /* anonymous struct used by BPF_*_GET_*_ID */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index ec68d3a23a2b7..4664531ff92ea 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2975,7 +2975,7 @@ static int bpf_prog_query(const union bpf_attr *attr,
}
}
-#define BPF_PROG_TEST_RUN_LAST_FIELD test.ctx_out
+#define BPF_PROG_TEST_RUN_LAST_FIELD test.cpu_plus
static int bpf_prog_test_run(const union bpf_attr *attr,
union bpf_attr __user *uattr)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index b2a5380eb1871..4553aebf53862 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1675,6 +1675,7 @@ const struct bpf_verifier_ops raw_tracepoint_verifier_ops = {
};
const struct bpf_prog_ops raw_tracepoint_prog_ops = {
+ .test_run = bpf_prog_test_run_raw_tp,
};
const struct bpf_verifier_ops tracing_verifier_ops = {
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 99eb8c6c0fbcc..b32e22f0ee16f 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -11,6 +11,7 @@
#include <net/sock.h>
#include <net/tcp.h>
#include <linux/error-injection.h>
+#include <linux/smp.h>
#define CREATE_TRACE_POINTS
#include <trace/events/bpf_test_run.h>
@@ -204,6 +205,9 @@ int bpf_prog_test_run_tracing(struct bpf_prog *prog,
int b = 2, err = -EFAULT;
u32 retval = 0;
+ if (kattr->test.cpu_plus)
+ return -EINVAL;
+
switch (prog->expected_attach_type) {
case BPF_TRACE_FENTRY:
case BPF_TRACE_FEXIT:
@@ -236,6 +240,84 @@ int bpf_prog_test_run_tracing(struct bpf_prog *prog,
return err;
}
+struct bpf_raw_tp_test_run_info {
+ struct bpf_prog *prog;
+ void *ctx;
+ u32 retval;
+};
+
+static void
+__bpf_prog_test_run_raw_tp(void *data)
+{
+ struct bpf_raw_tp_test_run_info *info = data;
+
+ rcu_read_lock();
+ migrate_disable();
+ info->retval = BPF_PROG_RUN(info->prog, info->ctx);
+ migrate_enable();
+ rcu_read_unlock();
+}
+
+int bpf_prog_test_run_raw_tp(struct bpf_prog *prog,
+ const union bpf_attr *kattr,
+ union bpf_attr __user *uattr)
+{
+ void __user *ctx_in = u64_to_user_ptr(kattr->test.ctx_in);
+ __u32 ctx_size_in = kattr->test.ctx_size_in;
+ struct bpf_raw_tp_test_run_info info;
+ int cpu, err = 0;
+
+ /* doesn't support data_in/out, ctx_out, duration, or repeat */
+ if (kattr->test.data_in || kattr->test.data_out ||
+ kattr->test.ctx_out || kattr->test.duration ||
+ kattr->test.repeat)
+ return -EINVAL;
+
+ if (ctx_size_in < prog->aux->max_ctx_offset)
+ return -EINVAL;
+
+ if (ctx_size_in) {
+ info.ctx = kzalloc(ctx_size_in, GFP_USER);
+ if (!info.ctx)
+ return -ENOMEM;
+ if (copy_from_user(info.ctx, ctx_in, ctx_size_in)) {
+ err = -EFAULT;
+ goto out;
+ }
+ } else {
+ info.ctx = NULL;
+ }
+
+ info.prog = prog;
+ cpu = kattr->test.cpu_plus - 1;
+
+ if (!kattr->test.cpu_plus || cpu == smp_processor_id()) {
+ __bpf_prog_test_run_raw_tp(&info);
+ } else {
+ /* smp_call_function_single() also checks cpu_online()
+ * after csd_lock(). However, since cpu_plus is from user
+ * space, let's do an extra quick check to filter out
+ * invalid value before smp_call_function_single().
+ */
+ if (!cpu_online(cpu)) {
+ err = -ENXIO;
+ goto out;
+ }
+
+ err = smp_call_function_single(cpu, __bpf_prog_test_run_raw_tp,
+ &info, 1);
+ if (err)
+ goto out;
+ }
+
+ if (copy_to_user(&uattr->test.retval, &info.retval, sizeof(u32)))
+ err = -EFAULT;
+
+out:
+ kfree(info.ctx);
+ return err;
+}
+
static void *bpf_ctx_init(const union bpf_attr *kattr, u32 max_size)
{
void __user *data_in = u64_to_user_ptr(kattr->test.ctx_in);
@@ -410,6 +492,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
void *data;
int ret;
+ if (kattr->test.cpu_plus)
+ return -EINVAL;
+
data = bpf_test_init(kattr, size, NET_SKB_PAD + NET_IP_ALIGN,
SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
if (IS_ERR(data))
@@ -607,6 +692,9 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
if (prog->type != BPF_PROG_TYPE_FLOW_DISSECTOR)
return -EINVAL;
+ if (kattr->test.cpu_plus)
+ return -EINVAL;
+
if (size < ETH_HLEN)
return -EINVAL;
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a22812561064a..89acf41913e70 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -566,6 +566,11 @@ union bpf_attr {
*/
__aligned_u64 ctx_in;
__aligned_u64 ctx_out;
+ __u32 cpu_plus; /* run this program on cpu
+ * (cpu_plus - 1).
+ * If cpu_plus == 0, run on
+ * current cpu.
+ */
} test;
struct { /* anonymous struct used by BPF_*_GET_*_ID */
--
2.24.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 bpf-next 2/3] libbpf: introduce bpf_prog_test_run_xattr_opts
2020-09-23 16:53 [PATCH v2 bpf-next 0/3] enable BPF_PROG_TEST_RUN for raw_tp Song Liu
2020-09-23 16:53 ` [PATCH v2 bpf-next 1/3] bpf: enable BPF_PROG_TEST_RUN for raw_tracepoint Song Liu
@ 2020-09-23 16:54 ` Song Liu
2020-09-23 19:31 ` Andrii Nakryiko
2020-09-23 16:54 ` [PATCH v2 bpf-next 3/3] selftests/bpf: add raw_tp_test_run Song Liu
2 siblings, 1 reply; 13+ messages in thread
From: Song Liu @ 2020-09-23 16:54 UTC (permalink / raw)
To: netdev, bpf; +Cc: kernel-team, ast, daniel, john.fastabend, kpsingh, Song Liu
This API supports new field cpu_plus in bpf_attr.test.
Acked-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Song Liu <songliubraving@fb.com>
---
tools/lib/bpf/bpf.c | 13 ++++++++++++-
tools/lib/bpf/bpf.h | 11 +++++++++++
tools/lib/bpf/libbpf.map | 1 +
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 2baa1308737c8..3228dd60fa32f 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -684,7 +684,8 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
return ret;
}
-int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
+int bpf_prog_test_run_xattr_opts(struct bpf_prog_test_run_attr *test_attr,
+ const struct bpf_prog_test_run_opts *opts)
{
union bpf_attr attr;
int ret;
@@ -693,6 +694,11 @@ int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
return -EINVAL;
memset(&attr, 0, sizeof(attr));
+ if (opts) {
+ if (!OPTS_VALID(opts, bpf_prog_test_run_opts))
+ return -EINVAL;
+ attr.test.cpu_plus = opts->cpu_plus;
+ }
attr.test.prog_fd = test_attr->prog_fd;
attr.test.data_in = ptr_to_u64(test_attr->data_in);
attr.test.data_out = ptr_to_u64(test_attr->data_out);
@@ -712,6 +718,11 @@ int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
return ret;
}
+int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
+{
+ return bpf_prog_test_run_xattr_opts(test_attr, NULL);
+}
+
static int bpf_obj_get_next_id(__u32 start_id, __u32 *next_id, int cmd)
{
union bpf_attr attr;
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 8c1ac4b42f908..61318f47c8e1b 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -251,6 +251,17 @@ struct bpf_prog_bind_opts {
LIBBPF_API int bpf_prog_bind_map(int prog_fd, int map_fd,
const struct bpf_prog_bind_opts *opts);
+
+struct bpf_prog_test_run_opts {
+ size_t sz; /* size of this struct for forward/backward compatibility */
+ __u32 cpu_plus;
+};
+#define bpf_prog_test_run_opts__last_field cpu_plus
+
+LIBBPF_API
+int bpf_prog_test_run_xattr_opts(struct bpf_prog_test_run_attr *test_attr,
+ const struct bpf_prog_test_run_opts *opts);
+
#ifdef __cplusplus
} /* extern "C" */
#endif
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 5f054dadf0829..c84a8bec57634 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -303,6 +303,7 @@ LIBBPF_0.1.0 {
LIBBPF_0.2.0 {
global:
bpf_prog_bind_map;
+ bpf_prog_test_run_xattr_opts;
bpf_program__section_name;
perf_buffer__buffer_cnt;
perf_buffer__buffer_fd;
--
2.24.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 bpf-next 3/3] selftests/bpf: add raw_tp_test_run
2020-09-23 16:53 [PATCH v2 bpf-next 0/3] enable BPF_PROG_TEST_RUN for raw_tp Song Liu
2020-09-23 16:53 ` [PATCH v2 bpf-next 1/3] bpf: enable BPF_PROG_TEST_RUN for raw_tracepoint Song Liu
2020-09-23 16:54 ` [PATCH v2 bpf-next 2/3] libbpf: introduce bpf_prog_test_run_xattr_opts Song Liu
@ 2020-09-23 16:54 ` Song Liu
2020-09-23 19:40 ` Andrii Nakryiko
2 siblings, 1 reply; 13+ messages in thread
From: Song Liu @ 2020-09-23 16:54 UTC (permalink / raw)
To: netdev, bpf; +Cc: kernel-team, ast, daniel, john.fastabend, kpsingh, Song Liu
This test runs test_run for raw_tracepoint program. The test covers ctx
input, retval output, and proper handling of cpu_plus field.
Signed-off-by: Song Liu <songliubraving@fb.com>
---
.../bpf/prog_tests/raw_tp_test_run.c | 73 +++++++++++++++++++
.../bpf/progs/test_raw_tp_test_run.c | 26 +++++++
2 files changed, 99 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/raw_tp_test_run.c
create mode 100644 tools/testing/selftests/bpf/progs/test_raw_tp_test_run.c
diff --git a/tools/testing/selftests/bpf/prog_tests/raw_tp_test_run.c b/tools/testing/selftests/bpf/prog_tests/raw_tp_test_run.c
new file mode 100644
index 0000000000000..3c6523b61afc1
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/raw_tp_test_run.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2019 Facebook */
+#include <test_progs.h>
+#include "bpf/libbpf_internal.h"
+#include "test_raw_tp_test_run.skel.h"
+
+static int duration;
+
+void test_raw_tp_test_run(void)
+{
+ struct bpf_prog_test_run_attr test_attr = {};
+ __u64 args[2] = {0x1234ULL, 0x5678ULL};
+ int comm_fd = -1, err, nr_online, i;
+ int expected_retval = 0x1234 + 0x5678;
+ struct test_raw_tp_test_run *skel;
+ char buf[] = "new_name";
+ bool *online = NULL;
+
+ err = parse_cpu_mask_file("/sys/devices/system/cpu/online", &online,
+ &nr_online);
+ if (CHECK(err, "parse_cpu_mask_file", "err %d\n", err))
+ return;
+
+ skel = test_raw_tp_test_run__open_and_load();
+ if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
+ return;
+ err = test_raw_tp_test_run__attach(skel);
+ if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
+ goto cleanup;
+
+ comm_fd = open("/proc/self/comm", O_WRONLY|O_TRUNC);
+ if (CHECK(comm_fd < 0, "open /proc/self/comm", "err %d\n", errno))
+ goto cleanup;
+
+ err = write(comm_fd, buf, sizeof(buf));
+ CHECK(err < 0, "task rename", "err %d", errno);
+
+ CHECK(skel->bss->count == 0, "check_count", "didn't increase\n");
+ CHECK(skel->data->on_cpu != 0xffffffff, "check_on_cpu", "got wrong value\n");
+
+ test_attr.prog_fd = bpf_program__fd(skel->progs.rename);
+ test_attr.ctx_in = args;
+ test_attr.ctx_size_in = sizeof(__u64);
+
+ err = bpf_prog_test_run_xattr(&test_attr);
+ CHECK(err == 0, "test_run", "should fail for too small ctx\n");
+
+ test_attr.ctx_size_in = sizeof(args);
+ err = bpf_prog_test_run_xattr(&test_attr);
+ CHECK(err < 0, "test_run", "err %d\n", errno);
+ CHECK(test_attr.retval != expected_retval, "check_retval",
+ "expect 0x%x, got 0x%x\n", expected_retval, test_attr.retval);
+
+ for (i = 0; i < nr_online; i++)
+ if (online[i]) {
+ DECLARE_LIBBPF_OPTS(bpf_prog_test_run_opts, opts,
+ .cpu_plus = i + 1,
+ );
+
+ test_attr.retval = 0;
+ err = bpf_prog_test_run_xattr_opts(&test_attr, &opts);
+ CHECK(err < 0, "test_run_with_opts", "err %d\n", errno);
+ CHECK(skel->data->on_cpu != i, "check_on_cpu",
+ "got wrong value\n");
+ CHECK(test_attr.retval != expected_retval,
+ "check_retval", "expect 0x%x, got 0x%x\n",
+ expected_retval, test_attr.retval);
+ }
+cleanup:
+ close(comm_fd);
+ test_raw_tp_test_run__destroy(skel);
+ free(online);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_raw_tp_test_run.c b/tools/testing/selftests/bpf/progs/test_raw_tp_test_run.c
new file mode 100644
index 0000000000000..9ceb648f096ea
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_raw_tp_test_run.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2020 Facebook */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+#include <bpf/bpf_tracing.h>
+
+__u32 count = 0;
+__u32 on_cpu = 0xffffffff;
+
+SEC("raw_tp/task_rename")
+int BPF_PROG(rename, struct task_struct *task, char *comm)
+{
+
+ count++;
+ if ((unsigned long long) task == 0x1234 &&
+ (unsigned long long) comm == 0x5678) {
+ on_cpu = bpf_get_smp_processor_id();
+ return (int)task + (int)comm;
+ }
+
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
--
2.24.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 bpf-next 2/3] libbpf: introduce bpf_prog_test_run_xattr_opts
2020-09-23 16:54 ` [PATCH v2 bpf-next 2/3] libbpf: introduce bpf_prog_test_run_xattr_opts Song Liu
@ 2020-09-23 19:31 ` Andrii Nakryiko
2020-09-23 22:04 ` Song Liu
2020-09-23 23:53 ` Song Liu
0 siblings, 2 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2020-09-23 19:31 UTC (permalink / raw)
To: Song Liu
Cc: Networking, bpf, Kernel Team, Alexei Starovoitov,
Daniel Borkmann, john fastabend, KP Singh
On Wed, Sep 23, 2020 at 9:55 AM Song Liu <songliubraving@fb.com> wrote:
>
> This API supports new field cpu_plus in bpf_attr.test.
>
> Acked-by: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
> tools/lib/bpf/bpf.c | 13 ++++++++++++-
> tools/lib/bpf/bpf.h | 11 +++++++++++
> tools/lib/bpf/libbpf.map | 1 +
> 3 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 2baa1308737c8..3228dd60fa32f 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -684,7 +684,8 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
> return ret;
> }
>
> -int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
> +int bpf_prog_test_run_xattr_opts(struct bpf_prog_test_run_attr *test_attr,
> + const struct bpf_prog_test_run_opts *opts)
opts are replacement for test_attr, not an addition to it. We chose to
use _xattr suffix for low-level APIs previously, but it's already
"taken". So I'd suggest to go with just bpf_prog_test_run_ops and
have prog_fd as a first argument and then put all the rest of
test_run_attr into opts.
BTW, it's also probably overdue to have a higher-level
bpf_program__test_run(), which can re-use the same
bpf_prog_test_run_opts options struct. It would be more convenient to
use it with libbpf bpf_object/bpf_program APIs.
> {
> union bpf_attr attr;
> int ret;
> @@ -693,6 +694,11 @@ int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
> return -EINVAL;
>
> memset(&attr, 0, sizeof(attr));
> + if (opts) {
you don't need to check opts for being not NULL, OPTS_VALID handle that already.
> + if (!OPTS_VALID(opts, bpf_prog_test_run_opts))
> + return -EINVAL;
> + attr.test.cpu_plus = opts->cpu_plus;
And here you should use OPTS_GET(), please see other examples in
libbpf for proper usage.
> + }
> attr.test.prog_fd = test_attr->prog_fd;
> attr.test.data_in = ptr_to_u64(test_attr->data_in);
> attr.test.data_out = ptr_to_u64(test_attr->data_out);
> @@ -712,6 +718,11 @@ int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
> return ret;
> }
>
[...]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 bpf-next 1/3] bpf: enable BPF_PROG_TEST_RUN for raw_tracepoint
2020-09-23 16:53 ` [PATCH v2 bpf-next 1/3] bpf: enable BPF_PROG_TEST_RUN for raw_tracepoint Song Liu
@ 2020-09-23 19:36 ` Andrii Nakryiko
2020-09-23 21:59 ` Song Liu
0 siblings, 1 reply; 13+ messages in thread
From: Andrii Nakryiko @ 2020-09-23 19:36 UTC (permalink / raw)
To: Song Liu
Cc: Networking, bpf, Kernel Team, Alexei Starovoitov,
Daniel Borkmann, john fastabend, KP Singh
On Wed, Sep 23, 2020 at 9:54 AM Song Liu <songliubraving@fb.com> wrote:
>
> Add .test_run for raw_tracepoint. Also, introduce a new feature that runs
> the target program on a specific CPU. This is achieved by a new flag in
> bpf_attr.test, cpu_plus. For compatibility, cpu_plus == 0 means run the
> program on current cpu, cpu_plus > 0 means run the program on cpu with id
> (cpu_plus - 1). This feature is needed for BPF programs that handle
> perf_event and other percpu resources, as the program can access these
> resource locally.
>
> Acked-by: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
> include/linux/bpf.h | 3 ++
> include/uapi/linux/bpf.h | 5 ++
> kernel/bpf/syscall.c | 2 +-
> kernel/trace/bpf_trace.c | 1 +
> net/bpf/test_run.c | 88 ++++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 5 ++
> 6 files changed, 103 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index d7c5a6ed87e30..23758c282eb4b 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1376,6 +1376,9 @@ int bpf_prog_test_run_tracing(struct bpf_prog *prog,
> int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
> const union bpf_attr *kattr,
> union bpf_attr __user *uattr);
> +int bpf_prog_test_run_raw_tp(struct bpf_prog *prog,
> + const union bpf_attr *kattr,
> + union bpf_attr __user *uattr);
> bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> const struct bpf_prog *prog,
> struct bpf_insn_access_aux *info);
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index a22812561064a..89acf41913e70 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -566,6 +566,11 @@ union bpf_attr {
> */
> __aligned_u64 ctx_in;
> __aligned_u64 ctx_out;
> + __u32 cpu_plus; /* run this program on cpu
> + * (cpu_plus - 1).
> + * If cpu_plus == 0, run on
> + * current cpu.
> + */
the "_plus" part of the name is so confusing, just as off-by-one
semantics.. Why not do what we do with BPF_PROG_ATTACH? I.e., we have
flags field, and if the specific bit is set then we use extra field's
value. In this case, you'd have:
__u32 flags;
__u32 cpu; /* naturally 0-based */
cpu indexing will be natural without any offsets, and you'll have
something like BPF_PROG_TEST_CPU flag, that needs to be specified.
This will work well with backward/forward compatibility. If you need a
special "current CPU" mode, you can achieve that by not specifying
BPF_PROG_TEST_CPU flag, or we can designate (__u32)-1 as a special
"current CPU" value.
WDYT?
> } test;
>
> struct { /* anonymous struct used by BPF_*_GET_*_ID */
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index ec68d3a23a2b7..4664531ff92ea 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2975,7 +2975,7 @@ static int bpf_prog_query(const union bpf_attr *attr,
> }
> }
>
> -#define BPF_PROG_TEST_RUN_LAST_FIELD test.ctx_out
> +#define BPF_PROG_TEST_RUN_LAST_FIELD test.cpu_plus
>
[...]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 bpf-next 3/3] selftests/bpf: add raw_tp_test_run
2020-09-23 16:54 ` [PATCH v2 bpf-next 3/3] selftests/bpf: add raw_tp_test_run Song Liu
@ 2020-09-23 19:40 ` Andrii Nakryiko
2020-09-23 21:30 ` Song Liu
0 siblings, 1 reply; 13+ messages in thread
From: Andrii Nakryiko @ 2020-09-23 19:40 UTC (permalink / raw)
To: Song Liu
Cc: Networking, bpf, Kernel Team, Alexei Starovoitov,
Daniel Borkmann, john fastabend, KP Singh
On Wed, Sep 23, 2020 at 9:55 AM Song Liu <songliubraving@fb.com> wrote:
>
> This test runs test_run for raw_tracepoint program. The test covers ctx
> input, retval output, and proper handling of cpu_plus field.
>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
> .../bpf/prog_tests/raw_tp_test_run.c | 73 +++++++++++++++++++
> .../bpf/progs/test_raw_tp_test_run.c | 26 +++++++
> 2 files changed, 99 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/raw_tp_test_run.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_raw_tp_test_run.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/raw_tp_test_run.c b/tools/testing/selftests/bpf/prog_tests/raw_tp_test_run.c
> new file mode 100644
> index 0000000000000..3c6523b61afc1
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/raw_tp_test_run.c
> @@ -0,0 +1,73 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2019 Facebook */
> +#include <test_progs.h>
> +#include "bpf/libbpf_internal.h"
> +#include "test_raw_tp_test_run.skel.h"
> +
> +static int duration;
> +
> +void test_raw_tp_test_run(void)
> +{
> + struct bpf_prog_test_run_attr test_attr = {};
> + __u64 args[2] = {0x1234ULL, 0x5678ULL};
> + int comm_fd = -1, err, nr_online, i;
> + int expected_retval = 0x1234 + 0x5678;
> + struct test_raw_tp_test_run *skel;
> + char buf[] = "new_name";
> + bool *online = NULL;
> +
> + err = parse_cpu_mask_file("/sys/devices/system/cpu/online", &online,
> + &nr_online);
> + if (CHECK(err, "parse_cpu_mask_file", "err %d\n", err))
> + return;
> +
> + skel = test_raw_tp_test_run__open_and_load();
> + if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
> + return;
leaking memory here
> + err = test_raw_tp_test_run__attach(skel);
> + if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
> + goto cleanup;
> +
> + comm_fd = open("/proc/self/comm", O_WRONLY|O_TRUNC);
> + if (CHECK(comm_fd < 0, "open /proc/self/comm", "err %d\n", errno))
> + goto cleanup;
> +
[...]
> +SEC("raw_tp/task_rename")
> +int BPF_PROG(rename, struct task_struct *task, char *comm)
> +{
> +
> + count++;
> + if ((unsigned long long) task == 0x1234 &&
> + (unsigned long long) comm == 0x5678) {
you can use shorter __u64?
> + on_cpu = bpf_get_smp_processor_id();
> + return (int)task + (int)comm;
> + }
> +
> + return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 bpf-next 3/3] selftests/bpf: add raw_tp_test_run
2020-09-23 19:40 ` Andrii Nakryiko
@ 2020-09-23 21:30 ` Song Liu
0 siblings, 0 replies; 13+ messages in thread
From: Song Liu @ 2020-09-23 21:30 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Networking, bpf, Kernel Team, Alexei Starovoitov,
Daniel Borkmann, john fastabend, KP Singh
> On Sep 23, 2020, at 12:40 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Sep 23, 2020 at 9:55 AM Song Liu <songliubraving@fb.com> wrote:
>>
>> This test runs test_run for raw_tracepoint program. The test covers ctx
>> input, retval output, and proper handling of cpu_plus field.
>>
>> Signed-off-by: Song Liu <songliubraving@fb.com>
>> ---
>> .../bpf/prog_tests/raw_tp_test_run.c | 73 +++++++++++++++++++
>> .../bpf/progs/test_raw_tp_test_run.c | 26 +++++++
>> 2 files changed, 99 insertions(+)
>> create mode 100644 tools/testing/selftests/bpf/prog_tests/raw_tp_test_run.c
>> create mode 100644 tools/testing/selftests/bpf/progs/test_raw_tp_test_run.c
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/raw_tp_test_run.c b/tools/testing/selftests/bpf/prog_tests/raw_tp_test_run.c
>> new file mode 100644
>> index 0000000000000..3c6523b61afc1
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/raw_tp_test_run.c
>> @@ -0,0 +1,73 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/* Copyright (c) 2019 Facebook */
>> +#include <test_progs.h>
>> +#include "bpf/libbpf_internal.h"
>> +#include "test_raw_tp_test_run.skel.h"
>> +
>> +static int duration;
>> +
>> +void test_raw_tp_test_run(void)
>> +{
>> + struct bpf_prog_test_run_attr test_attr = {};
>> + __u64 args[2] = {0x1234ULL, 0x5678ULL};
>> + int comm_fd = -1, err, nr_online, i;
>> + int expected_retval = 0x1234 + 0x5678;
>> + struct test_raw_tp_test_run *skel;
>> + char buf[] = "new_name";
>> + bool *online = NULL;
>> +
>> + err = parse_cpu_mask_file("/sys/devices/system/cpu/online", &online,
>> + &nr_online);
>> + if (CHECK(err, "parse_cpu_mask_file", "err %d\n", err))
>> + return;
>> +
>> + skel = test_raw_tp_test_run__open_and_load();
>> + if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
>> + return;
>
> leaking memory here
Good catch! Fixing it in the next version.
>
>> + err = test_raw_tp_test_run__attach(skel);
>> + if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
>> + goto cleanup;
>> +
>> + comm_fd = open("/proc/self/comm", O_WRONLY|O_TRUNC);
>> + if (CHECK(comm_fd < 0, "open /proc/self/comm", "err %d\n", errno))
>> + goto cleanup;
>> +
>
> [...]
>
>> +SEC("raw_tp/task_rename")
>> +int BPF_PROG(rename, struct task_struct *task, char *comm)
>> +{
>> +
>> + count++;
>> + if ((unsigned long long) task == 0x1234 &&
>> + (unsigned long long) comm == 0x5678) {
>
> you can use shorter __u64?
Sure.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 bpf-next 1/3] bpf: enable BPF_PROG_TEST_RUN for raw_tracepoint
2020-09-23 19:36 ` Andrii Nakryiko
@ 2020-09-23 21:59 ` Song Liu
0 siblings, 0 replies; 13+ messages in thread
From: Song Liu @ 2020-09-23 21:59 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Networking, bpf, Kernel Team, Alexei Starovoitov,
Daniel Borkmann, john fastabend, KP Singh
> On Sep 23, 2020, at 12:36 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Sep 23, 2020 at 9:54 AM Song Liu <songliubraving@fb.com> wrote:
>>
>> Add .test_run for raw_tracepoint. Also, introduce a new feature that runs
>> the target program on a specific CPU. This is achieved by a new flag in
>> bpf_attr.test, cpu_plus. For compatibility, cpu_plus == 0 means run the
>> program on current cpu, cpu_plus > 0 means run the program on cpu with id
>> (cpu_plus - 1). This feature is needed for BPF programs that handle
>> perf_event and other percpu resources, as the program can access these
>> resource locally.
>>
>> Acked-by: John Fastabend <john.fastabend@gmail.com>
>> Signed-off-by: Song Liu <songliubraving@fb.com>
>> ---
>> include/linux/bpf.h | 3 ++
>> include/uapi/linux/bpf.h | 5 ++
>> kernel/bpf/syscall.c | 2 +-
>> kernel/trace/bpf_trace.c | 1 +
>> net/bpf/test_run.c | 88 ++++++++++++++++++++++++++++++++++
>> tools/include/uapi/linux/bpf.h | 5 ++
>> 6 files changed, 103 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index d7c5a6ed87e30..23758c282eb4b 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -1376,6 +1376,9 @@ int bpf_prog_test_run_tracing(struct bpf_prog *prog,
>> int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
>> const union bpf_attr *kattr,
>> union bpf_attr __user *uattr);
>> +int bpf_prog_test_run_raw_tp(struct bpf_prog *prog,
>> + const union bpf_attr *kattr,
>> + union bpf_attr __user *uattr);
>> bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>> const struct bpf_prog *prog,
>> struct bpf_insn_access_aux *info);
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index a22812561064a..89acf41913e70 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -566,6 +566,11 @@ union bpf_attr {
>> */
>> __aligned_u64 ctx_in;
>> __aligned_u64 ctx_out;
>> + __u32 cpu_plus; /* run this program on cpu
>> + * (cpu_plus - 1).
>> + * If cpu_plus == 0, run on
>> + * current cpu.
>> + */
>
> the "_plus" part of the name is so confusing, just as off-by-one
> semantics.. Why not do what we do with BPF_PROG_ATTACH? I.e., we have
> flags field, and if the specific bit is set then we use extra field's
> value. In this case, you'd have:
>
> __u32 flags;
> __u32 cpu; /* naturally 0-based */
>
> cpu indexing will be natural without any offsets, and you'll have
> something like BPF_PROG_TEST_CPU flag, that needs to be specified.
> This will work well with backward/forward compatibility. If you need a
> special "current CPU" mode, you can achieve that by not specifying
> BPF_PROG_TEST_CPU flag, or we can designate (__u32)-1 as a special
> "current CPU" value.
>
> WDYT?
Yes, we can add a flag here. If there was already a flags field in
bpf_attr.test, I would have gone that way in the first place.
Thanks,
Song
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 bpf-next 2/3] libbpf: introduce bpf_prog_test_run_xattr_opts
2020-09-23 19:31 ` Andrii Nakryiko
@ 2020-09-23 22:04 ` Song Liu
2020-09-23 23:53 ` Song Liu
1 sibling, 0 replies; 13+ messages in thread
From: Song Liu @ 2020-09-23 22:04 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Networking, bpf, Kernel Team, Alexei Starovoitov,
Daniel Borkmann, john fastabend, KP Singh
> On Sep 23, 2020, at 12:31 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Sep 23, 2020 at 9:55 AM Song Liu <songliubraving@fb.com> wrote:
>>
>> This API supports new field cpu_plus in bpf_attr.test.
>>
>> Acked-by: John Fastabend <john.fastabend@gmail.com>
>> Signed-off-by: Song Liu <songliubraving@fb.com>
>> ---
>> tools/lib/bpf/bpf.c | 13 ++++++++++++-
>> tools/lib/bpf/bpf.h | 11 +++++++++++
>> tools/lib/bpf/libbpf.map | 1 +
>> 3 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>> index 2baa1308737c8..3228dd60fa32f 100644
>> --- a/tools/lib/bpf/bpf.c
>> +++ b/tools/lib/bpf/bpf.c
>> @@ -684,7 +684,8 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
>> return ret;
>> }
>>
>> -int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
>> +int bpf_prog_test_run_xattr_opts(struct bpf_prog_test_run_attr *test_attr,
>> + const struct bpf_prog_test_run_opts *opts)
>
> opts are replacement for test_attr, not an addition to it. We chose to
> use _xattr suffix for low-level APIs previously, but it's already
> "taken". So I'd suggest to go with just bpf_prog_test_run_ops and
> have prog_fd as a first argument and then put all the rest of
> test_run_attr into opts.
Sounds good. I will update it this way.
[...]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 bpf-next 2/3] libbpf: introduce bpf_prog_test_run_xattr_opts
2020-09-23 19:31 ` Andrii Nakryiko
2020-09-23 22:04 ` Song Liu
@ 2020-09-23 23:53 ` Song Liu
2020-09-24 1:11 ` Andrii Nakryiko
1 sibling, 1 reply; 13+ messages in thread
From: Song Liu @ 2020-09-23 23:53 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Networking, bpf, Kernel Team, Alexei Starovoitov,
Daniel Borkmann, john fastabend, KP Singh
> On Sep 23, 2020, at 12:31 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Sep 23, 2020 at 9:55 AM Song Liu <songliubraving@fb.com> wrote:
>>
>> This API supports new field cpu_plus in bpf_attr.test.
>>
>> Acked-by: John Fastabend <john.fastabend@gmail.com>
>> Signed-off-by: Song Liu <songliubraving@fb.com>
>> ---
>> tools/lib/bpf/bpf.c | 13 ++++++++++++-
>> tools/lib/bpf/bpf.h | 11 +++++++++++
>> tools/lib/bpf/libbpf.map | 1 +
>> 3 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>> index 2baa1308737c8..3228dd60fa32f 100644
>> --- a/tools/lib/bpf/bpf.c
>> +++ b/tools/lib/bpf/bpf.c
>> @@ -684,7 +684,8 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
>> return ret;
>> }
>>
>> -int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
>> +int bpf_prog_test_run_xattr_opts(struct bpf_prog_test_run_attr *test_attr,
>> + const struct bpf_prog_test_run_opts *opts)
>
> opts are replacement for test_attr, not an addition to it. We chose to
> use _xattr suffix for low-level APIs previously, but it's already
> "taken". So I'd suggest to go with just bpf_prog_test_run_ops and
> have prog_fd as a first argument and then put all the rest of
> test_run_attr into opts.
One question on this: from the code, most (if not all) of these xxx_opts
are used as input only. For example:
LIBBPF_API int bpf_prog_bind_map(int prog_fd, int map_fd,
const struct bpf_prog_bind_opts *opts);
However, bpf_prog_test_run_attr contains both input and output. Do you
have any concern we use bpf_prog_test_run_opts for both input and output?
Thanks,
Song
> BTW, it's also probably overdue to have a higher-level
> bpf_program__test_run(), which can re-use the same
> bpf_prog_test_run_opts options struct. It would be more convenient to
> use it with libbpf bpf_object/bpf_program APIs.
>
>> {
>> union bpf_attr attr;
>> int ret;
>> @@ -693,6 +694,11 @@ int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
>> return -EINVAL;
>>
>> memset(&attr, 0, sizeof(attr));
>> + if (opts) {
>
> you don't need to check opts for being not NULL, OPTS_VALID handle that already.
>
>> + if (!OPTS_VALID(opts, bpf_prog_test_run_opts))
>> + return -EINVAL;
>> + attr.test.cpu_plus = opts->cpu_plus;
>
> And here you should use OPTS_GET(), please see other examples in
> libbpf for proper usage.
>
>
>> + }
>> attr.test.prog_fd = test_attr->prog_fd;
>> attr.test.data_in = ptr_to_u64(test_attr->data_in);
>> attr.test.data_out = ptr_to_u64(test_attr->data_out);
>> @@ -712,6 +718,11 @@ int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
>> return ret;
>> }
>>
>
> [...]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 bpf-next 2/3] libbpf: introduce bpf_prog_test_run_xattr_opts
2020-09-23 23:53 ` Song Liu
@ 2020-09-24 1:11 ` Andrii Nakryiko
2020-09-24 1:20 ` Song Liu
0 siblings, 1 reply; 13+ messages in thread
From: Andrii Nakryiko @ 2020-09-24 1:11 UTC (permalink / raw)
To: Song Liu
Cc: Networking, bpf, Kernel Team, Alexei Starovoitov,
Daniel Borkmann, john fastabend, KP Singh
On Wed, Sep 23, 2020 at 4:54 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Sep 23, 2020, at 12:31 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > On Wed, Sep 23, 2020 at 9:55 AM Song Liu <songliubraving@fb.com> wrote:
> >>
> >> This API supports new field cpu_plus in bpf_attr.test.
> >>
> >> Acked-by: John Fastabend <john.fastabend@gmail.com>
> >> Signed-off-by: Song Liu <songliubraving@fb.com>
> >> ---
> >> tools/lib/bpf/bpf.c | 13 ++++++++++++-
> >> tools/lib/bpf/bpf.h | 11 +++++++++++
> >> tools/lib/bpf/libbpf.map | 1 +
> >> 3 files changed, 24 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> >> index 2baa1308737c8..3228dd60fa32f 100644
> >> --- a/tools/lib/bpf/bpf.c
> >> +++ b/tools/lib/bpf/bpf.c
> >> @@ -684,7 +684,8 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
> >> return ret;
> >> }
> >>
> >> -int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
> >> +int bpf_prog_test_run_xattr_opts(struct bpf_prog_test_run_attr *test_attr,
> >> + const struct bpf_prog_test_run_opts *opts)
> >
> > opts are replacement for test_attr, not an addition to it. We chose to
> > use _xattr suffix for low-level APIs previously, but it's already
> > "taken". So I'd suggest to go with just bpf_prog_test_run_ops and
> > have prog_fd as a first argument and then put all the rest of
> > test_run_attr into opts.
>
> One question on this: from the code, most (if not all) of these xxx_opts
> are used as input only. For example:
>
> LIBBPF_API int bpf_prog_bind_map(int prog_fd, int map_fd,
> const struct bpf_prog_bind_opts *opts);
>
> However, bpf_prog_test_run_attr contains both input and output. Do you
> have any concern we use bpf_prog_test_run_opts for both input and output?
>
I think it should be ok. opts are about passing optional things in a
way that would be backward/forward compatible. Whether it's input
only, output only, or input/output is secondary. We haven't had a need
for output params yet, so this will be the first, but I think it fits
here just fine. Just document it in the struct definition clearly and
that's it. As for the mechanics, we might want to do OPTS_SET() macro,
that will set some fields only if the user provided enough memory to
fir that output parameter. That should work here pretty cleanly,
right?
> Thanks,
> Song
>
>
> > BTW, it's also probably overdue to have a higher-level
> > bpf_program__test_run(), which can re-use the same
> > bpf_prog_test_run_opts options struct. It would be more convenient to
> > use it with libbpf bpf_object/bpf_program APIs.
> >
> >> {
> >> union bpf_attr attr;
> >> int ret;
> >> @@ -693,6 +694,11 @@ int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
> >> return -EINVAL;
> >>
> >> memset(&attr, 0, sizeof(attr));
> >> + if (opts) {
> >
> > you don't need to check opts for being not NULL, OPTS_VALID handle that already.
> >
> >> + if (!OPTS_VALID(opts, bpf_prog_test_run_opts))
> >> + return -EINVAL;
> >> + attr.test.cpu_plus = opts->cpu_plus;
> >
> > And here you should use OPTS_GET(), please see other examples in
> > libbpf for proper usage.
> >
> >
> >> + }
> >> attr.test.prog_fd = test_attr->prog_fd;
> >> attr.test.data_in = ptr_to_u64(test_attr->data_in);
> >> attr.test.data_out = ptr_to_u64(test_attr->data_out);
> >> @@ -712,6 +718,11 @@ int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
> >> return ret;
> >> }
> >>
> >
> > [...]
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 bpf-next 2/3] libbpf: introduce bpf_prog_test_run_xattr_opts
2020-09-24 1:11 ` Andrii Nakryiko
@ 2020-09-24 1:20 ` Song Liu
0 siblings, 0 replies; 13+ messages in thread
From: Song Liu @ 2020-09-24 1:20 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Networking, bpf, Kernel Team, Alexei Starovoitov,
Daniel Borkmann, john fastabend, KP Singh
> On Sep 23, 2020, at 6:11 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Sep 23, 2020 at 4:54 PM Song Liu <songliubraving@fb.com> wrote:
>>
>>
>>
>>> On Sep 23, 2020, at 12:31 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>>>
>>> On Wed, Sep 23, 2020 at 9:55 AM Song Liu <songliubraving@fb.com> wrote:
>>>>
>>>> This API supports new field cpu_plus in bpf_attr.test.
>>>>
>>>> Acked-by: John Fastabend <john.fastabend@gmail.com>
>>>> Signed-off-by: Song Liu <songliubraving@fb.com>
>>>> ---
>>>> tools/lib/bpf/bpf.c | 13 ++++++++++++-
>>>> tools/lib/bpf/bpf.h | 11 +++++++++++
>>>> tools/lib/bpf/libbpf.map | 1 +
>>>> 3 files changed, 24 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>>>> index 2baa1308737c8..3228dd60fa32f 100644
>>>> --- a/tools/lib/bpf/bpf.c
>>>> +++ b/tools/lib/bpf/bpf.c
>>>> @@ -684,7 +684,8 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
>>>> return ret;
>>>> }
>>>>
>>>> -int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
>>>> +int bpf_prog_test_run_xattr_opts(struct bpf_prog_test_run_attr *test_attr,
>>>> + const struct bpf_prog_test_run_opts *opts)
>>>
>>> opts are replacement for test_attr, not an addition to it. We chose to
>>> use _xattr suffix for low-level APIs previously, but it's already
>>> "taken". So I'd suggest to go with just bpf_prog_test_run_ops and
>>> have prog_fd as a first argument and then put all the rest of
>>> test_run_attr into opts.
>>
>> One question on this: from the code, most (if not all) of these xxx_opts
>> are used as input only. For example:
>>
>> LIBBPF_API int bpf_prog_bind_map(int prog_fd, int map_fd,
>> const struct bpf_prog_bind_opts *opts);
>>
>> However, bpf_prog_test_run_attr contains both input and output. Do you
>> have any concern we use bpf_prog_test_run_opts for both input and output?
>>
>
> I think it should be ok. opts are about passing optional things in a
> way that would be backward/forward compatible. Whether it's input
> only, output only, or input/output is secondary. We haven't had a need
> for output params yet, so this will be the first, but I think it fits
> here just fine. Just document it in the struct definition clearly and
> that's it. As for the mechanics, we might want to do OPTS_SET() macro,
> that will set some fields only if the user provided enough memory to
> fir that output parameter. That should work here pretty cleanly,
> right?
Yep, just sent v4 with OPTS_SET(). ;)
Thanks,
Song
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-09-24 1:40 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-23 16:53 [PATCH v2 bpf-next 0/3] enable BPF_PROG_TEST_RUN for raw_tp Song Liu
2020-09-23 16:53 ` [PATCH v2 bpf-next 1/3] bpf: enable BPF_PROG_TEST_RUN for raw_tracepoint Song Liu
2020-09-23 19:36 ` Andrii Nakryiko
2020-09-23 21:59 ` Song Liu
2020-09-23 16:54 ` [PATCH v2 bpf-next 2/3] libbpf: introduce bpf_prog_test_run_xattr_opts Song Liu
2020-09-23 19:31 ` Andrii Nakryiko
2020-09-23 22:04 ` Song Liu
2020-09-23 23:53 ` Song Liu
2020-09-24 1:11 ` Andrii Nakryiko
2020-09-24 1:20 ` Song Liu
2020-09-23 16:54 ` [PATCH v2 bpf-next 3/3] selftests/bpf: add raw_tp_test_run Song Liu
2020-09-23 19:40 ` Andrii Nakryiko
2020-09-23 21:30 ` Song Liu
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.