On Wed, Mar 12, 2025 at 3:31 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Wed, Mar 12, 2025 at 12:21 PM Jordan Rome <linux@xxxxxxxxxxxxxx> wrote:
> >
> > On Wed, Mar 12, 2025 at 2:40 PM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> > >
> > > On Tue, Mar 11, 2025 at 6:04 PM Jordan Rome <linux@xxxxxxxxxxxxxx> wrote:
> > > >
> > > > For kernels where btf is not mandatory
> > > > we should log loading errors with `pr_info`
> > > > and not retry where we increase the log level
> > > > as this is just added noise.
> > > >
> > > > Signed-off-by: Jordan Rome <linux@xxxxxxxxxxxxxx>
> > > > ---
> > > > tools/lib/bpf/btf.c | 36 ++++++++++++++++++---------------
> > > > tools/lib/bpf/libbpf.c | 3 ++-
> > > > tools/lib/bpf/libbpf_internal.h | 2 +-
> > > > 3 files changed, 23 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > > > index eea99c766a20..7da4807451bb 100644
> > > > --- a/tools/lib/bpf/btf.c
> > > > +++ b/tools/lib/bpf/btf.c
> > > > @@ -1379,9 +1379,10 @@ static void *btf_get_raw_data(const struct btf *btf, __u32 *size, bool swap_endi
> > > >
> > > > int btf_load_into_kernel(struct btf *btf,
> > > > char *log_buf, size_t log_sz, __u32 log_level,
> > > > - int token_fd)
> > > > + int token_fd, bool btf_mandatory)
> > > > {
> > > > LIBBPF_OPTS(bpf_btf_load_opts, opts);
> > > > + enum libbpf_print_level print_level;
> > > > __u32 buf_sz = 0, raw_size;
> > > > char *buf = NULL, *tmp;
> > > > void *raw_data;
> > > > @@ -1435,22 +1436,25 @@ int btf_load_into_kernel(struct btf *btf,
> > > >
> > > > btf->fd = bpf_btf_load(raw_data, raw_size, &opts);
> > > > if (btf->fd < 0) {
> > > > - /* time to turn on verbose mode and try again */
> > > > - if (log_level == 0) {
> > > > - log_level = 1;
> > > > - goto retry_load;
> > > > + if (btf_mandatory) {
> > > > + /* time to turn on verbose mode and try again */
> > > > + if (log_level == 0) {
> > > > + log_level = 1;
> > > > + goto retry_load;
> > > > + }
> > > > + /* only retry if caller didn't provide custom log_buf, but
> > > > + * make sure we can never overflow buf_sz
> > > > + */
> > > > + if (!log_buf && errno == ENOSPC && buf_sz <= UINT_MAX / 2)
> > >
> > > Original behavior was to go from log_level 0 to log_level 1 when the
> > > user provided custom log_buf, which would happen even for
> > > non-btf_mandatory case. I'd like to not change that behavior.
> > >
> >
> > I don't quite understand why we want to increase the log level
> > if btf is not mandatory. Users will still get an info message that
> > btf failed to load and if they are still curious, they can increase
> > the log level themselves right? The goal of this patch is to reduce
> > log noise in cases where btf fails to load and is not mandatory.
>
> Program's BTF is almost always not mandatory, so we'll basically never
> log BTF verification failure (only for struct_ops stuff), even with
> user-provided custom log buffer, which makes this whole logic much
> less useful.
>
> Perhaps we just need to keep existing logic as is? It's not expected
> for BTF to fail to be loaded into the kernel, even for old kernels.
> libbpf does BTF sanitization for that reason. So when this BTF loading
> fails, it usually does indicate a real issue.
>
> That's exactly what happened with the original issue that prompted
> your patches. And that's a good thing that the user was bothered by
> those warnings, which eventually led to fixing an issue with a missing
> patch in their backported kernel, right?
>
> Let's just keep things as is for now. I'd rather users complain about
> this rather than these BTF upload failures go unnoticed forever.
>
Fair enough.
> >
> > > Did you find some problem with the code I proposed a few emails back?
> >
> > Truth be told, I didn't like the added complexity in the conditionals.
> > I tried something similar in an earlier version and it led to a SEGFAULT
> > when trying to access `buf[0]` which had not been allocated.
> >
> > > If not, why not do that instead and preserve that custom log_buf and
> > > log_level upgrade behavior?
> > >
> > > pw-bot: cr
> > >
>
> [...]
