bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv6 bpf] bpf: Move iterator functions into special init section
@ 2020-11-10 15:40 Jiri Olsa
  2020-11-11 11:26 ` Daniel Borkmann
  0 siblings, 1 reply; 6+ messages in thread
From: Jiri Olsa @ 2020-11-10 15:40 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: Arnaldo Carvalho de Melo, Yonghong Song, netdev, bpf,
	Martin KaFai Lau, Song Liu, John Fastabend, KP Singh

With upcoming changes to pahole, that change the way how and
which kernel functions are stored in BTF data, we need a way
to recognize iterator functions.

Iterator functions need to be in BTF data, but have no real
body and are currently placed in .init.text section, so they
are freed after kernel init and are filtered out of BTF data
because of that.

The solution is to place these functions under new section:
  .init.bpf.preserve_type

And add 2 new symbols to mark that area:
  __init_bpf_preserve_type_begin
  __init_bpf_preserve_type_end

The code in pahole responsible for picking up the functions will
be able to recognize functions from this section and add them to
the BTF data and filter out all other .init.text functions.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Suggested-by: Yonghong Song <yhs@fb.com>
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
v6 changes:
  - simplify the change and add comment to bpf.h header based
    on David's suggestion and remove the init.h change
  - removing acks, because of the new changes

 include/asm-generic/vmlinux.lds.h | 16 +++++++++++++++-
 include/linux/bpf.h               | 12 +++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index b2b3d81b1535..f91029b3443b 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -685,8 +685,21 @@
 	.BTF_ids : AT(ADDR(.BTF_ids) - LOAD_OFFSET) {			\
 		*(.BTF_ids)						\
 	}
+
+/*
+ * .init.bpf.preserve_type
+ *
+ * This section store special BPF function and marks them
+ * with begin/end symbols pair for the sake of pahole tool.
+ */
+#define INIT_BPF_PRESERVE_TYPE						\
+	__init_bpf_preserve_type_begin = .;                             \
+	*(.init.bpf.preserve_type)                                      \
+	__init_bpf_preserve_type_end = .;				\
+	MEM_DISCARD(init.bpf.preserve_type)
 #else
 #define BTF
+#define INIT_BPF_PRESERVE_TYPE
 #endif
 
 /*
@@ -741,7 +754,8 @@
 #define INIT_TEXT							\
 	*(.init.text .init.text.*)					\
 	*(.text.startup)						\
-	MEM_DISCARD(init.text*)
+	MEM_DISCARD(init.text*)						\
+	INIT_BPF_PRESERVE_TYPE
 
 #define EXIT_DATA							\
 	*(.exit.data .exit.data.*)					\
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 2b16bf48aab6..1739a92516ed 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1276,10 +1276,20 @@ struct bpf_link *bpf_link_get_from_fd(u32 ufd);
 int bpf_obj_pin_user(u32 ufd, const char __user *pathname);
 int bpf_obj_get_user(const char __user *pathname, int flags);
 
+/* In case we generate BTF data, we need to group all iterator
+ * functions into special init section, so pahole can track them.
+ * Otherwise pure __init section is enough.
+ */
+#ifdef CONFIG_DEBUG_INFO_BTF
+#define __init_bpf_preserve_type __section(".init.bpf.preserve_type")
+#else
+#define __init_bpf_preserve_type __init
+#endif
+
 #define BPF_ITER_FUNC_PREFIX "bpf_iter_"
 #define DEFINE_BPF_ITER_FUNC(target, args...)			\
 	extern int bpf_iter_ ## target(args);			\
-	int __init bpf_iter_ ## target(args) { return 0; }
+	int __init_bpf_preserve_type bpf_iter_ ## target(args) { return 0; }
 
 struct bpf_iter_aux_info {
 	struct bpf_map *map;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCHv6 bpf] bpf: Move iterator functions into special init section
  2020-11-10 15:40 [PATCHv6 bpf] bpf: Move iterator functions into special init section Jiri Olsa
@ 2020-11-11 11:26 ` Daniel Borkmann
  2020-11-11 11:51   ` Jiri Olsa
  2020-11-11 12:37   ` Arnaldo Carvalho de Melo
  0 siblings, 2 replies; 6+ messages in thread
From: Daniel Borkmann @ 2020-11-11 11:26 UTC (permalink / raw)
  To: Jiri Olsa, Alexei Starovoitov, Andrii Nakryiko
  Cc: Arnaldo Carvalho de Melo, Yonghong Song, netdev, bpf,
	Martin KaFai Lau, Song Liu, John Fastabend, KP Singh

