On Tue, Feb 14, 2023 at 2:17 PM Kui-Feng Lee <kuifeng@xxxxxxxx> 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@xxxxxxxx>
> ---
> 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
> + 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()
> {
> 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?
> + /* Fake bpf_link */
> + link->link.fd = map->fd;
> + link->map_fd = -1;
> + return &link->link;
> + }
> +
