| Message ID | 20230214221718.503964-7-kuifeng@meta.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Transit between BPF TCP congestion controls. | expand |
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. > +{ > + 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? > + 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 >
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 >>
On Fri, Feb 17, 2023 at 4:22 PM Kui-Feng Lee <sinquersw@gmail.com> wrote: > > > > 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. > Of course. But I was thinking to move `bpf_map_update_elem(map->fd, &zero, st_ops->kern_vdata, 0);` part out of bpf_map__update_vdata() and make update_vdata() just prepare st_ops->kern_vdata only. > > > > >> + 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 > >>
On 2/17/23 17:10, Andrii Nakryiko wrote: > On Fri, Feb 17, 2023 at 4:22 PM Kui-Feng Lee <sinquersw@gmail.com> wrote: >> >> >> >> 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. >> > > Of course. But I was thinking to move `bpf_map_update_elem(map->fd, > &zero, st_ops->kern_vdata, 0);` part out of bpf_map__update_vdata() > and make update_vdata() just prepare st_ops->kern_vdata only. Ok! I will rename it as bpf_map_prepare_vdata(), and call bpf_map_update_elem() separately.
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) +{ + 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); + 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); 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;
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(+)