[PATCH bpf-next 0/5] libbpf: BTF field iterator

[PATCH bpf-next 0/5] libbpf: BTF field iterator

[Andrii Nakryiko]

Add BTF field (type and string fields, right now) iterator support instead of
using existing callback-based approaches, which make it harder to understand
and support BTF-processing code.

rfcv1->v1:
  - check errors when initializing iterators (Jiri);
  - split RFC patch into separate patches.

Andrii Nakryiko (5):
  libbpf: add BTF field iterator
  libbpf: make use of BTF field iterator in BPF linker code
  libbpf: make use of BTF field iterator in BTF handling code
  bpftool: use BTF field iterator in btfgen
  libbpf: remove callback-based type/string BTF field visitor helpers

 tools/bpf/bpftool/gen.c         |  16 +-
 tools/lib/bpf/btf.c             | 328 +++++++++++++++++++-------------
 tools/lib/bpf/libbpf_internal.h |  26 ++-
 tools/lib/bpf/linker.c          |  60 +++---
 4 files changed, 264 insertions(+), 166 deletions(-)

-- 
2.43.0

[PATCH bpf-next 1/5] libbpf: add BTF field iterator

[Andrii Nakryiko]

Implement iterator-based type ID and string offset BTF field iterator.
This is used extensively in BTF-handling code and BPF linker code for
various sanity checks, rewriting IDs/offsets, etc. Currently this is
implemented as visitor pattern calling custom callbacks, which makes the
logic (especially in simple cases) unnecessarily obscure and harder to
follow.

Having equivalent functionality using iterator pattern makes for simpler
to understand and maintain code. As we add more code for BTF processing
logic in libbpf, it's best to switch to iterator pattern before adding
more callback-based code.

The idea for iterator-based implementation is to record offsets of
necessary fields within fixed btf_type parts (which should be iterated
just once), and, for kinds that have multiple members (based on vlen
field), record where in each member necessary fields are located.

Generic iteration code then just keeps track of last offset that was
returned and handles N members correctly. Return type is just u32
pointer, where NULL is returned when all relevant fields were already
iterated.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/btf.c             | 162 ++++++++++++++++++++++++++++++++
 tools/lib/bpf/libbpf_internal.h |  24 +++++
 2 files changed, 186 insertions(+)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 2d0840ef599a..50ff8b6eaf36 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -5133,6 +5133,168 @@ int btf_type_visit_str_offs(struct btf_type *t, str_off_visit_fn visit, void *ct
 	return 0;
 }
 
