diff mbox series

[bpf-next,1/5] libbpf: BTF relocation followup fixing naming, loop logic

Message ID 20240618162449.809994-2-alan.maguire@oracle.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series bpf: resilient split BTF followups | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success Errors and warnings before: 1 this patch: 1
netdev/cc_maintainers success CCed 13 of 13 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Alan Maguire June 18, 2024, 4:24 p.m. UTC 
Use less verbose names in BTF relocation code and fix off-by-one error
and typo in btf_relocate.c.  Simplify loop over matching distilled
types, moving from assigning a _next value in loop body to moving
match check conditions into the guard.

Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 tools/lib/bpf/btf_relocate.c | 74 ++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 41 deletions(-)

Comments

Andrii Nakryiko June 18, 2024, 10:48 p.m. UTC | #1 
On Tue, Jun 18, 2024 at 9:25 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> Use less verbose names in BTF relocation code and fix off-by-one error
> and typo in btf_relocate.c.  Simplify loop over matching distilled
> types, moving from assigning a _next value in loop body to moving
> match check conditions into the guard.
>
> Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  tools/lib/bpf/btf_relocate.c | 74 ++++++++++++++++--------------------
>  1 file changed, 33 insertions(+), 41 deletions(-)
>

Few more nits, but generally looks great, thanks!

