| Message ID | 20200923165401.2284447-1-songliubraving@fb.com |
|---|---|
| Headers | show |
| Series | enable BPF_PROG_TEST_RUN for raw_tp | expand |
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; > } > [...]
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 > [...]
> 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
> 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. [...]
> 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; >> } >> > > [...]
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; > >> } > >> > > > > [...] >
> 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