On 11/10/20 4:40 PM, Jiri Olsa wrote:
> With upcoming changes to pahole, that change the way how and
> which kernel functions are stored in BTF data, we need a way
> to recognize iterator functions.
> 
> Iterator functions need to be in BTF data, but have no real
> body and are currently placed in .init.text section, so they
> are freed after kernel init and are filtered out of BTF data
> because of that.
> 
> The solution is to place these functions under new section:
>    .init.bpf.preserve_type
> 
> And add 2 new symbols to mark that area:
>    __init_bpf_preserve_type_begin
>    __init_bpf_preserve_type_end
> 
> The code in pahole responsible for picking up the functions will
> be able to recognize functions from this section and add them to
> the BTF data and filter out all other .init.text functions.
> 
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Suggested-by: Yonghong Song <yhs@fb.com>
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>

LGTM, applied, thanks! Also added a reference to the pahole commit
to the commit log so that this info doesn't get lost in the void
plus carried over prior Acks given nothing changed logically in the
patch.

P.s.: I've been wondering whether we also need to align the begin/end
symbols via ALIGN_FUNCTION() in case ld might realign to a different
boundary on later passes but this seems neither the case for .init.text
right now, likely since it doesn't matter for kallsyms data in our
particular case.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCHv6 bpf] bpf: Move iterator functions into special init section
  2020-11-11 11:26 ` Daniel Borkmann
@ 2020-11-11 11:51   ` Jiri Olsa
  2020-11-11 12:37   ` Arnaldo Carvalho de Melo
  1 sibling, 0 replies; 6+ messages in thread
From: Jiri Olsa @ 2020-11-11 11:51 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Jiri Olsa, Alexei Starovoitov, Andrii Nakryiko,
	Arnaldo Carvalho de Melo, Yonghong Song, netdev, bpf,
	Martin KaFai Lau, Song Liu, John Fastabend, KP Singh

On Wed, Nov 11, 2020 at 12:26:29PM +0100, Daniel Borkmann wrote:
> On 11/10/20 4:40 PM, Jiri Olsa wrote:
> > With upcoming changes to pahole, that change the way how and
> > which kernel functions are stored in BTF data, we need a way
> > to recognize iterator functions.
> > 
> > Iterator functions need to be in BTF data, but have no real
> > body and are currently placed in .init.text section, so they
> > are freed after kernel init and are filtered out of BTF data
> > because of that.
> > 
> > The solution is to place these functions under new section:
> >    .init.bpf.preserve_type
> > 
> > And add 2 new symbols to mark that area:
> >    __init_bpf_preserve_type_begin
> >    __init_bpf_preserve_type_end
> > 
> > The code in pahole responsible for picking up the functions will
> > be able to recognize functions from this section and add them to
> > the BTF data and filter out all other .init.text functions.
> > 
> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > Suggested-by: Yonghong Song <yhs@fb.com>
> > Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> 
> LGTM, applied, thanks! Also added a reference to the pahole commit
> to the commit log so that this info doesn't get lost in the void
> plus carried over prior Acks given nothing changed logically in the
> patch.
> 
> P.s.: I've been wondering whether we also need to align the begin/end
> symbols via ALIGN_FUNCTION() in case ld might realign to a different
> boundary on later passes but this seems neither the case for .init.text
> right now, likely since it doesn't matter for kallsyms data in our
> particular case.
> 

I'll check but I think it's not a problem as you said

thanks,
jirka


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCHv6 bpf] bpf: Move iterator functions into special init section
  2020-11-11 11:26 ` Daniel Borkmann
  2020-11-11 11:51   ` Jiri Olsa
@ 2020-11-11 12:37   ` Arnaldo Carvalho de Melo
  2020-11-11 12:38     ` Arnaldo Carvalho de Melo
  1 sibling, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-11-11 12:37 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Jiri Olsa, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song,
	netdev, bpf, Martin KaFai Lau, Song Liu, John Fastabend,
	KP Singh

Em Wed, Nov 11, 2020 at 12:26:29PM +0100, Daniel Borkmann escreveu:
> On 11/10/20 4:40 PM, Jiri Olsa wrote:
> > With upcoming changes to pahole, that change the way how and
> > which kernel functions are stored in BTF data, we need a way
> > to recognize iterator functions.
> > 
> > Iterator functions need to be in BTF data, but have no real
> > body and are currently placed in .init.text section, so they
> > are freed after kernel init and are filtered out of BTF data
> > because of that.
> > 
> > The solution is to place these functions under new section:
> >    .init.bpf.preserve_type
> > 
> > And add 2 new symbols to mark that area:
> >    __init_bpf_preserve_type_begin
> >    __init_bpf_preserve_type_end
> > 
> > The code in pahole responsible for picking up the functions will
> > be able to recognize functions from this section and add them to
> > the BTF data and filter out all other .init.text functions.
> > 
> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > Suggested-by: Yonghong Song <yhs@fb.com>
> > Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> 
> LGTM, applied, thanks! Also added a reference to the pahole commit

Applied to what branch? I'm trying to test it now :-)

- Arnaldo

> to the commit log so that this info doesn't get lost in the void
> plus carried over prior Acks given nothing changed logically in the
> patch.
> 
> P.s.: I've been wondering whether we also need to align the begin/end
> symbols via ALIGN_FUNCTION() in case ld might realign to a different
> boundary on later passes but this seems neither the case for .init.text
> right now, likely since it doesn't matter for kallsyms data in our
> particular case.

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCHv6 bpf] bpf: Move iterator functions into special init section
  2020-11-11 12:37   ` Arnaldo Carvalho de Melo
