Re: [PATCH bpf-next 2/2] libbpf: support new uapi for map element bpf iterator

[Yonghong Song <yhs@fb.com>]

On 8/2/20 6:35 PM, Andrii Nakryiko wrote:
> On Sat, Aug 1, 2020 at 9:22 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> Previous commit adjusted kernel uapi for map
>> element bpf iterator. This patch adjusted libbpf API
>> due to uapi change.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   tools/lib/bpf/bpf.c    | 4 +++-
>>   tools/lib/bpf/bpf.h    | 5 +++--
>>   tools/lib/bpf/libbpf.c | 7 +++++--
>>   3 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>> index eab14c97c15d..c75a84398d51 100644
>> --- a/tools/lib/bpf/bpf.c
>> +++ b/tools/lib/bpf/bpf.c
>> @@ -598,7 +598,9 @@ int bpf_link_create(int prog_fd, int target_fd,
>>          attr.link_create.prog_fd = prog_fd;
>>          attr.link_create.target_fd = target_fd;
>>          attr.link_create.attach_type = attach_type;
>> -       attr.link_create.flags = OPTS_GET(opts, flags, 0);
>> +       attr.link_create.iter_info =
>> +               ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0));
>> +       attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0);
>>
>>          return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr));
>>   }
>> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
>> index 28855fd5b5f4..c9895f191305 100644
>> --- a/tools/lib/bpf/bpf.h
>> +++ b/tools/lib/bpf/bpf.h
>> @@ -170,9 +170,10 @@ LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
>>
>>   struct bpf_link_create_opts {
>>          size_t sz; /* size of this struct for forward/backward compatibility */
>> -       __u32 flags;
> 
> I'd actually keep flags in link_create_ops, as it's part of the kernel
> UAPI anyways, we won't have to add it later. Just pass it through into
> bpf_attr.

Okay. Will keep it.

> 
>> +       union bpf_iter_link_info *iter_info;
>> +       __u32 iter_info_len;
>>   };
>> -#define bpf_link_create_opts__last_field flags
>> +#define bpf_link_create_opts__last_field iter_info_len
>>
>>   LIBBPF_API int bpf_link_create(int prog_fd, int target_fd,
>>                                 enum bpf_attach_type attach_type,
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 7be04e45d29c..dc8fabf9d30d 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -8298,6 +8298,7 @@ bpf_program__attach_iter(struct bpf_program *prog,
>>                           const struct bpf_iter_attach_opts *opts)
>>   {
>>          DECLARE_LIBBPF_OPTS(bpf_link_create_opts, link_create_opts);
>> +       union bpf_iter_link_info linfo;
>>          char errmsg[STRERR_BUFSIZE];
>>          struct bpf_link *link;
>>          int prog_fd, link_fd;
>> @@ -8307,8 +8308,10 @@ bpf_program__attach_iter(struct bpf_program *prog,
>>                  return ERR_PTR(-EINVAL);
>>
>>          if (OPTS_HAS(opts, map_fd)) {
>> -               target_fd = opts->map_fd;
>> -               link_create_opts.flags = BPF_ITER_LINK_MAP_FD;
>> +               memset(&linfo, 0, sizeof(linfo));
>> +               linfo.map.map_fd = opts->map_fd;
>> +               link_create_opts.iter_info = &linfo;
>> +               link_create_opts.iter_info_len = sizeof(linfo);
> 
> Maybe instead of having map_fd directly in bpf_iter_attach_opts, let's
> just accept bpf_iter_link_info and its len directly from the user?
> Right now kernel UAPI and libbpf API for customizing iterator
> attachment differ. It would be simpler to keep them in sync and we
> won't have to discuss how to evolve bpf_iter_attach_opts as we add
> more customization for different types of iterators. Thoughts?

Good suggestion. Previously we don't have a structure to encapsulate
map_fd so map_fd is added to the bpf_iter_attach_opts. Indeed, we can
directly add bpf_iter_link_info to link_iter_attach_opts, and this
will free libbpf from handling any future additions in bpf_iter_link_info.

> 
>>          }
>>
>>          prog_fd = bpf_program__fd(prog);
>> --
>> 2.24.1
>>