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/15/23 10:48, Stanislav Fomichev wrote:
> On Wed, Feb 15, 2023 at 10:44 AM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>>
>>
>> On 2/14/23 18:58, Stanislav Fomichev wrote:
>>> On 02/14, Kui-Feng Lee 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 {
>>>> +    struct bpf_link link;
>>>> +    int map_fd;
>>>> +};
>>>
>>> Ah, ok, now you're adding bpf_link_struct_ops_map. I guess I'm now
>>> confused why you haven't done it in the first patch :-/
>>
>> Just won't to mix the libbpf part and kernel part in one patch.
> 
> Ah, shoot, I completely missed that it's libbpf. So in this case, can
> we use the same strategy for the kernel links? Instead of a union,
> have an outer struct + container_of? If not, why not?

The reason I use `container_of` here is we need both FDs in libbpf to 
keep it as consistent with its existing behavior as possible.  The value 
of the struct_ops map should be deleted if a bpf_link is detached.

Back to your question.  We can go the `container_of` approach.  Only 
concern I have is additional few bytes although it is not a big issue. I 
will move to this approach in the next version.

> 
> 
>>
>>>
>>> And what are these fake bpf_links? Can you share more about it means?
>>
>> For the next version, I will detail it in the commit log. In a nutshell,
>> before this point, there was no bpf_link for struct_ops. Libbpf
>> attempted to create an equivalent interface to other BPF programs by
>> providing a simulated bpf_link instead of a true one from the kernel;
>> that fake bpf_link stores FDs associated with struct_ops maps rather
>> than real bpf_links.
>>
>>
>>>
>>>> +
>>>>    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)
>>>>    {
>>>>        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)) {
>>>> +        /* Fake bpf_link */
>>>> +        link->link.fd = map->fd;
>>>> +        link->map_fd = -1;
>>>> +        return &link->link;
>>>> +    }
>>>> +
>>>> +    fd = bpf_link_create(map->fd, -1, BPF_STRUCT_OPS_MAP, NULL);
>>>> +    if (fd < 0) {
>>>> +        err = -errno;
>>>> +        free(link);
>>>> +        return libbpf_err_ptr(err);
>>>> +    }
>>>> +
>>>> +    link->link.fd = fd;
>>>> +    link->map_fd = map->fd;
>>>> +
>>>> +    return &link->link;
>>>>    }
>>>
>>>>    typedef enum bpf_perf_event_ret (*bpf_perf_event_print_t)(struct
>>>> perf_event_header *hdr,
>>>> --
>>>> 2.30.2
>>>