Re: [PATCH bpf-next 1/2] libbpf: Probe for bounded loop support

[Paul Chaignon <paul@isovalent.com>]

On Fri, Dec 17, 2021 at 11:41:28AM -0800, Andrii Nakryiko wrote:
> On Fri, Dec 17, 2021 at 11:22 AM Paul Chaignon <paul@isovalent.com> wrote:
> >
> > On Fri, Dec 17, 2021 at 08:12:23AM -0800, Andrii Nakryiko wrote:
> > > On Fri, Dec 17, 2021 at 4:12 AM Paul Chaignon <paul@isovalent.com> wrote:
> > > >
> > > > This patch introduces a new probe to check whether the verifier supports
> > > > bounded loops as introduced in commit 2589726d12a1 ("bpf: introduce
> > > > bounded loops"). This patch will allow BPF users such as Cilium to probe
> > > > for loop support on startup and only unconditionally unroll loops on
> > > > older kernels.
> > > >
> > > > Reviewed-by: Quentin Monnet <quentin@isovalent.com>
> > > > Signed-off-by: Paul Chaignon <paul@isovalent.com>
> > > > ---
> > > >  tools/lib/bpf/libbpf.h        |  1 +
> > > >  tools/lib/bpf/libbpf.map      |  1 +
> > > >  tools/lib/bpf/libbpf_probes.c | 20 ++++++++++++++++++++
> > > >  3 files changed, 22 insertions(+)
> > > >
> > > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > > index 42b2f36fd9f0..3621aaaff67c 100644
> > > > --- a/tools/lib/bpf/libbpf.h
> > > > +++ b/tools/lib/bpf/libbpf.h
> > > > @@ -1058,6 +1058,7 @@ LIBBPF_API bool bpf_probe_map_type(enum bpf_map_type map_type, __u32 ifindex);
> > > >  LIBBPF_API bool bpf_probe_helper(enum bpf_func_id id,
> > > >                                  enum bpf_prog_type prog_type, __u32 ifindex);
> > > >  LIBBPF_API bool bpf_probe_large_insn_limit(__u32 ifindex);
> > > > +LIBBPF_API bool bpf_probe_bounded_loops(__u32 ifindex);
> > > >
> > >
> > > Nope, see [0], I'm removing bpf_probe_large_insn_limit, so no new
> > > ad-hoc feature probing APIs, please. There has to be some system to
> > > this. If you want to add it to bpftool, go ahead, but keep it inside
> > > bpftool code only. In practice I'd use CO-RE feature detection from
> > > the BPF program side to pick the best implementation. Worst case, I'd
> > > add two BPF program implementations and picked one or the other
> > > (bpf_program__set_autoload(false) to disable one of them) after doing
> > > feature detection from the process, not relying on shelling out to
> > > bpftool.
> >
> > Thanks for the pointer, I wasn't aware of that ongoing work.
> >
> > For CO-RE feature detection, do you have in mind a bpf_core_field_exists
> > call to check one of the bpf_func_state fields introduced in the same
> > commit as bounded loop support, or is there some other CO-RE magic I'm
> > not aware of?
> 
> yep, I had bpf_core_xxx() checks in mind. But even without CO-RE and
> vmlinux BTF, if you can detect it from user-space and set .rodata
> variables, BPF verifier will dead code eliminate gated parts that rely
> on bounded loops, if that's more convenient.

Yes, that's also the longer-term plan for Cilium, but IIRC one blocker
on older kernel is the lack (or smaller scope) of dead code elimination.
Today, we still ship the compiler with our image anyway.

> 
> But if bpftool works, by all means.
> 
> >
> > In any case, I don't think we can assume BTF support in Cilium yet
> > (soon, hopefully). I'll probably resend as a bpftool-only patch.
> 
> SGTM.
> 
> >
> > >
> > >   [0] https://patchwork.kernel.org/project/netdevbpf/patch/20211216070442.1492204-2-andrii@kernel.org/
> >
> > [...]