On Fri, Feb 17, 2023 at 4:05 PM Kui-Feng Lee <sinquersw@xxxxxxxxx> wrote:
>
>
>
> On 2/16/23 14:40, Andrii Nakryiko wrote:
> > 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
>
> 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,
> }
>
tcp_congestion_ops is defined in kernel and used by kernel internal
code. I don't think randomly setting and passing extra flag is generic
solution that will work for all struct_ops kinds.
>
> >
> >> + /* Fake bpf_link */
> >> + link->link.fd = map->fd;
> >> + link->map_fd = -1;
> >> + return &link->link;
> >> + }
> >> +
