On Mon, Mar 10, 2025 at 8:10 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Fri, Mar 7, 2025 at 7:00 AM 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 | 16 ++++++++++++----
> > tools/lib/bpf/libbpf.c | 3 ++-
> > tools/lib/bpf/libbpf_internal.h | 2 +-
> > 3 files changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index eea99c766a20..c8139c3bc9e0 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -1379,7 +1379,7 @@ 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);
> > __u32 buf_sz = 0, raw_size;
> > @@ -1435,6 +1435,15 @@ int btf_load_into_kernel(struct btf *btf,
> >
> > btf->fd = bpf_btf_load(raw_data, raw_size, &opts);
> > if (btf->fd < 0) {
> > + if (!btf_mandatory) {
> > + err = -errno;
> > + pr_info("BTF loading error: %s\n", errstr(err));
> > +
> > + if (!log_buf && log_level)
> > + pr_info("-- BEGIN BTF LOAD LOG ---\n%s\n-- END BTF LOAD LOG --\n", buf);
>
> I'm not a fan of duplicating this. Wouldn't something along the
> following lines work as well?
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index eea99c766a20..fc06f3a8e8d7 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -1436,18 +1436,19 @@ 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) {
> + if (log_level == 0 && (log_buf || btf_mandatory)) {
> 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)
> + if (!log_buf && btf_mandatory && errno == ENOSPC &&
> buf_sz <= UINT_MAX / 2)
> goto retry_load;
>
> err = -errno;
> - pr_warn("BTF loading error: %s\n", errstr(err));
> + __pr(btf_mandatory ? LIBBPF_WARN : LIBBPF_INFO,
> + "BTF loading error: %s\n", errstr(err));
> /* don't print out contents of custom log_buf */
> if (!log_buf && buf[0])
> pr_warn("-- BEGIN BTF LOAD LOG ---\n%s\n-- END
> BTF LOAD LOG --\n", buf);
>
>
> pw-bot: cr
>
Let me try another version where we don't complicate the conditionals too much
and don't repeat the log messages.
> > + goto done;
> > + }
> > +
> > /* time to turn on verbose mode and try again */
> > if (log_level == 0) {
> > log_level = 1;
> > @@ -1448,8 +1457,7 @@ int btf_load_into_kernel(struct btf *btf,
> >
> > err = -errno;
> > pr_warn("BTF loading error: %s\n", errstr(err));
> > - /* don't print out contents of custom log_buf */
> > - if (!log_buf && buf[0])
> > + if (!log_buf && log_level)
> > pr_warn("-- BEGIN BTF LOAD LOG ---\n%s\n-- END BTF LOAD LOG --\n", buf);
> > }
> >
> > @@ -1460,7 +1468,7 @@ int btf_load_into_kernel(struct btf *btf,
> >
> > int btf__load_into_kernel(struct btf *btf)
> > {
> > - return btf_load_into_kernel(btf, NULL, 0, 0, 0);
> > + return btf_load_into_kernel(btf, NULL, 0, 0, 0, true);
> > }
> >
> > int btf__fd(const struct btf *btf)
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 8e32286854ef..2cb3f067a12e 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -3604,9 +3604,10 @@ static int bpf_object__sanitize_and_load_btf(struct bpf_object *obj)
> > */
> > btf__set_fd(kern_btf, 0);
> > } else {
> > + btf_mandatory = kernel_needs_btf(obj);
> > /* currently BPF_BTF_LOAD only supports log_level 1 */
> > err = btf_load_into_kernel(kern_btf, obj->log_buf, obj->log_size,
> > - obj->log_level ? 1 : 0, obj->token_fd);
> > + obj->log_level ? 1 : 0, obj->token_fd, btf_mandatory);
> > }
> > if (sanitize) {
> > if (!err) {
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > index de498e2dd6b0..f1de2ba462c3 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -408,7 +408,7 @@ int libbpf__load_raw_btf(const char *raw_types, size_t types_len,
> > int token_fd);
> > 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);
> >
> > struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf);
> > void btf_get_kernel_prefix_kind(enum bpf_attach_type attach_type,
> > --
> > 2.43.5
> >
