Message-ID: <CAEf4Bza+pHVSGTC2vcjF-DmsVxKq2Ksq321E9CJEGdyT8hQn3g@mail.gmail.com>
Date: Wed, 5 Nov 2025 10:29:06 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Donglin Peng <dolinux.peng@...il.com>
Cc: ast@...nel.org, linux-kernel@...r.kernel.org, bpf@...r.kernel.org, 
	Eduard Zingerman <eddyz87@...il.com>, Alan Maguire <alan.maguire@...cle.com>, Song Liu <song@...nel.org>, 
	pengdonglin <pengdonglin@...omi.com>
Subject: Re: [RFC PATCH v4 2/7] libbpf: Add BTF permutation support for type reordering

On Wed, Nov 5, 2025 at 4:53 AM Donglin Peng <dolinux.peng@...il.com> wrote:
>
> On Wed, Nov 5, 2025 at 8:11 AM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> >
> > On Tue, Nov 4, 2025 at 5:40 AM Donglin Peng <dolinux.peng@...il.com> wrote:
> > >
> > > From: pengdonglin <pengdonglin@...omi.com>
> > >
> > > Introduce btf__permute() API to allow in-place rearrangement of BTF types.
> > > This function reorganizes BTF type order according to a provided array of
> > > type IDs, updating all type references to maintain consistency.
> > >
> > > The permutation process involves:
> > > 1. Shuffling types into new order based on the provided ID mapping
> > > 2. Remapping all type ID references to point to new locations
> > > 3. Handling BTF extension data if provided via options
> > >
> > > This is particularly useful for optimizing type locality after BTF
> > > deduplication or for meeting specific layout requirements in specialized
> > > use cases.
> > >
> > > Cc: Eduard Zingerman <eddyz87@...il.com>
> > > Cc: Alexei Starovoitov <ast@...nel.org>
> > > Cc: Andrii Nakryiko <andrii.nakryiko@...il.com>
> > > Cc: Alan Maguire <alan.maguire@...cle.com>
> > > Cc: Song Liu <song@...nel.org>
> > > Signed-off-by: pengdonglin <pengdonglin@...omi.com>
> > > Signed-off-by: Donglin Peng <dolinux.peng@...il.com>
> > > ---
> > >  tools/lib/bpf/btf.c      | 161 +++++++++++++++++++++++++++++++++++++++
> > >  tools/lib/bpf/btf.h      |  34 +++++++++
> > >  tools/lib/bpf/libbpf.map |   1 +
> > >  3 files changed, 196 insertions(+)
> > >
> > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > > index 5e1c09b5dce8..3bc03f7fe31f 100644
> > > --- a/tools/lib/bpf/btf.c
> > > +++ b/tools/lib/bpf/btf.c
> > > @@ -5830,3 +5830,164 @@ int btf__relocate(struct btf *btf, const struct btf *base_btf)
> > >                 btf->owns_base = false;
> > >         return libbpf_err(err);
> > >  }
> > > +
> > > +struct btf_permute {
> > > +       /* .BTF section to be permuted in-place */
> > > +       struct btf *btf;
> > > +       struct btf_ext *btf_ext;
> > > +       /* Array of type IDs used for permutation. The array length must equal
> >
> > /*
> >  * Use this comment style
> >  */
>
> Thanks.
>
> >
> > > +        * the number of types in the BTF being permuted, excluding the special
> > > +        * void type at ID 0. For split BTF, the length corresponds to the
> > > +        * number of types added on top of the base BTF.
> >
> > many words, but what exactly ids[i] means is still not clear, actually...
>
> Thanks. I'll clarify the description. Is the following parameter
> explanation acceptable?
>
> @param ids Array containing original type IDs (excluding VOID type ID
> 0) in user-defined order.
>                     The array size must match btf->nr_types, which

Users don't have access to btf->nr_types, so referring to it in API
description seems wrong.

But also, this all will change if we allow removing types, because
then array size might be smaller. But is it intentionally smaller or
user made a mistake? Let's go with the ID map approach, please.

> also excludes VOID type ID 0.
>
>
> >
> > > +        */
> > > +       __u32 *ids;
> > > +       /* Array of type IDs used to map from original type ID to a new permuted
> > > +        * type ID, its length equals to the above ids */
> >
> > wrong comment style
>
> Thanks, I will fix it in the next version.
>
> >
> > > +       __u32 *map;
> >
> > "map" is a bit generic. What if we use s/ids/id_map/ and
> > s/map/id_map_rev/ (for reverse)? I'd use "id_map" naming in the public
> > API to make it clear that it's a mapping of IDs, not just some array
> > of IDs.
>
> Thank you for the suggestion. While I agree that renaming 'map' to 'id_map'
> makes sense for clarity, but 'ids' seems correct as it denotes a collection of
> IDs, not a mapping structure.
>
> >
> > > +};
> > > +
> > > +static int btf_permute_shuffle_types(struct btf_permute *p);
> > > +static int btf_permute_remap_types(struct btf_permute *p);
> > > +static int btf_permute_remap_type_id(__u32 *type_id, void *ctx);
> > > +
> > > +int btf__permute(struct btf *btf, __u32 *ids, const struct btf_permute_opts *opts)
> >
> > Let's require user to pass id_map_cnt in addition to id_map itself.
> > It's easy to get this wrong (especially with that special VOID 0 type
> > that has to be excluded, which I can't even make up my mind if that's
> > a good idea or not), so having user explicitly say what they think is
> > necessary for permutation is good.
>
> Thank you for your suggestion. However, I am concerned that introducing
> an additional `id_map_cnt` parameter could increase complexity. Specifically,
> if `id_map_cnt` is less than `btf->nr_types`, we might need to consider whether
> to resize the BTF. This could lead to missing types, potential ID remapping
> failures, or even require BTF re-deduplication if certain name strings are no
> longer referenced by any types.
>

