On Mon, Aug 28, 2023 at 1:22 PM Martin Kelly
<martin.kelly@xxxxxxxxxxxxxxx> wrote:
>
> On 8/17/23 10:07, Martin Kelly wrote:
> > On 8/17/23 07:17, Daniel Borkmann wrote:
> >> On 8/16/23 7:30 PM, Martin Kelly wrote:
> >>> For map-in-map types, the first time the map is loaded, we get a scary
> >>> error looking like this:
> >>>
> >>> libbpf: bpf_create_map_xattr(map_name):ERROR:
> >>> strerror_r(-524)=22(-524). Retrying without BTF.
> >>>
> >>> On the second try without BTF, everything works fine. However, as this
> >>> is logged at error level, it looks needlessly scary to users. Soften
> >>> this to be at debug level; if the second attempt still fails, we'll
> >>> still get an error as expected.
> >>>
> >>> Signed-off-by: Martin Kelly <martin.kelly@xxxxxxxxxxxxxxx>
> >>
> >> nit: $subj should be for bpf-next instead of bpf
> >
> > I had purposefully sent to "bpf" instead of "bpf-next" as it felt like
> > a fix, but I'm fine with "bpf-next" instead if that's better.
> >
> >>
> >>> ---
> >>> tools/lib/bpf/libbpf.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >>> index b14a4376a86e..0ca0c8d01707 100644
> >>> --- a/tools/lib/bpf/libbpf.c
> >>> +++ b/tools/lib/bpf/libbpf.c
> >>> @@ -5123,7 +5123,7 @@ static int bpf_object__create_map(struct
> >>> bpf_object *obj, struct bpf_map *map, b
> >>> err = -errno;
> >>> cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
> >>> - pr_warn("Error in bpf_create_map_xattr(%s):%s(%d). Retrying
> >>> without BTF.\n",
> >>> + pr_debug("bpf_create_map_xattr(%s):%s(%d). Retrying without
> >>> BTF.\n",
> >>> map->name, cp, err);
> >>
> >> There are also several other places with pr_warns about BTF when
> >> loading an obj. Did
> >> you audit them as well under !BTF kernel? nit: Why changing the fmt
> >> string itself,
> >> looked ok as-is, no?
> >
> > This message is actually printed even for a BTF-supported kernel.
> > Basically, the first call to bpf_create_map_xattr using BTF *always*
> > fails for map-in-map types, printing this message, and then the second
> > always succeeds. So this isn't really about BTF support but simply
> > about an over-zealous message.
> >
> > I changed the format string because calling this an "Error" feels (to
> > me) unnecessarily alarming, given that this is totally normal
> > behavior. I'm OK keeping the "Error in" part if you think that's
> > better. The most important thing to me was that, when the program
> > loads successfully, we shouldn't be logging to stderr and scaring the
> > user.
> >
> > Let me know if you'd like me to keep the "Error in" part for a v2 patch.
> >
> >>
> >> There is also libbpf_needs_btf(obj), perhaps this could be left as
> >> pr_warn similar
> >> as in bpf_object__init_btf() - or would this still trigger in your case?
> >
> > I think this one should stay as a warning, as it looks like the code
> > path is a fatal error, and if you try to load a BTF-requiring program
> > but don't have BTF, that seems like an error to me. This patch was
> > more about an error being logged 100% of the time in a totally normal,
> > non-fatal, BTF-supported case due to the quirks of map-in-map types
> > and BTF.
> >
> >>
> >>> create_attr.btf_fd = 0;
> >>> create_attr.btf_key_type_id = 0;
> >>>
> >>
> >> Thanks,
> >> Daniel
>
> (ping) any thoughts on the above? I'm happy to send a v2 patch but want
> to make sure we're on the same page with what should be in it.
>
map creation failing due to having a BTF information should be a
rather exception than the norm, for two reasons: 1) libbpf sanitizes
BTF, so even older kernels can still retain some (modified) BTF
information, and 2) libbpf drops BTF for maps that don't support BTF
type info for keys/values. So if you see this, then it would be useful
to actually look into why this is happening.
So I'd prefer this to keep pretty visible. Can you try to debug what
makes the kernel reject your BTF information?
