From: pengdonglin <pengdonglin@xiaomi.com>
Refactor btf_dedup_remap_types() by extracting its core logic into a new
btf_remap_types() helper function. This eliminates code duplication
and improves modularity while maintaining the same functionality.
The new function encapsulates iteration over BTF types and BTF ext
sections, accepting a callback for flexible type ID remapping. This
makes the type remapping logic more maintainable and reusable.
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Alan Maguire <alan.maguire@oracle.com>
Cc: Song Liu <song@kernel.org>
Signed-off-by: pengdonglin <pengdonglin@xiaomi.com>
Signed-off-by: Donglin Peng <dolinux.peng@gmail.com>
---
tools/lib/bpf/btf.c | 63 +++++++++++++++++----------------
tools/lib/bpf/libbpf_internal.h | 1 +
2 files changed, 33 insertions(+), 31 deletions(-)
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 18907f0fcf9f..5e1c09b5dce8 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -3400,6 +3400,37 @@ int btf_ext__set_endianness(struct btf_ext *btf_ext, enum btf_endianness endian)
return 0;
}
+static int btf_remap_types(struct btf *btf, struct btf_ext *btf_ext,
+ btf_remap_type_fn visit, void *ctx)
+{
+ int i, r;
+
+ for (i = 0; i < btf->nr_types; i++) {
+ struct btf_type *t = btf_type_by_id(btf, btf->start_id + i);
+ struct btf_field_iter it;
+ __u32 *type_id;
+
+ r = btf_field_iter_init(&it, t, BTF_FIELD_ITER_IDS);
+ if (r)
+ return r;
+
+ while ((type_id = btf_field_iter_next(&it))) {
+ r = visit(type_id, ctx);
+ if (r)
+ return r;
+ }
+ }
+
+ if (!btf_ext)
+ return 0;
+
+ r = btf_ext_visit_type_ids(btf_ext, visit, ctx);
+ if (r)
+ return r;
+
+ return 0;
+}
+
struct btf_dedup;
static struct btf_dedup *btf_dedup_new(struct btf *btf, const struct btf_dedup_opts *opts);
@@ -5320,37 +5351,7 @@ static int btf_dedup_remap_type_id(__u32 *type_id, void *ctx)
*/
static int btf_dedup_remap_types(struct btf_dedup *d)
{
- int i, r;
-
- for (i = 0; i < d->btf->nr_types; i++) {
- struct btf_type *t = btf_type_by_id(d->btf, d->btf->start_id + i);
- struct btf_field_iter it;
- __u32 *type_id;
-
- r = btf_field_iter_init(&it, t, BTF_FIELD_ITER_IDS);
- if (r)
- return r;
-
- while ((type_id = btf_field_iter_next(&it))) {
- __u32 resolved_id, new_id;
-
- resolved_id = resolve_type_id(d, *type_id);
- new_id = d->hypot_map[resolved_id];
- if (new_id > BTF_MAX_NR_TYPES)
- return -EINVAL;
-
- *type_id = new_id;
- }
- }
-
- if (!d->btf_ext)
- return 0;
-
- r = btf_ext_visit_type_ids(d->btf_ext, btf_dedup_remap_type_id, d);
- if (r)
- return r;
-
- return 0;
+ return btf_remap_types(d->btf, d->btf_ext, btf_dedup_remap_type_id, d);
}
/*
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 35b2527bedec..b09d6163f5c3 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -582,6 +582,7 @@ int btf_ext_visit_type_ids(struct btf_ext *btf_ext, type_id_visit_fn visit, void
int btf_ext_visit_str_offs(struct btf_ext *btf_ext, str_off_visit_fn visit, void *ctx);
__s32 btf__find_by_name_kind_own(const struct btf *btf, const char *type_name,
__u32 kind);
+typedef int (*btf_remap_type_fn)(__u32 *type_id, void *ctx);
/* handle direct returned errors */
static inline int libbpf_err(int ret)
--
2.34.1On Tue, Nov 4, 2025 at 5:40 AM Donglin Peng <dolinux.peng@gmail.com> wrote:
>
> From: pengdonglin <pengdonglin@xiaomi.com>
>
> Refactor btf_dedup_remap_types() by extracting its core logic into a new
> btf_remap_types() helper function. This eliminates code duplication
> and improves modularity while maintaining the same functionality.
>
> The new function encapsulates iteration over BTF types and BTF ext
> sections, accepting a callback for flexible type ID remapping. This
> makes the type remapping logic more maintainable and reusable.
>
> Cc: Eduard Zingerman <eddyz87@gmail.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Cc: Alan Maguire <alan.maguire@oracle.com>
> Cc: Song Liu <song@kernel.org>
> Signed-off-by: pengdonglin <pengdonglin@xiaomi.com>
Signed-off-by is supposed to have properly spelled and capitalized
real name of the contributor
> Signed-off-by: Donglin Peng <dolinux.peng@gmail.com>
> ---
> tools/lib/bpf/btf.c | 63 +++++++++++++++++----------------
> tools/lib/bpf/libbpf_internal.h | 1 +
> 2 files changed, 33 insertions(+), 31 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 18907f0fcf9f..5e1c09b5dce8 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -3400,6 +3400,37 @@ int btf_ext__set_endianness(struct btf_ext *btf_ext, enum btf_endianness endian)
> return 0;
> }
>
> +static int btf_remap_types(struct btf *btf, struct btf_ext *btf_ext,
> + btf_remap_type_fn visit, void *ctx)
tbh, my goal is to reduce the amount of callback usage within libbpf,
not add more of it...
I don't like this refactoring. We should convert
btf_ext_visit_type_ids() into iterators, have btf_field_iter_init +
btf_field_iter_next usable in for_each() form, and not try to reuse 5
lines of code. See my comments in the next patch.
> +{
> + int i, r;
> +
> + for (i = 0; i < btf->nr_types; i++) {
> + struct btf_type *t = btf_type_by_id(btf, btf->start_id + i);
> + struct btf_field_iter it;
> + __u32 *type_id;
> +
> + r = btf_field_iter_init(&it, t, BTF_FIELD_ITER_IDS);
> + if (r)
> + return r;
> +
> + while ((type_id = btf_field_iter_next(&it))) {
> + r = visit(type_id, ctx);
> + if (r)
> + return r;
> + }
> + }
> +
> + if (!btf_ext)
> + return 0;
> +
> + r = btf_ext_visit_type_ids(btf_ext, visit, ctx);
> + if (r)
> + return r;
> +
> + return 0;
> +}
> +
> struct btf_dedup;
>
> static struct btf_dedup *btf_dedup_new(struct btf *btf, const struct btf_dedup_opts *opts);
> @@ -5320,37 +5351,7 @@ static int btf_dedup_remap_type_id(__u32 *type_id, void *ctx)
> */
> static int btf_dedup_remap_types(struct btf_dedup *d)
> {
> - int i, r;
> -
> - for (i = 0; i < d->btf->nr_types; i++) {
> - struct btf_type *t = btf_type_by_id(d->btf, d->btf->start_id + i);
> - struct btf_field_iter it;
> - __u32 *type_id;
> -
> - r = btf_field_iter_init(&it, t, BTF_FIELD_ITER_IDS);
> - if (r)
> - return r;
> -
> - while ((type_id = btf_field_iter_next(&it))) {
> - __u32 resolved_id, new_id;
> -
> - resolved_id = resolve_type_id(d, *type_id);
> - new_id = d->hypot_map[resolved_id];
> - if (new_id > BTF_MAX_NR_TYPES)
> - return -EINVAL;
> -
> - *type_id = new_id;
> - }
> - }
> -
> - if (!d->btf_ext)
> - return 0;
> -
> - r = btf_ext_visit_type_ids(d->btf_ext, btf_dedup_remap_type_id, d);
> - if (r)
> - return r;
> -
> - return 0;
> + return btf_remap_types(d->btf, d->btf_ext, btf_dedup_remap_type_id, d);
> }
>
> /*
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 35b2527bedec..b09d6163f5c3 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -582,6 +582,7 @@ int btf_ext_visit_type_ids(struct btf_ext *btf_ext, type_id_visit_fn visit, void
> int btf_ext_visit_str_offs(struct btf_ext *btf_ext, str_off_visit_fn visit, void *ctx);
> __s32 btf__find_by_name_kind_own(const struct btf *btf, const char *type_name,
> __u32 kind);
> +typedef int (*btf_remap_type_fn)(__u32 *type_id, void *ctx);
>
> /* handle direct returned errors */
> static inline int libbpf_err(int ret)
> --
> 2.34.1
>
On Tue, 2025-11-04 at 16:11 -0800, Andrii Nakryiko wrote: [...] > > @@ -3400,6 +3400,37 @@ int btf_ext__set_endianness(struct btf_ext *btf_ext, enum btf_endianness endian) > > return 0; > > } > > > > +static int btf_remap_types(struct btf *btf, struct btf_ext *btf_ext, > > + btf_remap_type_fn visit, void *ctx) > > tbh, my goal is to reduce the amount of callback usage within libbpf, > not add more of it... > > I don't like this refactoring. We should convert > btf_ext_visit_type_ids() into iterators, have btf_field_iter_init + > btf_field_iter_next usable in for_each() form, and not try to reuse 5 > lines of code. See my comments in the next patch. Remapping types is a concept. I hate duplicating code for concepts. Similarly, having patch #3 == patch #5 and patch #4 == patch #6 is plain ugly. Just waiting for a bug because we changed the one but forgot to change another in a year or two. [...]
On Tue, Nov 4, 2025 at 4:36 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Tue, 2025-11-04 at 16:11 -0800, Andrii Nakryiko wrote: > > [...] > > > > @@ -3400,6 +3400,37 @@ int btf_ext__set_endianness(struct btf_ext *btf_ext, enum btf_endianness endian) > > > return 0; > > > } > > > > > > +static int btf_remap_types(struct btf *btf, struct btf_ext *btf_ext, > > > + btf_remap_type_fn visit, void *ctx) > > > > tbh, my goal is to reduce the amount of callback usage within libbpf, > > not add more of it... > > > > I don't like this refactoring. We should convert > > btf_ext_visit_type_ids() into iterators, have btf_field_iter_init + > > btf_field_iter_next usable in for_each() form, and not try to reuse 5 > > lines of code. See my comments in the next patch. > > Remapping types is a concept. > I hate duplicating code for concepts. > Similarly, having patch #3 == patch #5 and patch #4 == patch #6 is > plain ugly. Just waiting for a bug because we changed the one but > forgot to change another in a year or two. Tricky and evolving part (iterating all type ID fields) is abstracted behind the iterator (and we should do the same for btf_ext). Iterating types is not something tricky or requiring constant maintenance. Same for binary search, I don't see why we'd need to adjust it. So no, I don't want to share code between kernel and libbpf just to reuse binary search implementation, sorry. > > [...]
On Tue, 2025-11-04 at 16:57 -0800, Andrii Nakryiko wrote: > On Tue, Nov 4, 2025 at 4:36 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > On Tue, 2025-11-04 at 16:11 -0800, Andrii Nakryiko wrote: > > > > [...] > > > > > > @@ -3400,6 +3400,37 @@ int btf_ext__set_endianness(struct btf_ext *btf_ext, enum btf_endianness endian) > > > > return 0; > > > > } > > > > > > > > +static int btf_remap_types(struct btf *btf, struct btf_ext *btf_ext, > > > > + btf_remap_type_fn visit, void *ctx) > > > > > > tbh, my goal is to reduce the amount of callback usage within libbpf, > > > not add more of it... > > > > > > I don't like this refactoring. We should convert > > > btf_ext_visit_type_ids() into iterators, have btf_field_iter_init + > > > btf_field_iter_next usable in for_each() form, and not try to reuse 5 > > > lines of code. See my comments in the next patch. > > > > Remapping types is a concept. > > I hate duplicating code for concepts. > > Similarly, having patch #3 == patch #5 and patch #4 == patch #6 is > > plain ugly. Just waiting for a bug because we changed the one but > > forgot to change another in a year or two. > > Tricky and evolving part (iterating all type ID fields) is abstracted > behind the iterator (and we should do the same for btf_ext). Iterating > types is not something tricky or requiring constant maintenance. > > Same for binary search, I don't see why we'd need to adjust it. So no, > I don't want to share code between kernel and libbpf just to reuse > binary search implementation, sorry. <rant> Sure binary search is trivial, but did you count how many times you asked people to re-implement binary search as in [1]? [1] https://elixir.bootlin.com/linux/v6.18-rc4/source/kernel/bpf/verifier.c#L2952 </rant>
On Tue, Nov 4, 2025 at 5:23 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Tue, 2025-11-04 at 16:57 -0800, Andrii Nakryiko wrote: > > On Tue, Nov 4, 2025 at 4:36 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > > > On Tue, 2025-11-04 at 16:11 -0800, Andrii Nakryiko wrote: > > > > > > [...] > > > > > > > > @@ -3400,6 +3400,37 @@ int btf_ext__set_endianness(struct btf_ext *btf_ext, enum btf_endianness endian) > > > > > return 0; > > > > > } > > > > > > > > > > +static int btf_remap_types(struct btf *btf, struct btf_ext *btf_ext, > > > > > + btf_remap_type_fn visit, void *ctx) > > > > > > > > tbh, my goal is to reduce the amount of callback usage within libbpf, > > > > not add more of it... > > > > > > > > I don't like this refactoring. We should convert > > > > btf_ext_visit_type_ids() into iterators, have btf_field_iter_init + > > > > btf_field_iter_next usable in for_each() form, and not try to reuse 5 > > > > lines of code. See my comments in the next patch. > > > > > > Remapping types is a concept. > > > I hate duplicating code for concepts. > > > Similarly, having patch #3 == patch #5 and patch #4 == patch #6 is > > > plain ugly. Just waiting for a bug because we changed the one but > > > forgot to change another in a year or two. > > > > Tricky and evolving part (iterating all type ID fields) is abstracted > > behind the iterator (and we should do the same for btf_ext). Iterating > > types is not something tricky or requiring constant maintenance. > > > > Same for binary search, I don't see why we'd need to adjust it. So no, > > I don't want to share code between kernel and libbpf just to reuse > > binary search implementation, sorry. > > <rant> > > Sure binary search is trivial, but did you count how many times you > asked people to re-implement binary search as in [1]? Exact match binary search can be called trivial, yes. Lower/upper bound binary search looks deceivingly simple, but it requires attention to every single line of code. But the end result is simple and straightforward, yes. I'm not sure what point you are trying to make, though. Yes, I've asked people many times to implement upper/lower bound binary search similarly to the one in find_linfo(), because usually people have various unnecessary checks, keeping track not just of bounds, but also remembering some element that we know satisfied the condition at some point before, etc. It's not elegant, harder to reason about, and can be done more succinctly. You don't like that I ask people to improve implementation? You don't like the implementation itself? Or are you suggesting that we should add a "generic" C implementation of lower_bound/upper_bound and use callbacks for comparison logic? What are you ranting about, exactly? As I said, once binary search (of whatever kind, bounds or exact) is written for something like this, it doesn't have to ever be modified. I don't see this as a maintainability hurdle at all. But sharing code between libbpf and kernel is something to be avoided. Look at #ifdef __KERNEL__ sections of relo_core.c as one reason why. > > [1] https://elixir.bootlin.com/linux/v6.18-rc4/source/kernel/bpf/verifier.c#L2952 > > </rant>
On Wed, 2025-11-05 at 10:20 -0800, Andrii Nakryiko wrote: [...] > You don't like that I ask people to improve implementation? Not at all. > You don't like the implementation itself? Or are you suggesting that > we should add a "generic" C implementation of > lower_bound/upper_bound and use callbacks for comparison logic? What > are you ranting about, exactly? Actually, having it as a static inline function in a header would be nice. I just tried that, and gcc is perfectly capable of inlining the comparison function in -O2 mode. I'm ranting about patch #5 being 101 insertions(+), 10 deletions(-) and patch #4 being 119 insertions(+), 23 deletions(-), while doing exactly the same thing. And yes, this copy of binary search routine probably won't ever change. But changes to the comparator logic are pretty much possible, if we decide to include 'kind' as a secondary key one day. And that change will have to happen twice. > As I said, once binary search (of whatever kind, bounds or exact) is > written for something like this, it doesn't have to ever be modified. > I don't see this as a maintainability hurdle at all. But sharing code > between libbpf and kernel is something to be avoided. Look at #ifdef > __KERNEL__ sections of relo_core.c as one reason why.
On Wed, Nov 5, 2025 at 11:41 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Wed, 2025-11-05 at 10:20 -0800, Andrii Nakryiko wrote: > > [...] > > > You don't like that I ask people to improve implementation? > > Not at all. > > > You don't like the implementation itself? Or are you suggesting that > > we should add a "generic" C implementation of > > lower_bound/upper_bound and use callbacks for comparison logic? What > > are you ranting about, exactly? > > Actually, having it as a static inline function in a header would be > nice. I just tried that, and gcc is perfectly capable of inlining the > comparison function in -O2 mode. I dislike callbacks in principle, but I don't mind having such a reusable primitive, if it's reasonably abstracted. Do it. > > I'm ranting about patch #5 being 101 insertions(+), 10 deletions(-) > and patch #4 being 119 insertions(+), 23 deletions(-), > while doing exactly the same thing. Understandable, but code reuse is not (at least it should not be) the goal for its own sake. It should help manage complexity and improve maintainability. Code sharing is not all pros, it creates unnecessary entanglement and dependencies. I don't think sharing this code between libbpf and kernel is justified here. 100 lines of code is not a big deal, IMO. > > And yes, this copy of binary search routine probably won't ever > change. But changes to the comparator logic are pretty much possible, > if we decide to include 'kind' as a secondary key one day. > And that change will have to happen twice. > > > As I said, once binary search (of whatever kind, bounds or exact) is > > written for something like this, it doesn't have to ever be modified. > > I don't see this as a maintainability hurdle at all. But sharing code > > between libbpf and kernel is something to be avoided. Look at #ifdef > > __KERNEL__ sections of relo_core.c as one reason why.
On Tue, 2025-11-04 at 21:40 +0800, Donglin Peng wrote:
> From: pengdonglin <pengdonglin@xiaomi.com>
>
> Refactor btf_dedup_remap_types() by extracting its core logic into a new
> btf_remap_types() helper function. This eliminates code duplication
> and improves modularity while maintaining the same functionality.
>
> The new function encapsulates iteration over BTF types and BTF ext
> sections, accepting a callback for flexible type ID remapping. This
> makes the type remapping logic more maintainable and reusable.
>
> Cc: Eduard Zingerman <eddyz87@gmail.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Cc: Alan Maguire <alan.maguire@oracle.com>
> Cc: Song Liu <song@kernel.org>
> Signed-off-by: pengdonglin <pengdonglin@xiaomi.com>
> Signed-off-by: Donglin Peng <dolinux.peng@gmail.com>
> ---
Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>
> tools/lib/bpf/btf.c | 63 +++++++++++++++++----------------
> tools/lib/bpf/libbpf_internal.h | 1 +
> 2 files changed, 33 insertions(+), 31 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 18907f0fcf9f..5e1c09b5dce8 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -3400,6 +3400,37 @@ int btf_ext__set_endianness(struct btf_ext *btf_ext, enum btf_endianness endian)
> return 0;
> }
>
> +static int btf_remap_types(struct btf *btf, struct btf_ext *btf_ext,
> + btf_remap_type_fn visit, void *ctx)
^^^^^^^^^^^^^^^^^
Nit: there is already 'type_id_visit_fn', no need to add new type.
> +{
> + int i, r;
> +
> + for (i = 0; i < btf->nr_types; i++) {
> + struct btf_type *t = btf_type_by_id(btf, btf->start_id + i);
> + struct btf_field_iter it;
> + __u32 *type_id;
> +
> + r = btf_field_iter_init(&it, t, BTF_FIELD_ITER_IDS);
> + if (r)
> + return r;
> +
> + while ((type_id = btf_field_iter_next(&it))) {
> + r = visit(type_id, ctx);
> + if (r)
> + return r;
> + }
> + }
> +
> + if (!btf_ext)
> + return 0;
> +
> + r = btf_ext_visit_type_ids(btf_ext, visit, ctx);
> + if (r)
> + return r;
> +
> + return 0;
> +}
> +
> struct btf_dedup;
>
> static struct btf_dedup *btf_dedup_new(struct btf *btf, const struct btf_dedup_opts *opts);
[...]
© 2016 - 2026 Red Hat, Inc.