Hi Quentin,
Thanks for your review.
On 2022-03-09 14:31:18 +0000, Quentin Monnet wrote:
> 2022-03-09 09:54 UTC+0100 ~ Niklas Söderlund <niklas.soderlund@xxxxxxxxxxxx>
> > Commit 1a56c18e6c2e4e74 ("bpftool: Stop supporting BPF offload-enabled
> > feature probing") removed the support to probe for BPF offload features.
> > This is still something that is useful for NFP NIC that can support
> > offloading of BPF programs.
> >
> > The reason for the dropped support was that libbpf starting with v1.0
> > would drop support for passing the ifindex to the BPF prog/map/helper
> > feature probing APIs. In order to keep this useful feature for NFP
> > restore the functionality by moving it directly into bpftool.
> >
> > The code restored is a simplified version of the code that existed in
> > libbpf which supposed passing the ifindex. The simplification is that it
> > only targets the cases where ifindex is given and call into libbpf for
> > the cases where it's not.
>
> [...]
>
> > +static bool probe_prog_type_ifindex(enum bpf_prog_type prog_type, __u32 ifindex)
> > +{
> > + struct bpf_insn insns[2] = {
> > + BPF_MOV64_IMM(BPF_REG_0, 0),
> > + BPF_EXIT_INSN()
> > + };
> > +
> > + switch (prog_type) {
> > + case BPF_PROG_TYPE_SCHED_CLS:
> > + /* nfp returns -EINVAL on exit(0) with TC offload */
> > + insns[0].imm = 2;
> > + break;
> > + case BPF_PROG_TYPE_XDP:
> > + break;
> > + default:
> > + return false;
> > + }
>
> Looking again at this block, you can remove this switch/case completely.
> Moving 0 by default into R0 was best in generic libbpf's probes because
> there were a number of types that wouldn't accept 2 as a return value;
> but here you can just return 2 all the time, for XDP this will mean
> XDP_PASS and the nfp accepts it. (Do keep the comment on nfp returning
> -EINVAL on exit(0), though, please.)
This make sens and I will do so in a v3. I appreciate your expertise in
this area, it allows this patch to do more then just restore the code
from libbpf with as few modifications as possible out of fear of breaking
things.
>
> > +
> > + return probe_prog_load_ifindex(prog_type, insns, ARRAY_SIZE(insns),
> > + NULL, 0, ifindex);
> > +}
> > +
> > static void
> > probe_prog_type(enum bpf_prog_type prog_type, bool *supported_types,
> > const char *define_prefix, __u32 ifindex)
> > @@ -488,11 +564,19 @@ probe_prog_type(enum bpf_prog_type prog_type, bool *supported_types,
> > bool res;
> >
> > if (ifindex) {
> > - p_info("BPF offload feature probing is not supported");
> > - return;
> > + switch (prog_type) {
> > + case BPF_PROG_TYPE_SCHED_CLS:
> > + case BPF_PROG_TYPE_XDP:
> > + break;
> > + default:
> > + return;
> > + }
> > +
> > + res = probe_prog_type_ifindex(prog_type, ifindex);
> > + } else {
> > + res = libbpf_probe_bpf_prog_type(prog_type, NULL);
> > }
> >
> > - res = libbpf_probe_bpf_prog_type(prog_type, NULL);
> > #ifdef USE_LIBCAP
> > /* Probe may succeed even if program load fails, for unprivileged users
> > * check that we did not fail because of insufficient permissions
> > @@ -521,6 +605,35 @@ probe_prog_type(enum bpf_prog_type prog_type, bool *supported_types,
> > define_prefix);
> > }
> >
> > +static bool probe_map_type_ifindex(enum bpf_map_type map_type, __u32 ifindex)
> > +{
> > + LIBBPF_OPTS(bpf_map_create_opts, opts);
> > + int key_size, value_size, max_entries;
> > + int fd;
> > +
> > + opts.map_ifindex = ifindex;
> > +
> > + key_size = sizeof(__u32);
> > + value_size = sizeof(__u32);
> > + max_entries = 1;
> > +
> > + switch (map_type) {
> > + case BPF_MAP_TYPE_HASH:
> > + case BPF_MAP_TYPE_ARRAY:
> > + break;
> > + default:
> > + return false;
> > + }
>
> This switch/case should probably not be here. Either skip
> probe_map_type_ifindex() entirely if you assume other map types are not
> supported, or just probe all types for real.
>
> Speaking of which, we agreed yesterday on probing for all map types. But
> I think my suggestion was wrong (apologies!), because the simplified
> probes that you're adding back does not contain the necessary
> adjustments to work with all map types (the switch(map_type) in libbpf's
> probe_map_create()). So we should probably probe just hash/array maps,
> but move the switch/case above to probe_map_type(), like we do for prog
> types, to avoid printing any output for other map types. This will
> prevent other drivers to probe for additional map types, but a large
> portion of those probes would be broken, so it's for the best. We can
> always extend probe_map_type_ifindex() in the future to support more
> types if necessary.
Make sens, will do so in v3.
>
> > +
> > + fd = bpf_map_create(map_type, NULL, key_size, value_size, max_entries,
> > + &opts);
> > +
> > + if (fd >= 0)
> > + close(fd);
> > +
> > + return fd >= 0;
> > +}
> > +
> > static void
> > probe_map_type(enum bpf_map_type map_type, const char *define_prefix,
> > __u32 ifindex)
--
Regards,
Niklas Söderlund
