| Message ID | 20240227204556.17524-8-eddyz87@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: type suffixes and autocreate flag for struct_ops maps | expand |
On 2/27/24 12:45, Eduard Zingerman wrote: > Make bpf_map__set_autocreate() for struct_ops maps toggle autoload > state for referenced programs. > > E.g. for the BPF code below: > > SEC("struct_ops/test_1") int BPF_PROG(foo) { ... } > SEC("struct_ops/test_2") int BPF_PROG(bar) { ... } > > SEC(".struct_ops.link") > struct test_ops___v1 A = { > .foo = (void *)foo > }; > > SEC(".struct_ops.link") > struct test_ops___v2 B = { > .foo = (void *)foo, > .bar = (void *)bar, > }; > > And the following libbpf API calls: > > bpf_map__set_autocreate(skel->maps.A, true); > bpf_map__set_autocreate(skel->maps.B, false); > > The autoload would be enabled for program 'foo' and disabled for > program 'bar'. > > Do not apply such toggling if program autoload state is set by a call > to bpf_program__set_autoload(). > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> > --- > tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++- > 1 file changed, 34 insertions(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index b39d3f2898a1..1ea3046724f8 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -446,13 +446,18 @@ struct bpf_program { > struct bpf_object *obj; > > int fd; > - bool autoload; > + bool autoload:1; > + bool autoload_user_set:1; > bool autoattach; > bool sym_global; > bool mark_btf_static; > enum bpf_prog_type type; > enum bpf_attach_type expected_attach_type; > int exception_cb_idx; > + /* total number of struct_ops maps with autocreate == true > + * that reference this program > + */ > + __u32 struct_ops_refs; > > int prog_ifindex; > __u32 attach_btf_obj_fd; > @@ -4509,6 +4514,28 @@ static int bpf_get_map_info_from_fdinfo(int fd, struct bpf_map_info *info) > return 0; > } > > +/* Sync autoload and autocreate state between struct_ops map and > + * referenced programs. > + */ > +static void bpf_map__struct_ops_toggle_progs_autoload(struct bpf_map *map, bool autocreate) > +{ > + struct bpf_program *prog; > + int i; > + > + for (i = 0; i < btf_vlen(map->st_ops->type); ++i) { > + prog = map->st_ops->progs[i]; > + > + if (!prog || prog->autoload_user_set) > + continue; > + > + if (autocreate) > + prog->struct_ops_refs++; > + else > + prog->struct_ops_refs--; > + prog->autoload = prog->struct_ops_refs != 0; > + } > +} > + This part is related to the other patch [1], which allows a user to change the value of a function pointer field. The behavior of autocreate and autoload may suprise a user if the user call bpf_map__set_autocreate() after changing the value of a function pointer field. [1] https://lore.kernel.org/all/20240227010432.714127-1-thinker.li@gmail.com/ > bool bpf_map__autocreate(const struct bpf_map *map) > { > return map->autocreate; > @@ -4519,6 +4546,9 @@ int bpf_map__set_autocreate(struct bpf_map *map, bool autocreate) > if (map->obj->loaded) > return libbpf_err(-EBUSY); > > + if (map->st_ops && map->autocreate != autocreate) > + bpf_map__struct_ops_toggle_progs_autoload(map, autocreate); > + > map->autocreate = autocreate; > return 0; > } > @@ -8801,6 +8831,7 @@ int bpf_program__set_autoload(struct bpf_program *prog, bool autoload) > return libbpf_err(-EINVAL); > > prog->autoload = autoload; > + prog->autoload_user_set = 1; > return 0; > } > > @@ -9428,6 +9459,8 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj, > return -EINVAL; > } > > + if (map->autocreate) > + prog->struct_ops_refs++; > st_ops->progs[member_idx] = prog; > } >
On Tue, 2024-02-27 at 14:55 -0800, Kui-Feng Lee wrote: [...] > > @@ -4509,6 +4514,28 @@ static int bpf_get_map_info_from_fdinfo(int fd, struct bpf_map_info *info) > > return 0; > > } > > > > +/* Sync autoload and autocreate state between struct_ops map and > > + * referenced programs. > > + */ > > +static void bpf_map__struct_ops_toggle_progs_autoload(struct bpf_map *map, bool autocreate) > > +{ > > + struct bpf_program *prog; > > + int i; > > + > > + for (i = 0; i < btf_vlen(map->st_ops->type); ++i) { > > + prog = map->st_ops->progs[i]; > > + > > + if (!prog || prog->autoload_user_set) > > + continue; > > + > > + if (autocreate) > > + prog->struct_ops_refs++; > > + else > > + prog->struct_ops_refs--; > > + prog->autoload = prog->struct_ops_refs != 0; > > + } > > +} > > + > > This part is related to the other patch [1], which allows > a user to change the value of a function pointer field. The behavior of > autocreate and autoload may suprise a user if the user call > bpf_map__set_autocreate() after changing the value of a function pointer > field. > > [1] > https://lore.kernel.org/all/20240227010432.714127-1-thinker.li@gmail.com/ So, it appears that with shadow types users would have more or less convenient way to disable / enable related BPF programs (the references to programs are available, but reference counting would have to be implemented by user using some additional data structure, if needed). I don't see a way to reconcile shadow types with this autoload/autocreate toggling => my last two patches would have to be dropped. Wdyt?
On 2/27/24 15:09, Eduard Zingerman wrote: > On Tue, 2024-02-27 at 14:55 -0800, Kui-Feng Lee wrote: > [...] > >>> @@ -4509,6 +4514,28 @@ static int bpf_get_map_info_from_fdinfo(int fd, struct bpf_map_info *info) >>> return 0; >>> } >>> >>> +/* Sync autoload and autocreate state between struct_ops map and >>> + * referenced programs. >>> + */ >>> +static void bpf_map__struct_ops_toggle_progs_autoload(struct bpf_map *map, bool autocreate) >>> +{ >>> + struct bpf_program *prog; >>> + int i; >>> + >>> + for (i = 0; i < btf_vlen(map->st_ops->type); ++i) { >>> + prog = map->st_ops->progs[i]; >>> + >>> + if (!prog || prog->autoload_user_set) >>> + continue; >>> + >>> + if (autocreate) >>> + prog->struct_ops_refs++; >>> + else >>> + prog->struct_ops_refs--; >>> + prog->autoload = prog->struct_ops_refs != 0; >>> + } >>> +} >>> + >> >> This part is related to the other patch [1], which allows >> a user to change the value of a function pointer field. The behavior of >> autocreate and autoload may suprise a user if the user call >> bpf_map__set_autocreate() after changing the value of a function pointer >> field. >> >> [1] >> https://lore.kernel.org/all/20240227010432.714127-1-thinker.li@gmail.com/ > > So, it appears that with shadow types users would have more or less > convenient way to disable / enable related BPF programs > (the references to programs are available, but reference counting > would have to be implemented by user using some additional data > structure, if needed). > > I don't see a way to reconcile shadow types with this autoload/autocreate toggling > => my last two patches would have to be dropped. How about to update autoload according to the value of autocreate of maps before loading the programs? For example, update autoload in bpf_map__init_kern_struct_ops()? > > Wdyt?
On Tue, 2024-02-27 at 15:16 -0800, Kui-Feng Lee wrote: [...] > > So, it appears that with shadow types users would have more or less > > convenient way to disable / enable related BPF programs > > (the references to programs are available, but reference counting > > would have to be implemented by user using some additional data > > structure, if needed). > > > > I don't see a way to reconcile shadow types with this autoload/autocreate toggling > > => my last two patches would have to be dropped. > > How about to update autoload according to the value of autocreate of > maps before loading the programs? For example, update autoload in > bpf_map__init_kern_struct_ops()? This can be done, but it would have to be a separate pass: first scanning all maps and setting up reference counters for programs, then scanning all programs and disabling those unused. I can do that in v2, thank you for the suggestion. Still, this overlaps a bit with shadow types. Do you have an idea if such auto-toggling would be helpful from libbpf users point of view?
On 2/27/24 15:30, Eduard Zingerman wrote: > On Tue, 2024-02-27 at 15:16 -0800, Kui-Feng Lee wrote: > [...] > >>> So, it appears that with shadow types users would have more or less >>> convenient way to disable / enable related BPF programs >>> (the references to programs are available, but reference counting >>> would have to be implemented by user using some additional data >>> structure, if needed). >>> >>> I don't see a way to reconcile shadow types with this autoload/autocreate toggling >>> => my last two patches would have to be dropped. >> >> How about to update autoload according to the value of autocreate of >> maps before loading the programs? For example, update autoload in >> bpf_map__init_kern_struct_ops()? > > This can be done, but it would have to be a separate pass: > first scanning all maps and setting up reference counters for programs, > then scanning all programs and disabling those unused. > I can do that in v2, thank you for the suggestion. > > Still, this overlaps a bit with shadow types. > Do you have an idea if such auto-toggling would be helpful from libbpf > users point of view? For me, it is useful. For minor adjustments, shadow types is easier and simpler; for example, change a flag or a function. But, the features presented here are useful when a user want to switch between several very different configurations. They may not want to change a large number of fields.
On Tue, 2024-02-27 at 15:40 -0800, Kui-Feng Lee wrote: [...] > For me, it is useful. For minor adjustments, shadow types is easier and > simpler; for example, change a flag or a function. But, the features > presented here are useful when a user want to switch between several > very different configurations. They may not want to change a large > number of fields. Great, thank you for reviewing the patch-set. I'll wait a bit for additional comments and post v2 tomorrow.
On 2/27/24 15:30, Eduard Zingerman wrote: > On Tue, 2024-02-27 at 15:16 -0800, Kui-Feng Lee wrote: > [...] > >>> So, it appears that with shadow types users would have more or less >>> convenient way to disable / enable related BPF programs >>> (the references to programs are available, but reference counting >>> would have to be implemented by user using some additional data >>> structure, if needed). >>> >>> I don't see a way to reconcile shadow types with this autoload/autocreate toggling >>> => my last two patches would have to be dropped. >> >> How about to update autoload according to the value of autocreate of >> maps before loading the programs? For example, update autoload in >> bpf_map__init_kern_struct_ops()? > > This can be done, but it would have to be a separate pass: > first scanning all maps and setting up reference counters for programs, > then scanning all programs and disabling those unused. > I can do that in v2, thank you for the suggestion. > It only has to scan once with an additional flag. The value of the autoload of a prog should be true if its autoload_user_set is false and autocreate of any one of struct_ops maps pointing to the prog is true. Let's say the flag is autoload_autocreate. In bpf_map__init_kern_struct_ops(), it has to check prog->autoload_user_set, and do prog->autoload |= map->autocreate if prog->autoload_user_set is false and autoload_autocreate is true. Do prog->autoload = map->autocreate if autoload_autocreate is false I think it is enough, right? if (!prog->autoload_user_set) { if (!prog->autoload_autocreate) prog->autoload = map->autocreate; else prog->autoload |= map->autocreate; prog->autoload_autocreate = true; }
On Tue, 2024-02-27 at 16:12 -0800, Kui-Feng Lee wrote: [...] > It only has to scan once with an additional flag. > The value of the autoload of a prog should be > true if its autoload_user_set is false and autocreate of any one of > struct_ops maps pointing to the prog is true. > > Let's say the flag is autoload_autocreate. > In bpf_map__init_kern_struct_ops(), it has to check > prog->autoload_user_set, and do prog->autoload |= map->autocreate if > prog->autoload_user_set is false and autoload_autocreate is true. Do > prog->autoload = map->autocreate if autoload_autocreate is false I think > it is enough, right? > > if (!prog->autoload_user_set) { > if (!prog->autoload_autocreate) > prog->autoload = map->autocreate; > else > prog->autoload |= map->autocreate; > prog->autoload_autocreate = true; > } Since the full thing is moved to load phase I was hoping to make do w/o changes to struct bpf_program. To have it more contained. E.g. the code below apperas to work (needs more testing): --- 8< ------------------------------------- static int bpf_object__adjust_struct_ops_autoload(struct bpf_object *obj) { struct bpf_program *prog; struct bpf_map *map; int i, j, k, vlen; struct { __u8 autoload:1; /* only change autoload for programs that are * referenced from some struct_ops maps */ __u8 used:1; } *refs; refs = calloc(obj->nr_programs, sizeof(refs[0])); if (!refs) return -ENOMEM; for (i = 0; i < obj->nr_maps; i++) { map = &obj->maps[i]; if (!bpf_map__is_struct_ops(map)) continue; vlen = btf_vlen(map->st_ops->type); for (j = 0; j < vlen; ++j) { prog = map->st_ops->progs[j]; if (!prog) continue; k = prog - obj->programs; refs[k].used = true; refs[k].autoload |= map->autocreate; } } for (i = 0; i < obj->nr_programs; ++i) { prog = &obj->programs[i]; if (prog->autoload_user_set || !refs[i].used) continue; prog->autoload = refs[i].autoload; } free(refs); return 0; } ------------------------------------- >8 ---
On 2/27/24 12:45 PM, Eduard Zingerman wrote: > Make bpf_map__set_autocreate() for struct_ops maps toggle autoload > state for referenced programs. > > E.g. for the BPF code below: > > SEC("struct_ops/test_1") int BPF_PROG(foo) { ... } > SEC("struct_ops/test_2") int BPF_PROG(bar) { ... } > > SEC(".struct_ops.link") > struct test_ops___v1 A = { > .foo = (void *)foo > }; > > SEC(".struct_ops.link") > struct test_ops___v2 B = { > .foo = (void *)foo, > .bar = (void *)bar, > }; > > And the following libbpf API calls: > > bpf_map__set_autocreate(skel->maps.A, true); > bpf_map__set_autocreate(skel->maps.B, false); > > The autoload would be enabled for program 'foo' and disabled for > program 'bar'. > > Do not apply such toggling if program autoload state is set by a call > to bpf_program__set_autoload(). > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> > --- > tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++- > 1 file changed, 34 insertions(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index b39d3f2898a1..1ea3046724f8 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -446,13 +446,18 @@ struct bpf_program { > struct bpf_object *obj; > > int fd; > - bool autoload; > + bool autoload:1; > + bool autoload_user_set:1; > bool autoattach; > bool sym_global; > bool mark_btf_static; > enum bpf_prog_type type; > enum bpf_attach_type expected_attach_type; > int exception_cb_idx; > + /* total number of struct_ops maps with autocreate == true > + * that reference this program > + */ > + __u32 struct_ops_refs; Instead of adding struct_ops_refs and autoload_user_set, for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at load time and is only set if it is used by at least one autocreate map, if I read patch 2 & 3 correctly. Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used by one struct_ops map with autocreate == true. If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be loaded even the autoload is set. If bpf prog is used in a struct_ops map and its autoload is set to false, the struct_ops map will be in broken state. Thus, prog->autoload does not fit very well with struct_ops prog and may as well depend on whether the struct_ops prog is used by a struct_ops map alone?
On Tue, 2024-02-27 at 18:10 -0800, Martin KaFai Lau wrote: [...] > Instead of adding struct_ops_refs and autoload_user_set, > > for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking > prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at > load time and is only set if it is used by at least one autocreate map, if I > read patch 2 & 3 correctly. > > Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used > by one struct_ops map with autocreate == true. > > If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be > loaded even the autoload is set. If bpf prog is used in a struct_ops map and its > autoload is set to false, the struct_ops map will be in broken state. Thus, > prog->autoload does not fit very well with struct_ops prog and may as well > depend on whether the struct_ops prog is used by a struct_ops map alone? This makes sense. The drawback is that introspection capability to query which programs would be loaded is lost, maybe that is not a big deal. It could be put back by adding an ugly loop iterating over all maps in bpf_program__autoload() for struct_ops programs. I think I'll post v2 with changes you suggest and see what others have to say.
On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > > On 2/27/24 12:45 PM, Eduard Zingerman wrote: > > Make bpf_map__set_autocreate() for struct_ops maps toggle autoload > > state for referenced programs. > > > > E.g. for the BPF code below: > > > > SEC("struct_ops/test_1") int BPF_PROG(foo) { ... } > > SEC("struct_ops/test_2") int BPF_PROG(bar) { ... } > > > > SEC(".struct_ops.link") > > struct test_ops___v1 A = { > > .foo = (void *)foo > > }; > > > > SEC(".struct_ops.link") > > struct test_ops___v2 B = { > > .foo = (void *)foo, > > .bar = (void *)bar, > > }; > > > > And the following libbpf API calls: > > > > bpf_map__set_autocreate(skel->maps.A, true); > > bpf_map__set_autocreate(skel->maps.B, false); > > > > The autoload would be enabled for program 'foo' and disabled for > > program 'bar'. > > > > Do not apply such toggling if program autoload state is set by a call > > to bpf_program__set_autoload(). > > > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> > > --- > > tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++- > > 1 file changed, 34 insertions(+), 1 deletion(-) > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index b39d3f2898a1..1ea3046724f8 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -446,13 +446,18 @@ struct bpf_program { > > struct bpf_object *obj; > > > > int fd; > > - bool autoload; > > + bool autoload:1; > > + bool autoload_user_set:1; > > bool autoattach; > > bool sym_global; > > bool mark_btf_static; > > enum bpf_prog_type type; > > enum bpf_attach_type expected_attach_type; > > int exception_cb_idx; > > + /* total number of struct_ops maps with autocreate == true > > + * that reference this program > > + */ > > + __u32 struct_ops_refs; > > Instead of adding struct_ops_refs and autoload_user_set, > > for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking > prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at > load time and is only set if it is used by at least one autocreate map, if I > read patch 2 & 3 correctly. > > Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used > by one struct_ops map with autocreate == true. > > If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be > loaded even the autoload is set. If bpf prog is used in a struct_ops map and its > autoload is set to false, the struct_ops map will be in broken state. Thus, We can easily detect this condition and report meaningful error. > prog->autoload does not fit very well with struct_ops prog and may as well > depend on whether the struct_ops prog is used by a struct_ops map alone? I think it's probably fine from a usability standpoint to disable loading the BPF program if its struct_ops map was explicitly set to not auto-create. It's a bit of deviation from other program types, but in practice this logic will make it easier for users. One question I have, though, is whether we should report as an error a stand-alone struct_ops BPF program that is not used from any struct_ops map? Or should we load it nevertheless? Or should we silently not load it? I feel like silently not loading is the worst behavior here. So either loading it anyway or reporting an error would be my preference, probably.
On Wed, 2024-02-28 at 15:55 -0800, Andrii Nakryiko wrote: > On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: [...] > > Instead of adding struct_ops_refs and autoload_user_set, > > > > for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking > > prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at > > load time and is only set if it is used by at least one autocreate map, if I > > read patch 2 & 3 correctly. > > > > Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used > > by one struct_ops map with autocreate == true. > > > > If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be > > loaded even the autoload is set. If bpf prog is used in a struct_ops map and its > > autoload is set to false, the struct_ops map will be in broken state. Thus, > > We can easily detect this condition and report meaningful error. > > > prog->autoload does not fit very well with struct_ops prog and may as well > > depend on whether the struct_ops prog is used by a struct_ops map alone? > > I think it's probably fine from a usability standpoint to disable > loading the BPF program if its struct_ops map was explicitly set to > not auto-create. It's a bit of deviation from other program types, but > in practice this logic will make it easier for users. > > One question I have, though, is whether we should report as an error a > stand-alone struct_ops BPF program that is not used from any > struct_ops map? Or should we load it nevertheless? Or should we > silently not load it? > > I feel like silently not loading is the worst behavior here. So either > loading it anyway or reporting an error would be my preference, > probably. The following properties of the struct_ops program are set based on the corresponding struct_ops map: - attach_btf_id - BTF id of the kernel struct_ops type; - expected_attach_type - member index of function pointer inside the kernel type. No corresponding map means above fields are not set, means program fails to load with error report. So I think it is fine to try loading such programs w/o any additional processing.
On Wed, Feb 28, 2024 at 4:04 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Wed, 2024-02-28 at 15:55 -0800, Andrii Nakryiko wrote: > > On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > > [...] > > > > Instead of adding struct_ops_refs and autoload_user_set, > > > > > > for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking > > > prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at > > > load time and is only set if it is used by at least one autocreate map, if I > > > read patch 2 & 3 correctly. > > > > > > Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used > > > by one struct_ops map with autocreate == true. > > > > > > If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be > > > loaded even the autoload is set. If bpf prog is used in a struct_ops map and its > > > autoload is set to false, the struct_ops map will be in broken state. Thus, > > > > We can easily detect this condition and report meaningful error. > > > > > prog->autoload does not fit very well with struct_ops prog and may as well > > > depend on whether the struct_ops prog is used by a struct_ops map alone? > > > > I think it's probably fine from a usability standpoint to disable > > loading the BPF program if its struct_ops map was explicitly set to > > not auto-create. It's a bit of deviation from other program types, but > > in practice this logic will make it easier for users. > > > > One question I have, though, is whether we should report as an error a > > stand-alone struct_ops BPF program that is not used from any > > struct_ops map? Or should we load it nevertheless? Or should we > > silently not load it? > > > > I feel like silently not loading is the worst behavior here. So either > > loading it anyway or reporting an error would be my preference, > > probably. > > The following properties of the struct_ops program are set based on > the corresponding struct_ops map: > - attach_btf_id - BTF id of the kernel struct_ops type; > - expected_attach_type - member index of function pointer inside > the kernel type. > > No corresponding map means above fields are not set, > means program fails to load with error report. > > So I think it is fine to try loading such programs w/o any additional > processing. But Martin is proposing to *not load* programs if their attach_btf_id is not set. Which is why I'm asking if we should distinguish situations where a BPF program is stand-alone (never was referenced from struct_ops map) vs auto-disabling it because struct_ops map that it was referenced from was explicitly disabled.
On 2/28/24 3:55 PM, Andrii Nakryiko wrote: > On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: >> >> On 2/27/24 12:45 PM, Eduard Zingerman wrote: >>> Make bpf_map__set_autocreate() for struct_ops maps toggle autoload >>> state for referenced programs. >>> >>> E.g. for the BPF code below: >>> >>> SEC("struct_ops/test_1") int BPF_PROG(foo) { ... } >>> SEC("struct_ops/test_2") int BPF_PROG(bar) { ... } >>> >>> SEC(".struct_ops.link") >>> struct test_ops___v1 A = { >>> .foo = (void *)foo >>> }; >>> >>> SEC(".struct_ops.link") >>> struct test_ops___v2 B = { >>> .foo = (void *)foo, >>> .bar = (void *)bar, >>> }; >>> >>> And the following libbpf API calls: >>> >>> bpf_map__set_autocreate(skel->maps.A, true); >>> bpf_map__set_autocreate(skel->maps.B, false); >>> >>> The autoload would be enabled for program 'foo' and disabled for >>> program 'bar'. >>> >>> Do not apply such toggling if program autoload state is set by a call >>> to bpf_program__set_autoload(). >>> >>> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> >>> --- >>> tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++- >>> 1 file changed, 34 insertions(+), 1 deletion(-) >>> >>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >>> index b39d3f2898a1..1ea3046724f8 100644 >>> --- a/tools/lib/bpf/libbpf.c >>> +++ b/tools/lib/bpf/libbpf.c >>> @@ -446,13 +446,18 @@ struct bpf_program { >>> struct bpf_object *obj; >>> >>> int fd; >>> - bool autoload; >>> + bool autoload:1; >>> + bool autoload_user_set:1; >>> bool autoattach; >>> bool sym_global; >>> bool mark_btf_static; >>> enum bpf_prog_type type; >>> enum bpf_attach_type expected_attach_type; >>> int exception_cb_idx; >>> + /* total number of struct_ops maps with autocreate == true >>> + * that reference this program >>> + */ >>> + __u32 struct_ops_refs; >> >> Instead of adding struct_ops_refs and autoload_user_set, >> >> for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking >> prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at >> load time and is only set if it is used by at least one autocreate map, if I >> read patch 2 & 3 correctly. >> >> Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used >> by one struct_ops map with autocreate == true. >> >> If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be >> loaded even the autoload is set. If bpf prog is used in a struct_ops map and its >> autoload is set to false, the struct_ops map will be in broken state. Thus, > > We can easily detect this condition and report meaningful error. > >> prog->autoload does not fit very well with struct_ops prog and may as well >> depend on whether the struct_ops prog is used by a struct_ops map alone? > > I think it's probably fine from a usability standpoint to disable > loading the BPF program if its struct_ops map was explicitly set to > not auto-create. It's a bit of deviation from other program types, but > in practice this logic will make it easier for users. > > One question I have, though, is whether we should report as an error a > stand-alone struct_ops BPF program that is not used from any > struct_ops map? Or should we load it nevertheless? Or should we > silently not load it? I think the libbpf could report an error if the prog is not used in any struct_ops map at the source code level, not sure if it is useful. However, it probably should not report error if that struct_ops map (where the prog is resided) does not have autocreate set to true. If a BPF program is not used in any struct_ops map, it cannot be loaded anyway because the prog->attach_btf_id is not set. If libbpf tries to load the prog, the kernel will reject it also. I think it may be a question on whether it is the user intention of not loading the prog if the prog is not used in any struct_ops map. I tend to think it is the user intention of not loading it in this case. SEC("struct_ops/test1") int BPF_PROG(test1) { ... } SEC("struct_ops/test2") int BPF_PROG(test2) { ... } SEC("?.struct_ops.link") struct some_ops___v1 a = { .test1 = test1 } SEC("?.struct_ops.link") struct some_ops___v2 b = { .test1 = test1, .test2 = test2, } In the above, the userspace may try to load with a newer some_ops___v2 first, failed and then try a lower version some_ops___v1 and then succeeded. The test2 prog will not be used and not expected to be loaded. > > I feel like silently not loading is the worst behavior here. So either > loading it anyway or reporting an error would be my preference, > probably.
On Wed, Feb 28, 2024 at 4:25 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > > On 2/28/24 3:55 PM, Andrii Nakryiko wrote: > > On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > >> > >> On 2/27/24 12:45 PM, Eduard Zingerman wrote: > >>> Make bpf_map__set_autocreate() for struct_ops maps toggle autoload > >>> state for referenced programs. > >>> > >>> E.g. for the BPF code below: > >>> > >>> SEC("struct_ops/test_1") int BPF_PROG(foo) { ... } > >>> SEC("struct_ops/test_2") int BPF_PROG(bar) { ... } > >>> > >>> SEC(".struct_ops.link") > >>> struct test_ops___v1 A = { > >>> .foo = (void *)foo > >>> }; > >>> > >>> SEC(".struct_ops.link") > >>> struct test_ops___v2 B = { > >>> .foo = (void *)foo, > >>> .bar = (void *)bar, > >>> }; > >>> > >>> And the following libbpf API calls: > >>> > >>> bpf_map__set_autocreate(skel->maps.A, true); > >>> bpf_map__set_autocreate(skel->maps.B, false); > >>> > >>> The autoload would be enabled for program 'foo' and disabled for > >>> program 'bar'. > >>> > >>> Do not apply such toggling if program autoload state is set by a call > >>> to bpf_program__set_autoload(). > >>> > >>> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> > >>> --- > >>> tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++- > >>> 1 file changed, 34 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >>> index b39d3f2898a1..1ea3046724f8 100644 > >>> --- a/tools/lib/bpf/libbpf.c > >>> +++ b/tools/lib/bpf/libbpf.c > >>> @@ -446,13 +446,18 @@ struct bpf_program { > >>> struct bpf_object *obj; > >>> > >>> int fd; > >>> - bool autoload; > >>> + bool autoload:1; > >>> + bool autoload_user_set:1; > >>> bool autoattach; > >>> bool sym_global; > >>> bool mark_btf_static; > >>> enum bpf_prog_type type; > >>> enum bpf_attach_type expected_attach_type; > >>> int exception_cb_idx; > >>> + /* total number of struct_ops maps with autocreate == true > >>> + * that reference this program > >>> + */ > >>> + __u32 struct_ops_refs; > >> > >> Instead of adding struct_ops_refs and autoload_user_set, > >> > >> for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking > >> prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at > >> load time and is only set if it is used by at least one autocreate map, if I > >> read patch 2 & 3 correctly. > >> > >> Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used > >> by one struct_ops map with autocreate == true. > >> > >> If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be > >> loaded even the autoload is set. If bpf prog is used in a struct_ops map and its > >> autoload is set to false, the struct_ops map will be in broken state. Thus, > > > > We can easily detect this condition and report meaningful error. > > > >> prog->autoload does not fit very well with struct_ops prog and may as well > >> depend on whether the struct_ops prog is used by a struct_ops map alone? > > > > I think it's probably fine from a usability standpoint to disable > > loading the BPF program if its struct_ops map was explicitly set to > > not auto-create. It's a bit of deviation from other program types, but > > in practice this logic will make it easier for users. > > > > One question I have, though, is whether we should report as an error a > > stand-alone struct_ops BPF program that is not used from any > > struct_ops map? Or should we load it nevertheless? Or should we > > silently not load it? > > I think the libbpf could report an error if the prog is not used in any > struct_ops map at the source code level, not sure if it is useful. > > However, it probably should not report error if that struct_ops map (where the > prog is resided) does not have autocreate set to true. > > If a BPF program is not used in any struct_ops map, it cannot be loaded anyway > because the prog->attach_btf_id is not set. If libbpf tries to load the prog, > the kernel will reject it also. I think it may be a question on whether it is > the user intention of not loading the prog if the prog is not used in any > struct_ops map. I tend to think it is the user intention of not loading it in > this case. > > SEC("struct_ops/test1") > int BPF_PROG(test1) { ... } > > SEC("struct_ops/test2") > int BPF_PROG(test2) { ... } > > SEC("?.struct_ops.link") struct some_ops___v1 a = { .test1 = test1 } > SEC("?.struct_ops.link") struct some_ops___v2 b = { .test1 = test1, > .test2 = test2, } > > In the above, the userspace may try to load with a newer some_ops___v2 first, > failed and then try a lower version some_ops___v1 and then succeeded. The test2 > prog will not be used and not expected to be loaded. > Yes, it's all sane in the above example. But imagine a stand-alone struct_ops program with no SEC(".struct_ops") at all: SEC("struct_ops/test1") int BPF_PROG(test1) { ... } /* nothing else */ Currently this will fail, right? And with your proposal it will succeed without actually even attempting to load the BPF program. Or am I misunderstanding? > > > > I feel like silently not loading is the worst behavior here. So either > > loading it anyway or reporting an error would be my preference, > > probably. >
On 2/28/24 4:30 PM, Andrii Nakryiko wrote: > On Wed, Feb 28, 2024 at 4:25 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: >> >> On 2/28/24 3:55 PM, Andrii Nakryiko wrote: >>> On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: >>>> >>>> On 2/27/24 12:45 PM, Eduard Zingerman wrote: >>>>> Make bpf_map__set_autocreate() for struct_ops maps toggle autoload >>>>> state for referenced programs. >>>>> >>>>> E.g. for the BPF code below: >>>>> >>>>> SEC("struct_ops/test_1") int BPF_PROG(foo) { ... } >>>>> SEC("struct_ops/test_2") int BPF_PROG(bar) { ... } >>>>> >>>>> SEC(".struct_ops.link") >>>>> struct test_ops___v1 A = { >>>>> .foo = (void *)foo >>>>> }; >>>>> >>>>> SEC(".struct_ops.link") >>>>> struct test_ops___v2 B = { >>>>> .foo = (void *)foo, >>>>> .bar = (void *)bar, >>>>> }; >>>>> >>>>> And the following libbpf API calls: >>>>> >>>>> bpf_map__set_autocreate(skel->maps.A, true); >>>>> bpf_map__set_autocreate(skel->maps.B, false); >>>>> >>>>> The autoload would be enabled for program 'foo' and disabled for >>>>> program 'bar'. >>>>> >>>>> Do not apply such toggling if program autoload state is set by a call >>>>> to bpf_program__set_autoload(). >>>>> >>>>> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> >>>>> --- >>>>> tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++- >>>>> 1 file changed, 34 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >>>>> index b39d3f2898a1..1ea3046724f8 100644 >>>>> --- a/tools/lib/bpf/libbpf.c >>>>> +++ b/tools/lib/bpf/libbpf.c >>>>> @@ -446,13 +446,18 @@ struct bpf_program { >>>>> struct bpf_object *obj; >>>>> >>>>> int fd; >>>>> - bool autoload; >>>>> + bool autoload:1; >>>>> + bool autoload_user_set:1; >>>>> bool autoattach; >>>>> bool sym_global; >>>>> bool mark_btf_static; >>>>> enum bpf_prog_type type; >>>>> enum bpf_attach_type expected_attach_type; >>>>> int exception_cb_idx; >>>>> + /* total number of struct_ops maps with autocreate == true >>>>> + * that reference this program >>>>> + */ >>>>> + __u32 struct_ops_refs; >>>> >>>> Instead of adding struct_ops_refs and autoload_user_set, >>>> >>>> for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking >>>> prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at >>>> load time and is only set if it is used by at least one autocreate map, if I >>>> read patch 2 & 3 correctly. >>>> >>>> Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used >>>> by one struct_ops map with autocreate == true. >>>> >>>> If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be >>>> loaded even the autoload is set. If bpf prog is used in a struct_ops map and its >>>> autoload is set to false, the struct_ops map will be in broken state. Thus, >>> >>> We can easily detect this condition and report meaningful error. >>> >>>> prog->autoload does not fit very well with struct_ops prog and may as well >>>> depend on whether the struct_ops prog is used by a struct_ops map alone? >>> >>> I think it's probably fine from a usability standpoint to disable >>> loading the BPF program if its struct_ops map was explicitly set to >>> not auto-create. It's a bit of deviation from other program types, but >>> in practice this logic will make it easier for users. >>> >>> One question I have, though, is whether we should report as an error a >>> stand-alone struct_ops BPF program that is not used from any >>> struct_ops map? Or should we load it nevertheless? Or should we >>> silently not load it? >> >> I think the libbpf could report an error if the prog is not used in any >> struct_ops map at the source code level, not sure if it is useful. >> >> However, it probably should not report error if that struct_ops map (where the >> prog is resided) does not have autocreate set to true. >> >> If a BPF program is not used in any struct_ops map, it cannot be loaded anyway >> because the prog->attach_btf_id is not set. If libbpf tries to load the prog, >> the kernel will reject it also. I think it may be a question on whether it is >> the user intention of not loading the prog if the prog is not used in any >> struct_ops map. I tend to think it is the user intention of not loading it in >> this case. >> >> SEC("struct_ops/test1") >> int BPF_PROG(test1) { ... } >> >> SEC("struct_ops/test2") >> int BPF_PROG(test2) { ... } >> >> SEC("?.struct_ops.link") struct some_ops___v1 a = { .test1 = test1 } >> SEC("?.struct_ops.link") struct some_ops___v2 b = { .test1 = test1, >> .test2 = test2, } >> >> In the above, the userspace may try to load with a newer some_ops___v2 first, >> failed and then try a lower version some_ops___v1 and then succeeded. The test2 >> prog will not be used and not expected to be loaded. >> > > Yes, it's all sane in the above example. But imagine a stand-alone > struct_ops program with no SEC(".struct_ops") at all: > > > SEC("struct_ops/test1") > int BPF_PROG(test1) { ... } > > /* nothing else */ > > Currently this will fail, right? > > And with your proposal it will succeed without actually even > attempting to load the BPF program. Or am I misunderstanding? Yep, currently it should fail. Agree that we need to distinguish this case and prog->attach_btf_id is not enough. This probably can be tracked in collect_st_ops_relos at the open phase.
On Wed, 2024-02-28 at 16:37 -0800, Martin KaFai Lau wrote: [...] > > Yes, it's all sane in the above example. But imagine a stand-alone > > struct_ops program with no SEC(".struct_ops") at all: > > > > > > SEC("struct_ops/test1") > > int BPF_PROG(test1) { ... } > > > > /* nothing else */ > > > > Currently this will fail, right? > > > > And with your proposal it will succeed without actually even > > attempting to load the BPF program. Or am I misunderstanding? > > Yep, currently it should fail. > > Agree that we need to distinguish this case and prog->attach_btf_id is not > enough. This probably can be tracked in collect_st_ops_relos at the open phase. collect_st_ops_relos() should work, I'll add a flag to track this.
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index b39d3f2898a1..1ea3046724f8 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -446,13 +446,18 @@ struct bpf_program { struct bpf_object *obj; int fd; - bool autoload; + bool autoload:1; + bool autoload_user_set:1; bool autoattach; bool sym_global; bool mark_btf_static; enum bpf_prog_type type; enum bpf_attach_type expected_attach_type; int exception_cb_idx; + /* total number of struct_ops maps with autocreate == true + * that reference this program + */ + __u32 struct_ops_refs; int prog_ifindex; __u32 attach_btf_obj_fd; @@ -4509,6 +4514,28 @@ static int bpf_get_map_info_from_fdinfo(int fd, struct bpf_map_info *info) return 0; } +/* Sync autoload and autocreate state between struct_ops map and + * referenced programs. + */ +static void bpf_map__struct_ops_toggle_progs_autoload(struct bpf_map *map, bool autocreate) +{ + struct bpf_program *prog; + int i; + + for (i = 0; i < btf_vlen(map->st_ops->type); ++i) { + prog = map->st_ops->progs[i]; + + if (!prog || prog->autoload_user_set) + continue; + + if (autocreate) + prog->struct_ops_refs++; + else + prog->struct_ops_refs--; + prog->autoload = prog->struct_ops_refs != 0; + } +} + bool bpf_map__autocreate(const struct bpf_map *map) { return map->autocreate; @@ -4519,6 +4546,9 @@ int bpf_map__set_autocreate(struct bpf_map *map, bool autocreate) if (map->obj->loaded) return libbpf_err(-EBUSY); + if (map->st_ops && map->autocreate != autocreate) + bpf_map__struct_ops_toggle_progs_autoload(map, autocreate); + map->autocreate = autocreate; return 0; } @@ -8801,6 +8831,7 @@ int bpf_program__set_autoload(struct bpf_program *prog, bool autoload) return libbpf_err(-EINVAL); prog->autoload = autoload; + prog->autoload_user_set = 1; return 0; } @@ -9428,6 +9459,8 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj, return -EINVAL; } + if (map->autocreate) + prog->struct_ops_refs++; st_ops->progs[member_idx] = prog; }
Make bpf_map__set_autocreate() for struct_ops maps toggle autoload state for referenced programs. E.g. for the BPF code below: SEC("struct_ops/test_1") int BPF_PROG(foo) { ... } SEC("struct_ops/test_2") int BPF_PROG(bar) { ... } SEC(".struct_ops.link") struct test_ops___v1 A = { .foo = (void *)foo }; SEC(".struct_ops.link") struct test_ops___v2 B = { .foo = (void *)foo, .bar = (void *)bar, }; And the following libbpf API calls: bpf_map__set_autocreate(skel->maps.A, true); bpf_map__set_autocreate(skel->maps.B, false); The autoload would be enabled for program 'foo' and disabled for program 'bar'. Do not apply such toggling if program autoload state is set by a call to bpf_program__set_autoload(). Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> --- tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)