| Message ID | 20200923165401.2284447-3-songliubraving@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| 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 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
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;