> On Sep 23, 2020, at 6:11 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Wed, Sep 23, 2020 at 4:54 PM Song Liu <songliubraving@xxxxxx> wrote: >> >> >> >>> On Sep 23, 2020, at 12:31 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: >>> >>> On Wed, Sep 23, 2020 at 9:55 AM Song Liu <songliubraving@xxxxxx> wrote: >>>> >>>> This API supports new field cpu_plus in bpf_attr.test. >>>> >>>> Acked-by: John Fastabend <john.fastabend@xxxxxxxxx> >>>> Signed-off-by: Song Liu <songliubraving@xxxxxx> >>>> --- >>>> 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
