Re: [PATCH bpf-next 6/7] libbpf: Update a bpf_link with another struct_ops.

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

On 2/16/23 14:48, Andrii Nakryiko wrote:
> On Tue, Feb 14, 2023 at 2:17 PM Kui-Feng Lee <kuifeng@meta.com> wrote:
>>
>> Introduce bpf_link__update_struct_ops(), which will allow you to
>> effortlessly transition the struct_ops map of any given bpf_link into
>> an alternative.
>>
>> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
>> ---
>>   tools/lib/bpf/libbpf.c   | 35 +++++++++++++++++++++++++++++++++++
>>   tools/lib/bpf/libbpf.h   |  1 +
>>   tools/lib/bpf/libbpf.map |  1 +
>>   3 files changed, 37 insertions(+)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 1eff6a03ddd9..6f7c72e312d4 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -11524,6 +11524,41 @@ struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map)
>>          return &link->link;
>>   }
>>
>> +/*
>> + * Swap the back struct_ops of a link with a new struct_ops map.
>> + */
>> +int bpf_link__update_struct_ops(struct bpf_link *link, const struct bpf_map *map)
> 
> we have bpf_link__update_program(), and so the generic counterpart for
> map-based links would be bpf_link__update_map(). Let's call it that.
> And it shouldn't probably assume so much struct_ops specific things.

Sure

> 
>> +{
>> +       struct bpf_link_struct_ops_map *st_ops_link;
>> +       int err, fd;
>> +
>> +       if (!bpf_map__is_struct_ops(map) || map->fd == -1)
>> +               return -EINVAL;
>> +
>> +       /* Ensure the type of a link is correct */
>> +       if (link->detach != bpf_link__detach_struct_ops)
>> +               return -EINVAL;
>> +
>> +       err = bpf_map__update_vdata(map);
> 
> it's a bit weird we do this at attach time, not when bpf_map is
> actually instantiated. Should we move this map contents initialization
> to bpf_object__load() phase? Same for bpf_map__attach_struct_ops().
> What do we lose by doing it after all the BPF programs are loaded in
> load phase?

With the current behavior (w/o links), a struct_ops will be registered 
when updating its value.  If we move bpf_map__update_vdata() to 
bpf_object__load(), a congestion control algorithm will be activated at 
the moment loading it before attaching it.  However, we should activate 
an algorithm at attach time.


> 
>> +       if (err) {
>> +               err = -errno;
>> +               free(link);
>> +               return err;
>> +       }
>> +
>> +       fd = bpf_link_update(link->fd, map->fd, NULL);
>> +       if (fd < 0) {
>> +               err = -errno;
>> +               free(link);
>> +               return err;
>> +       }
>> +
>> +       st_ops_link = container_of(link, struct bpf_link_struct_ops_map, link);
>> +       st_ops_link->map_fd = map->fd;
>> +
>> +       return 0;
>> +}
>> +
>>   typedef enum bpf_perf_event_ret (*bpf_perf_event_print_t)(struct perf_event_header *hdr,
>>                                                            void *private_data);
>>
>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> index 2efd80f6f7b9..dd25cd6759d4 100644
>> --- a/tools/lib/bpf/libbpf.h
>> +++ b/tools/lib/bpf/libbpf.h
>> @@ -695,6 +695,7 @@ bpf_program__attach_freplace(const struct bpf_program *prog,
>>   struct bpf_map;
>>
>>   LIBBPF_API struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map);
>> +LIBBPF_API int bpf_link__update_struct_ops(struct bpf_link *link, const struct bpf_map *map);
> 
> let's rename to bpf_link__update_map() and put it next to
> bpf_link__update_program() in libbpf.h
> 
>>
>>   struct bpf_iter_attach_opts {
>>          size_t sz; /* size of this struct for forward/backward compatibility */
>> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
>> index 11c36a3c1a9f..ca6993c744b6 100644
>> --- a/tools/lib/bpf/libbpf.map
>> +++ b/tools/lib/bpf/libbpf.map
>> @@ -373,6 +373,7 @@ LIBBPF_1.1.0 {
>>          global:
>>                  bpf_btf_get_fd_by_id_opts;
>>                  bpf_link_get_fd_by_id_opts;
>> +               bpf_link__update_struct_ops;
>>                  bpf_map_get_fd_by_id_opts;
>>                  bpf_prog_get_fd_by_id_opts;
>>                  user_ring_buffer__discard;
> 
> we are in LIBBPF_1.2.0 already, please move
> 
> 
>> --
>> 2.30.2
>>