Re: [PATCH bpf-next 4/7] libbpf: Create a bpf_link in bpf_map__attach_struct_ops().

[Kui-Feng Lee <sinquersw@gmail.com>]

On 2/16/23 14:40, Andrii Nakryiko wrote:
> On Tue, Feb 14, 2023 at 2:17 PM Kui-Feng Lee <kuifeng@meta.com> wrote:
>>
>> bpf_map__attach_struct_ops() was creating a dummy bpf_link as a
>> placeholder, but now it is constructing an authentic one by calling
>> bpf_link_create() if the map has the BPF_F_LINK flag.
>>
>> You can flag a struct_ops map with BPF_F_LINK by calling
>> bpf_map__set_map_flags().
>>
>> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
>> ---
>>   tools/lib/bpf/libbpf.c | 73 +++++++++++++++++++++++++++++++++---------
>>   1 file changed, 58 insertions(+), 15 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 75ed95b7e455..1eff6a03ddd9 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -11430,29 +11430,41 @@ struct bpf_link *bpf_program__attach(const struct bpf_program *prog)
>>          return link;
>>   }
>>
>> +struct bpf_link_struct_ops_map {
> 
> let's drop the "_map" suffix? struct_ops is always a map, so no need
> to point this out

Sure!

> 
>> +       struct bpf_link link;
>> +       int map_fd;
>> +};
>> +
>>   static int bpf_link__detach_struct_ops(struct bpf_link *link)
>>   {
>> +       struct bpf_link_struct_ops_map *st_link;
>>          __u32 zero = 0;
>>
>> -       if (bpf_map_delete_elem(link->fd, &zero))
>> +       st_link = container_of(link, struct bpf_link_struct_ops_map, link);
>> +
>> +       if (st_link->map_fd < 0) {
>> +               /* Fake bpf_link */
>> +               if (bpf_map_delete_elem(link->fd, &zero))
>> +                       return -errno;
>> +               return 0;
>> +       }
>> +
>> +       if (bpf_map_delete_elem(st_link->map_fd, &zero))
>> +               return -errno;
>> +
>> +       if (close(link->fd))
>>                  return -errno;
>>
>>          return 0;
>>   }
>>
>> -struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map)
>> +/*
>> + * Update the map with the prepared vdata.
>> + */
>> +static int bpf_map__update_vdata(const struct bpf_map *map)
> 
> this is internal helper, so let's not use double underscores, just
> bpf_map_update_vdata()

Ok!

> 
>>   {
>>          struct bpf_struct_ops *st_ops;
>> -       struct bpf_link *link;
>>          __u32 i, zero = 0;
>> -       int err;
>> -
>> -       if (!bpf_map__is_struct_ops(map) || map->fd == -1)
>> -               return libbpf_err_ptr(-EINVAL);
>> -
>> -       link = calloc(1, sizeof(*link));
>> -       if (!link)
>> -               return libbpf_err_ptr(-EINVAL);
>>
>>          st_ops = map->st_ops;
>>          for (i = 0; i < btf_vlen(st_ops->type); i++) {
>> @@ -11468,17 +11480,48 @@ struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map)
>>                  *(unsigned long *)kern_data = prog_fd;
>>          }
>>
>> -       err = bpf_map_update_elem(map->fd, &zero, st_ops->kern_vdata, 0);
>> +       return bpf_map_update_elem(map->fd, &zero, st_ops->kern_vdata, 0);
>> +}
>> +
>> +struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map)
>> +{
>> +       struct bpf_link_struct_ops_map *link;
>> +       int err, fd;
>> +
>> +       if (!bpf_map__is_struct_ops(map) || map->fd == -1)
>> +               return libbpf_err_ptr(-EINVAL);
>> +
>> +       link = calloc(1, sizeof(*link));
>> +       if (!link)
>> +               return libbpf_err_ptr(-EINVAL);
>> +
>> +       err = bpf_map__update_vdata(map);
>>          if (err) {
>>                  err = -errno;
>>                  free(link);
>>                  return libbpf_err_ptr(err);
>>          }
>>
>> -       link->detach = bpf_link__detach_struct_ops;
>> -       link->fd = map->fd;
>> +       link->link.detach = bpf_link__detach_struct_ops;
>>
>> -       return link;
>> +       if (!(map->def.map_flags & BPF_F_LINK)) {
> 
> So this will always require a programmatic bpf_map__set_map_flags()
> call, there is currently no declarative way to do this, right?
> 
> Is there any way to avoid this BPF_F_LINK flag approach? How bad would
> it be if kernel just always created bpf_link-backed struct_ops?
> 
> Alternatively, should we think about SEC(".struct_ops.link") or
> something like that to instruct libbpf to add this BPF_F_LINK flag
> automatically?

Agree!

The other solution is to add a flag when declare a struct_ops.

  SEC(".struct_ops")
  tcp_congestion_ops ops = {
      ...
      .flags = WITH_LINK,
  }


> 
>> +               /* Fake bpf_link */
>> +               link->link.fd = map->fd;
>> +               link->map_fd = -1;
>> +               return &link->link;
>> +       }
>> +