Message-ID: <CAErzpmuCQUTZTnCkNoL0ZcHsvXmu_m0sp7X+Lt8K4tYwQ7BiMg@mail.gmail.com>
Date: Sat, 20 Dec 2025 16:39:03 +0800
From: Donglin Peng <dolinux.peng@...il.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: ast@...nel.org, eddyz87@...il.com, zhangxiaoqin@...omi.com, 
	ihor.solodrai@...ux.dev, linux-kernel@...r.kernel.org, bpf@...r.kernel.org, 
	pengdonglin <pengdonglin@...omi.com>, Alan Maguire <alan.maguire@...cle.com>
Subject: Re: [PATCH bpf-next v10 01/13] libbpf: Add BTF permutation support
 for type reordering

On Sat, Dec 20, 2025 at 1:07 AM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Thu, Dec 18, 2025 at 7:15 PM Donglin Peng <dolinux.peng@...il.com> wrote:
> >
> > On Fri, Dec 19, 2025 at 7:02 AM Andrii Nakryiko
> > <andrii.nakryiko@...il.com> wrote:
> > >
> > > On Thu, Dec 18, 2025 at 3:31 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.
> > > >
> > > > 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: Ihor Solodrai <ihor.solodrai@...ux.dev>
> > > > Cc: Xiaoqin Zhang <zhangxiaoqin@...omi.com>
> > > > Signed-off-by: pengdonglin <pengdonglin@...omi.com>
> > > > Acked-by: Eduard Zingerman <eddyz87@...il.com>
> > > > ---
> > > >  tools/lib/bpf/btf.c      | 119 +++++++++++++++++++++++++++++++++++++++
> > > >  tools/lib/bpf/btf.h      |  36 ++++++++++++
> > > >  tools/lib/bpf/libbpf.map |   1 +
> > > >  3 files changed, 156 insertions(+)
> > > >
>
> [...]
>
> > > > +/**
> > > > + * @brief **btf__permute()** performs in-place BTF type rearrangement
> > > > + * @param btf BTF object to permute
> > > > + * @param id_map Array mapping original type IDs to new IDs
> > > > + * @param id_map_cnt Number of elements in @id_map
> > > > + * @param opts Optional parameters for BTF extension updates
> > > > + * @return 0 on success, negative error code on failure
> > > > + *
> > > > + * **btf__permute()** rearranges BTF types according to the specified ID mapping.
> > > > + * The @id_map array defines the new type ID for each original type ID.
> > > > + *
> > > > + * @id_map must include all types from ID `start_id` to `btf__type_cnt(btf) - 1`.
> > > > + * @id_map_cnt should be `btf__type_cnt(btf) - start_id`
> > > > + * The mapping is defined as: `id_map[original_id - start_id] = new_id`
> > >
> > > Would you mind paying attention to the feedback I left in [0]? Thank you.
> >
> > Apologies for the delayed response, I would like to hear if someone has a
> > different idea.
>
> Delayed response?.. You ignored my feedback and never even replied to
> it. And then posted a new revision two days later, while still not
> taking the feedback into account. This is not a delayed response, it's
> ignoring the feedback. You don't have to agree with all the feedback,
> but you have to respond to the feedback you disagree with and provide
> your arguments, not just silently disregard it.

Thank you for the reminder, and I sincerely apologize for
my mistake in handling the feedback. You are absolutely right.
I should not have posted a new revision without first replying
to your comments on the previous version. The correct process,
as outlined in the kernel development documentation and community
norms, is to address all feedback—either by implementing the
suggested changes or by providing a clear explanation if I
disagree.

I appreciate you taking the time to review my work and for
holding me to the community standard. I will ensure this
does not happen again.

>
> >
> > >
> > > The contract should be id_map[original_id] = new_id for base BTF and
> > > id_map[original_id - btf__type_cnt(base_btf)] = new_id for split BTF.
> > > Special BTF type #0 (VOID) is considered to be part of base BTF,
> > > having id_map[0] = 0 is easy to check and enforce. And then it leaves
> > > us with a simple and logical rule for id_map. For split BTF we make
> > > necessary type ID shifts to avoid tons of wasted memory. But for base
> > > BTF there is no need to shift anything. So mapping the original type
> > > #X to #Y is id_map[X] = Y. Literally, "map X to Y", as simple as that.
> > >
> > >   [0] https://lore.kernel.org/bpf/CAEf4BzY_k721TBfRSUeq5mB-7fgJhVKCeXVKO-W2EjQ0aS9AgA@mail.gmail.com/
> >
> > Thanks. I implemented the approach in v6, but it had inconsistent internal
> > details for base and split BTF. It seems we prioritize external contract
> > consistency over internal inconsistencies, so I’ll revert to the v6 approach
> > and refine it for clarity.
>
> Yes, we always prioritize external contract consistency, of course!
> You are overpivoting on *internal implementation detail* of base BTF's
> start_id being set to 1, which is convenient in some other places due
> to type_offs shifted by one mapping due to &btf_void special handling.
> We can always change that, if we wanted, but this shouldn't spill into
> public API though. But conceptually BTF types start at type #0, which
> is defined to be VOID and is not user controlled.

Thanks,  I understood.

>
>
> This is not much of a complication or inconsistency:
>
> type_shift = base_btf ? btf__type_cnt(base_btf) : 0;
> id_map[type_id - type_shift] = ...

Thank you,  I agree and will do it in the next version.

>
>
> >
> > >
> > > > + *
> > > > + * For base BTF, its `start_id` is fixed to 1, i.e. the VOID type can
> > > > + * not be redefined or remapped and its ID is fixed to 0.
> > > > + *
> > > > + * For split BTF, its `start_id` can be retrieved by calling
> > > > + * `btf__type_cnt(btf__base_btf(btf))`.
> > > > + *
> > > > + * On error, returns negative error code and sets errno:
> > > > + *   - `-EINVAL`: Invalid parameters or ID mapping (duplicates, out-of-range)
> > > > + *   - `-ENOMEM`: Memory allocation failure
> > > > + */
> > > > +LIBBPF_API int btf__permute(struct btf *btf, __u32 *id_map, __u32 id_map_cnt,
> > > > +                           const struct btf_permute_opts *opts);
> > > > +
> > > >  struct btf_dump;
> > > >
> > > >  struct btf_dump_opts {
> > > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > > > index 84fb90a016c9..d18fbcea7578 100644
> > > > --- a/tools/lib/bpf/libbpf.map
> > > > +++ b/tools/lib/bpf/libbpf.map
> > > > @@ -453,4 +453,5 @@ LIBBPF_1.7.0 {
> > > >                 bpf_map__exclusive_program;
> > > >                 bpf_prog_assoc_struct_ops;
> > > >                 bpf_program__assoc_struct_ops;
> > > > +               btf__permute;
> > > >  } LIBBPF_1.6.0;
> > > > --
> > > > 2.34.1
> > > >

Powered by blists - more mailing lists