On Wed, Aug 28, 2024 at 3:02 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
On Wed, 2024-08-28 at 17:46 +0000, Ihor Solodrai wrote:
%.bpf.o objects depend on vmlinux.h, which makes them transitively
dependent on unnecessary libbpf headers. However vmlinux.h doesn't
actually change as often.
When generating vmlinux.h, compare it to a previous version and update
it only if there are changes.
Example of build time improvement (after first clean build):
$ touch ../../../lib/bpf/bpf.h
$ time make -j8
Before: real 1m37.592s
After: real 0m27.310s
Notice that %.bpf.o gen step is skipped if vmlinux.h hasn't changed.
Link: https://lore.kernel.org/bpf/CAEf4BzY1z5cC7BKye8=A8aTVxpsCzD=p1jdTfKC7i0XVuYoHUQ@xxxxxxxxxxxxxx
Signed-off-by: Ihor Solodrai <ihor.solodrai@xxxxx>
---
Unfortunately, I think that this is a half-measure.
E.g. the following command forces tests rebuild for me:
touch ../../../../kernel/bpf/verifier.c; \
make -j22 -C ../../../../; \
time make test_progs
To workaround this we need to enable reproducible_build option:
diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
index b75f09f3f424..8cd648f3e32b 100644
--- a/scripts/Makefile.btf
+++ b/scripts/Makefile.btf
@@ -19,7 +19,7 @@ pahole-flags-$(call test-ge, $(pahole-ver), 125) += --skip_encoding_btf_inconsis
else
# Switch to using --btf_features for v1.26 and later.
-pahole-flags-$(call test-ge, $(pahole-ver), 126) = -j --btf_features=encode_force,var,float,enum64,decl_tag,type_tag,optimized_func,consistent_func,decl_tag_kfuncs
+pahole-flags-$(call test-ge, $(pahole-ver), 126) = -j --btf_features=encode_force,var,float,enum64,decl_tag,type_tag,optimized_func,consistent_func,decl_tag_kfuncs,reproducible_build
ifneq ($(KBUILD_EXTMOD),)
module-pahole-flags-$(call test-ge, $(pahole-ver), 126) += --btf_features=distilled_base
Question to the mailing list: do we want this?
Alan, can you please give us a summary of what are the consequences of
the reproducible_build pahole option? In terms of performance and
otherwise.
I've applied patches as is, despite them not solving the issue
completely, as they are moving us in the right direction anyways. I do
get slightly different BTF every single time I rebuild my kernel, so
the change in patch #2 doesn't yet help me.
For libbpf headers, Ihor, can you please follow up with adding
bpf_helper_defs.h as a dependency?
I have some ideas on how to make BTF regeneration in vmlinux.h itself
unnecessary, that might help with this issue. Separately (depending on
what are the negatives of the reproducible_build option) we can look
into making pahole have more consistent internal BTF type ordering
without negatively affecting the overall BTF dedup performance in
pahole. Hopefully I can work with Ihor on this as follow ups.
P.S. I also spent more time than I'm willing to admit trying to
improve bpftool's BTF sorting to minimize the chance of vmlinux.h
contents being different, and I think I removed a bunch of cases where
we had unnecessary differences, but still, it's fundamentally
non-deterministic to do everything based on type and field names,
unfortunately.
Anyways, Mykyta (cc'ed), what do you think about the changes below?
Note that I'm also fixing the incorrect handling of enum64 (would be
nice to prepare a proper patch and send it upstream, if you get a
chance).
diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 6789c7a4d5ca..e8a244b09d56 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -50,6 +50,7 @@ struct sort_datum {
int type_rank;
const char *sort_name;
const char *own_name;
+ __u64 disambig_hash;
};
static const char *btf_int_enc_str(__u8 encoding)
@@ -552,35 +553,92 @@ static int btf_type_rank(const struct btf *btf,
__u32 index, bool has_name)
}
}
-static const char *btf_type_sort_name(const struct btf *btf, __u32
index, bool from_ref)
+static const char *btf_type_sort_name(const struct btf *btf, __u32
index, bool from_ref, const char *typedef_name)
{
const struct btf_type *t = btf__type_by_id(btf, index);
+ int name_off;
switch (btf_kind(t)) {
case BTF_KIND_ENUM:
- case BTF_KIND_ENUM64: {
- int name_off = t->name_off;
-
/* Use name of the first element for anonymous enums
if allowed */
if (!from_ref && !t->name_off && btf_vlen(t))
name_off = btf_enum(t)->name_off;
+ else
+ name_off = t->name_off;
+
+ return btf__name_by_offset(btf, name_off);
+ case BTF_KIND_ENUM64:
+ /* Use name of the first element for anonymous enums
if allowed */
+ if (!from_ref && !t->name_off && btf_vlen(t))
+ name_off = btf_enum64(t)->name_off;
+ else
+ name_off = t->name_off;
return btf__name_by_offset(btf, name_off);
- }
case BTF_KIND_ARRAY:
- return btf_type_sort_name(btf, btf_array(t)->type, true);
+ return btf_type_sort_name(btf, btf_array(t)->type,
true, typedef_name);
+ case BTF_KIND_STRUCT:
+ case BTF_KIND_UNION:
+ if (t->name_off == 0)
+ return typedef_name;
+ return btf__name_by_offset(btf, t->name_off);
+ case BTF_KIND_TYPEDEF:
+ return btf_type_sort_name(btf, t->type, true,
+ btf__name_by_offset(btf,
t->name_off));
case BTF_KIND_TYPE_TAG:
case BTF_KIND_CONST:
case BTF_KIND_PTR:
case BTF_KIND_VOLATILE:
case BTF_KIND_RESTRICT:
- case BTF_KIND_TYPEDEF:
case BTF_KIND_DECL_TAG:
- return btf_type_sort_name(btf, t->type, true);
+ return btf_type_sort_name(btf, t->type, true, typedef_name);
default:
return btf__name_by_offset(btf, t->name_off);
}
- return NULL;
+}
+
+static __u64 hasher(__u64 hash, __u64 val)
+{
+ return hash * 31 + val;
+}
+
+static __u64 btf_type_disambig_hash(const struct btf *btf, __u32 index)
+{
+ const struct btf_type *t = btf__type_by_id(btf, index);
+ int i;
+ size_t hash = 0;
+
+ switch (btf_kind(t)) {
+ case BTF_KIND_ENUM:
+ hash = hasher(hash, t->size);
+ for (i = 0; i < btf_vlen(t); i++)
+ hash = hasher(hash, btf_enum(t)[i].name_off);
+ break;
+ case BTF_KIND_ENUM64:
+ hash = hasher(hash, t->size);
+ for (i = 0; i < btf_vlen(t); i++)
+ hash = hasher(hash, btf_enum64(t)[i].name_off);
+ break;
+ case BTF_KIND_STRUCT:
+ case BTF_KIND_UNION: {
+ const struct btf_member *m;
+ const char *ftname;
+
+ hash = hasher(hash, t->size);
+ for (i = 0; i < btf_vlen(t); i++) {
+ m = btf_members(t) + i;
+ hash = hasher(hash, m->name_off);
+
+ /* resolve field type's name and hash it as well */
+ ftname = btf_type_sort_name(btf, m->type, false, "");
+ hash = hasher(hash, str_hash(ftname));
+ }
+ break;
+ }
+ default:
+ break;
+ }
+ return hash;
}
static int btf_type_compare(const void *left, const void *right)
@@ -596,7 +654,14 @@ static int btf_type_compare(const void *left,
const void *right)
if (r)
return r;
- return strcmp(d1->own_name, d2->own_name);
+ r = strcmp(d1->own_name, d2->own_name);
+ if (r)
+ return r;
+
+ if (d1->disambig_hash != d2->disambig_hash)
+ return d1->disambig_hash < d2->disambig_hash ? -1 : 1;
+
+ return d1->index < d2->index ? -1 : 1;
}
static struct sort_datum *sort_btf_c(const struct btf *btf)
@@ -615,8 +680,9 @@ static struct sort_datum *sort_btf_c(const struct btf *btf)
d->index = i;
d->type_rank = btf_type_rank(btf, i, false);
- d->sort_name = btf_type_sort_name(btf, i, false);
+ d->sort_name = btf_type_sort_name(btf, i, false, "");
d->own_name = btf__name_by_offset(btf, t->name_off);
+ d->disambig_hash = btf_type_disambig_hash(btf, i);
}
qsort(datums, n, sizeof(struct sort_datum), btf_type_compare);
