[bpf-next,v3,2/3] bpf: btf: Add BTF_KFUNCS_START/END macro pair

Commit Message

Daniel Xu Jan. 6, 2024, 6:24 p.m. UTC

This macro pair is functionally equivalent to BTF_SET8_START/END, except
with BTF_SET8_KFUNCS flag set in the btf_id_set8 flags field. The next
commit will codemod all kfunc set8s to this new variant such that all
kfuncs are tagged as such in .BTF_ids section.

Signed-off-by: Daniel Xu <[e-mail]>
---
 include/linux/btf_ids.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Lorenz Bauer Jan. 8, 2024, 9:14 a.m. UTC | #1

On Sat, Jan 6, 2024 at 7:25 PM Daniel Xu <[e-mail]> wrote:
>
> This macro pair is functionally equivalent to BTF_SET8_START/END, except
> with BTF_SET8_KFUNCS flag set in the btf_id_set8 flags field. The next
> commit will codemod all kfunc set8s to this new variant such that all
> kfuncs are tagged as such in .BTF_ids section.
>
> Signed-off-by: Daniel Xu <[e-mail]>
> ---
>  include/linux/btf_ids.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> index dca09b7f21dc..0fe4f1cd1918 100644
> --- a/include/linux/btf_ids.h
> +++ b/include/linux/btf_ids.h
> @@ -8,6 +8,9 @@ struct btf_id_set {
>         u32 ids[];
>  };
>
> +/* This flag implies BTF_SET8 holds kfunc(s) */
> +#define BTF_SET8_KFUNCS                (1 << 0)

Nit: could this be an enum so that the flag is discoverable via BTF?
Also, isn't this UAPI if pahole interprets this flag?

Daniel Xu Jan. 8, 2024, 5:59 p.m. UTC | #2

On Mon, Jan 08, 2024 at 10:14:13AM +0100, Lorenz Bauer wrote:
> On Sat, Jan 6, 2024 at 7:25 PM Daniel Xu <[e-mail]> wrote:
> >
> > This macro pair is functionally equivalent to BTF_SET8_START/END, except
> > with BTF_SET8_KFUNCS flag set in the btf_id_set8 flags field. The next
> > commit will codemod all kfunc set8s to this new variant such that all
> > kfuncs are tagged as such in .BTF_ids section.
> >
> > Signed-off-by: Daniel Xu <[e-mail]>
> > ---
> >  include/linux/btf_ids.h | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> > index dca09b7f21dc..0fe4f1cd1918 100644
> > --- a/include/linux/btf_ids.h
> > +++ b/include/linux/btf_ids.h
> > @@ -8,6 +8,9 @@ struct btf_id_set {
> >         u32 ids[];
> >  };
> >
> > +/* This flag implies BTF_SET8 holds kfunc(s) */
> > +#define BTF_SET8_KFUNCS                (1 << 0)
> 
> Nit: could this be an enum so that the flag is discoverable via BTF?

Sure, makes sense.

> Also, isn't this UAPI if pahole interprets this flag?

Not sure. I guess it'd fall under same category as any of the structs
the kernel lays out in .BTF_ids, like `struct btf_id_set8`. IMO it's
not, as that's kinda confusing to call anything in ELF uapi. Eg I don't
think people would consider layout of `.data..percpu` section uapi.

Thanks,
Daniel

Daniel Xu Jan. 11, 2024, 1:27 a.m. UTC | #3

On Mon, Jan 08, 2024 at 10:59:53AM -0700, Daniel Xu wrote:
> On Mon, Jan 08, 2024 at 10:14:13AM +0100, Lorenz Bauer wrote:
> > On Sat, Jan 6, 2024 at 7:25 PM Daniel Xu <[e-mail]> wrote:
> > >
> > > This macro pair is functionally equivalent to BTF_SET8_START/END, except
> > > with BTF_SET8_KFUNCS flag set in the btf_id_set8 flags field. The next
> > > commit will codemod all kfunc set8s to this new variant such that all
> > > kfuncs are tagged as such in .BTF_ids section.
> > >
> > > Signed-off-by: Daniel Xu <[e-mail]>
> > > ---
> > >  include/linux/btf_ids.h | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> > > index dca09b7f21dc..0fe4f1cd1918 100644
> > > --- a/include/linux/btf_ids.h
> > > +++ b/include/linux/btf_ids.h
> > > @@ -8,6 +8,9 @@ struct btf_id_set {
> > >         u32 ids[];
> > >  };
> > >
> > > +/* This flag implies BTF_SET8 holds kfunc(s) */
> > > +#define BTF_SET8_KFUNCS                (1 << 0)
> > 
> > Nit: could this be an enum so that the flag is discoverable via BTF?
> 
> Sure, makes sense.

I took a look - don't think we can make it an enum. See
include/linux/btf.h:

      /* These need to be macros, as the expressions are used in assembler input */
      #define KF_ACQUIRE      (1 << 0) /* kfunc is an acquire function */
      #define KF_RELEASE      (1 << 1) /* kfunc is a release function */
      [..]

Could do some redefines but maybe not worth it. The new flag is a pretty
deep impl detail anyways.

Thanks,
Daniel

Patch

diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
index dca09b7f21dc..0fe4f1cd1918 100644
--- a/include/linux/btf_ids.h
+++ b/include/linux/btf_ids.h
@@ -8,6 +8,9 @@  struct btf_id_set {
 	u32 ids[];
 };
 
+/* This flag implies BTF_SET8 holds kfunc(s) */
+#define BTF_SET8_KFUNCS		(1 << 0)
+
 struct btf_id_set8 {
 	u32 cnt;
 	u32 flags;
@@ -204,6 +207,12 @@  asm(							\
 ".popsection;                                 \n");	\
 extern struct btf_id_set8 name;
 
+#define BTF_KFUNCS_START(name)				\
+__BTF_SET8_START(name, local, BTF_SET8_KFUNCS)
+
+#define BTF_KFUNCS_END(name)				\
+BTF_SET8_END(name)
+
 #else
 
 #define BTF_ID_LIST(name) static u32 __maybe_unused name[64];
@@ -218,6 +227,8 @@  extern struct btf_id_set8 name;
 #define BTF_SET_END(name)
 #define BTF_SET8_START(name) static struct btf_id_set8 __maybe_unused name = { 0 };
 #define BTF_SET8_END(name)
+#define BTF_KFUNCS_START(name) static struct btf_id_set8 __maybe_unused name = { 0 };
+#define BTF_KFUNCS_END(name)
 
 #endif /* CONFIG_DEBUG_INFO_BTF */