@ 2020-11-11 12:38     ` Arnaldo Carvalho de Melo
  2020-11-11 20:09       ` Alexei Starovoitov
  0 siblings, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-11-11 12:38 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Jiri Olsa, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song,
	netdev, bpf, Martin KaFai Lau, Song Liu, John Fastabend,
	KP Singh

Em Wed, Nov 11, 2020 at 09:37:38AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Nov 11, 2020 at 12:26:29PM +0100, Daniel Borkmann escreveu:
> > On 11/10/20 4:40 PM, Jiri Olsa wrote:
> > > With upcoming changes to pahole, that change the way how and
> > > which kernel functions are stored in BTF data, we need a way
> > > to recognize iterator functions.
> > > 
> > > Iterator functions need to be in BTF data, but have no real
> > > body and are currently placed in .init.text section, so they
> > > are freed after kernel init and are filtered out of BTF data
> > > because of that.
> > > 
> > > The solution is to place these functions under new section:
> > >    .init.bpf.preserve_type
> > > 
> > > And add 2 new symbols to mark that area:
> > >    __init_bpf_preserve_type_begin
> > >    __init_bpf_preserve_type_end
> > > 
> > > The code in pahole responsible for picking up the functions will
> > > be able to recognize functions from this section and add them to
> > > the BTF data and filter out all other .init.text functions.
> > > 
> > > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > > Suggested-by: Yonghong Song <yhs@fb.com>
> > > Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> > 
> > LGTM, applied, thanks! Also added a reference to the pahole commit
> 
> Applied to what branch? I'm trying to test it now :-)

Nevermind, bpf/master, I was looking at bpf-next/master.

- Arnaldo

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCHv6 bpf] bpf: Move iterator functions into special init section
  2020-11-11 12:38     ` Arnaldo Carvalho de Melo
@ 2020-11-11 20:09       ` Alexei Starovoitov
  0 siblings, 0 replies; 6+ messages in thread
From: Alexei Starovoitov @ 2020-11-11 20:09 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Daniel Borkmann, Jiri Olsa, Alexei Starovoitov, Andrii Nakryiko,
	Yonghong Song, Network Development, bpf, Martin KaFai Lau,
	Song Liu, John Fastabend, KP Singh

On Wed, Nov 11, 2020 at 4:38 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Wed, Nov 11, 2020 at 09:37:38AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Wed, Nov 11, 2020 at 12:26:29PM +0100, Daniel Borkmann escreveu:
> > > On 11/10/20 4:40 PM, Jiri Olsa wrote:
> > > > With upcoming changes to pahole, that change the way how and
> > > > which kernel functions are stored in BTF data, we need a way
> > > > to recognize iterator functions.
> > > >
> > > > Iterator functions need to be in BTF data, but have no real
> > > > body and are currently placed in .init.text section, so they
> > > > are freed after kernel init and are filtered out of BTF data
> > > > because of that.
> > > >
> > > > The solution is to place these functions under new section:
> > > >    .init.bpf.preserve_type
> > > >
> > > > And add 2 new symbols to mark that area:
> > > >    __init_bpf_preserve_type_begin
> > > >    __init_bpf_preserve_type_end
> > > >
> > > > The code in pahole responsible for picking up the functions will
> > > > be able to recognize functions from this section and add them to
> > > > the BTF data and filter out all other .init.text functions.
> > > >
> > > > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > > > Suggested-by: Yonghong Song <yhs@fb.com>
> > > > Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> > >
> > > LGTM, applied, thanks! Also added a reference to the pahole commit
> >
> > Applied to what branch? I'm trying to test it now :-)
>
> Nevermind, bpf/master, I was looking at bpf-next/master.

I've dropped this patch from bpf tree.
I think we need to agree on the whole approach first.
This filtering based on section name with special handling in pahole doesn't
feel like solid long term direction.
I think we have to brainstorm more on it.
I'm not saying we will not go back to a special section approach.
This revert is only buying us time to discuss what's the right path here.
Mainly I reverted to unbreak bpf tree CI which currently fails due to
two tests in test_progs
failing with the latest pahole and this patch.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-11-11 20:09 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-10 15:40 [PATCHv6 bpf] bpf: Move iterator functions into special init section Jiri Olsa
2020-11-11 11:26 ` Daniel Borkmann
2020-11-11 11:51   ` Jiri Olsa
2020-11-11 12:37   ` Arnaldo Carvalho de Melo
2020-11-11 12:38     ` Arnaldo Carvalho de Melo
2020-11-11 20:09       ` Alexei Starovoitov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).