Re: [PATCH v2 bpf-next 2/3] libbpf: introduce bpf_prog_test_run_xattr_opts

On Wed, Sep 23, 2020 at 4:54 PM Song Liu [off-list ref] wrote:

quoted

On Sep 23, 2020, at 12:31 PM, Andrii Nakryiko [off-list ref] wrote:

On Wed, Sep 23, 2020 at 9:55 AM Song Liu [off-list ref] wrote:

quoted

This API supports new field cpu_plus in bpf_attr.test.

Acked-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Song Liu <redacted>
---
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

quoted

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.

quoted

{
       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.

quoted

+               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.

quoted

+       }
       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;

}

[...]