BPF List
 help / color / mirror / Atom feed
From: Fuyu Zhao <zhaofuyu@vivo.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf@vger.kernel.org, eddyz87@gmail.com, alan.maguire@oracle.com
Subject: Re: [PATCH bpf-next v6 1/2] libbpf: support selective kernel module BTF loading via bpf_object_open_opts
Date: Thu, 3 Sep 2026 20:52:39 +0800	[thread overview]
Message-ID: <8506da55-e99b-4f23-93bb-e11229760917@vivo.com> (raw)
In-Reply-To: <CAEf4BzaHTTEob-vmhmcRt1pmED+eBBLWnehEDUk=t7O7GUyyFg@mail.gmail.com>



On 9/3/2026 8:12 AM, Andrii Nakryiko wrote:
> On Mon, Aug 31, 2026 at 6:38 AM Fuyu Zhao <zhaofuyu@vivo.com> wrote:
>>
>> Add btf_module_names and nr_btf_module_names fields to
>> bpf_object_open_opts to support selective kernel module BTF loading.
>>
>> With btf_module_names:
>> - when provided, only the specified kernel module BTFs are loaded;
>> - when an empty list is provided, no module BTFs are loaded;
>> - when NULL, all module BTFs are loaded as before.
>>
>> This avoids unnecessary module BTF loading and reduces BPF object
>> loading time when only a subset of kernel module BTFs is needed.
>>
>> Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> 
> nit: use andrii@kernel.org as my email, thanks
> 

Will do.

>> Signed-off-by: Fuyu Zhao <zhaofuyu@vivo.com>
>> ---
>>  tools/lib/bpf/libbpf.c | 117 +++++++++++++++++++++++++++++++++++++++++
>>  tools/lib/bpf/libbpf.h |  24 ++++++++-
>>  2 files changed, 140 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 514e4e9daa82..e19d0afd2592 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -779,6 +779,9 @@ struct bpf_object {
>>         char *token_path;
>>         int token_fd;
>>
>> +       char **btf_module_names;
>> +       size_t nr_btf_module_names;
> 
> there is entire module related set of fields higher up, put these new
> fields close to them
> 

Sure, I'll move them next to the existing module-related fields.

> but also naming-wise:
>   - use blah_cnt instead of nr_blah naming
>   - we should call this "btf_module_allowlist" and
> btf_module_allowlist_cnt, a bit verbose but explains what it is and
> leaves doors open for denylist, if we ever need it. Please use these
> names internally in bpf_object and in bpf_object_open_opts
> 

Makes sense. I'll rename them to btf_module_allowlist and
btf_module_allowlist_cnt.

> overall, the code as is super verbose and doesn't feel like libbpf
> code, please try to preserve naming/styling/succinct approach as much
> as possible
> 

Understood. I'll simplify the implementation and keep it closer to the
existing libbpf style.

>> +
>>         char path[];
>>  };
>>
>> @@ -5803,6 +5806,103 @@ int bpf_core_add_cands(struct bpf_core_cand *local_cand,
>>         return 0;
>>  }
>>
>> +static void bpf_object_free_btf_module_names(struct bpf_object *obj)
>> +{
>> +       size_t i;
>> +
>> +       if (!obj->btf_module_names)
>> +               return;
>> +
> 
> no need for this check, count should be zero and everything works as
> is without extra guard
> 

Right, I'll drop it.

>> +       for (i = 0; i < obj->nr_btf_module_names; i++)
>> +               zfree(&obj->btf_module_names[i]);
>> +       zfree(&obj->btf_module_names);
>> +       obj->nr_btf_module_names = 0;
> 
> just inline this logic in bpf_object__close()
> 

Sure.

>> +}
>> +
>> +static int bpf_object_init_btf_module_names(struct bpf_object *obj,
>> +                                           const struct bpf_object_open_opts *opts)
> 
> if you look at bpf_object_open(), all the opts processing is done
> inline , let's do the same here. we can do validation and duplicate
> detection before we even get to bpf_object__new. and alloc and strdup
> loop after obj is created
> 

Agreed.

