| Message ID | 20211118103335.1208372-1-revest@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] libbpf: Add ability to clear per-program load flags | expand |
On Thu, Nov 18, 2021 at 2:33 AM Florent Revest <revest@chromium.org> wrote: > > Recently, bpf_program__flags and bpf_program__set_extra_flags were > introduced to the libbpf API but they only allow adding load flags. > > We have a use-case where we construct a skeleton with a sleepable > program and if it fails to load then we want to make it non-sleepable by > clearing BPF_F_SLEEPABLE. I'd say the better way to do this is to have two programs (that share the logic, of course) and pick one or another at runtime: static int whatever_logic(bool sleepable) { ... } SEC("fentry.s/whatever") int BPF_PROG(whatever_sleepable, ...) { return whatever_logic(true); } SEC("fentry/whatever") int BPF_PROG(whatever_nonsleepable, ...) { return whatever_logic(false); } Then at runtime you can bpf_program__autoload(..., false) for a variant you don't want to load. This clear_flags business seems too low-level and too limited. Next thing we'll be adding a few more bit manipulation variants (e.g, reset flags). Let's see how far you can get with the use of existing features. I'd set_extra_flags() to be almost never used, btw. And they shouldn't, if can be avoided. So I'm hesitant to keep extending operations around prog_flags. But given we just added set_extra_flags() and it's already too limiting, let's change set_extra flags to just set_flags() that will override the flags with whatever user provides. Then with bpf_program__flags() and bpf_program__set_flags() you can express whatever you want without adding extra APIs. Care to fix that? > > Signed-off-by: Florent Revest <revest@chromium.org> > --- > tools/lib/bpf/libbpf.c | 9 +++++++++ > tools/lib/bpf/libbpf.h | 1 + > tools/lib/bpf/libbpf.map | 1 + > 3 files changed, 11 insertions(+) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index de7e09a6b5ec..dcb7fced5fd2 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -8305,6 +8305,15 @@ int bpf_program__set_extra_flags(struct bpf_program *prog, __u32 extra_flags) > return 0; > } > > +int bpf_program__clear_flags(struct bpf_program *prog, __u32 flags) > +{ > + if (prog->obj->loaded) > + return libbpf_err(-EBUSY); > + > + prog->prog_flags &= ~flags; > + return 0; > +} > + > #define SEC_DEF(sec_pfx, ptype, atype, flags, ...) { \ > .sec = sec_pfx, \ > .prog_type = BPF_PROG_TYPE_##ptype, \ > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index 4ec69f224342..08f108e49841 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -495,6 +495,7 @@ bpf_program__set_expected_attach_type(struct bpf_program *prog, > > LIBBPF_API __u32 bpf_program__flags(const struct bpf_program *prog); > LIBBPF_API int bpf_program__set_extra_flags(struct bpf_program *prog, __u32 extra_flags); > +LIBBPF_API int bpf_program__clear_flags(struct bpf_program *prog, __u32 flags); > > LIBBPF_API int > bpf_program__set_attach_target(struct bpf_program *prog, int attach_prog_fd, > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > index 6a59514a48cf..eeff700240dc 100644 > --- a/tools/lib/bpf/libbpf.map > +++ b/tools/lib/bpf/libbpf.map > @@ -401,6 +401,7 @@ LIBBPF_0.6.0 { > bpf_program__insn_cnt; > bpf_program__insns; > bpf_program__set_extra_flags; > + bpf_program__clear_flags; > btf__add_btf; > btf__add_decl_tag; > btf__add_type_tag; > -- > 2.34.0.rc2.393.gf8c9666880-goog >
On Thu, Nov 18, 2021 at 6:49 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Nov 18, 2021 at 2:33 AM Florent Revest <revest@chromium.org> wrote: > > > > We have a use-case where we construct a skeleton with a sleepable > > program and if it fails to load then we want to make it non-sleepable by > > clearing BPF_F_SLEEPABLE. > > I'd say the better way to do this is to have two programs (that share > the logic, of course) and pick one or another at runtime: > > static int whatever_logic(bool sleepable) { ... } > > SEC("fentry.s/whatever") > int BPF_PROG(whatever_sleepable, ...) > { > return whatever_logic(true); > } > > SEC("fentry/whatever") > int BPF_PROG(whatever_nonsleepable, ...) > { > return whatever_logic(false); > } > > > Then at runtime you can bpf_program__autoload(..., false) for a > variant you don't want to load. Ah cool, thanks! That's a good idea :) it will also look cleaner. > This clear_flags business seems too low-level and too limited. Next > thing we'll be adding a few more bit manipulation variants (e.g, reset > flags). Let's see how far you can get with the use of existing > features. I'd set_extra_flags() to be almost never used, btw. And they > shouldn't, if can be avoided. So I'm hesitant to keep extending > operations around prog_flags. I agree > But given we just added set_extra_flags() and it's already too > limiting, let's change set_extra flags to just set_flags() that will > override the flags with whatever user provides. Then with > bpf_program__flags() and bpf_program__set_flags() you can express > whatever you want without adding extra APIs. Care to fix that? Sure, I'll send a patch for this! :)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index de7e09a6b5ec..dcb7fced5fd2 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -8305,6 +8305,15 @@ int bpf_program__set_extra_flags(struct bpf_program *prog, __u32 extra_flags) return 0; } +int bpf_program__clear_flags(struct bpf_program *prog, __u32 flags) +{ + if (prog->obj->loaded) + return libbpf_err(-EBUSY); + + prog->prog_flags &= ~flags; + return 0; +} + #define SEC_DEF(sec_pfx, ptype, atype, flags, ...) { \ .sec = sec_pfx, \ .prog_type = BPF_PROG_TYPE_##ptype, \ diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 4ec69f224342..08f108e49841 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -495,6 +495,7 @@ bpf_program__set_expected_attach_type(struct bpf_program *prog, LIBBPF_API __u32 bpf_program__flags(const struct bpf_program *prog); LIBBPF_API int bpf_program__set_extra_flags(struct bpf_program *prog, __u32 extra_flags); +LIBBPF_API int bpf_program__clear_flags(struct bpf_program *prog, __u32 flags); LIBBPF_API int bpf_program__set_attach_target(struct bpf_program *prog, int attach_prog_fd, diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index 6a59514a48cf..eeff700240dc 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -401,6 +401,7 @@ LIBBPF_0.6.0 { bpf_program__insn_cnt; bpf_program__insns; bpf_program__set_extra_flags; + bpf_program__clear_flags; btf__add_btf; btf__add_decl_tag; btf__add_type_tag;
Recently, bpf_program__flags and bpf_program__set_extra_flags were introduced to the libbpf API but they only allow adding load flags. We have a use-case where we construct a skeleton with a sleepable program and if it fails to load then we want to make it non-sleepable by clearing BPF_F_SLEEPABLE. Signed-off-by: Florent Revest <revest@chromium.org> --- tools/lib/bpf/libbpf.c | 9 +++++++++ tools/lib/bpf/libbpf.h | 1 + tools/lib/bpf/libbpf.map | 1 + 3 files changed, 11 insertions(+)