On Tue, May 26, 2020 at 11:35 AM Jesper Dangaard Brouer <brouer@xxxxxxxxxx> wrote: > > On Tue, 26 May 2020 11:16:50 -0700 > Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > > On Tue, May 26, 2020 at 7:59 AM Jesper Dangaard Brouer > > <brouer@xxxxxxxxxx> wrote: > > > > > > When a BPF-map type doesn't support having a BTF info associated, the > > > bpf_map_ops->map_check_btf is set to map_check_no_btf(). This function > > > map_check_no_btf() currently returns -ENOTSUPP, which result in a very > > > confusing error message in libbpf, see below. > > > > > > The errno ENOTSUPP is part of the kernels internal errno in file > > > include/linux/errno.h. As is stated in the file, these "should never be seen > > > by user programs." > > > > > > Choosing errno EUCLEAN instead, which translated to "Structure needs > > > cleaning" by strerror(3). This hopefully leads people to think about data > > > structures which BTF is all about. > > > > How about instead of tweaking error code > > Regardless we still need to change the error code used, as strerror(3) > cannot convert it to something meaningful. As the code comment says > this errno "should never be seen by user programs.". "Structure needs cleaning" doesn't really make sense to me either, to be honest. If -524 bothers you so much, maybe switch to EOPNOTSUPP (with alias ENOTSUP in user-space)? Or we can just handle this case better in libbpf for better user error? Either way there will be a lot of old kernels around that will still produce such error. > > My notes are here: > https://github.com/xdp-project/xdp-project/blob/BTF01-notes.public/areas/core/BTF_01_notes.org > > > we actually just add support > > for BTF key/values for all maps. For special maps, we can just enforce > > that BTF is 4-byte integer (or typedef of that), so that in practice > > you'll be defining it as: > > > > struct { > > __uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY); > > __type(key, u32); > > __type(value, u32); > > } my_map SEC(".maps"); > > > > and it will just work? > > Nope, this will also fail. Why? If kernel supports 4-byte integers as BTF types for key/value for such maps, then what would fail in this case? > > I'm all for adding support for more BPF-maps in follow up patches. I > will soon be adding support for cpumap and devmap. And I will likely > be asking all kind of weird questions, I hope I can get some help from > you to figure out all the details ;-) Of course. > > > > > > > Before this change end-users of libbpf will see: > > > libbpf: Error in bpf_create_map_xattr(cpu_map):ERROR: strerror_r(-524)=22(-524). Retrying without BTF. > > > > > > After this change end-users of libbpf will see: > > > libbpf: Error in bpf_create_map_xattr(cpu_map):Structure needs cleaning(-117). Retrying without BTF. > > > > > > Fixes: e8d2bec04579 ("bpf: decouple btf from seq bpf fs dump and enable more maps") > > > Signed-off-by: Jesper Dangaard Brouer <brouer@xxxxxxxxxx> > > > --- > > > kernel/bpf/syscall.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > > index d13b804ff045..ecde7d938421 100644 > > > --- a/kernel/bpf/syscall.c > > > +++ b/kernel/bpf/syscall.c > > > @@ -732,7 +732,7 @@ int map_check_no_btf(const struct bpf_map *map, > > > const struct btf_type *key_type, > > > const struct btf_type *value_type) > > > { > > > - return -ENOTSUPP; > > > + return -EUCLEAN; > > > } > > > > > > static int map_check_btf(struct bpf_map *map, const struct btf *btf, > > > > > -- > Best regards, > Jesper Dangaard Brouer > MSc.CS, Principal Kernel Engineer at Red Hat > LinkedIn: http://www.linkedin.com/in/brouer >
