| Message ID | 20220514031253.3242578-1-yhs@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Add 64bit enum value support | expand |
On Fri, May 13, 2022 at 8:13 PM Yonghong Song <yhs@fb.com> wrote: > > Add enum64 deduplication support. BTF_KIND_ENUM64 handling > is very similar to BTF_KIND_ENUM. > > Signed-off-by: Yonghong Song <yhs@fb.com> > --- > tools/lib/bpf/btf.c | 55 +++++++++++++++++++++++++++++++++------------ > tools/lib/bpf/btf.h | 5 +++++ > 2 files changed, 46 insertions(+), 14 deletions(-) > [...] > +static bool btf_equal_enum64_val(struct btf_type *t1, struct btf_type *t2) > +{ > + const struct btf_enum64 *m1, *m2; > + __u16 vlen = btf_vlen(t1); > + int i; > + > + m1 = btf_enum64(t1); > + m2 = btf_enum64(t2); > + for (i = 0; i < vlen; i++) { > + if (m1->name_off != m2->name_off || m1->val_lo32 != m2->val_lo32 || > + m1->val_hi32 != m2->val_hi32) > + return false; > + m1++; > + m2++; > + } > + return true; > +} > + > +/* Check structural equality of two ENUMs. */ > +static bool btf_equal_enum_or_enum64(struct btf_type *t1, struct btf_type *t2) I find this helper quite confusing. It implies it can compare any enum or enum64 with each other, but it really allows only enum vs enum and enum64 vs enum64 (as it should!). Let's keep btf_equal_enum()/btf_compat_enum() completely intact and add btf_equal_enum64()/btf_compat_enum64() separately (few lines of copy-pasted code is totally fine to keep them separate, IMO). See below. > +{ > + if (!btf_equal_common(t1, t2)) > + return false; > + > + if (btf_is_enum(t1)) > + return btf_equal_enum32_val(t1, t2); > + return btf_equal_enum64_val(t1, t2); > +} > + > static inline bool btf_is_enum_fwd(struct btf_type *t) > { > - return btf_is_enum(t) && btf_vlen(t) == 0; > + return btf_type_is_any_enum(t) && btf_vlen(t) == 0; > } > > -static bool btf_compat_enum(struct btf_type *t1, struct btf_type *t2) > +static bool btf_compat_enum_or_enum64(struct btf_type *t1, struct btf_type *t2) > { > if (!btf_is_enum_fwd(t1) && !btf_is_enum_fwd(t2)) > - return btf_equal_enum(t1, t2); > + return btf_equal_enum_or_enum64(t1, t2); > /* ignore vlen when comparing */ > return t1->name_off == t2->name_off && > (t1->info & ~0xffff) == (t2->info & ~0xffff) && > @@ -3829,6 +3853,7 @@ static int btf_dedup_prep(struct btf_dedup *d) > h = btf_hash_int_decl_tag(t); > break; > case BTF_KIND_ENUM: > + case BTF_KIND_ENUM64: > h = btf_hash_enum(t); > break; > case BTF_KIND_STRUCT: > @@ -3898,15 +3923,16 @@ static int btf_dedup_prim_type(struct btf_dedup *d, __u32 type_id) > break; > > case BTF_KIND_ENUM: > + case BTF_KIND_ENUM64: > h = btf_hash_enum(t); > for_each_dedup_cand(d, hash_entry, h) { > cand_id = (__u32)(long)hash_entry->value; > cand = btf_type_by_id(d->btf, cand_id); > - if (btf_equal_enum(t, cand)) { > + if (btf_equal_enum_or_enum64(t, cand)) { I'd rather have this enum vs enum64 distinction right here: if ((btf_is_enum(t) && btf_equal_enum(t, cand)) || (btf_is_enum64(t) && btf_equal_enum64(t, cand))) { ... } > new_id = cand_id; > break; > } > - if (btf_compat_enum(t, cand)) { > + if (btf_compat_enum_or_enum64(t, cand)) { and same here with (btf_is_enum && btf_compat_enum) || (btf_is_num64 && btf_compat_enum64) ? Basically, I'd like to avoid worrying if we are accidentally mixing enum and enum64 or not. WDYT? > if (btf_is_enum_fwd(t)) { > /* resolve fwd to full enum */ > new_id = cand_id; > @@ -4211,7 +4237,8 @@ static int btf_dedup_is_equiv(struct btf_dedup *d, __u32 cand_id, > return btf_equal_int_tag(cand_type, canon_type); > > case BTF_KIND_ENUM: > - return btf_compat_enum(cand_type, canon_type); > + case BTF_KIND_ENUM64: > + return btf_compat_enum_or_enum64(cand_type, canon_type); > and here we just know whether we need btf_compat_enum vs btf_compat_enum64. > case BTF_KIND_FWD: > case BTF_KIND_FLOAT: > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h > index a41463bf9060..b22c648c69ff 100644 > --- a/tools/lib/bpf/btf.h > +++ b/tools/lib/bpf/btf.h > @@ -531,6 +531,11 @@ static inline bool btf_is_type_tag(const struct btf_type *t) > return btf_kind(t) == BTF_KIND_TYPE_TAG; > } > > +static inline bool btf_type_is_any_enum(const struct btf_type *t) > +{ > + return btf_is_enum(t) || btf_is_enum64(t); > +} > + > static inline __u8 btf_int_encoding(const struct btf_type *t) > { > return BTF_INT_ENCODING(*(__u32 *)(t + 1)); > -- > 2.30.2 >
On 5/16/22 5:28 PM, Andrii Nakryiko wrote: > On Fri, May 13, 2022 at 8:13 PM Yonghong Song <yhs@fb.com> wrote: >> >> Add enum64 deduplication support. BTF_KIND_ENUM64 handling >> is very similar to BTF_KIND_ENUM. >> >> Signed-off-by: Yonghong Song <yhs@fb.com> >> --- >> tools/lib/bpf/btf.c | 55 +++++++++++++++++++++++++++++++++------------ >> tools/lib/bpf/btf.h | 5 +++++ >> 2 files changed, 46 insertions(+), 14 deletions(-) >> > > [...] > >> +static bool btf_equal_enum64_val(struct btf_type *t1, struct btf_type *t2) >> +{ >> + const struct btf_enum64 *m1, *m2; >> + __u16 vlen = btf_vlen(t1); >> + int i; >> + >> + m1 = btf_enum64(t1); >> + m2 = btf_enum64(t2); >> + for (i = 0; i < vlen; i++) { >> + if (m1->name_off != m2->name_off || m1->val_lo32 != m2->val_lo32 || >> + m1->val_hi32 != m2->val_hi32) >> + return false; >> + m1++; >> + m2++; >> + } >> + return true; >> +} >> + >> +/* Check structural equality of two ENUMs. */ >> +static bool btf_equal_enum_or_enum64(struct btf_type *t1, struct btf_type *t2) > > I find this helper quite confusing. It implies it can compare any enum > or enum64 with each other, but it really allows only enum vs enum and > enum64 vs enum64 (as it should!). Let's keep > btf_equal_enum()/btf_compat_enum() completely intact and add > btf_equal_enum64()/btf_compat_enum64() separately (few lines of > copy-pasted code is totally fine to keep them separate, IMO). See > below. I debate with myself about whether I should use separate functions or use one function for both enum/enum64. My current approach will have less code changes. But I can do what you suggested to have separate functions for enum and enum64. This will apply to btf_compat_enum as well. > >> +{ >> + if (!btf_equal_common(t1, t2)) >> + return false; >> + >> + if (btf_is_enum(t1)) >> + return btf_equal_enum32_val(t1, t2); >> + return btf_equal_enum64_val(t1, t2); >> +} >> + >> static inline bool btf_is_enum_fwd(struct btf_type *t) >> { >> - return btf_is_enum(t) && btf_vlen(t) == 0; >> + return btf_type_is_any_enum(t) && btf_vlen(t) == 0; >> } >> [...]
On Tue, May 17, 2022 at 10:11 AM Yonghong Song <yhs@fb.com> wrote: > > > > On 5/16/22 5:28 PM, Andrii Nakryiko wrote: > > On Fri, May 13, 2022 at 8:13 PM Yonghong Song <yhs@fb.com> wrote: > >> > >> Add enum64 deduplication support. BTF_KIND_ENUM64 handling > >> is very similar to BTF_KIND_ENUM. > >> > >> Signed-off-by: Yonghong Song <yhs@fb.com> > >> --- > >> tools/lib/bpf/btf.c | 55 +++++++++++++++++++++++++++++++++------------ > >> tools/lib/bpf/btf.h | 5 +++++ > >> 2 files changed, 46 insertions(+), 14 deletions(-) > >> > > > > [...] > > > >> +static bool btf_equal_enum64_val(struct btf_type *t1, struct btf_type *t2) > >> +{ > >> + const struct btf_enum64 *m1, *m2; > >> + __u16 vlen = btf_vlen(t1); > >> + int i; > >> + > >> + m1 = btf_enum64(t1); > >> + m2 = btf_enum64(t2); > >> + for (i = 0; i < vlen; i++) { > >> + if (m1->name_off != m2->name_off || m1->val_lo32 != m2->val_lo32 || > >> + m1->val_hi32 != m2->val_hi32) > >> + return false; > >> + m1++; > >> + m2++; > >> + } > >> + return true; > >> +} > >> + > >> +/* Check structural equality of two ENUMs. */ > >> +static bool btf_equal_enum_or_enum64(struct btf_type *t1, struct btf_type *t2) > > > > I find this helper quite confusing. It implies it can compare any enum > > or enum64 with each other, but it really allows only enum vs enum and > > enum64 vs enum64 (as it should!). Let's keep > > btf_equal_enum()/btf_compat_enum() completely intact and add > > btf_equal_enum64()/btf_compat_enum64() separately (few lines of > > copy-pasted code is totally fine to keep them separate, IMO). See > > below. > > I debate with myself about whether I should use separate functions or > use one function for both enum/enum64. My current approach will have > less code changes. But I can do what you suggested to have separate > functions for enum and enum64. This will apply to btf_compat_enum > as well. yep, thanks! > > > > >> +{ > >> + if (!btf_equal_common(t1, t2)) > >> + return false; > >> + > >> + if (btf_is_enum(t1)) > >> + return btf_equal_enum32_val(t1, t2); > >> + return btf_equal_enum64_val(t1, t2); > >> +} > >> + > >> static inline bool btf_is_enum_fwd(struct btf_type *t) > >> { > >> - return btf_is_enum(t) && btf_vlen(t) == 0; > >> + return btf_type_is_any_enum(t) && btf_vlen(t) == 0; > >> } > >> > [...]
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c index 3bb6780f710d..016cec84e3dd 100644 --- a/tools/lib/bpf/btf.c +++ b/tools/lib/bpf/btf.c @@ -3568,7 +3568,7 @@ static bool btf_equal_int_tag(struct btf_type *t1, struct btf_type *t2) return info1 == info2; } -/* Calculate type signature hash of ENUM. */ +/* Calculate type signature hash of ENUM/ENUM64. */ static long btf_hash_enum(struct btf_type *t) { long h; @@ -3580,17 +3580,12 @@ static long btf_hash_enum(struct btf_type *t) return h; } -/* Check structural equality of two ENUMs. */ -static bool btf_equal_enum(struct btf_type *t1, struct btf_type *t2) +static bool btf_equal_enum32_val(struct btf_type *t1, struct btf_type *t2) { const struct btf_enum *m1, *m2; - __u16 vlen; + __u16 vlen = btf_vlen(t1); int i; - if (!btf_equal_common(t1, t2)) - return false; - - vlen = btf_vlen(t1); m1 = btf_enum(t1); m2 = btf_enum(t2); for (i = 0; i < vlen; i++) { @@ -3602,15 +3597,44 @@ static bool btf_equal_enum(struct btf_type *t1, struct btf_type *t2) return true; } +static bool btf_equal_enum64_val(struct btf_type *t1, struct btf_type *t2) +{ + const struct btf_enum64 *m1, *m2; + __u16 vlen = btf_vlen(t1); + int i; + + m1 = btf_enum64(t1); + m2 = btf_enum64(t2); + for (i = 0; i < vlen; i++) { + if (m1->name_off != m2->name_off || m1->val_lo32 != m2->val_lo32 || + m1->val_hi32 != m2->val_hi32) + return false; + m1++; + m2++; + } + return true; +} + +/* Check structural equality of two ENUMs. */ +static bool btf_equal_enum_or_enum64(struct btf_type *t1, struct btf_type *t2) +{ + if (!btf_equal_common(t1, t2)) + return false; + + if (btf_is_enum(t1)) + return btf_equal_enum32_val(t1, t2); + return btf_equal_enum64_val(t1, t2); +} + static inline bool btf_is_enum_fwd(struct btf_type *t) { - return btf_is_enum(t) && btf_vlen(t) == 0; + return btf_type_is_any_enum(t) && btf_vlen(t) == 0; } -static bool btf_compat_enum(struct btf_type *t1, struct btf_type *t2) +static bool btf_compat_enum_or_enum64(struct btf_type *t1, struct btf_type *t2) { if (!btf_is_enum_fwd(t1) && !btf_is_enum_fwd(t2)) - return btf_equal_enum(t1, t2); + return btf_equal_enum_or_enum64(t1, t2); /* ignore vlen when comparing */ return t1->name_off == t2->name_off && (t1->info & ~0xffff) == (t2->info & ~0xffff) && @@ -3829,6 +3853,7 @@ static int btf_dedup_prep(struct btf_dedup *d) h = btf_hash_int_decl_tag(t); break; case BTF_KIND_ENUM: + case BTF_KIND_ENUM64: h = btf_hash_enum(t); break; case BTF_KIND_STRUCT: @@ -3898,15 +3923,16 @@ static int btf_dedup_prim_type(struct btf_dedup *d, __u32 type_id) break; case BTF_KIND_ENUM: + case BTF_KIND_ENUM64: h = btf_hash_enum(t); for_each_dedup_cand(d, hash_entry, h) { cand_id = (__u32)(long)hash_entry->value; cand = btf_type_by_id(d->btf, cand_id); - if (btf_equal_enum(t, cand)) { + if (btf_equal_enum_or_enum64(t, cand)) { new_id = cand_id; break; } - if (btf_compat_enum(t, cand)) { + if (btf_compat_enum_or_enum64(t, cand)) { if (btf_is_enum_fwd(t)) { /* resolve fwd to full enum */ new_id = cand_id; @@ -4211,7 +4237,8 @@ static int btf_dedup_is_equiv(struct btf_dedup *d, __u32 cand_id, return btf_equal_int_tag(cand_type, canon_type); case BTF_KIND_ENUM: - return btf_compat_enum(cand_type, canon_type); + case BTF_KIND_ENUM64: + return btf_compat_enum_or_enum64(cand_type, canon_type); case BTF_KIND_FWD: case BTF_KIND_FLOAT: diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h index a41463bf9060..b22c648c69ff 100644 --- a/tools/lib/bpf/btf.h +++ b/tools/lib/bpf/btf.h @@ -531,6 +531,11 @@ static inline bool btf_is_type_tag(const struct btf_type *t) return btf_kind(t) == BTF_KIND_TYPE_TAG; } +static inline bool btf_type_is_any_enum(const struct btf_type *t) +{ + return btf_is_enum(t) || btf_is_enum64(t); +} + static inline __u8 btf_int_encoding(const struct btf_type *t) { return BTF_INT_ENCODING(*(__u32 *)(t + 1));
Add enum64 deduplication support. BTF_KIND_ENUM64 handling is very similar to BTF_KIND_ENUM. Signed-off-by: Yonghong Song <yhs@fb.com> --- tools/lib/bpf/btf.c | 55 +++++++++++++++++++++++++++++++++------------ tools/lib/bpf/btf.h | 5 +++++ 2 files changed, 46 insertions(+), 14 deletions(-)