| Message ID | 20240916091921.2929615-1-eddyz87@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [dwarves,v1] pahole: generate "bpf_fastcall" decl tags for eligible kfuncs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, 2024-09-16 at 02:19 -0700, Eduard Zingerman wrote: > For kfuncs marked with KF_FASTCALL flag generate the following pair of > decl tags: > > $ bpftool btf dump file vmlinux > ... > [A] FUNC 'bpf_rdonly_cast' type_id=... > ... > [B] DECL_TAG 'bpf_kfunc' type_id=A component_idx=-1 > [C] DECL_TAG 'bpf_fastcall' type_id=A component_idx=-1 Note, corresponding kernel changes are submitted here: https://lore.kernel.org/bpf/20240916091712.2929279-1-eddyz87@gmail.com/ [...]
On 16/09/2024 10:19, Eduard Zingerman wrote: > For kfuncs marked with KF_FASTCALL flag generate the following pair of > decl tags: > > $ bpftool btf dump file vmlinux > ... > [A] FUNC 'bpf_rdonly_cast' type_id=... > ... > [B] DECL_TAG 'bpf_kfunc' type_id=A component_idx=-1 > [C] DECL_TAG 'bpf_fastcall' type_id=A component_idx=-1 > > So that bpftool could find 'bpf_fastcall' decl tag and generate > appropriate C declarations for such kfuncs, e.g.: > > #ifndef __VMLINUX_H__ > #define __VMLINUX_H__ > ... > #define __bpf_fastcall __attribute__((bpf_fastcall)) > ... > __bpf_fastcall extern void *bpf_rdonly_cast(...) ...; > > For additional information about 'bpf_fastcall' attribute, > see the following commit in the LLVM source tree: > > 64e464349bfc ("[BPF] introduce __attribute__((bpf_fastcall))") > > And the following Linux kernel commit: > > 52839f31cece ("Merge branch 'no_caller_saved_registers-attribute-for-helper-calls'") > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> hi Eduard, you've added support for multiple declaration tags as part of this, but I wonder if we could go slightly further to simplify any additional future KF_* flags -> decl tag needs? Specifically if we had an array of <set8 flags, tag name> mappings such that we can add support for new declaration tags by simply adding a new flag and declaration tag string. When checking flags value in btf_encoder__tag_kfunc(), we'd just walk the array entries, and for each matching flag add the associated decl tag. Would that work? > --- > btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 43 insertions(+), 16 deletions(-) > > diff --git a/btf_encoder.c b/btf_encoder.c > index 8a2d92e..ae059e0 100644 > --- a/btf_encoder.c > +++ b/btf_encoder.c > @@ -39,15 +39,19 @@ > #define BTF_ID_SET8_PFX "__BTF_ID__set8__" > #define BTF_SET8_KFUNCS (1 << 0) > #define BTF_KFUNC_TYPE_TAG "bpf_kfunc" > +#define BTF_FASTCALL_TAG "bpf_fastcall" > +#define KF_FASTCALL (1 << 12) > + probably need an #ifndef KF_FASTCALL/#endif here once this makes it into uapi. > +struct btf_id_and_flag { > + uint32_t id; > + uint32_t flags; > +}; > > /* Adapted from include/linux/btf_ids.h */ > struct btf_id_set8 { > uint32_t cnt; > uint32_t flags; > - struct { > - uint32_t id; > - uint32_t flags; > - } pairs[]; > + struct btf_id_and_flag pairs[]; > }; > > /* state used to do later encoding of saved functions */ > @@ -1517,21 +1521,34 @@ out: > return err; > } > > -static int btf_encoder__tag_kfunc(struct btf_encoder *encoder, struct gobuffer *funcs, const char *kfunc) > +static int add_kfunc_decl_tag(struct btf *btf, const char *tag, __u32 id, const char *kfunc) > +{ > + int err; > + > + err = btf__add_decl_tag(btf, tag, id, -1); > + if (err < 0) { > + fprintf(stderr, "%s: failed to insert kfunc decl tag for '%s': %d\n", > + __func__, kfunc, err); > + return err; > + } > + return 0; > +} > + > +static int btf_encoder__tag_kfunc(struct btf_encoder *encoder, struct gobuffer *funcs, const char *kfunc, __u32 flags) > { > struct btf_func key = { .name = kfunc }; > struct btf *btf = encoder->btf; > struct btf_func *target; > const void *base; > unsigned int cnt; > - int err = -1; > + int err; > > base = gobuffer__entries(funcs); > cnt = gobuffer__nr_entries(funcs); > target = bsearch(&key, base, cnt, sizeof(key), btf_func_cmp); > if (!target) { > fprintf(stderr, "%s: failed to find kfunc '%s' in BTF\n", __func__, kfunc); > - goto out; > + return -1; > } > > /* Note we are unconditionally adding the btf_decl_tag even > @@ -1539,16 +1556,16 @@ static int btf_encoder__tag_kfunc(struct btf_encoder *encoder, struct gobuffer * > * We are ok to do this b/c we will later btf__dedup() to remove > * any duplicates. > */ > - err = btf__add_decl_tag(btf, BTF_KFUNC_TYPE_TAG, target->type_id, -1); > - if (err < 0) { > - fprintf(stderr, "%s: failed to insert kfunc decl tag for '%s': %d\n", > - __func__, kfunc, err); > - goto out; > + err = add_kfunc_decl_tag(btf, BTF_KFUNC_TYPE_TAG, target->type_id, kfunc); > + if (err < 0) > + return err; > + if (flags & KF_FASTCALL) { > + err = add_kfunc_decl_tag(btf, BTF_FASTCALL_TAG, target->type_id, kfunc); > + if (err < 0) > + return err; > } > > - err = 0; > -out: > - return err; > + return 0; > } > > static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder) > @@ -1675,8 +1692,10 @@ static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder) > /* Now inject BTF with kfunc decl tag for detected kfuncs */ > for (i = 0; i < nr_syms; i++) { > const struct btf_kfunc_set_range *ranges; > + const struct btf_id_and_flag *pair; > unsigned int ranges_cnt; > char *func, *name; > + ptrdiff_t off; > GElf_Sym sym; > bool found; > int err; > @@ -1704,6 +1723,14 @@ static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder) > > if (ranges[j].start <= addr && addr < ranges[j].end) { > found = true; > + off = addr - idlist_addr; > + if (off < 0 || off + sizeof(*pair) > idlist->d_size) { > + fprintf(stderr, "%s: kfunc '%s' offset outside section '%s'\n", > + __func__, func, BTF_IDS_SECTION); > + free(func); > + goto out; > + } > + pair = idlist->d_buf + off; > break; > } > } > @@ -1712,7 +1739,7 @@ static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder) > continue; > } > > - err = btf_encoder__tag_kfunc(encoder, &btf_funcs, func); > + err = btf_encoder__tag_kfunc(encoder, &btf_funcs, func, pair->flags); > if (err) { > fprintf(stderr, "%s: failed to tag kfunc '%s'\n", __func__, func); > free(func);
On Mon, 2024-09-16 at 11:16 +0100, Alan Maguire wrote: [...] > hi Eduard, > > you've added support for multiple declaration tags as part of this, but > I wonder if we could go slightly further to simplify any additional > future KF_* flags -> decl tag needs? > > Specifically if we had an array of <set8 flags, tag name> mappings such > that we can add support for new declaration tags by simply adding a new > flag and declaration tag string. When checking flags value in > btf_encoder__tag_kfunc(), we'd just walk the array entries, and for each > matching flag add the associated decl tag. Would that work? Hi Alan, That would be something like below. It works, but looks a bit over-complicated for my taste, wdyt? --- 8< ---------------------------------------- iff --git a/btf_encoder.c b/btf_encoder.c index ae059e0..b6178c3 100644 --- a/btf_encoder.c +++ b/btf_encoder.c @@ -39,7 +39,6 @@ #define BTF_ID_SET8_PFX "__BTF_ID__set8__" #define BTF_SET8_KFUNCS (1 << 0) #define BTF_KFUNC_TYPE_TAG "bpf_kfunc" -#define BTF_FASTCALL_TAG "bpf_fastcall" #define KF_FASTCALL (1 << 12) struct btf_id_and_flag { @@ -1534,6 +1533,15 @@ static int add_kfunc_decl_tag(struct btf *btf, const char *tag, __u32 id, const return 0; } +enum kf_bit_nums { + KF_BIT_NUM_FASTCALL = 12, + KF_BIT_NUM_FASTCALL_NR +}; + +static const char *kfunc_tags[KF_BIT_NUM_FASTCALL_NR] = { + [KF_BIT_NUM_FASTCALL] = "bpf_fastcall" +}; + static int btf_encoder__tag_kfunc(struct btf_encoder *encoder, struct gobuffer *funcs, const char *kfunc, __u32 flags) { struct btf_func key = { .name = kfunc }; @@ -1559,8 +1567,11 @@ static int btf_encoder__tag_kfunc(struct btf_encoder *encoder, struct gobuffer * err = add_kfunc_decl_tag(btf, BTF_KFUNC_TYPE_TAG, target->type_id, kfunc); if (err < 0) return err; - if (flags & KF_FASTCALL) { - err = add_kfunc_decl_tag(btf, BTF_FASTCALL_TAG, target->type_id, kfunc); + + for (uint32_t i = 0; i < KF_BIT_NUM_FASTCALL_NR; i++) { + if (!(flags & (1u << i)) || !kfunc_tags[i]) + continue; + err = add_kfunc_decl_tag(btf, kfunc_tags[i], target->type_id, kfunc); if (err < 0) return err; } ---------------------------------------- >8 --- > > > --- > > btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++-------------- > > 1 file changed, 43 insertions(+), 16 deletions(-) > > > > diff --git a/btf_encoder.c b/btf_encoder.c > > index 8a2d92e..ae059e0 100644 > > --- a/btf_encoder.c > > +++ b/btf_encoder.c > > @@ -39,15 +39,19 @@ > > #define BTF_ID_SET8_PFX "__BTF_ID__set8__" > > #define BTF_SET8_KFUNCS (1 << 0) > > #define BTF_KFUNC_TYPE_TAG "bpf_kfunc" > > +#define BTF_FASTCALL_TAG "bpf_fastcall" > > +#define KF_FASTCALL (1 << 12) > > + > > probably need an #ifndef KF_FASTCALL/#endif here once this makes it into > uapi. kfunc flags are defined in include/linux/btf.h so these should not be visible in the uapi/linux/btf.h, unless I'm confused. > > > > +struct btf_id_and_flag { > > + uint32_t id; > > + uint32_t flags; > > +}; [...]
On 17/09/2024 05:40, Eduard Zingerman wrote: > On Mon, 2024-09-16 at 11:16 +0100, Alan Maguire wrote: > > [...] > >> hi Eduard, >> >> you've added support for multiple declaration tags as part of this, but >> I wonder if we could go slightly further to simplify any additional >> future KF_* flags -> decl tag needs? >> >> Specifically if we had an array of <set8 flags, tag name> mappings such >> that we can add support for new declaration tags by simply adding a new >> flag and declaration tag string. When checking flags value in >> btf_encoder__tag_kfunc(), we'd just walk the array entries, and for each >> matching flag add the associated decl tag. Would that work? > > Hi Alan, > > That would be something like below. > It works, but looks a bit over-complicated for my taste, wdyt? yeah, I guess I was thinking the kfunc flag would be checked along with KF_FASTCALL, but the code already knows it's a kfunc, so no need. We can add something like this if/when other flags are added. Sorry for the noise! Reviewed-by: Alan Maguire <alan.maguire@oracle.com> > > --- 8< ---------------------------------------- > iff --git a/btf_encoder.c b/btf_encoder.c > index ae059e0..b6178c3 100644 > --- a/btf_encoder.c > +++ b/btf_encoder.c > @@ -39,7 +39,6 @@ > #define BTF_ID_SET8_PFX "__BTF_ID__set8__" > #define BTF_SET8_KFUNCS (1 << 0) > #define BTF_KFUNC_TYPE_TAG "bpf_kfunc" > -#define BTF_FASTCALL_TAG "bpf_fastcall" > #define KF_FASTCALL (1 << 12) > > struct btf_id_and_flag { > @@ -1534,6 +1533,15 @@ static int add_kfunc_decl_tag(struct btf *btf, const char *tag, __u32 id, const > return 0; > } > > +enum kf_bit_nums { > + KF_BIT_NUM_FASTCALL = 12, > + KF_BIT_NUM_FASTCALL_NR > +}; > + > +static const char *kfunc_tags[KF_BIT_NUM_FASTCALL_NR] = { > + [KF_BIT_NUM_FASTCALL] = "bpf_fastcall" > +}; > + > static int btf_encoder__tag_kfunc(struct btf_encoder *encoder, struct gobuffer *funcs, const char *kfunc, __u32 flags) > { > struct btf_func key = { .name = kfunc }; > @@ -1559,8 +1567,11 @@ static int btf_encoder__tag_kfunc(struct btf_encoder *encoder, struct gobuffer * > err = add_kfunc_decl_tag(btf, BTF_KFUNC_TYPE_TAG, target->type_id, kfunc); > if (err < 0) > return err; > - if (flags & KF_FASTCALL) { > - err = add_kfunc_decl_tag(btf, BTF_FASTCALL_TAG, target->type_id, kfunc); > + > + for (uint32_t i = 0; i < KF_BIT_NUM_FASTCALL_NR; i++) { > + if (!(flags & (1u << i)) || !kfunc_tags[i]) > + continue; > + err = add_kfunc_decl_tag(btf, kfunc_tags[i], target->type_id, kfunc); > if (err < 0) > return err; > } > ---------------------------------------- >8 --- > >> >>> --- >>> btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++-------------- >>> 1 file changed, 43 insertions(+), 16 deletions(-) >>> >>> diff --git a/btf_encoder.c b/btf_encoder.c >>> index 8a2d92e..ae059e0 100644 >>> --- a/btf_encoder.c >>> +++ b/btf_encoder.c >>> @@ -39,15 +39,19 @@ >>> #define BTF_ID_SET8_PFX "__BTF_ID__set8__" >>> #define BTF_SET8_KFUNCS (1 << 0) >>> #define BTF_KFUNC_TYPE_TAG "bpf_kfunc" >>> +#define BTF_FASTCALL_TAG "bpf_fastcall" >>> +#define KF_FASTCALL (1 << 12) >>> + >> >> probably need an #ifndef KF_FASTCALL/#endif here once this makes it into >> uapi. > > kfunc flags are defined in include/linux/btf.h so these should not be > visible in the uapi/linux/btf.h, unless I'm confused. > ah, ok sorry I thought it was UAPI, never mind. >> >> >>> +struct btf_id_and_flag { >>> + uint32_t id; >>> + uint32_t flags; >>> +}; > > [...] >
On Mon, Sep 16, 2024 at 2:19 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > For kfuncs marked with KF_FASTCALL flag generate the following pair of > decl tags: > > $ bpftool btf dump file vmlinux > ... > [A] FUNC 'bpf_rdonly_cast' type_id=... > ... > [B] DECL_TAG 'bpf_kfunc' type_id=A component_idx=-1 > [C] DECL_TAG 'bpf_fastcall' type_id=A component_idx=-1 > > So that bpftool could find 'bpf_fastcall' decl tag and generate > appropriate C declarations for such kfuncs, e.g.: > > #ifndef __VMLINUX_H__ > #define __VMLINUX_H__ > ... > #define __bpf_fastcall __attribute__((bpf_fastcall)) > ... > __bpf_fastcall extern void *bpf_rdonly_cast(...) ...; > > For additional information about 'bpf_fastcall' attribute, > see the following commit in the LLVM source tree: > > 64e464349bfc ("[BPF] introduce __attribute__((bpf_fastcall))") > > And the following Linux kernel commit: > > 52839f31cece ("Merge branch 'no_caller_saved_registers-attribute-for-helper-calls'") > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> > --- > btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 43 insertions(+), 16 deletions(-) > LGTM, Acked-by: Andrii Nakryiko <andrii@kernel.org> Arnaldo, can you please take a look and if everything seems sane apply it to pahole master, so it's easier to use it locally? Thanks! [...]
On Fri, Sep 27, 2024 at 02:52:24PM -0700, Andrii Nakryiko wrote: > On Mon, Sep 16, 2024 at 2:19 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > For kfuncs marked with KF_FASTCALL flag generate the following pair of > > decl tags: > > > > $ bpftool btf dump file vmlinux > > ... > > [A] FUNC 'bpf_rdonly_cast' type_id=... > > ... > > [B] DECL_TAG 'bpf_kfunc' type_id=A component_idx=-1 > > [C] DECL_TAG 'bpf_fastcall' type_id=A component_idx=-1 > > > > So that bpftool could find 'bpf_fastcall' decl tag and generate > > appropriate C declarations for such kfuncs, e.g.: > > > > #ifndef __VMLINUX_H__ > > #define __VMLINUX_H__ > > ... > > #define __bpf_fastcall __attribute__((bpf_fastcall)) > > ... > > __bpf_fastcall extern void *bpf_rdonly_cast(...) ...; > > > > For additional information about 'bpf_fastcall' attribute, > > see the following commit in the LLVM source tree: > > > > 64e464349bfc ("[BPF] introduce __attribute__((bpf_fastcall))") > > > > And the following Linux kernel commit: > > > > 52839f31cece ("Merge branch 'no_caller_saved_registers-attribute-for-helper-calls'") > > > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> > > --- > > btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++-------------- > > 1 file changed, 43 insertions(+), 16 deletions(-) > > > > LGTM, > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > Arnaldo, can you please take a look and if everything seems sane apply > it to pahole master, so it's easier to use it locally? Thanks! Minor clash with Alan's Fixes: 5205d02d8e84a775 ("btf_encoder: record BTF-centric function state instead of DWARF-centric") Fixed up, testing. - Arnaldo
diff --git a/btf_encoder.c b/btf_encoder.c index 8a2d92e..ae059e0 100644 --- a/btf_encoder.c +++ b/btf_encoder.c @@ -39,15 +39,19 @@ #define BTF_ID_SET8_PFX "__BTF_ID__set8__" #define BTF_SET8_KFUNCS (1 << 0) #define BTF_KFUNC_TYPE_TAG "bpf_kfunc" +#define BTF_FASTCALL_TAG "bpf_fastcall" +#define KF_FASTCALL (1 << 12) + +struct btf_id_and_flag { + uint32_t id; + uint32_t flags; +}; /* Adapted from include/linux/btf_ids.h */ struct btf_id_set8 { uint32_t cnt; uint32_t flags; - struct { - uint32_t id; - uint32_t flags; - } pairs[]; + struct btf_id_and_flag pairs[]; }; /* state used to do later encoding of saved functions */ @@ -1517,21 +1521,34 @@ out: return err; } -static int btf_encoder__tag_kfunc(struct btf_encoder *encoder, struct gobuffer *funcs, const char *kfunc) +static int add_kfunc_decl_tag(struct btf *btf, const char *tag, __u32 id, const char *kfunc) +{ + int err; + + err = btf__add_decl_tag(btf, tag, id, -1); + if (err < 0) { + fprintf(stderr, "%s: failed to insert kfunc decl tag for '%s': %d\n", + __func__, kfunc, err); + return err; + } + return 0; +} + +static int btf_encoder__tag_kfunc(struct btf_encoder *encoder, struct gobuffer *funcs, const char *kfunc, __u32 flags) { struct btf_func key = { .name = kfunc }; struct btf *btf = encoder->btf; struct btf_func *target; const void *base; unsigned int cnt; - int err = -1; + int err; base = gobuffer__entries(funcs); cnt = gobuffer__nr_entries(funcs); target = bsearch(&key, base, cnt, sizeof(key), btf_func_cmp); if (!target) { fprintf(stderr, "%s: failed to find kfunc '%s' in BTF\n", __func__, kfunc); - goto out; + return -1; } /* Note we are unconditionally adding the btf_decl_tag even @@ -1539,16 +1556,16 @@ static int btf_encoder__tag_kfunc(struct btf_encoder *encoder, struct gobuffer * * We are ok to do this b/c we will later btf__dedup() to remove * any duplicates. */ - err = btf__add_decl_tag(btf, BTF_KFUNC_TYPE_TAG, target->type_id, -1); - if (err < 0) { - fprintf(stderr, "%s: failed to insert kfunc decl tag for '%s': %d\n", - __func__, kfunc, err); - goto out; + err = add_kfunc_decl_tag(btf, BTF_KFUNC_TYPE_TAG, target->type_id, kfunc); + if (err < 0) + return err; + if (flags & KF_FASTCALL) { + err = add_kfunc_decl_tag(btf, BTF_FASTCALL_TAG, target->type_id, kfunc); + if (err < 0) + return err; } - err = 0; -out: - return err; + return 0; } static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder) @@ -1675,8 +1692,10 @@ static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder) /* Now inject BTF with kfunc decl tag for detected kfuncs */ for (i = 0; i < nr_syms; i++) { const struct btf_kfunc_set_range *ranges; + const struct btf_id_and_flag *pair; unsigned int ranges_cnt; char *func, *name; + ptrdiff_t off; GElf_Sym sym; bool found; int err; @@ -1704,6 +1723,14 @@ static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder) if (ranges[j].start <= addr && addr < ranges[j].end) { found = true; + off = addr - idlist_addr; + if (off < 0 || off + sizeof(*pair) > idlist->d_size) { + fprintf(stderr, "%s: kfunc '%s' offset outside section '%s'\n", + __func__, func, BTF_IDS_SECTION); + free(func); + goto out; + } + pair = idlist->d_buf + off; break; } } @@ -1712,7 +1739,7 @@ static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder) continue; } - err = btf_encoder__tag_kfunc(encoder, &btf_funcs, func); + err = btf_encoder__tag_kfunc(encoder, &btf_funcs, func, pair->flags); if (err) { fprintf(stderr, "%s: failed to tag kfunc '%s'\n", __func__, func); free(func);
For kfuncs marked with KF_FASTCALL flag generate the following pair of decl tags: $ bpftool btf dump file vmlinux ... [A] FUNC 'bpf_rdonly_cast' type_id=... ... [B] DECL_TAG 'bpf_kfunc' type_id=A component_idx=-1 [C] DECL_TAG 'bpf_fastcall' type_id=A component_idx=-1 So that bpftool could find 'bpf_fastcall' decl tag and generate appropriate C declarations for such kfuncs, e.g.: #ifndef __VMLINUX_H__ #define __VMLINUX_H__ ... #define __bpf_fastcall __attribute__((bpf_fastcall)) ... __bpf_fastcall extern void *bpf_rdonly_cast(...) ...; For additional information about 'bpf_fastcall' attribute, see the following commit in the LLVM source tree: 64e464349bfc ("[BPF] introduce __attribute__((bpf_fastcall))") And the following Linux kernel commit: 52839f31cece ("Merge branch 'no_caller_saved_registers-attribute-for-helper-calls'") Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> --- btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 43 insertions(+), 16 deletions(-)