diff mbox series

[RFC,bpf-next,08/13] kbuild, bpf: add module-specific pahole/resolve_btfids flags for base reference BTF

Message ID 20240322102455.98558-10-alan.maguire@oracle.com (mailing list archive)
State RFC
Delegated to: BPF
Headers show
Series bpf: support resilient split BTF | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/apply fail Patch does not apply to bpf-next-0
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat

Commit Message

Alan Maguire March 22, 2024, 10:24 a.m. UTC 
Support creation of module BTF along with base reference BTF;
the latter is stored in a .BTF.base_ref ELF section and supplements
split BTF references to base BTF with information about base types,
allowing for later reconciliation of split BTF with a (possibly
changed) base.  resolve_btfids uses the "-r" option to specify
that the BTF.ids section should be populated with split BTF
relative to the added .BTF.base_ref section rather than relative
to the vmlinux base.

Modules using base reference BTF can be built via

BTF_BASE_REF=1 make -C. -M=path2/module

The default is still to use split BTF relative to vmlinux.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 scripts/Makefile.btf      | 7 +++++++
 scripts/Makefile.modfinal | 4 ++--
 2 files changed, 9 insertions(+), 2 deletions(-)

Comments

Alexei Starovoitov March 23, 2024, 2:50 a.m. UTC | #1 
On Fri, Mar 22, 2024 at 3:26 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> Support creation of module BTF along with base reference BTF;
> the latter is stored in a .BTF.base_ref ELF section and supplements
> split BTF references to base BTF with information about base types,
> allowing for later reconciliation of split BTF with a (possibly
> changed) base.  resolve_btfids uses the "-r" option to specify
> that the BTF.ids section should be populated with split BTF
> relative to the added .BTF.base_ref section rather than relative
> to the vmlinux base.
>
> Modules using base reference BTF can be built via
>
> BTF_BASE_REF=1 make -C. -M=path2/module
>
> The default is still to use split BTF relative to vmlinux.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  scripts/Makefile.btf      | 7 +++++++
>  scripts/Makefile.modfinal | 4 ++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
> index 9694ca3c5252..c8212b2ab7ca 100644
> --- a/scripts/Makefile.btf
> +++ b/scripts/Makefile.btf
> @@ -19,4 +19,11 @@ pahole-flags-$(call test-ge, $(pahole-ver), 126)     = -j --btf_features=encode_forc
>
>  pahole-flags-$(CONFIG_PAHOLE_HAS_LANG_EXCLUDE)         += --lang_exclude=rust
>
> +ifeq ($(BTF_BASE_REF),1)
> +module-pahole-flags-$(call test-ge, $(pahole-ver), 126)        += --btf_features=base_ref
> +module-resolve-btfids-flags-$(call test-ge, $(pahole-ver), 126) = -r
> +endif

The patch set looks great to me.
I wonder whether we should drop this extra BTF_BASE_REF flag
and do it unconditionally.
Currently btf_parse_module() doesn't have any real checks
whether module's btf matches base_btf.
All the btf_check_*() might succeed by luck even if vmlinux btf
is different.
Since .BTF.base_ref is small we can always emit it and
during module load the btf_reconcile() step will be that safety check.
Less build options, less moving pieces and doing it all the time
will make the whole process robust.
Conditional BTF_BASE_REF=1 will likely mean that there will be
bugs that only few folks will hit.
Alan Maguire March 25, 2024, 9:51 a.m. UTC | #2 
On 23/03/2024 02:50, Alexei Starovoitov wrote:
> On Fri, Mar 22, 2024 at 3:26 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>
>> Support creation of module BTF along with base reference BTF;
>> the latter is stored in a .BTF.base_ref ELF section and supplements
>> split BTF references to base BTF with information about base types,
>> allowing for later reconciliation of split BTF with a (possibly
>> changed) base.  resolve_btfids uses the "-r" option to specify
>> that the BTF.ids section should be populated with split BTF
>> relative to the added .BTF.base_ref section rather than relative
>> to the vmlinux base.
>>
>> Modules using base reference BTF can be built via
>>
>> BTF_BASE_REF=1 make -C. -M=path2/module
>>
>> The default is still to use split BTF relative to vmlinux.
>>
>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>> ---
>>  scripts/Makefile.btf      | 7 +++++++
>>  scripts/Makefile.modfinal | 4 ++--
>>  2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
>> index 9694ca3c5252..c8212b2ab7ca 100644
>> --- a/scripts/Makefile.btf
>> +++ b/scripts/Makefile.btf
>> @@ -19,4 +19,11 @@ pahole-flags-$(call test-ge, $(pahole-ver), 126)     = -j --btf_features=encode_forc
>>
>>  pahole-flags-$(CONFIG_PAHOLE_HAS_LANG_EXCLUDE)         += --lang_exclude=rust
>>
>> +ifeq ($(BTF_BASE_REF),1)
>> +module-pahole-flags-$(call test-ge, $(pahole-ver), 126)        += --btf_features=base_ref
>> +module-resolve-btfids-flags-$(call test-ge, $(pahole-ver), 126) = -r
>> +endif
> 
> The patch set looks great to me.
> I wonder whether we should drop this extra BTF_BASE_REF flag
> and do it unconditionally.
> Currently btf_parse_module() doesn't have any real checks
> whether module's btf matches base_btf.
> All the btf_check_*() might succeed by luck even if vmlinux btf
> is different.
> Since .BTF.base_ref is small we can always emit it and
> during module load the btf_reconcile() step will be that safety check.
> Less build options, less moving pieces and doing it all the time
> will make the whole process robust.
> Conditional BTF_BASE_REF=1 will likely mean that there will be
> bugs that only few folks will hit.