No, if the user provided a wrong id_map_cnt, it's an error and we
return -EINVAL. No resizing.

> >
> > > +{
> > > +       struct btf_permute p;
> > > +       int i, err = 0;
> > > +       __u32 *map = NULL;
> > > +
> > > +       if (!OPTS_VALID(opts, btf_permute_opts) || !ids)
> >

[...]

> > > +               goto done;
> > > +       }
> > > +
> > > +done:
> > > +       free(map);
> > > +       return libbpf_err(err);
> > > +}
> > > +
> > > +/* Shuffle BTF types.
> > > + *
> > > + * Rearranges types according to the permutation map in p->ids. The p->map
> > > + * array stores the mapping from original type IDs to new shuffled IDs,
> > > + * which is used in the next phase to update type references.
> > > + *
> > > + * Validates that all IDs in the permutation array are valid and unique.
> > > + */
> > > +static int btf_permute_shuffle_types(struct btf_permute *p)
> > > +{
> > > +       struct btf *btf = p->btf;
> > > +       const struct btf_type *t;
> > > +       __u32 *new_offs = NULL, *map;
> > > +       void *nt, *new_types = NULL;
> > > +       int i, id, len, err;
> > > +
> > > +       new_offs = calloc(btf->nr_types, sizeof(*new_offs));
> >
> > we don't really need to allocate memory and maintain this, we can just
> > shift types around and then do what btf_parse_type_sec() does -- just
> > go over types one by one and calculate offsets, and update them
> > in-place inside btf->type_offs
>
> Thank you for the suggestion. However, this approach is not viable because
> the `btf__type_by_id()` function relies critically on the integrity of the
> `btf->type_offs` data structure. Attempting to modify `type_offs` through
> in-place operations could corrupt memory and lead to segmentation faults
> due to invalid pointer dereferencing.

Huh? By the time this API returns, we'll fix up type_offs, users will
never notice. And to recalculate new type_offs we don't need
type_offs. One of us is missing something important, what is it?

[...]

Powered by blists - more mailing lists