| Message ID | 20230214221718.503964-5-kuifeng@meta.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Transit between BPF TCP congestion controls. | expand |
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 :-/ And what are these fake bpf_links? Can you share more about it means? > + > 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
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. > > 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 >
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? > > > > > 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 > >
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 >>>
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 > + 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; > + } > +
On 2/16/23 2:40 PM, Andrii Nakryiko wrote: > 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? It still needs to support the per-link behavior. > Alternatively, should we think about SEC(".struct_ops.link") or > something like that to instruct libbpf to add this BPF_F_LINK flag > automatically? I like this idea. Easier for users that is always link only. The users can also stay with SEC(".struct_ops") and decide later if BPF_F_LINK should be set depending on the runtime config and environment like kernel version...etc.
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; >> + } >> +
On Fri, Feb 17, 2023 at 4:05 PM Kui-Feng Lee <sinquersw@gmail.com> wrote: > > > > 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, > } > 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; > >> + } > >> +
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; +}; + 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,
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(-)