We could certainly do it unconditionally for out-of-tree module builds
(the KBUILD_EXTMOD case); so anytime a user does "make -C.
M=path/2/module", they would get base reference BTF. With this series,
that would just mean applying this change:

diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
index c8212b2ab7ca..0322f8450a89 100644
--- a/scripts/Makefile.btf
+++ b/scripts/Makefile.btf
@@ -19,7 +19,7 @@ pahole-flags-$(call test-ge, $(pahole-ver), 126)
= -j --btf_features=encode_forc

 pahole-flags-$(CONFIG_PAHOLE_HAS_LANG_EXCLUDE)         +=
--lang_exclude=rust

-ifeq ($(BTF_BASE_REF),1)
+ifneq ($(KBUILD_EXTMOD),)
 module-pahole-flags-$(call test-ge, $(pahole-ver), 126)        +=
--btf_features=base_ref
 module-resolve-btfids-flags-$(call test-ge, $(pahole-ver), 126) = -r
 endif

So with this approach, in-tree modules don't add base reference BTF
since they don't need it, while out-of-tree module builds always do.
We could arrange for bpf_testmod to be built both ways perhaps to ensure
both approaches get tested in selftests along with kfunc addition etc.

For adding base reference BTF in all cases, I ran the numbers on adding
base reference BTF across all kernel modules and for x86_64 with 2667
modules. Base reference BTF adds around 4MB in total. Not huge, but for
in-tree builds, the only cases we've seen that triggered BTF mismatches
have been broken cases where vmlinux got built twice during kernel
build. So my suggestion would be to limit base reference BTF addition to
the KBUILD_EXTMOD case, and make it unconditional for that case only.

Thanks!