+int btf_field_iter_init(struct btf_field_iter *it, struct btf_type *t, enum btf_field_iter_kind iter_kind)
+{
+	it->p = NULL;
+	it->m_idx = -1;
+	it->off_idx = 0;
+	it->vlen = 0;
+
+	switch (iter_kind) {
+	case BTF_FIELD_ITER_IDS:
+		switch (btf_kind(t)) {
+		case BTF_KIND_UNKN:
+		case BTF_KIND_INT:
+		case BTF_KIND_FLOAT:
+		case BTF_KIND_ENUM:
+		case BTF_KIND_ENUM64:
+			it->desc = (struct btf_field_desc) {};
+			break;
+		case BTF_KIND_FWD:
+		case BTF_KIND_CONST:
+		case BTF_KIND_VOLATILE:
+		case BTF_KIND_RESTRICT:
+		case BTF_KIND_PTR:
+		case BTF_KIND_TYPEDEF:
+		case BTF_KIND_FUNC:
+		case BTF_KIND_VAR:
+		case BTF_KIND_DECL_TAG:
+		case BTF_KIND_TYPE_TAG:
+			it->desc = (struct btf_field_desc) { 1, {offsetof(struct btf_type, type)} };
+			break;
+		case BTF_KIND_ARRAY:
+			it->desc = (struct btf_field_desc) {
+				2, {sizeof(struct btf_type) + offsetof(struct btf_array, type),
+				    sizeof(struct btf_type) + offsetof(struct btf_array, index_type)}
+			};
+			break;
+		case BTF_KIND_STRUCT:
+		case BTF_KIND_UNION:
+			it->desc = (struct btf_field_desc) {
+				0, {},
+				sizeof(struct btf_member),
+				1, {offsetof(struct btf_member, type)}
+			};
+			break;
+		case BTF_KIND_FUNC_PROTO:
+			it->desc = (struct btf_field_desc) {
+				1, {offsetof(struct btf_type, type)},
+				sizeof(struct btf_param),
+				1, {offsetof(struct btf_param, type)}
+			};
+			break;
+		case BTF_KIND_DATASEC:
+			it->desc = (struct btf_field_desc) {
+				0, {},
+				sizeof(struct btf_var_secinfo),
+				1, {offsetof(struct btf_var_secinfo, type)}
+			};
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case BTF_FIELD_ITER_STRS:
+		switch (btf_kind(t)) {
+		case BTF_KIND_UNKN:
+			it->desc = (struct btf_field_desc) {};
+			break;
+		case BTF_KIND_INT:
+		case BTF_KIND_FLOAT:
+		case BTF_KIND_FWD:
+		case BTF_KIND_ARRAY:
+		case BTF_KIND_CONST:
+		case BTF_KIND_VOLATILE:
+		case BTF_KIND_RESTRICT:
+		case BTF_KIND_PTR:
+		case BTF_KIND_TYPEDEF:
+		case BTF_KIND_FUNC:
+		case BTF_KIND_VAR:
+		case BTF_KIND_DECL_TAG:
+		case BTF_KIND_TYPE_TAG:
+		case BTF_KIND_DATASEC:
+			it->desc = (struct btf_field_desc) {
+				1, {offsetof(struct btf_type, name_off)}
+			};
+			break;
+		case BTF_KIND_ENUM:
+			it->desc = (struct btf_field_desc) {
+				1, {offsetof(struct btf_type, name_off)},
+				sizeof(struct btf_enum),
+				1, {offsetof(struct btf_enum, name_off)}
+			};
+			break;
+		case BTF_KIND_ENUM64:
+			it->desc = (struct btf_field_desc) {
+				1, {offsetof(struct btf_type, name_off)},
+				sizeof(struct btf_enum64),
+				1, {offsetof(struct btf_enum64, name_off)}
+			};
+			break;
+		case BTF_KIND_STRUCT:
+		case BTF_KIND_UNION:
+			it->desc = (struct btf_field_desc) {
+				1, {offsetof(struct btf_type, name_off)},
+				sizeof(struct btf_member),
+				1, {offsetof(struct btf_member, name_off)}
+			};
+			break;
+		case BTF_KIND_FUNC_PROTO:
+			it->desc = (struct btf_field_desc) {
+				1, {offsetof(struct btf_type, name_off)},
+				sizeof(struct btf_param),
+				1, {offsetof(struct btf_param, name_off)}
+			};
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (it->desc.m_sz)
+		it->vlen = btf_vlen(t);
+
+	it->p = t;
+	return 0;
+}
+
+__u32 *btf_field_iter_next(struct btf_field_iter *it)
+{
+	if (!it->p)
+		return NULL;
+
+	if (it->m_idx < 0) {
+		if (it->off_idx < it->desc.t_cnt)
+			return it->p + it->desc.t_offs[it->off_idx++];
+		/* move to per-member iteration */
+		it->m_idx = 0;
+		it->p += sizeof(struct btf_type);
+		it->off_idx = 0;
+	}
+
+	/* if type doesn't have members, stop */
+	if (it->desc.m_sz == 0) {
+		it->p = NULL;
+		return NULL;
+	}
+
+	if (it->off_idx >= it->desc.m_cnt) {
+		/* exhausted this member's fields, go to the next member */
+		it->m_idx++;
+		it->p += it->desc.m_sz;
+		it->off_idx = 0;
+	}
+
+	if (it->m_idx < it->vlen)
+		return it->p + it->desc.m_offs[it->off_idx++];
+
+	it->p = NULL;
+	return NULL;
+}
+
 int btf_ext_visit_type_ids(struct btf_ext *btf_ext, type_id_visit_fn visit, void *ctx)
 {
 	const struct btf_ext_info *seg;
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 7e7e686008c6..80f3d346db33 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -508,6 +508,30 @@ struct bpf_line_info_min {
 	__u32	line_col;
 };
 
+enum btf_field_iter_kind {
+	BTF_FIELD_ITER_IDS,
+	BTF_FIELD_ITER_STRS,
+};
+
+struct btf_field_desc {
+	/* once-per-type offsets */
+	int t_cnt, t_offs[2];
+	/* member struct size, or zero, if no members */
+	int m_sz;
+	/* repeated per-member offsets */
+	int m_cnt, m_offs[1];
+};
+
+struct btf_field_iter {
+	struct btf_field_desc desc;
+	void *p;
+	int m_idx;
+	int off_idx;
+	int vlen;
+};
+
+int btf_field_iter_init(struct btf_field_iter *it, struct btf_type *t, enum btf_field_iter_kind iter_kind);
+__u32 *btf_field_iter_next(struct btf_field_iter *it);
 
 typedef int (*type_id_visit_fn)(__u32 *type_id, void *ctx);
 typedef int (*str_off_visit_fn)(__u32 *str_off, void *ctx);
-- 
2.43.0

[PATCH bpf-next 2/5] libbpf: make use of BTF field iterator in BPF linker code

[Andrii Nakryiko]

Switch all BPF linker code dealing with iterating BTF type ID and string
offset fields to new btf_field_iter facilities.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/linker.c | 60 ++++++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
index 0d4be829551b..be6539e59cf6 100644
--- a/tools/lib/bpf/linker.c
+++ b/tools/lib/bpf/linker.c
@@ -957,19 +957,35 @@ static int check_btf_str_off(__u32 *str_off, void *ctx)
 static int linker_sanity_check_btf(struct src_obj *obj)
 {
 	struct btf_type *t;
-	int i, n, err = 0;
+	int i, n, err;
 
 	if (!obj->btf)
 		return 0;
 
 	n = btf__type_cnt(obj->btf);
 	for (i = 1; i < n; i++) {
+		struct btf_field_iter it;
+		__u32 *type_id, *str_off;
+		const char *s;
+
 		t = btf_type_by_id(obj->btf, i);
 
-		err = err ?: btf_type_visit_type_ids(t, check_btf_type_id, obj->btf);
-		err = err ?: btf_type_visit_str_offs(t, check_btf_str_off, obj->btf);
+		err = btf_field_iter_init(&it, t, BTF_FIELD_ITER_IDS);
 		if (err)
 			return err;
+		while ((type_id = btf_field_iter_next(&it))) {
+			if (*type_id >= n)
+				return -EINVAL;
+		}
+
+		err = btf_field_iter_init(&it, t, BTF_FIELD_ITER_STRS);
+		if (err)
+			return err;
+		while ((str_off = btf_field_iter_next(&it))) {
+			s = btf__str_by_offset(obj->btf, *str_off);
+			if (!s)
+				return -EINVAL;
+		}
 	}
 
 	return 0;
@@ -2234,26 +2250,10 @@ static int linker_fixup_btf(struct src_obj *obj)
 	return 0;
 }
 
-static int remap_type_id(__u32 *type_id, void *ctx)
-{
-	int *id_map = ctx;
-	int new_id = id_map[*type_id];
-
-	/* Error out if the type wasn't remapped. Ignore VOID which stays VOID. */
-	if (new_id == 0 && *type_id != 0) {
-		pr_warn("failed to find new ID mapping for original BTF type ID %u\n", *type_id);
-		return -EINVAL;
-	}
-
-	*type_id = id_map[*type_id];
-
-	return 0;
-}
-
 static int linker_append_btf(struct bpf_linker *linker, struct src_obj *obj)
 {
 	const struct btf_type *t;
-	int i, j, n, start_id, id;
+	int i, j, n, start_id, id, err;
 	const char *name;
 
 	if (!obj->btf)
@@ -2324,9 +2324,25 @@ static int linker_append_btf(struct bpf_linker *linker, struct src_obj *obj)
 	n = btf__type_cnt(linker->btf);
 	for (i = start_id; i < n; i++) {
 		struct btf_type *dst_t = btf_type_by_id(linker->btf, i);
+		struct btf_field_iter it;
+		__u32 *type_id;
 
-		if (btf_type_visit_type_ids(dst_t, remap_type_id, obj->btf_type_map))
-			return -EINVAL;
+		err = btf_field_iter_init(&it, dst_t, BTF_FIELD_ITER_IDS);
+		if (err)
+			return err;
+
+		while ((type_id = btf_field_iter_next(&it))) {
+			int new_id = obj->btf_type_map[*type_id];
+
+			/* Error out if the type wasn't remapped. Ignore VOID which stays VOID. */
+			if (new_id == 0 && *type_id != 0) {
+				pr_warn("failed to find new ID mapping for original BTF type ID %u\n",
+					*type_id);
+				return -EINVAL;
+			}
+
+			*type_id = obj->btf_type_map[*type_id];
+		}
 	}
 
 	/* Rewrite VAR/FUNC underlying types (i.e., FUNC's FUNC_PROTO and VAR's
-- 
2.43.0

[PATCH bpf-next 3/5] libbpf: make use of BTF field iterator in BTF handling code

[Andrii Nakryiko]

Use new BTF field iterator logic to replace all the callback-based
visitor calls. There is still a .BTF.ext callback-based visitor APIs
that should be converted, which will happens as a follow up.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/btf.c | 76 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 22 deletions(-)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 50ff8b6eaf36..3fe20a902b42 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -1739,9 +1739,8 @@ struct btf_pipe {
 	struct hashmap *str_off_map; /* map string offsets from src to dst */
 };
 
-static int btf_rewrite_str(__u32 *str_off, void *ctx)
+static int btf_rewrite_str(struct btf_pipe *p, __u32 *str_off)
 {
-	struct btf_pipe *p = ctx;
 	long mapped_off;
 	int off, err;
 
@@ -1774,7 +1773,9 @@ static int btf_rewrite_str(__u32 *str_off, void *ctx)
 int btf__add_type(struct btf *btf, const struct btf *src_btf, const struct btf_type *src_type)
 {
 	struct btf_pipe p = { .src = src_btf, .dst = btf };
+	struct btf_field_iter it;
 	struct btf_type *t;
+	__u32 *str_off;
 	int sz, err;
 
 	sz = btf_type_size(src_type);
@@ -1791,26 +1792,17 @@ int btf__add_type(struct btf *btf, const struct btf *src_btf, const struct btf_t
 
 	memcpy(t, src_type, sz);
 
-	err = btf_type_visit_str_offs(t, btf_rewrite_str, &p);
+	err = btf_field_iter_init(&it, t, BTF_FIELD_ITER_STRS);
 	if (err)
 		return libbpf_err(err);
 
-	return btf_commit_type(btf, sz);
-}
-
-static int btf_rewrite_type_ids(__u32 *type_id, void *ctx)
-{
-	struct btf *btf = ctx;
-
-	if (!*type_id) /* nothing to do for VOID references */
-		return 0;
+	while ((str_off = btf_field_iter_next(&it))) {
+		err = btf_rewrite_str(&p, str_off);
+		if (err)
+			return libbpf_err(err);
+	}
 
-	/* we haven't updated btf's type count yet, so
-	 * btf->start_id + btf->nr_types - 1 is the type ID offset we should
-	 * add to all newly added BTF types
-	 */
-	*type_id += btf->start_id + btf->nr_types - 1;
-	return 0;
+	return btf_commit_type(btf, sz);
 }
 
 static size_t btf_dedup_identity_hash_fn(long key, void *ctx);
@@ -1858,6 +1850,9 @@ int btf__add_btf(struct btf *btf, const struct btf *src_btf)
 	memcpy(t, src_btf->types_data, data_sz);
 
 	for (i = 0; i < cnt; i++) {
+		struct btf_field_iter it;
+		__u32 *type_id, *str_off;
+
 		sz = btf_type_size(t);
 		if (sz < 0) {
 			/* unlikely, has to be corrupted src_btf */
@@ -1869,15 +1864,31 @@ int btf__add_btf(struct btf *btf, const struct btf *src_btf)
 		*off = t - btf->types_data;
 
 		/* add, dedup, and remap strings referenced by this BTF type */
-		err = btf_type_visit_str_offs(t, btf_rewrite_str, &p);
+		err = btf_field_iter_init(&it, t, BTF_FIELD_ITER_STRS);
 		if (err)
 			goto err_out;
+		while ((str_off = btf_field_iter_next(&it))) {
+			err = btf_rewrite_str(&p, str_off);
+			if (err)
+				goto err_out;
+		}
 
 		/* remap all type IDs referenced from this BTF type */
-		err = btf_type_visit_type_ids(t, btf_rewrite_type_ids, btf);
+		err = btf_field_iter_init(&it, t, BTF_FIELD_ITER_IDS);
 		if (err)
 			goto err_out;
 
+		while ((type_id = btf_field_iter_next(&it))) {
+			if (!*type_id) /* nothing to do for VOID references */
+				continue;
+
+			/* we haven't updated btf's type count yet, so
+			 * btf->start_id + btf->nr_types - 1 is the type ID offset we should
+			 * add to all newly added BTF types
+			 */
+			*type_id += btf->start_id + btf->nr_types - 1;
+		}
+
 		/* go to next type data and type offset index entry */
 		t += sz;
 		off++;
@@ -3453,11 +3464,19 @@ static int btf_for_each_str_off(struct btf_dedup *d, str_off_visit_fn fn, void *
 	int i, r;
 
 	for (i = 0; i < d->btf->nr_types; i++) {
+		struct btf_field_iter it;
 		struct btf_type *t = btf_type_by_id(d->btf, d->btf->start_id + i);
+		__u32 *str_off;
 
-		r = btf_type_visit_str_offs(t, fn, ctx);
+		r = btf_field_iter_init(&it, t, BTF_FIELD_ITER_STRS);
 		if (r)
 			return r;
+
+		while ((str_off = btf_field_iter_next(&it))) {
+			r = fn(str_off, ctx);
+			if (r)
+				return r;
+		}
 	}
 
 	if (!d->btf_ext)
@@ -4919,10 +4938,23 @@ static int btf_dedup_remap_types(struct btf_dedup *d)
 
 	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_type_visit_type_ids(t, btf_dedup_remap_type_id, d);
+		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)
-- 
2.43.0

[PATCH bpf-next 4/5] bpftool: use BTF field iterator in btfgen

[Andrii Nakryiko]

Switch bpftool's code which is using libbpf-internal
btf_type_visit_type_ids() helper to new btf_field_iter functionality.

This makes bpftool code simpler, but also unblocks removing libbpf's
btf_type_visit_type_ids() helper completely.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/bpf/bpftool/gen.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index b3979ddc0189..d244a7de387e 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -2379,15 +2379,6 @@ static int btfgen_record_obj(struct btfgen_info *info, const char *obj_path)
 	return err;
 }
 
-static int btfgen_remap_id(__u32 *type_id, void *ctx)
-{
-	unsigned int *ids = ctx;
-
-	*type_id = ids[*type_id];
-
-	return 0;
-}
-
 /* Generate BTF from relocation information previously recorded */
 static struct btf *btfgen_get_btf(struct btfgen_info *info)
 {
@@ -2467,10 +2458,15 @@ static struct btf *btfgen_get_btf(struct btfgen_info *info)
 	/* second pass: fix up type ids */
 	for (i = 1; i < btf__type_cnt(btf_new); i++) {
 		struct btf_type *btf_type = (struct btf_type *) btf__type_by_id(btf_new, i);
+		struct btf_field_iter it;
+		__u32 *type_id;
 
-		err = btf_type_visit_type_ids(btf_type, btfgen_remap_id, ids);
+		err = btf_field_iter_init(&it, btf_type, BTF_FIELD_ITER_IDS);
 		if (err)
 			goto err_out;
+
+		while ((type_id = btf_field_iter_next(&it)))
+			*type_id = ids[*type_id];
 	}
 
 	free(ids);
-- 
2.43.0

[PATCH bpf-next 5/5] libbpf: remove callback-based type/string BTF field visitor helpers

[Andrii Nakryiko]

Now that all libbpf/bpftool code switched to btf_field_iter, remove
btf_type_visit_type_ids() and btf_type_visit_str_offs() callback-based
helpers as not needed anymore.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/btf.c             | 130 --------------------------------
 tools/lib/bpf/libbpf_internal.h |   2 -
 2 files changed, 132 deletions(-)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 3fe20a902b42..1de7579f2a08 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -5035,136 +5035,6 @@ struct btf *btf__load_module_btf(const char *module_name, struct btf *vmlinux_bt
 	return btf__parse_split(path, vmlinux_btf);
 }
 
-int btf_type_visit_type_ids(struct btf_type *t, type_id_visit_fn visit, void *ctx)
-{
-	int i, n, err;
-
-	switch (btf_kind(t)) {
-	case BTF_KIND_INT:
-	case BTF_KIND_FLOAT:
-	case BTF_KIND_ENUM:
-	case BTF_KIND_ENUM64:
-		return 0;
-
-	case BTF_KIND_FWD:
-	case BTF_KIND_CONST:
-	case BTF_KIND_VOLATILE:
-	case BTF_KIND_RESTRICT:
-	case BTF_KIND_PTR:
-	case BTF_KIND_TYPEDEF:
-	case BTF_KIND_FUNC:
-	case BTF_KIND_VAR:
-	case BTF_KIND_DECL_TAG:
-	case BTF_KIND_TYPE_TAG:
-		return visit(&t->type, ctx);
-
-	case BTF_KIND_ARRAY: {
-		struct btf_array *a = btf_array(t);
-
-		err = visit(&a->type, ctx);
-		err = err ?: visit(&a->index_type, ctx);
-		return err;
-	}
-
-	case BTF_KIND_STRUCT:
-	case BTF_KIND_UNION: {
-		struct btf_member *m = btf_members(t);
-
-		for (i = 0, n = btf_vlen(t); i < n; i++, m++) {
-			err = visit(&m->type, ctx);
-			if (err)
-				return err;
-		}
-		return 0;
-	}
-
-	case BTF_KIND_FUNC_PROTO: {
-		struct btf_param *m = btf_params(t);
-
-		err = visit(&t->type, ctx);
-		if (err)
-			return err;
-		for (i = 0, n = btf_vlen(t); i < n; i++, m++) {
-			err = visit(&m->type, ctx);
-			if (err)
-				return err;
-		}
-		return 0;
-	}
-
-	case BTF_KIND_DATASEC: {
-		struct btf_var_secinfo *m = btf_var_secinfos(t);
-
-		for (i = 0, n = btf_vlen(t); i < n; i++, m++) {
-			err = visit(&m->type, ctx);
-			if (err)
-				return err;
-		}
-		return 0;
-	}
-
-	default:
-		return -EINVAL;
-	}
-}
-
-int btf_type_visit_str_offs(struct btf_type *t, str_off_visit_fn visit, void *ctx)
-{
-	int i, n, err;
-
-	err = visit(&t->name_off, ctx);
-	if (err)
-		return err;
-
-	switch (btf_kind(t)) {
-	case BTF_KIND_STRUCT:
-	case BTF_KIND_UNION: {
-		struct btf_member *m = btf_members(t);
-
-		for (i = 0, n = btf_vlen(t); i < n; i++, m++) {
-			err = visit(&m->name_off, ctx);
-			if (err)
-				return err;
-		}
-		break;
-	}
-	case BTF_KIND_ENUM: {
-		struct btf_enum *m = btf_enum(t);
-
-		for (i = 0, n = btf_vlen(t); i < n; i++, m++) {
-			err = visit(&m->name_off, ctx);
-			if (err)
-				return err;
-		}
-		break;
-	}
-	case BTF_KIND_ENUM64: {
-		struct btf_enum64 *m = btf_enum64(t);
-
-		for (i = 0, n = btf_vlen(t); i < n; i++, m++) {
-			err = visit(&m->name_off, ctx);
-			if (err)
-				return err;
-		}
-		break;
-	}
-	case BTF_KIND_FUNC_PROTO: {
-		struct btf_param *m = btf_params(t);
-
-		for (i = 0, n = btf_vlen(t); i < n; i++, m++) {
-			err = visit(&m->name_off, ctx);
-			if (err)
-				return err;
-		}
-		break;
-	}
-	default:
-		break;
-	}
-
-	return 0;
-}
-
 int btf_field_iter_init(struct btf_field_iter *it, struct btf_type *t, enum btf_field_iter_kind iter_kind)
 {
 	it->p = NULL;
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 80f3d346db33..9f4a04367287 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -535,8 +535,6 @@ __u32 *btf_field_iter_next(struct btf_field_iter *it);
 
 typedef int (*type_id_visit_fn)(__u32 *type_id, void *ctx);
 typedef int (*str_off_visit_fn)(__u32 *str_off, void *ctx);
-int btf_type_visit_type_ids(struct btf_type *t, type_id_visit_fn visit, void *ctx);
-int btf_type_visit_str_offs(struct btf_type *t, str_off_visit_fn visit, void *ctx);
 int btf_ext_visit_type_ids(struct btf_ext *btf_ext, type_id_visit_fn visit, void *ctx);
 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,
-- 
2.43.0

Re: [PATCH bpf-next 4/5] bpftool: use BTF field iterator in btfgen

[Quentin Monnet]

On 04/06/2024 00:17, Andrii Nakryiko wrote:
> Switch bpftool's code which is using libbpf-internal
> btf_type_visit_type_ids() helper to new btf_field_iter functionality.
> 
> This makes bpftool code simpler, but also unblocks removing libbpf's
> btf_type_visit_type_ids() helper completely.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Thanks for the bpftool update, it looks good.

Reviewed-by: Quentin Monnet <qmo@kernel.org>

Re: [PATCH bpf-next 2/5] libbpf: make use of BTF field iterator in BPF linker code

[Jiri Olsa]

On Mon, Jun 03, 2024 at 04:17:16PM -0700, Andrii Nakryiko wrote:
> Switch all BPF linker code dealing with iterating BTF type ID and string
> offset fields to new btf_field_iter facilities.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  tools/lib/bpf/linker.c | 60 ++++++++++++++++++++++++++----------------
>  1 file changed, 38 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> index 0d4be829551b..be6539e59cf6 100644
> --- a/tools/lib/bpf/linker.c
> +++ b/tools/lib/bpf/linker.c
> @@ -957,19 +957,35 @@ static int check_btf_str_off(__u32 *str_off, void *ctx)
>  static int linker_sanity_check_btf(struct src_obj *obj)
>  {
>  	struct btf_type *t;
> -	int i, n, err = 0;
> +	int i, n, err;
>  
>  	if (!obj->btf)
>  		return 0;
>  
>  	n = btf__type_cnt(obj->btf);
>  	for (i = 1; i < n; i++) {
> +		struct btf_field_iter it;
> +		__u32 *type_id, *str_off;
> +		const char *s;
> +
>  		t = btf_type_by_id(obj->btf, i);
>  
> -		err = err ?: btf_type_visit_type_ids(t, check_btf_type_id, obj->btf);
> -		err = err ?: btf_type_visit_str_offs(t, check_btf_str_off, obj->btf);
> +		err = btf_field_iter_init(&it, t, BTF_FIELD_ITER_IDS);
>  		if (err)
>  			return err;
> +		while ((type_id = btf_field_iter_next(&it))) {
> +			if (*type_id >= n)
> +				return -EINVAL;
> +		}
> +
> +		err = btf_field_iter_init(&it, t, BTF_FIELD_ITER_STRS);
> +		if (err)
> +			return err;
> +		while ((str_off = btf_field_iter_next(&it))) {
> +			s = btf__str_by_offset(obj->btf, *str_off);
> +			if (!s)
> +				return -EINVAL;

nit, we could drop 's' and just do (!btf__str_by_offset(obj->btf, *str_off))


otherwise the patchset lgtm

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

> +		}
>  	}
>  
>  	return 0;
> @@ -2234,26 +2250,10 @@ static int linker_fixup_btf(struct src_obj *obj)
>  	return 0;
>  }

SNIP

Re: [PATCH bpf-next 2/5] libbpf: make use of BTF field iterator in BPF linker code

[Andrii Nakryiko]

On Tue, Jun 4, 2024 at 4:02 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Mon, Jun 03, 2024 at 04:17:16PM -0700, Andrii Nakryiko wrote:
> > Switch all BPF linker code dealing with iterating BTF type ID and string
> > offset fields to new btf_field_iter facilities.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  tools/lib/bpf/linker.c | 60 ++++++++++++++++++++++++++----------------
> >  1 file changed, 38 insertions(+), 22 deletions(-)
> >
> > diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> > index 0d4be829551b..be6539e59cf6 100644
> > --- a/tools/lib/bpf/linker.c
> > +++ b/tools/lib/bpf/linker.c
> > @@ -957,19 +957,35 @@ static int check_btf_str_off(__u32 *str_off, void *ctx)
> >  static int linker_sanity_check_btf(struct src_obj *obj)
> >  {
> >       struct btf_type *t;
> > -     int i, n, err = 0;
> > +     int i, n, err;
> >
> >       if (!obj->btf)
> >               return 0;
> >
> >       n = btf__type_cnt(obj->btf);
> >       for (i = 1; i < n; i++) {
> > +             struct btf_field_iter it;
> > +             __u32 *type_id, *str_off;
> > +             const char *s;
> > +
> >               t = btf_type_by_id(obj->btf, i);
> >
> > -             err = err ?: btf_type_visit_type_ids(t, check_btf_type_id, obj->btf);
> > -             err = err ?: btf_type_visit_str_offs(t, check_btf_str_off, obj->btf);
> > +             err = btf_field_iter_init(&it, t, BTF_FIELD_ITER_IDS);
> >               if (err)
> >                       return err;
> > +             while ((type_id = btf_field_iter_next(&it))) {
> > +                     if (*type_id >= n)
> > +                             return -EINVAL;
> > +             }
> > +
> > +             err = btf_field_iter_init(&it, t, BTF_FIELD_ITER_STRS);
> > +             if (err)
> > +                     return err;
> > +             while ((str_off = btf_field_iter_next(&it))) {
> > +                     s = btf__str_by_offset(obj->btf, *str_off);
> > +                     if (!s)
> > +                             return -EINVAL;
>
> nit, we could drop 's' and just do (!btf__str_by_offset(obj->btf, *str_off))
>

yep, we could, but I probably won't resubmit just for this. So if
something else comes up I'll clean this up, but it can go in as is
just as well, IMO.

>
> otherwise the patchset lgtm
>
> Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks for taking a look!

>
> jirka
>
> > +             }
> >       }
> >
> >       return 0;
> > @@ -2234,26 +2250,10 @@ static int linker_fixup_btf(struct src_obj *obj)
> >       return 0;
> >  }
>
> SNIP

Re: [PATCH bpf-next 1/5] libbpf: add BTF field iterator

[Eduard Zingerman]

On Mon, 2024-06-03 at 16:17 -0700, Andrii Nakryiko wrote:
> Implement iterator-based type ID and string offset BTF field iterator.
> This is used extensively in BTF-handling code and BPF linker code for
> various sanity checks, rewriting IDs/offsets, etc. Currently this is
> implemented as visitor pattern calling custom callbacks, which makes the
> logic (especially in simple cases) unnecessarily obscure and harder to
> follow.
> 
> Having equivalent functionality using iterator pattern makes for simpler
> to understand and maintain code. As we add more code for BTF processing
> logic in libbpf, it's best to switch to iterator pattern before adding
> more callback-based code.
> 
> The idea for iterator-based implementation is to record offsets of
> necessary fields within fixed btf_type parts (which should be iterated
> just once), and, for kinds that have multiple members (based on vlen
> field), record where in each member necessary fields are located.
> 
> Generic iteration code then just keeps track of last offset that was
> returned and handles N members correctly. Return type is just u32
> pointer, where NULL is returned when all relevant fields were already
> iterated.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> +__u32 *btf_field_iter_next(struct btf_field_iter *it)
> +{
> +	if (!it->p)
> +		return NULL;
> +
> +	if (it->m_idx < 0) {
> +		if (it->off_idx < it->desc.t_cnt)
> +			return it->p + it->desc.t_offs[it->off_idx++];
> +		/* move to per-member iteration */
> +		it->m_idx = 0;
> +		it->p += sizeof(struct btf_type);
> +		it->off_idx = 0;
> +	}
> +
> +	/* if type doesn't have members, stop */
> +	if (it->desc.m_sz == 0) {
> +		it->p = NULL;
> +		return NULL;
> +	}
> +
> +	if (it->off_idx >= it->desc.m_cnt) {
> +		/* exhausted this member's fields, go to the next member */
> +		it->m_idx++;
> +		it->p += it->desc.m_sz;
> +		it->off_idx = 0;
> +	}
> +
> +	if (it->m_idx < it->vlen)
> +		return it->p + it->desc.m_offs[it->off_idx++];

Nit: it is a bit confusing that for two 'if' statements above
     m_idx is guarded by vlen and off_idx is guarded by m_cnt :)

> +
> +	it->p = NULL;
> +	return NULL;
> +}
> +

[...]

Re: [PATCH bpf-next 0/5] libbpf: BTF field iterator

[Eduard Zingerman]

On Mon, 2024-06-03 at 16:17 -0700, Andrii Nakryiko wrote:
> Add BTF field (type and string fields, right now) iterator support instead of
> using existing callback-based approaches, which make it harder to understand
> and support BTF-processing code.
> 
> rfcv1->v1:
>   - check errors when initializing iterators (Jiri);
>   - split RFC patch into separate patches.
> 
> Andrii Nakryiko (5):
>   libbpf: add BTF field iterator
>   libbpf: make use of BTF field iterator in BPF linker code
>   libbpf: make use of BTF field iterator in BTF handling code
>   bpftool: use BTF field iterator in btfgen
>   libbpf: remove callback-based type/string BTF field visitor helpers

All looks good, thank you.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

Re: [PATCH bpf-next 1/5] libbpf: add BTF field iterator

[Andrii Nakryiko]

On Tue, Jun 4, 2024 at 1:37 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Mon, 2024-06-03 at 16:17 -0700, Andrii Nakryiko wrote:
> > Implement iterator-based type ID and string offset BTF field iterator.
> > This is used extensively in BTF-handling code and BPF linker code for
> > various sanity checks, rewriting IDs/offsets, etc. Currently this is
> > implemented as visitor pattern calling custom callbacks, which makes the
> > logic (especially in simple cases) unnecessarily obscure and harder to
> > follow.
> >
> > Having equivalent functionality using iterator pattern makes for simpler
> > to understand and maintain code. As we add more code for BTF processing
> > logic in libbpf, it's best to switch to iterator pattern before adding
> > more callback-based code.
> >
> > The idea for iterator-based implementation is to record offsets of
> > necessary fields within fixed btf_type parts (which should be iterated
> > just once), and, for kinds that have multiple members (based on vlen
> > field), record where in each member necessary fields are located.
> >
> > Generic iteration code then just keeps track of last offset that was
> > returned and handles N members correctly. Return type is just u32
> > pointer, where NULL is returned when all relevant fields were already
> > iterated.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
>
> [...]
>
> > +__u32 *btf_field_iter_next(struct btf_field_iter *it)
> > +{
> > +     if (!it->p)
> > +             return NULL;
> > +
> > +     if (it->m_idx < 0) {
> > +             if (it->off_idx < it->desc.t_cnt)
> > +                     return it->p + it->desc.t_offs[it->off_idx++];
> > +             /* move to per-member iteration */
> > +             it->m_idx = 0;
> > +             it->p += sizeof(struct btf_type);
> > +             it->off_idx = 0;
> > +     }
> > +
> > +     /* if type doesn't have members, stop */
> > +     if (it->desc.m_sz == 0) {
> > +             it->p = NULL;
> > +             return NULL;
> > +     }
> > +
> > +     if (it->off_idx >= it->desc.m_cnt) {
> > +             /* exhausted this member's fields, go to the next member */
> > +             it->m_idx++;
> > +             it->p += it->desc.m_sz;
> > +             it->off_idx = 0;
> > +     }
> > +
> > +     if (it->m_idx < it->vlen)
> > +             return it->p + it->desc.m_offs[it->off_idx++];
>
> Nit: it is a bit confusing that for two 'if' statements above
>      m_idx is guarded by vlen and off_idx is guarded by m_cnt :)

I'm open to suggestions. m_idx stands for "current member index",
m_cnt is for "per-member offset count", while "off_idx" is generic
"offset index" which indexes either a singular set of offsets or
per-member set of offsets. Easy ;)

>
> > +
> > +     it->p = NULL;
> > +     return NULL;
> > +}
> > +
>
> [...]

Re: [PATCH bpf-next 1/5] libbpf: add BTF field iterator

[Eduard Zingerman]

On Tue, 2024-06-04 at 14:40 -0700, Andrii Nakryiko wrote:

[...]

> > Nit: it is a bit confusing that for two 'if' statements above
> >      m_idx is guarded by vlen and off_idx is guarded by m_cnt :)
> 
> I'm open to suggestions. m_idx stands for "current member index",
> m_cnt is for "per-member offset count", while "off_idx" is generic
> "offset index" which indexes either a singular set of offsets or
> per-member set of offsets. Easy ;)

Well, since you've asked, how about renaming like below?
At-least 'off_idx' is always compared to something with 'off' in it's name.

---

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 1de7579f2a08..9ea09b808459 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -5169,7 +5169,7 @@ __u32 *btf_field_iter_next(struct btf_field_iter *it)
 		return NULL;
 
 	if (it->m_idx < 0) {
-		if (it->off_idx < it->desc.t_cnt)
+		if (it->off_idx < it->desc.t_offs_cnt)
 			return it->p + it->desc.t_offs[it->off_idx++];
 		/* move to per-member iteration */
 		it->m_idx = 0;
@@ -5183,7 +5183,7 @@ __u32 *btf_field_iter_next(struct btf_field_iter *it)
 		return NULL;
 	}
 
-	if (it->off_idx >= it->desc.m_cnt) {
+	if (it->off_idx >= it->desc.m_offs_cnt) {
 		/* exhausted this member's fields, go to the next member */
 		it->m_idx++;
 		it->p += it->desc.m_sz;
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 9f4a04367287..aa32b4537dba 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -515,11 +515,11 @@ enum btf_field_iter_kind {
 
 struct btf_field_desc {
 	/* once-per-type offsets */
-	int t_cnt, t_offs[2];
+	int t_offs_cnt, t_offs[2];
 	/* member struct size, or zero, if no members */
 	int m_sz;
 	/* repeated per-member offsets */
-	int m_cnt, m_offs[1];
+	int m_offs_cnt, m_offs[1];
 };
 
 struct btf_field_iter {

Re: [PATCH bpf-next 0/5] libbpf: BTF field iterator

[Alan Maguire]

On 04/06/2024 21:56, Eduard Zingerman wrote:
> On Mon, 2024-06-03 at 16:17 -0700, Andrii Nakryiko wrote:
>> Add BTF field (type and string fields, right now) iterator support instead of
>> using existing callback-based approaches, which make it harder to understand
>> and support BTF-processing code.
>>
>> rfcv1->v1:
>>   - check errors when initializing iterators (Jiri);
>>   - split RFC patch into separate patches.
>>
>> Andrii Nakryiko (5):
>>   libbpf: add BTF field iterator
>>   libbpf: make use of BTF field iterator in BPF linker code
>>   libbpf: make use of BTF field iterator in BTF handling code
>>   bpftool: use BTF field iterator in btfgen
>>   libbpf: remove callback-based type/string BTF field visitor helpers
> 
> All looks good, thank you.
> 
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>

looks great to me too; I tested and put together a selftest of field
iteration across all BTF kinds which I'll send out after this lands;
all tests pass as expected and for all kinds we see the expected strings
and ids.

For the series,

Tested-by: Alan Maguire <alan.maguire@oracle.com>