Message-ID: <CAErzpmvDk0Tvr9h772EDZk_4tRtLtAZv-r4yKCxEOM+_gc+G7A@mail.gmail.com>
Date: Thu, 6 Nov 2025 15:31:37 +0800
From: Donglin Peng <dolinux.peng@...il.com>
To: Andrii Nakryiko <andrii.nakryiko@...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 Thu, Nov 6, 2025 at 2:29 AM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> 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.

Thanks. I can implement both approaches, then we can assess their
pros and cons.

>
> > 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?

Thanks, however the bad news is that the btf__type_by_id is indeed called
within the API.

static int btf_permute_shuffle_types(struct btf_permute *p)
{
        struct btf *btf = p->btf;
        const struct btf_type *t;
        __u32 *new_offs = NULL, *ids_map;
        void *nt, *new_types = NULL;
        int i, id, len, err;

        new_offs = calloc(btf->nr_types, sizeof(*new_offs));
        new_types = calloc(btf->hdr->type_len, 1);
        ......
        nt = new_types;
       for (i = 0; i < btf->nr_types; i++) {
                id = p->ids[i];
                ......
                /* must be a valid type ID */
                t = btf__type_by_id(btf, id);  <<<<<<<<<<<<<
                ......
                len = btf_type_size(t);
                memcpy(nt, t, len);
                new_offs[i] = nt - new_types;
                *ids_map = btf->start_id + i;
                nt += len;
        }
......
}

>
> [...]

Powered by blists - more mailing lists