On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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.
