On Fri, Dec 17, 2021 at 11:22 AM Paul Chaignon <paul@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > Signed-off-by: Paul Chaignon <paul@xxxxxxxxxxxxx>
> > > ---
> > > 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.
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@xxxxxxxxxx/
>
> [...]
