| Message ID | 20200107130308.20242-3-mrostecki@opensuse.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpftool/libbpf: Add probe for large INSN limit | expand |
Nit: typo in subject ("secion").
2020-01-07 14:03 UTC+0100 ~ Michal Rostecki <mrostecki@opensuse.org>
> Introduce a new probe section (misc) for probes not related to concrete
> map types, program types, functions or kernel configuration. Introduce a
> probe for large INSN limit as the first one in that section.
>
> Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
> ---
> tools/bpf/bpftool/feature.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
> index 03bdc5b3ac49..d8ce93092c45 100644
> --- a/tools/bpf/bpftool/feature.c
> +++ b/tools/bpf/bpftool/feature.c
> @@ -572,6 +572,18 @@ probe_helpers_for_progtype(enum bpf_prog_type prog_type, bool supported_type,
> printf("\n");
> }
>
> +static void
> +probe_large_insn_limit(const char *define_prefix, __u32 ifindex)
> +{
> + bool res;
> +
> + res = bpf_probe_large_insn_limit(ifindex);
> + print_bool_feature("have_large_insn_limit",
> + "Large complexity and program size limit",
I am not sure we should mention "complexity" here. Although it is
related to program size in the kernel commit you describe, the probe
that is run is only on instruction number. This can make a difference
for offloaded programs: When you probe a device, if kernel has commit
c04c0d2b968a and supports up to 1M instructions, but hardware supports
no more than 4k instructions, you may still benefit from the new value
for BPF_COMPLEXITY_LIMIT_INSNS for complexity, but not for the total
number of available instructions. In that case the probe will fail, and
the message on complexity would not be accurate.
Looks good otherwise, thanks Michal!
Quentin
On Tue, Jan 07, 2020 at 02:36:15PM +0000, Quentin Monnet wrote: > Nit: typo in subject ("secion"). > > 2020-01-07 14:03 UTC+0100 ~ Michal Rostecki <mrostecki@opensuse.org> > > Introduce a new probe section (misc) for probes not related to concrete > > map types, program types, functions or kernel configuration. Introduce a > > probe for large INSN limit as the first one in that section. > > > > Signed-off-by: Michal Rostecki <mrostecki@opensuse.org> > > --- > > tools/bpf/bpftool/feature.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c > > index 03bdc5b3ac49..d8ce93092c45 100644 > > --- a/tools/bpf/bpftool/feature.c > > +++ b/tools/bpf/bpftool/feature.c > > @@ -572,6 +572,18 @@ probe_helpers_for_progtype(enum bpf_prog_type prog_type, bool supported_type, > > printf("\n"); > > } > > > > +static void > > +probe_large_insn_limit(const char *define_prefix, __u32 ifindex) > > +{ > > + bool res; > > + > > + res = bpf_probe_large_insn_limit(ifindex); > > + print_bool_feature("have_large_insn_limit", > > + "Large complexity and program size limit", > > I am not sure we should mention "complexity" here. Although it is > related to program size in the kernel commit you describe, the probe > that is run is only on instruction number. This can make a difference > for offloaded programs: When you probe a device, if kernel has commit > c04c0d2b968a and supports up to 1M instructions, but hardware supports > no more than 4k instructions, you may still benefit from the new value > for BPF_COMPLEXITY_LIMIT_INSNS for complexity, but not for the total > number of available instructions. In that case the probe will fail, and > the message on complexity would not be accurate. > > Looks good otherwise, thanks Michal! > > Quentin Thanks for review! Should I change the description just to "Large program size limit" or "Large instruction limit"? Michal
2020-01-08 13:41 UTC+0000 ~ Michal Rostecki <mrostecki@opensuse.org> > On Tue, Jan 07, 2020 at 02:36:15PM +0000, Quentin Monnet wrote: >> Nit: typo in subject ("secion"). >> >> 2020-01-07 14:03 UTC+0100 ~ Michal Rostecki <mrostecki@opensuse.org> >>> Introduce a new probe section (misc) for probes not related to concrete >>> map types, program types, functions or kernel configuration. Introduce a >>> probe for large INSN limit as the first one in that section. >>> >>> Signed-off-by: Michal Rostecki <mrostecki@opensuse.org> >>> --- >>> tools/bpf/bpftool/feature.c | 18 ++++++++++++++++++ >>> 1 file changed, 18 insertions(+) >>> >>> diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c >>> index 03bdc5b3ac49..d8ce93092c45 100644 >>> --- a/tools/bpf/bpftool/feature.c >>> +++ b/tools/bpf/bpftool/feature.c >>> @@ -572,6 +572,18 @@ probe_helpers_for_progtype(enum bpf_prog_type prog_type, bool supported_type, >>> printf("\n"); >>> } >>> >>> +static void >>> +probe_large_insn_limit(const char *define_prefix, __u32 ifindex) >>> +{ >>> + bool res; >>> + >>> + res = bpf_probe_large_insn_limit(ifindex); >>> + print_bool_feature("have_large_insn_limit", >>> + "Large complexity and program size limit", >> >> I am not sure we should mention "complexity" here. Although it is >> related to program size in the kernel commit you describe, the probe >> that is run is only on instruction number. This can make a difference >> for offloaded programs: When you probe a device, if kernel has commit >> c04c0d2b968a and supports up to 1M instructions, but hardware supports >> no more than 4k instructions, you may still benefit from the new value >> for BPF_COMPLEXITY_LIMIT_INSNS for complexity, but not for the total >> number of available instructions. In that case the probe will fail, and >> the message on complexity would not be accurate. >> >> Looks good otherwise, thanks Michal! >> >> Quentin > > Thanks for review! Should I change the description just to "Large > program size limit" or "Large instruction limit"? > > Michal > I don't really have a preference here, let's keep "program size"? Quentin
diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c index 03bdc5b3ac49..d8ce93092c45 100644 --- a/tools/bpf/bpftool/feature.c +++ b/tools/bpf/bpftool/feature.c @@ -572,6 +572,18 @@ probe_helpers_for_progtype(enum bpf_prog_type prog_type, bool supported_type, printf("\n"); } +static void +probe_large_insn_limit(const char *define_prefix, __u32 ifindex) +{ + bool res; + + res = bpf_probe_large_insn_limit(ifindex); + print_bool_feature("have_large_insn_limit", + "Large complexity and program size limit", + "HAVE_LARGE_INSN_LIMIT", + res, define_prefix); +} + static int do_probe(int argc, char **argv) { enum probe_component target = COMPONENT_UNSPEC; @@ -724,6 +736,12 @@ static int do_probe(int argc, char **argv) probe_helpers_for_progtype(i, supported_types[i], define_prefix, ifindex); + print_end_then_start_section("misc", + "Scanning miscellaneous eBPF features...", + "/*** eBPF misc features ***/", + define_prefix); + probe_large_insn_limit(define_prefix, ifindex); + exit_close_json: if (json_output) { /* End current "section" of probes */
Introduce a new probe section (misc) for probes not related to concrete map types, program types, functions or kernel configuration. Introduce a probe for large INSN limit as the first one in that section. Signed-off-by: Michal Rostecki <mrostecki@opensuse.org> --- tools/bpf/bpftool/feature.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)