> pw-bot: cr
> 
>> +{
>> +       const char **names;
>> +       size_t i, j, cnt;
>> +       int err;
>> +
>> +       names = OPTS_GET(opts, btf_module_names, NULL);
>> +       if (!names)
>> +               return 0;
> 
> don't exit early, we need to validate that if allowlist is not
> specified, corresponding count is zero and vice versa
> 

Understood.

>> +
>> +       cnt = OPTS_GET(opts, nr_btf_module_names, 0);
>> +
>> +       /*
>> +        * Allocate one entry for an empty list to distinguish it from the
>> +        * default behavior.
>> +        */
>> +       obj->btf_module_names = calloc(cnt ?: 1,
> 
> this count trick is kind of ugly... should we just set count to <0
> instead? we'll still be able to do for loop without any extra
> handling, but -1 vs 0 for count will be a sign of having no or empty
> allowlist
> 

Yes, setting count to -1 can better address this issue.

>> +                                      sizeof(*obj->btf_module_names));
>> +       if (!obj->btf_module_names)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0; i < cnt; i++) {
>> +               if (!names[i] || !names[i][0]) {
>> +                       pr_warn("invalid kernel module BTF name at index %zu\n", i);
> 
> I don't think this will be some common mistake, let's drop all this
> verbosity with pt_warn and so on, just error out
> 

Sure, I'll remove the warning and return the error directly.

>> +                       err = -EINVAL;
>> +                       goto err_out;
>> +               }
>> +
>> +               /*
>> +                * The list is expected to be small, so a simple nested scan is
>> +                * sufficient for duplicate detection.
>> +                */
> 
> unnecessary comment, drop it
> 

Got it.

>> +               for (j = 0; j < i; j++) {
>> +                       if (strcmp(obj->btf_module_names[j], names[i]) == 0) {
>> +                               pr_warn("duplicate kernel module BTF name '%s'\n",
>> +                                       names[i]);
> 
> ditto, just error out, no pr_warn()
> 

Will do.

>> +                               err = -EINVAL;
>> +                               goto err_out;
>> +                       }
>> +               }
>> +
>> +               obj->btf_module_names[i] = strdup(names[i]);
>> +               if (!obj->btf_module_names[i]) {
>> +                       err = -ENOMEM;
>> +                       goto err_out;
>> +               }
>> +
>> +               obj->nr_btf_module_names++;
>> +       }
>> +       return 0;
>> +
>> +err_out:
>> +       bpf_object_free_btf_module_names(obj);
> 
> I don't think we need to clean up here, bpf_object__close() will be
> closed and will take care of this cleanup, keep the logic simple
> 

Agreed.

>> +       return err;
>> +}
>> +
>> +static bool is_module_btf_needed(const struct bpf_object *obj, const char *name)
> 
> let's call this is_btf_mod_allowed()
> 

Sure, I'll rename it to is_btf_mod_allowed().

>> +{
>> +       size_t i;
>> +
>> +       if (!obj->btf_module_names)
>> +               return true;
>> +
>> +       for (i = 0; i < obj->nr_btf_module_names; i++) {
>> +               if (strcmp(obj->btf_module_names[i], name) == 0)
>> +                       return true;
>> +       }
>> +
>> +       pr_debug("skipping module BTF '%s', not in btf_module_names\n", name);
> 
> drop this pr_debug(), do you think it will be very useful? this whole
> feature feels super niche, whoever is going to use it should know what
> they are doing, so I find it unlikely that they might be surprised by
> ignoring kernel module they explicitly excluded
> 

Makes sense. I'll remove it.

>> +       return false;
>> +}
>> +
>> +static bool no_module_btfs_needed(const struct bpf_object *obj)
>> +{
>> +       return obj->btf_module_names && !obj->nr_btf_module_names;
>> +}
>> +
>> +static bool all_needed_module_btfs_loaded(const struct bpf_object *obj)
>> +{
>> +       return obj->btf_module_names &&
>> +              obj->nr_btf_module_names == obj->btf_module_cnt;
>> +}
>> +
> 
> these two helpers are just verbosity, let's get rid of them and do
> corresponding checks inline
> 

Understood. I'll remove both helpers and inline the checks.

>>  static int load_module_btfs(struct bpf_object *obj)
>>  {
>>         struct bpf_btf_info info;
>> @@ -5825,6 +5925,9 @@ static int load_module_btfs(struct bpf_object *obj)
>>         if (!kernel_supports(obj, FEAT_MODULE_BTF))
>>                 return 0;
>>
>> +       if (no_module_btfs_needed(obj))
>> +               return 0;
> 
> under new convention, it will be `if (btf_mod_allowlist_cnt == 0) return 0;`
> 

Right. I'll handle it with the new convention.

>> +
>>         while (true) {
>>                 err = bpf_btf_get_next_id(id, &id);
>>                 if (err && errno == ENOENT)
>> @@ -5867,6 +5970,11 @@ static int load_module_btfs(struct bpf_object *obj)
>>                         continue;
>>                 }
>>
>> +               if (!is_module_btf_needed(obj, name)) {
>> +                       close(fd);
>> +                       continue;
>> +               }
>> +
>>                 btf = btf_get_from_fd(fd, obj->btf_vmlinux);
>>                 err = libbpf_get_error(btf);
>>                 if (err) {
>> @@ -5891,6 +5999,9 @@ static int load_module_btfs(struct bpf_object *obj)
>>                         break;
>>                 }
>>                 obj->btf_module_cnt++;
>> +
>> +               if (all_needed_module_btfs_loaded(obj))
>> +                       break;
>>         }
>>
>>         if (err) {
>> @@ -8508,6 +8619,10 @@ static struct bpf_object *bpf_object_open(const char *path, const void *obj_buf,
>>                 }
>>         }
>>
>> +       err = bpf_object_init_btf_module_names(obj, opts);
>> +       if (err)
>> +               goto out;
>> +
>>         err = bpf_object__elf_init(obj);
>>         err = err ? : bpf_object__elf_collect(obj);
>>         err = err ? : bpf_object__collect_externs(obj);
>> @@ -9629,6 +9744,8 @@ void bpf_object__close(struct bpf_object *obj)
>>                 close(obj->jumptable_maps[i].fd);
>>         zfree(&obj->jumptable_maps);
>>
>> +       bpf_object_free_btf_module_names(obj);
>> +
> 
> just inline that simple clean up logic here without helper function
> 