> diff --git a/tools/lib/bpf/btf_relocate.c b/tools/lib/bpf/btf_relocate.c
> index eabb8755f662..64cd8bdc0105 100644
> --- a/tools/lib/bpf/btf_relocate.c
> +++ b/tools/lib/bpf/btf_relocate.c
> @@ -160,7 +160,7 @@ static int btf_mark_embedded_composite_type_ids(struct btf_relocate *r, __u32 i)
>   */
>  static int btf_relocate_map_distilled_base(struct btf_relocate *r)
>  {
> -       struct btf_name_info *dist_base_info_sorted, *dist_base_info_sorted_end;
> +       struct btf_name_info *info, *info_end;
>         struct btf_type *base_t, *dist_t;
>         __u8 *base_name_cnt = NULL;
>         int err = 0;
> @@ -169,26 +169,25 @@ static int btf_relocate_map_distilled_base(struct btf_relocate *r)
>         /* generate a sort index array of name/type ids sorted by name for
>          * distilled base BTF to speed name-based lookups.
>          */
> -       dist_base_info_sorted = calloc(r->nr_dist_base_types, sizeof(*dist_base_info_sorted));
> -       if (!dist_base_info_sorted) {
> +       info = calloc(r->nr_dist_base_types, sizeof(*info));
> +       if (!info) {
>                 err = -ENOMEM;
>                 goto done;
>         }
> -       dist_base_info_sorted_end = dist_base_info_sorted + r->nr_dist_base_types;
> +       info_end = info + r->nr_dist_base_types;
>         for (id = 0; id < r->nr_dist_base_types; id++) {
>                 dist_t = btf_type_by_id(r->dist_base_btf, id);
> -               dist_base_info_sorted[id].name = btf__name_by_offset(r->dist_base_btf,
> -                                                                    dist_t->name_off);
> -               dist_base_info_sorted[id].id = id;
> -               dist_base_info_sorted[id].size = dist_t->size;
> -               dist_base_info_sorted[id].needs_size = true;
> +               info[id].name = btf__name_by_offset(r->dist_base_btf,
> +                                                   dist_t->name_off);

please make it a single line, now that it's much shorter

> +               info[id].id = id;
> +               info[id].size = dist_t->size;
> +               info[id].needs_size = true;
>         }

[...]

>                 /* iterate over all matching distilled base types */
> -               for (dist_name_info = search_btf_name_size(&base_name_info, dist_base_info_sorted,
> -                                                          r->nr_dist_base_types);
> -                    dist_name_info != NULL; dist_name_info = dist_name_info_next) {
> -                       /* Are there more distilled matches to process after
> -                        * this one?
> -                        */
> -                       dist_name_info_next = dist_name_info + 1;
> -                       if (dist_name_info_next >= dist_base_info_sorted_end ||
> -                           cmp_btf_name_size(&base_name_info, dist_name_info_next))
> -                               dist_name_info_next = NULL;
> -
> -                       if (!dist_name_info->id || dist_name_info->id > r->nr_dist_base_types) {
> +               for (dist_info = search_btf_name_size(&base_info, info,
> +                                                     r->nr_dist_base_types);

does it fit under 100? please prioritize keeping single-line code as
much as possible

> +                    dist_info && dist_info < info_end &&

I missed the need for `dist_info < info_end` in my original
suggestion, but yes, this looks much better, thanks (and yeah, I don't
think one extra cmp_btf_name_size() call matters much).

> +                    !cmp_btf_name_size(&base_info, dist_info);

nit: given this is strcmp()-like function, I'd prefer explicit `== 0`
instead of boolean-like !


> +                    dist_info++) {
> +                       if (!dist_info->id || dist_info->id >= r->nr_dist_base_types) {
>                                 pr_warn("base BTF id [%d] maps to invalid distilled base BTF id [%d]\n",
> -                                       id, dist_name_info->id);
> +                                       id, dist_info->id);
>                                 err = -EINVAL;
>                                 goto done;
>                         }
> -                       dist_t = btf_type_by_id(r->dist_base_btf, dist_name_info->id);
> +                       dist_t = btf_type_by_id(r->dist_base_btf, dist_info->id);
>                         dist_kind = btf_kind(dist_t);
>

[...]
diff mbox series

Patch

diff --git a/tools/lib/bpf/btf_relocate.c b/tools/lib/bpf/btf_relocate.c
index eabb8755f662..64cd8bdc0105 100644
--- a/tools/lib/bpf/btf_relocate.c
+++ b/tools/lib/bpf/btf_relocate.c
@@ -160,7 +160,7 @@  static int btf_mark_embedded_composite_type_ids(struct btf_relocate *r, __u32 i)
  */
 static int btf_relocate_map_distilled_base(struct btf_relocate *r)
 {
-	struct btf_name_info *dist_base_info_sorted, *dist_base_info_sorted_end;
+	struct btf_name_info *info, *info_end;
 	struct btf_type *base_t, *dist_t;
 	__u8 *base_name_cnt = NULL;
 	int err = 0;
@@ -169,26 +169,25 @@  static int btf_relocate_map_distilled_base(struct btf_relocate *r)
 	/* generate a sort index array of name/type ids sorted by name for
 	 * distilled base BTF to speed name-based lookups.
 	 */
-	dist_base_info_sorted = calloc(r->nr_dist_base_types, sizeof(*dist_base_info_sorted));
-	if (!dist_base_info_sorted) {
+	info = calloc(r->nr_dist_base_types, sizeof(*info));
+	if (!info) {
 		err = -ENOMEM;
 		goto done;
 	}
-	dist_base_info_sorted_end = dist_base_info_sorted + r->nr_dist_base_types;
+	info_end = info + r->nr_dist_base_types;
 	for (id = 0; id < r->nr_dist_base_types; id++) {
 		dist_t = btf_type_by_id(r->dist_base_btf, id);
-		dist_base_info_sorted[id].name = btf__name_by_offset(r->dist_base_btf,
-								     dist_t->name_off);
-		dist_base_info_sorted[id].id = id;
-		dist_base_info_sorted[id].size = dist_t->size;
-		dist_base_info_sorted[id].needs_size = true;
+		info[id].name = btf__name_by_offset(r->dist_base_btf,
+						    dist_t->name_off);
+		info[id].id = id;
+		info[id].size = dist_t->size;
+		info[id].needs_size = true;
 	}
-	qsort(dist_base_info_sorted, r->nr_dist_base_types, sizeof(*dist_base_info_sorted),
-	      cmp_btf_name_size);
+	qsort(info, r->nr_dist_base_types, sizeof(*info), cmp_btf_name_size);
 
 	/* Mark distilled base struct/union members of split BTF structs/unions
 	 * in id_map with BTF_IS_EMBEDDED; this signals that these types
-	 * need to match both name and size, otherwise embeddding the base
+	 * need to match both name and size, otherwise embedding the base
 	 * struct/union in the split type is invalid.
 	 */
 	for (id = r->nr_dist_base_types; id < r->nr_split_types; id++) {
@@ -216,8 +215,7 @@  static int btf_relocate_map_distilled_base(struct btf_relocate *r)
 
 	/* Now search base BTF for matching distilled base BTF types. */
 	for (id = 1; id < r->nr_base_types; id++) {
-		struct btf_name_info *dist_name_info, *dist_name_info_next = NULL;
-		struct btf_name_info base_name_info = {};
+		struct btf_name_info *dist_info, base_info = {};
 		int dist_kind, base_kind;
 
 		base_t = btf_type_by_id(r->base_btf, id);
@@ -225,16 +223,16 @@  static int btf_relocate_map_distilled_base(struct btf_relocate *r)
 		if (!base_t->name_off)
 			continue;
 		base_kind = btf_kind(base_t);
-		base_name_info.id = id;
-		base_name_info.name = btf__name_by_offset(r->base_btf, base_t->name_off);
+		base_info.id = id;
+		base_info.name = btf__name_by_offset(r->base_btf, base_t->name_off);
 		switch (base_kind) {
 		case BTF_KIND_INT:
 		case BTF_KIND_FLOAT:
 		case BTF_KIND_ENUM:
 		case BTF_KIND_ENUM64:
 			/* These types should match both name and size */
-			base_name_info.needs_size = true;
-			base_name_info.size = base_t->size;
+			base_info.needs_size = true;
+			base_info.size = base_t->size;
 			break;
 		case BTF_KIND_FWD:
 			/* No size considerations for fwds. */
@@ -248,31 +246,25 @@  static int btf_relocate_map_distilled_base(struct btf_relocate *r)
 			 * unless corresponding _base_ types to match them are
 			 * missing.
 			 */
-			base_name_info.needs_size = base_name_cnt[base_t->name_off] > 1;
-			base_name_info.size = base_t->size;
+			base_info.needs_size = base_name_cnt[base_t->name_off] > 1;
+			base_info.size = base_t->size;
 			break;
 		default:
 			continue;
 		}
 		/* iterate over all matching distilled base types */
-		for (dist_name_info = search_btf_name_size(&base_name_info, dist_base_info_sorted,
-							   r->nr_dist_base_types);
-		     dist_name_info != NULL; dist_name_info = dist_name_info_next) {
-			/* Are there more distilled matches to process after
-			 * this one?
-			 */
-			dist_name_info_next = dist_name_info + 1;
-			if (dist_name_info_next >= dist_base_info_sorted_end ||
-			    cmp_btf_name_size(&base_name_info, dist_name_info_next))
-				dist_name_info_next = NULL;
-
-			if (!dist_name_info->id || dist_name_info->id > r->nr_dist_base_types) {
+		for (dist_info = search_btf_name_size(&base_info, info,
+						      r->nr_dist_base_types);
+		     dist_info && dist_info < info_end &&
+		     !cmp_btf_name_size(&base_info, dist_info);
+		     dist_info++) {
+			if (!dist_info->id || dist_info->id >= r->nr_dist_base_types) {
 				pr_warn("base BTF id [%d] maps to invalid distilled base BTF id [%d]\n",
-					id, dist_name_info->id);
+					id, dist_info->id);
 				err = -EINVAL;
 				goto done;
 			}
-			dist_t = btf_type_by_id(r->dist_base_btf, dist_name_info->id);
+			dist_t = btf_type_by_id(r->dist_base_btf, dist_info->id);
 			dist_kind = btf_kind(dist_t);
 
 			/* Validate that the found distilled type is compatible.
@@ -319,15 +311,15 @@  static int btf_relocate_map_distilled_base(struct btf_relocate *r)
 				/* size verification is required for embedded
 				 * struct/unions.
 				 */
-				if (r->id_map[dist_name_info->id] == BTF_IS_EMBEDDED &&
+				if (r->id_map[dist_info->id] == BTF_IS_EMBEDDED &&
 				    base_t->size != dist_t->size)
 					continue;
 				break;
 			default:
 				continue;
 			}
-			if (r->id_map[dist_name_info->id] &&
-			    r->id_map[dist_name_info->id] != BTF_IS_EMBEDDED) {
+			if (r->id_map[dist_info->id] &&
+			    r->id_map[dist_info->id] != BTF_IS_EMBEDDED) {
 				/* we already have a match; this tells us that
 				 * multiple base types of the same name
 				 * have the same size, since for cases where
@@ -337,13 +329,13 @@  static int btf_relocate_map_distilled_base(struct btf_relocate *r)
 				 * to in base BTF, so error out.
 				 */
 				pr_warn("distilled base BTF type '%s' [%u], size %u has multiple candidates of the same size (ids [%u, %u]) in base BTF\n",
-					base_name_info.name, dist_name_info->id,
-					base_t->size, id, r->id_map[dist_name_info->id]);
+					base_info.name, dist_info->id,
+					base_t->size, id, r->id_map[dist_info->id]);
 				err = -EINVAL;
 				goto done;
 			}
 			/* map id and name */
-			r->id_map[dist_name_info->id] = id;
+			r->id_map[dist_info->id] = id;
 			r->str_map[dist_t->name_off] = base_t->name_off;
 		}
 	}
@@ -362,7 +354,7 @@  static int btf_relocate_map_distilled_base(struct btf_relocate *r)
 	}
 done:
 	free(base_name_cnt);
-	free(dist_base_info_sorted);
+	free(info);
 	return err;
 }