Alan
Alexei Starovoitov March 25, 2024, 4:41 p.m. UTC | #3 
On Mon, Mar 25, 2024 at 2:51 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On 23/03/2024 02:50, Alexei Starovoitov wrote:
> > On Fri, Mar 22, 2024 at 3:26 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >>
> >> Support creation of module BTF along with base reference BTF;
> >> the latter is stored in a .BTF.base_ref ELF section and supplements
> >> split BTF references to base BTF with information about base types,
> >> allowing for later reconciliation of split BTF with a (possibly
> >> changed) base.  resolve_btfids uses the "-r" option to specify
> >> that the BTF.ids section should be populated with split BTF
> >> relative to the added .BTF.base_ref section rather than relative
> >> to the vmlinux base.
> >>
> >> Modules using base reference BTF can be built via
> >>
> >> BTF_BASE_REF=1 make -C. -M=path2/module
> >>
> >> The default is still to use split BTF relative to vmlinux.
> >>
> >> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> >> ---
> >>  scripts/Makefile.btf      | 7 +++++++
> >>  scripts/Makefile.modfinal | 4 ++--
> >>  2 files changed, 9 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
> >> index 9694ca3c5252..c8212b2ab7ca 100644
> >> --- a/scripts/Makefile.btf
> >> +++ b/scripts/Makefile.btf
> >> @@ -19,4 +19,11 @@ pahole-flags-$(call test-ge, $(pahole-ver), 126)     = -j --btf_features=encode_forc
> >>
> >>  pahole-flags-$(CONFIG_PAHOLE_HAS_LANG_EXCLUDE)         += --lang_exclude=rust
> >>
> >> +ifeq ($(BTF_BASE_REF),1)
> >> +module-pahole-flags-$(call test-ge, $(pahole-ver), 126)        += --btf_features=base_ref
> >> +module-resolve-btfids-flags-$(call test-ge, $(pahole-ver), 126) = -r
> >> +endif
> >
> > The patch set looks great to me.
> > I wonder whether we should drop this extra BTF_BASE_REF flag
> > and do it unconditionally.
> > Currently btf_parse_module() doesn't have any real checks
> > whether module's btf matches base_btf.
> > All the btf_check_*() might succeed by luck even if vmlinux btf
> > is different.
> > Since .BTF.base_ref is small we can always emit it and
> > during module load the btf_reconcile() step will be that safety check.
> > Less build options, less moving pieces and doing it all the time
> > will make the whole process robust.
> > Conditional BTF_BASE_REF=1 will likely mean that there will be
> > bugs that only few folks will hit.
>
> We could certainly do it unconditionally for out-of-tree module builds
> (the KBUILD_EXTMOD case); so anytime a user does "make -C.
> M=path/2/module", they would get base reference BTF. With this series,
> that would just mean applying this change:
>
> diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
> index c8212b2ab7ca..0322f8450a89 100644
> --- a/scripts/Makefile.btf
> +++ b/scripts/Makefile.btf
> @@ -19,7 +19,7 @@ pahole-flags-$(call test-ge, $(pahole-ver), 126)
> = -j --btf_features=encode_forc
>
>  pahole-flags-$(CONFIG_PAHOLE_HAS_LANG_EXCLUDE)         +=
> --lang_exclude=rust
>
> -ifeq ($(BTF_BASE_REF),1)
> +ifneq ($(KBUILD_EXTMOD),)
>  module-pahole-flags-$(call test-ge, $(pahole-ver), 126)        +=
> --btf_features=base_ref
>  module-resolve-btfids-flags-$(call test-ge, $(pahole-ver), 126) = -r
>  endif
>
> So with this approach, in-tree modules don't add base reference BTF
> since they don't need it, while out-of-tree module builds always do.
> We could arrange for bpf_testmod to be built both ways perhaps to ensure
> both approaches get tested in selftests along with kfunc addition etc.
>
> For adding base reference BTF in all cases, I ran the numbers on adding
> base reference BTF across all kernel modules and for x86_64 with 2667
> modules. Base reference BTF adds around 4MB in total. Not huge, but for
> in-tree builds, the only cases we've seen that triggered BTF mismatches
> have been broken cases where vmlinux got built twice during kernel
> build. So my suggestion would be to limit base reference BTF addition to
> the KBUILD_EXTMOD case, and make it unconditional for that case only.

So 4M across 2667 modules is ~1.5Kbyte per module ?
And how many modules out of 2667 will be loaded?
Even for 100 loaded modules it's just 150Kbyte of extra memory.

But I'm fine with sticking to KBUILD_EXTMOD only.
Let's make bpf_testmod with it though unconditionally.
No need to build it twice.
diff mbox series

Patch

diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
index 9694ca3c5252..c8212b2ab7ca 100644
--- a/scripts/Makefile.btf
+++ b/scripts/Makefile.btf
@@ -19,4 +19,11 @@  pahole-flags-$(call test-ge, $(pahole-ver), 126)	= -j --btf_features=encode_forc
 
 pahole-flags-$(CONFIG_PAHOLE_HAS_LANG_EXCLUDE)		+= --lang_exclude=rust
 
+ifeq ($(BTF_BASE_REF),1)
+module-pahole-flags-$(call test-ge, $(pahole-ver), 126)	+= --btf_features=base_ref
+module-resolve-btfids-flags-$(call test-ge, $(pahole-ver), 126) = -r
+endif
+
 export PAHOLE_FLAGS := $(pahole-flags-y)
+export MODULE_PAHOLE_FLAGS := $(module-pahole-flags-y)
+export MODULE_RESOLVE_BTFIDS_FLAGS := $(module-resolve-btfids-flags-y)
diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal
index 8568d256d6fb..22f5bb0a60a6 100644
--- a/scripts/Makefile.modfinal
+++ b/scripts/Makefile.modfinal
@@ -39,8 +39,8 @@  quiet_cmd_btf_ko = BTF [M] $@
 	if [ ! -f vmlinux ]; then					\
 		printf "Skipping BTF generation for %s due to unavailability of vmlinux\n" $@ 1>&2; \
 	else								\
-		LLVM_OBJCOPY="$(OBJCOPY)" $(PAHOLE) -J $(PAHOLE_FLAGS) --btf_base vmlinux $@; \
-		$(RESOLVE_BTFIDS) -b vmlinux $@; 			\
+		LLVM_OBJCOPY="$(OBJCOPY)" $(PAHOLE) -J $(PAHOLE_FLAGS) $(MODULE_PAHOLE_FLAGS) --btf_base vmlinux $@; \
+		$(RESOLVE_BTFIDS) $(MODULE_RESOLVE_BTFIDS_FLAGS) -b vmlinux $@; 			\
 	fi;
 
 # Same as newer-prereqs, but allows to exclude specified extra dependencies