Will do. I'll inline this.

>>         free(obj);
>>  }
>>
>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> index b965ad571540..1cabf7e46554 100644
>> --- a/tools/lib/bpf/libbpf.h
>> +++ b/tools/lib/bpf/libbpf.h
>> @@ -224,10 +224,32 @@ struct bpf_object_open_opts {
>>          * point (/sys/fs/bpf), in case this default behavior is undesirable.
>>          */
>>         const char *bpf_token_path;
>> +       /*
>> +        * Optional list of kernel module names whose BTFs should be loaded.
>> +        * nr_btf_module_names specifies the number of entries in
>> +        * btf_module_names.
> 
> not really, it's not "what should be loaded" but what is allowed to be
> loaded. quite a difference, because with "should be loaded" we should
> be failing if we don't find all those modules, but I don't think that
> was the intent, was it?
> 

Yes, exactly as you said.

>> +        *
>> +        * With btf_module_names:
>> +        * - when provided, only the BTFs of the specified modules are loaded;
>> +        * - when an empty list is provided, no module BTFs are loaded;
>> +        * - when NULL, all module BTFs are loaded as before.
>> +        *
> 
> also mention that count should be zero for empty or NULL list
> 

Sure, I'll clarify the count requirement in the documentation.

>> +        * The list must contain valid, non-empty module names and must not
>> +        * contain duplicate entries; otherwise -EINVAL is returned.
>> +        *
>> +        * This affects:
> 
> just say that this allowlist affects any place where kernel module BTF
> might be needed by limiting which kernel module BTFs libbpf will
> consult. no need to enumerate all the different places where we use
> BTF, it will get out of sync too easily.
> 

Makes sense. I'll simplify the documentation to describe the general
allowlist behavior.

>> +        * - BPF CO-RE relocations against types defined in modules;
>> +        * - BTF-based resolution of attach targets;
>> +        * - module-qualified tracing multi-attach targets;
>> +        * - struct_ops kernel type resolution;
>> +        * - extern (ksym) resolution for kernel symbols defined in modules.
>> +        */
>> +       const char **btf_module_names;
>> +       size_t nr_btf_module_names;
>>
>>         size_t :0;
>>  };
>> -#define bpf_object_open_opts__last_field bpf_token_path
>> +#define bpf_object_open_opts__last_field nr_btf_module_names
>>
>>  /**
>>   * @brief **bpf_object__open()** creates a bpf_object by opening
>> --
>> 2.34.1
>>


  reply	other threads:[~2026-09-03 12:52 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-31 13:38 [PATCH bpf-next v6 0/2] libbpf: Improve BPF load performance by selectively loading module BTFs Fuyu Zhao
2026-08-31 13:38 ` [PATCH bpf-next v6 1/2] libbpf: support selective kernel module BTF loading via bpf_object_open_opts Fuyu Zhao
2026-08-31 14:46   ` bot+bpf-ci
2026-09-01  3:55     ` Fuyu Zhao
2026-08-31 14:46   ` Alan Maguire
2026-09-01  3:15     ` Fuyu Zhao
2026-09-01  7:33       ` Alan Maguire
2026-09-03  0:12   ` Andrii Nakryiko
2026-09-03 12:52     ` Fuyu Zhao [this message]
2026-08-31 13:38 ` [PATCH bpf-next v6 2/2] selftests/bpf: add tests for selective module BTF loading Fuyu Zhao
2026-08-31 14:46   ` bot+bpf-ci
2026-09-01  3:56     ` Fuyu Zhao
2026-09-03  0:12   ` Andrii Nakryiko
2026-09-03 12:52     ` Fuyu Zhao

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8506da55-e99b-4f23-93bb-e11229760917@vivo.com \
    --to=zhaofuyu@vivo.com \
    --cc=alan.maguire@oracle.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=bpf@vger.kernel.org \
    --cc=eddyz87@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox