diff mbox series

[RESEND,v4,1/3] kselftests: lib.mk: Add TEST_GEN_MODS_DIR variable

Message ID 20231220-send-lp-kselftests-v4-1-3458ec1b1a38@suse.com
State New
Headers
Series livepatch: Move modules to selftests and add a new test |

Commit Message

Marcos Paulo de Souza Dec. 20, 2023, 4:53 p.m. UTC 
  Add TEST_GEN_MODS_DIR variable for kselftests. It can point to
a directory containing kernel modules that will be used by
selftest scripts.

The modules are built as external modules for the running kernel.
As a result they are always binary compatible and the same tests
can be used for older or newer kernels.

The build requires "kernel-devel" package to be installed.
For example, in the upstream sources, the rpm devel package
is produced by "make rpm-pkg"

The modules can be built independently by

  make -C tools/testing/selftests/livepatch/

or they will be automatically built before running the tests via

  make -C tools/testing/selftests/livepatch/ run_tests

Note that they are _not_ built when running the standalone
tests by calling, for example, ./test-state.sh.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 Documentation/dev-tools/kselftest.rst |  4 ++++
 tools/testing/selftests/lib.mk        | 20 +++++++++++++++-----
 2 files changed, 19 insertions(+), 5 deletions(-)

Comments

Joe Lawrence Jan. 2, 2024, 10:31 p.m. UTC | #1 
On Wed, Dec 20, 2023 at 01:53:12PM -0300, Marcos Paulo de Souza wrote:
> Add TEST_GEN_MODS_DIR variable for kselftests. It can point to
> a directory containing kernel modules that will be used by
> selftest scripts.
> 
> The modules are built as external modules for the running kernel.
> As a result they are always binary compatible and the same tests
> can be used for older or newer kernels.
> 
> The build requires "kernel-devel" package to be installed.
> For example, in the upstream sources, the rpm devel package
> is produced by "make rpm-pkg"
> 
> The modules can be built independently by
> 
>   make -C tools/testing/selftests/livepatch/
> 
> or they will be automatically built before running the tests via
> 
>   make -C tools/testing/selftests/livepatch/ run_tests
> 
> Note that they are _not_ built when running the standalone
> tests by calling, for example, ./test-state.sh.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  Documentation/dev-tools/kselftest.rst |  4 ++++
>  tools/testing/selftests/lib.mk        | 20 +++++++++++++++-----
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst
> index ab376b316c36..7f3582a67318 100644
> --- a/Documentation/dev-tools/kselftest.rst
> +++ b/Documentation/dev-tools/kselftest.rst
> @@ -245,6 +245,10 @@ Contributing new tests (details)
>     TEST_PROGS, TEST_GEN_PROGS mean it is the executable tested by
>     default.
>  
> +   TEST_GEN_MODS_DIR should be used by tests that require modules to be built
> +   before the test starts. The variable will contain the name of the directory
> +   containing the modules.
> +
>     TEST_CUSTOM_PROGS should be used by tests that require custom build
>     rules and prevent common build rule use.
>  
> diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> index 118e0964bda9..6c7c5a0112cf 100644
> --- a/tools/testing/selftests/lib.mk
> +++ b/tools/testing/selftests/lib.mk
> @@ -70,12 +70,15 @@ KHDR_INCLUDES := -isystem $(KHDR_DIR)
>  # TEST_PROGS are for test shell scripts.
>  # TEST_CUSTOM_PROGS and TEST_PROGS will be run by common run_tests
>  # and install targets. Common clean doesn't touch them.
> +# TEST_GEN_MODS_DIR is used to specify a directory with modules to be built
> +# before the test executes. These modules are cleaned on the clean target as well.
>  TEST_GEN_PROGS := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_PROGS))
>  TEST_GEN_PROGS_EXTENDED := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_PROGS_EXTENDED))
>  TEST_GEN_FILES := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_FILES))
> +TEST_GEN_MODS_DIR := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_MODS_DIR))
>  
>  all: kernel_header_files $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) \
> -     $(TEST_GEN_FILES)
> +     $(TEST_GEN_FILES) $(if $(TEST_GEN_MODS_DIR),gen_mods_dir)
>  
>  kernel_header_files:
>  	@ls $(KHDR_DIR)/linux/*.h >/dev/null 2>/dev/null;                      \
> @@ -105,8 +108,8 @@ endef
>  
>  run_tests: all
>  ifdef building_out_of_srctree
> -	@if [ "X$(TEST_PROGS)$(TEST_PROGS_EXTENDED)$(TEST_FILES)" != "X" ]; then \
> -		rsync -aq --copy-unsafe-links $(TEST_PROGS) $(TEST_PROGS_EXTENDED) $(TEST_FILES) $(OUTPUT); \
> +	@if [ "X$(TEST_PROGS)$(TEST_PROGS_EXTENDED)$(TEST_FILES)$(TEST_GEN_MODS_DIR)" != "X" ]; then \
> +		rsync -aq --copy-unsafe-links $(TEST_PROGS) $(TEST_PROGS_EXTENDED) $(TEST_FILES) $(TEST_GEN_MODS_DIR) $(OUTPUT); \
>  	fi
>  	@if [ "X$(TEST_PROGS)" != "X" ]; then \
>  		$(call RUN_TESTS, $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) \
> @@ -118,6 +121,12 @@ else
>  	@$(call RUN_TESTS, $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) $(TEST_PROGS))
>  endif
>  
> +gen_mods_dir:
> +	$(Q)$(MAKE) -C $(TEST_GEN_MODS_DIR)
> +
> +clean_mods_dir:
> +	$(Q)$(MAKE) -C $(TEST_GEN_MODS_DIR) clean
> +
>  define INSTALL_SINGLE_RULE
>  	$(if $(INSTALL_LIST),@mkdir -p $(INSTALL_PATH))
>  	$(if $(INSTALL_LIST),rsync -a --copy-unsafe-links $(INSTALL_LIST) $(INSTALL_PATH)/)
> @@ -131,6 +140,7 @@ define INSTALL_RULE
>  	$(eval INSTALL_LIST = $(TEST_CUSTOM_PROGS)) $(INSTALL_SINGLE_RULE)
>  	$(eval INSTALL_LIST = $(TEST_GEN_PROGS_EXTENDED)) $(INSTALL_SINGLE_RULE)
>  	$(eval INSTALL_LIST = $(TEST_GEN_FILES)) $(INSTALL_SINGLE_RULE)
> +	$(eval INSTALL_LIST = $(TEST_GEN_MODS_DIR)) $(INSTALL_SINGLE_RULE)

Hi Marcos,

Sorry for the late reply on this, but I'm reviewing this version by
trying to retrofit it into our selftest packaging (pre-build the test
module .ko's and stash those into an rpm rather than building on the
test host).

Since $TEST_GEN_MODS_DIR is treated as a directory, I found that the
selftest install target copies a bunch of intermediate object and kbuild
files:

  $ mkdir /tmp/test-install
  $ make KDIR=$(pwd) INSTALL_PATH=/tmp/test-install TARGETS=livepatch \
       -C tools/testing/selftests/ install

  [ ... builds livepatch selftests ... ]

the rsync in question:

  rsync -a --copy-unsafe-links /home/jolawren/src/kernel/tools/testing/selftests/livepatch/test_modules /tmp/test-install/livepatch/
  ...

and then looking at the destination:

  $ tree -a /tmp/test-install/
  /tmp/test-install/
  ├── kselftest
  │   ├── module.sh
  │   ├── prefix.pl
  │   └── runner.sh
  ├── kselftest-list.txt
  ├── livepatch
  │   ├── config
  │   ├── functions.sh
  │   ├── settings
  │   ├── test-callbacks.sh
  │   ├── test-ftrace.sh
  │   ├── test_klp-call_getpid
  │   ├── test-livepatch.sh
  │   ├── test_modules
  │   │   ├── Makefile
  │   │   ├── modules.order
  │   │   ├── .modules.order.cmd
  │   │   ├── Module.symvers
  │   │   ├── .Module.symvers.cmd
  │   │   ├── test_klp_atomic_replace.c
  │   │   ├── test_klp_atomic_replace.ko
  │   │   ├── .test_klp_atomic_replace.ko.cmd
  │   │   ├── test_klp_atomic_replace.mod
  │   │   ├── test_klp_atomic_replace.mod.c
  │   │   ├── .test_klp_atomic_replace.mod.cmd
  │   │   ├── test_klp_atomic_replace.mod.o
  │   │   ├── .test_klp_atomic_replace.mod.o.cmd
  │   │   ├── test_klp_atomic_replace.o
  │   │   ├── .test_klp_atomic_replace.o.cmd
  ...

On the other hand, variables like $TEST_GEN_FILES specify individual
files, so only final binaries like test_klp-call_getpid (and not
test_klp-call_getpid.c) are copied to $INSTALL_PATH.

Since the selftest module builds appear to ignore
CONFIG_MODULE_COMPRESS_* the smallest tweak I can think of to avoid the
above scenario is:

  --- a/tools/testing/selftests/lib.mk
  +++ b/tools/testing/selftests/lib.mk
  @@ -106,7 +106,7 @@ define INSTALL_RULE
          $(eval INSTALL_LIST = $(TEST_CUSTOM_PROGS)) $(INSTALL_SINGLE_RULE)
          $(eval INSTALL_LIST = $(TEST_GEN_PROGS_EXTENDED)) $(INSTALL_SINGLE_RULE)
          $(eval INSTALL_LIST = $(TEST_GEN_FILES)) $(INSTALL_SINGLE_RULE)
  -       $(eval INSTALL_LIST = $(TEST_GEN_MODS_DIR)) $(INSTALL_SINGLE_RULE)
  +       $(eval INSTALL_LIST = $(shell sed 's/.o$$/.ko/' $(TEST_GEN_MODS_DIR)/modules.order)) $(INSTALL_SINGLE_RULE)
          $(eval INSTALL_LIST = $(wildcard config settings)) $(INSTALL_SINGLE_RULE)
   endef

However, that will copy .ko's directly into $INSTALL_PATH and out of the
$TEST_GEN_MODS_DIR subdirectory(s), so maybe not a great solution after
all.

Anyway, I thought I might mention this in case it runs against the
spirit of the selftest install target.  I only tripped over it while
digging into the bowels of our kernel specfile and discovered that it
invoked this target.

--
Joe
Shuah Khan Jan. 3, 2024, 10:09 p.m. UTC | #2 
On 1/2/24 15:31, Joe Lawrence wrote:
> On Wed, Dec 20, 2023 at 01:53:12PM -0300, Marcos Paulo de Souza wrote:
>> Add TEST_GEN_MODS_DIR variable for kselftests. It can point to
>> a directory containing kernel modules that will be used by
>> selftest scripts.
>>
>> The modules are built as external modules for the running kernel.
>> As a result they are always binary compatible and the same tests
>> can be used for older or newer kernels.
>>
>> The build requires "kernel-devel" package to be installed.
>> For example, in the upstream sources, the rpm devel package
>> is produced by "make rpm-pkg"
>>
>> The modules can be built independently by
>>
>>    make -C tools/testing/selftests/livepatch/
>>
>> or they will be automatically built before running the tests via
>>
>>    make -C tools/testing/selftests/livepatch/ run_tests
>>
>> Note that they are _not_ built when running the standalone
>> tests by calling, for example, ./test-state.sh.
>>
>> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
>> ---
>>   Documentation/dev-tools/kselftest.rst |  4 ++++
>>   tools/testing/selftests/lib.mk        | 20 +++++++++++++++-----
>>   2 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst
>> index ab376b316c36..7f3582a67318 100644
>> --- a/Documentation/dev-tools/kselftest.rst
>> +++ b/Documentation/dev-tools/kselftest.rst
>> @@ -245,6 +245,10 @@ Contributing new tests (details)
>>      TEST_PROGS, TEST_GEN_PROGS mean it is the executable tested by
>>      default.
>>   
>> +   TEST_GEN_MODS_DIR should be used by tests that require modules to be built
>> +   before the test starts. The variable will contain the name of the directory
>> +   containing the modules.
>> +
>>      TEST_CUSTOM_PROGS should be used by tests that require custom build
>>      rules and prevent common build rule use.
>>   
>> diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
>> index 118e0964bda9..6c7c5a0112cf 100644
>> --- a/tools/testing/selftests/lib.mk
>> +++ b/tools/testing/selftests/lib.mk
>> @@ -70,12 +70,15 @@ KHDR_INCLUDES := -isystem $(KHDR_DIR)
>>   # TEST_PROGS are for test shell scripts.
>>   # TEST_CUSTOM_PROGS and TEST_PROGS will be run by common run_tests
>>   # and install targets. Common clean doesn't touch them.
>> +# TEST_GEN_MODS_DIR is used to specify a directory with modules to be built
>> +# before the test executes. These modules are cleaned on the clean target as well.
>>   TEST_GEN_PROGS := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_PROGS))
>>   TEST_GEN_PROGS_EXTENDED := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_PROGS_EXTENDED))
>>   TEST_GEN_FILES := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_FILES))
>> +TEST_GEN_MODS_DIR := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_MODS_DIR))
>>   
>>   all: kernel_header_files $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) \
>> -     $(TEST_GEN_FILES)
>> +     $(TEST_GEN_FILES) $(if $(TEST_GEN_MODS_DIR),gen_mods_dir)
>>   
>>   kernel_header_files:
>>   	@ls $(KHDR_DIR)/linux/*.h >/dev/null 2>/dev/null;                      \
>> @@ -105,8 +108,8 @@ endef
>>   
>>   run_tests: all
>>   ifdef building_out_of_srctree
>> -	@if [ "X$(TEST_PROGS)$(TEST_PROGS_EXTENDED)$(TEST_FILES)" != "X" ]; then \
>> -		rsync -aq --copy-unsafe-links $(TEST_PROGS) $(TEST_PROGS_EXTENDED) $(TEST_FILES) $(OUTPUT); \
>> +	@if [ "X$(TEST_PROGS)$(TEST_PROGS_EXTENDED)$(TEST_FILES)$(TEST_GEN_MODS_DIR)" != "X" ]; then \
>> +		rsync -aq --copy-unsafe-links $(TEST_PROGS) $(TEST_PROGS_EXTENDED) $(TEST_FILES) $(TEST_GEN_MODS_DIR) $(OUTPUT); \
>>   	fi
>>   	@if [ "X$(TEST_PROGS)" != "X" ]; then \
>>   		$(call RUN_TESTS, $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) \
>> @@ -118,6 +121,12 @@ else
>>   	@$(call RUN_TESTS, $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) $(TEST_PROGS))
>>   endif
>>   
>> +gen_mods_dir:
>> +	$(Q)$(MAKE) -C $(TEST_GEN_MODS_DIR)
>> +
>> +clean_mods_dir:
>> +	$(Q)$(MAKE) -C $(TEST_GEN_MODS_DIR) clean
>> +
>>   define INSTALL_SINGLE_RULE
>>   	$(if $(INSTALL_LIST),@mkdir -p $(INSTALL_PATH))
>>   	$(if $(INSTALL_LIST),rsync -a --copy-unsafe-links $(INSTALL_LIST) $(INSTALL_PATH)/)
>> @@ -131,6 +140,7 @@ define INSTALL_RULE
>>   	$(eval INSTALL_LIST = $(TEST_CUSTOM_PROGS)) $(INSTALL_SINGLE_RULE)
>>   	$(eval INSTALL_LIST = $(TEST_GEN_PROGS_EXTENDED)) $(INSTALL_SINGLE_RULE)
>>   	$(eval INSTALL_LIST = $(TEST_GEN_FILES)) $(INSTALL_SINGLE_RULE)
>> +	$(eval INSTALL_LIST = $(TEST_GEN_MODS_DIR)) $(INSTALL_SINGLE_RULE)
> 
> Hi Marcos,
> 
> Sorry for the late reply on this, but I'm reviewing this version by
> trying to retrofit it into our selftest packaging (pre-build the test
> module .ko's and stash those into an rpm rather than building on the
> test host).
> 
> Since $TEST_GEN_MODS_DIR is treated as a directory, I found that the
> selftest install target copies a bunch of intermediate object and kbuild
> files:
> 
>    $ mkdir /tmp/test-install
>    $ make KDIR=$(pwd) INSTALL_PATH=/tmp/test-install TARGETS=livepatch \
>         -C tools/testing/selftests/ install
> 
>    [ ... builds livepatch selftests ... ]
> 
> the rsync in question:
> 
>    rsync -a --copy-unsafe-links /home/jolawren/src/kernel/tools/testing/selftests/livepatch/test_modules /tmp/test-install/livepatch/
>    ...
> 
> and then looking at the destination:
> 
>    $ tree -a /tmp/test-install/
>    /tmp/test-install/
>    ├── kselftest
>    │   ├── module.sh
>    │   ├── prefix.pl
>    │   └── runner.sh
>    ├── kselftest-list.txt
>    ├── livepatch
>    │   ├── config
>    │   ├── functions.sh
>    │   ├── settings
>    │   ├── test-callbacks.sh
>    │   ├── test-ftrace.sh
>    │   ├── test_klp-call_getpid
>    │   ├── test-livepatch.sh
>    │   ├── test_modules
>    │   │   ├── Makefile
>    │   │   ├── modules.order
>    │   │   ├── .modules.order.cmd
>    │   │   ├── Module.symvers
>    │   │   ├── .Module.symvers.cmd
>    │   │   ├── test_klp_atomic_replace.c
>    │   │   ├── test_klp_atomic_replace.ko
>    │   │   ├── .test_klp_atomic_replace.ko.cmd
>    │   │   ├── test_klp_atomic_replace.mod
>    │   │   ├── test_klp_atomic_replace.mod.c
>    │   │   ├── .test_klp_atomic_replace.mod.cmd
>    │   │   ├── test_klp_atomic_replace.mod.o
>    │   │   ├── .test_klp_atomic_replace.mod.o.cmd
>    │   │   ├── test_klp_atomic_replace.o
>    │   │   ├── .test_klp_atomic_replace.o.cmd
>    ...
> 
> On the other hand, variables like $TEST_GEN_FILES specify individual
> files, so only final binaries like test_klp-call_getpid (and not
> test_klp-call_getpid.c) are copied to $INSTALL_PATH.


Thank you Joe for finding this problem.

Copying source files and object files doesn't sound right. This isn't
how the ksleftest installs work. Let's fix this.

thanks,
--Shuah
Marcos Paulo de Souza Jan. 8, 2024, 5:13 p.m. UTC | #3 
On Wed, 2024-01-03 at 15:09 -0700, Shuah Khan wrote:
> On 1/2/24 15:31, Joe Lawrence wrote:
> > On Wed, Dec 20, 2023 at 01:53:12PM -0300, Marcos Paulo de Souza
> > wrote:
> > > Add TEST_GEN_MODS_DIR variable for kselftests. It can point to
> > > a directory containing kernel modules that will be used by
> > > selftest scripts.
> > > 
> > > The modules are built as external modules for the running kernel.
> > > As a result they are always binary compatible and the same tests
> > > can be used for older or newer kernels.
> > > 
> > > The build requires "kernel-devel" package to be installed.
> > > For example, in the upstream sources, the rpm devel package
> > > is produced by "make rpm-pkg"
> > > 
> > > The modules can be built independently by
> > > 
> > >    make -C tools/testing/selftests/livepatch/
> > > 
> > > or they will be automatically built before running the tests via
> > > 
> > >    make -C tools/testing/selftests/livepatch/ run_tests
> > > 
> > > Note that they are _not_ built when running the standalone
> > > tests by calling, for example, ./test-state.sh.
> > > 
> > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > > ---
> > >   Documentation/dev-tools/kselftest.rst |  4 ++++
> > >   tools/testing/selftests/lib.mk        | 20 +++++++++++++++-----
> > >   2 files changed, 19 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/Documentation/dev-tools/kselftest.rst
> > > b/Documentation/dev-tools/kselftest.rst
> > > index ab376b316c36..7f3582a67318 100644
> > > --- a/Documentation/dev-tools/kselftest.rst
> > > +++ b/Documentation/dev-tools/kselftest.rst
> > > @@ -245,6 +245,10 @@ Contributing new tests (details)
> > >      TEST_PROGS, TEST_GEN_PROGS mean it is the executable tested
> > > by
> > >      default.
> > >   
> > > +   TEST_GEN_MODS_DIR should be used by tests that require
> > > modules to be built
> > > +   before the test starts. The variable will contain the name of
> > > the directory
> > > +   containing the modules.
> > > +
> > >      TEST_CUSTOM_PROGS should be used by tests that require
> > > custom build
> > >      rules and prevent common build rule use.
> > >   
> > > diff --git a/tools/testing/selftests/lib.mk
> > > b/tools/testing/selftests/lib.mk
> > > index 118e0964bda9..6c7c5a0112cf 100644
> > > --- a/tools/testing/selftests/lib.mk
> > > +++ b/tools/testing/selftests/lib.mk
> > > @@ -70,12 +70,15 @@ KHDR_INCLUDES := -isystem $(KHDR_DIR)
> > >   # TEST_PROGS are for test shell scripts.
> > >   # TEST_CUSTOM_PROGS and TEST_PROGS will be run by common
> > > run_tests
> > >   # and install targets. Common clean doesn't touch them.
> > > +# TEST_GEN_MODS_DIR is used to specify a directory with modules
> > > to be built
> > > +# before the test executes. These modules are cleaned on the
> > > clean target as well.
> > >   TEST_GEN_PROGS := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_PROGS))
> > >   TEST_GEN_PROGS_EXTENDED := $(patsubst
> > > %,$(OUTPUT)/%,$(TEST_GEN_PROGS_EXTENDED))
> > >   TEST_GEN_FILES := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_FILES))
> > > +TEST_GEN_MODS_DIR := $(patsubst
> > > %,$(OUTPUT)/%,$(TEST_GEN_MODS_DIR))
> > >   
> > >   all: kernel_header_files $(TEST_GEN_PROGS)
> > > $(TEST_GEN_PROGS_EXTENDED) \
> > > -     $(TEST_GEN_FILES)
> > > +     $(TEST_GEN_FILES) $(if $(TEST_GEN_MODS_DIR),gen_mods_dir)
> > >   
> > >   kernel_header_files:
> > >   	@ls $(KHDR_DIR)/linux/*.h >/dev/null
> > > 2>/dev/null;                      \
> > > @@ -105,8 +108,8 @@ endef
> > >   
> > >   run_tests: all
> > >   ifdef building_out_of_srctree
> > > -	@if [
> > > "X$(TEST_PROGS)$(TEST_PROGS_EXTENDED)$(TEST_FILES)" != "X" ];
> > > then \
> > > -		rsync -aq --copy-unsafe-links $(TEST_PROGS)
> > > $(TEST_PROGS_EXTENDED) $(TEST_FILES) $(OUTPUT); \
> > > +	@if [
> > > "X$(TEST_PROGS)$(TEST_PROGS_EXTENDED)$(TEST_FILES)$(TEST_GEN_MODS
> > > _DIR)" != "X" ]; then \
> > > +		rsync -aq --copy-unsafe-links $(TEST_PROGS)
> > > $(TEST_PROGS_EXTENDED) $(TEST_FILES) $(TEST_GEN_MODS_DIR)
> > > $(OUTPUT); \
> > >   	fi
> > >   	@if [ "X$(TEST_PROGS)" != "X" ]; then \
> > >   		$(call RUN_TESTS, $(TEST_GEN_PROGS)
> > > $(TEST_CUSTOM_PROGS) \
> > > @@ -118,6 +121,12 @@ else
> > >   	@$(call RUN_TESTS, $(TEST_GEN_PROGS)
> > > $(TEST_CUSTOM_PROGS) $(TEST_PROGS))
> > >   endif
> > >   
> > > +gen_mods_dir:
> > > +	$(Q)$(MAKE) -C $(TEST_GEN_MODS_DIR)
> > > +
> > > +clean_mods_dir:
> > > +	$(Q)$(MAKE) -C $(TEST_GEN_MODS_DIR) clean
> > > +
> > >   define INSTALL_SINGLE_RULE
> > >   	$(if $(INSTALL_LIST),@mkdir -p $(INSTALL_PATH))
> > >   	$(if $(INSTALL_LIST),rsync -a --copy-unsafe-links
> > > $(INSTALL_LIST) $(INSTALL_PATH)/)
> > > @@ -131,6 +140,7 @@ define INSTALL_RULE
> > >   	$(eval INSTALL_LIST = $(TEST_CUSTOM_PROGS))
> > > $(INSTALL_SINGLE_RULE)
> > >   	$(eval INSTALL_LIST = $(TEST_GEN_PROGS_EXTENDED))
> > > $(INSTALL_SINGLE_RULE)
> > >   	$(eval INSTALL_LIST = $(TEST_GEN_FILES))
> > > $(INSTALL_SINGLE_RULE)
> > > +	$(eval INSTALL_LIST = $(TEST_GEN_MODS_DIR))
> > > $(INSTALL_SINGLE_RULE)
> > 
> > Hi Marcos,
> > 
> > Sorry for the late reply on this, but I'm reviewing this version by
> > trying to retrofit it into our selftest packaging (pre-build the
> > test
> > module .ko's and stash those into an rpm rather than building on
> > the
> > test host).
> > 
> > Since $TEST_GEN_MODS_DIR is treated as a directory, I found that
> > the
> > selftest install target copies a bunch of intermediate object and
> > kbuild
> > files:
> > 
> >    $ mkdir /tmp/test-install
> >    $ make KDIR=$(pwd) INSTALL_PATH=/tmp/test-install
> > TARGETS=livepatch \
> >         -C tools/testing/selftests/ install
> > 
> >    [ ... builds livepatch selftests ... ]
> > 
> > the rsync in question:
> > 
> >    rsync -a --copy-unsafe-links
> > /home/jolawren/src/kernel/tools/testing/selftests/livepatch/test_mo
> > dules /tmp/test-install/livepatch/
> >    ...
> > 
> > and then looking at the destination:
> > 
> >    $ tree -a /tmp/test-install/
> >    /tmp/test-install/
> >    ├── kselftest
> >    │   ├── module.sh
> >    │   ├── prefix.pl
> >    │   └── runner.sh
> >    ├── kselftest-list.txt
> >    ├── livepatch
> >    │   ├── config
> >    │   ├── functions.sh
> >    │   ├── settings
> >    │   ├── test-callbacks.sh
> >    │   ├── test-ftrace.sh
> >    │   ├── test_klp-call_getpid
> >    │   ├── test-livepatch.sh
> >    │   ├── test_modules
> >    │   │   ├── Makefile
> >    │   │   ├── modules.order
> >    │   │   ├── .modules.order.cmd
> >    │   │   ├── Module.symvers
> >    │   │   ├── .Module.symvers.cmd
> >    │   │   ├── test_klp_atomic_replace.c
> >    │   │   ├── test_klp_atomic_replace.ko
> >    │   │   ├── .test_klp_atomic_replace.ko.cmd
> >    │   │   ├── test_klp_atomic_replace.mod
> >    │   │   ├── test_klp_atomic_replace.mod.c
> >    │   │   ├── .test_klp_atomic_replace.mod.cmd
> >    │   │   ├── test_klp_atomic_replace.mod.o
> >    │   │   ├── .test_klp_atomic_replace.mod.o.cmd
> >    │   │   ├── test_klp_atomic_replace.o
> >    │   │   ├── .test_klp_atomic_replace.o.cmd
> >    ...
> > 
> > On the other hand, variables like $TEST_GEN_FILES specify
> > individual
> > files, so only final binaries like test_klp-call_getpid (and not
> > test_klp-call_getpid.c) are copied to $INSTALL_PATH.

Hi Joe,

thanks for catching this issue. I crafted the attached patch and it
fixes the issue for me, copying only the resulting .ko objects as
expected. Can you please check if this fixes the issue for you?

> 
> 
> Thank you Joe for finding this problem.
> 
> Copying source files and object files doesn't sound right. This isn't
> how the ksleftest installs work. Let's fix this.

Hi Shuah,

what do you think about the proposed solution? Could you please amend
the fix into the first patch if you think it's the right approach?

Thanks in advance!
  Marcos

> 
> thanks,
> --Shuah
>
Shuah Khan Jan. 9, 2024, 7:31 p.m. UTC | #4 
On 1/8/24 10:13, Marcos Paulo de Souza wrote:
> On Wed, 2024-01-03 at 15:09 -0700, Shuah Khan wrote:

>>
>> Copying source files and object files doesn't sound right. This isn't
>> how the ksleftest installs work. Let's fix this.
> 
> Hi Shuah,
> 
> what do you think about the proposed solution? Could you please amend
> the fix into the first patch if you think it's the right approach?
> 

I would like to see a new revision of the patch series with the fix to
the problem. I will pull this into a separate test branch for us all
to test different scenarios. I would like to make sure the repo will
stay clean after install in the case of when out of tree builds.

Sorry I can't amend the patch as this isn't a trivial merge change.
This change requires more testing.

thanks,
-- Shuah
Marcos Paulo de Souza Jan. 10, 2024, 12:27 a.m. UTC | #5 
On Tue, 2024-01-09 at 12:31 -0700, Shuah Khan wrote:
> On 1/8/24 10:13, Marcos Paulo de Souza wrote:
> > On Wed, 2024-01-03 at 15:09 -0700, Shuah Khan wrote:
> 
> > > 
> > > Copying source files and object files doesn't sound right. This
> > > isn't
> > > how the ksleftest installs work. Let's fix this.
> > 
> > Hi Shuah,
> > 
> > what do you think about the proposed solution? Could you please
> > amend
> > the fix into the first patch if you think it's the right approach?
> > 
> 
> I would like to see a new revision of the patch series with the fix
> to
> the problem. I will pull this into a separate test branch for us all
> to test different scenarios. I would like to make sure the repo will
> stay clean after install in the case of when out of tree builds.
> 
> Sorry I can't amend the patch as this isn't a trivial merge change.
> This change requires more testing.

I sent a v5 of the patches. This new version has that diff that I sent
earlier to avoid copying the Kbuild files. It worked on make install
and with gen_tar.

Feel free to use this version in your test branch then.

Thanks in advance,
  Marcos

> 
> thanks,
> -- Shuah
> 
> 
>
diff mbox series

Patch

diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst
index ab376b316c36..7f3582a67318 100644
--- a/Documentation/dev-tools/kselftest.rst
+++ b/Documentation/dev-tools/kselftest.rst
@@ -245,6 +245,10 @@  Contributing new tests (details)
    TEST_PROGS, TEST_GEN_PROGS mean it is the executable tested by
    default.
 
+   TEST_GEN_MODS_DIR should be used by tests that require modules to be built
+   before the test starts. The variable will contain the name of the directory
+   containing the modules.
+
    TEST_CUSTOM_PROGS should be used by tests that require custom build
    rules and prevent common build rule use.
 
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 118e0964bda9..6c7c5a0112cf 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -70,12 +70,15 @@  KHDR_INCLUDES := -isystem $(KHDR_DIR)
 # TEST_PROGS are for test shell scripts.
 # TEST_CUSTOM_PROGS and TEST_PROGS will be run by common run_tests
 # and install targets. Common clean doesn't touch them.
+# TEST_GEN_MODS_DIR is used to specify a directory with modules to be built
+# before the test executes. These modules are cleaned on the clean target as well.
 TEST_GEN_PROGS := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_PROGS))
 TEST_GEN_PROGS_EXTENDED := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_PROGS_EXTENDED))
 TEST_GEN_FILES := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_FILES))
+TEST_GEN_MODS_DIR := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_MODS_DIR))
 
 all: kernel_header_files $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) \
-     $(TEST_GEN_FILES)
+     $(TEST_GEN_FILES) $(if $(TEST_GEN_MODS_DIR),gen_mods_dir)
 
 kernel_header_files:
 	@ls $(KHDR_DIR)/linux/*.h >/dev/null 2>/dev/null;                      \
@@ -105,8 +108,8 @@  endef
 
 run_tests: all
 ifdef building_out_of_srctree
-	@if [ "X$(TEST_PROGS)$(TEST_PROGS_EXTENDED)$(TEST_FILES)" != "X" ]; then \
-		rsync -aq --copy-unsafe-links $(TEST_PROGS) $(TEST_PROGS_EXTENDED) $(TEST_FILES) $(OUTPUT); \
+	@if [ "X$(TEST_PROGS)$(TEST_PROGS_EXTENDED)$(TEST_FILES)$(TEST_GEN_MODS_DIR)" != "X" ]; then \
+		rsync -aq --copy-unsafe-links $(TEST_PROGS) $(TEST_PROGS_EXTENDED) $(TEST_FILES) $(TEST_GEN_MODS_DIR) $(OUTPUT); \
 	fi
 	@if [ "X$(TEST_PROGS)" != "X" ]; then \
 		$(call RUN_TESTS, $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) \
@@ -118,6 +121,12 @@  else
 	@$(call RUN_TESTS, $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) $(TEST_PROGS))
 endif
 
+gen_mods_dir:
+	$(Q)$(MAKE) -C $(TEST_GEN_MODS_DIR)
+
+clean_mods_dir:
+	$(Q)$(MAKE) -C $(TEST_GEN_MODS_DIR) clean
+
 define INSTALL_SINGLE_RULE
 	$(if $(INSTALL_LIST),@mkdir -p $(INSTALL_PATH))
 	$(if $(INSTALL_LIST),rsync -a --copy-unsafe-links $(INSTALL_LIST) $(INSTALL_PATH)/)
@@ -131,6 +140,7 @@  define INSTALL_RULE
 	$(eval INSTALL_LIST = $(TEST_CUSTOM_PROGS)) $(INSTALL_SINGLE_RULE)
 	$(eval INSTALL_LIST = $(TEST_GEN_PROGS_EXTENDED)) $(INSTALL_SINGLE_RULE)
 	$(eval INSTALL_LIST = $(TEST_GEN_FILES)) $(INSTALL_SINGLE_RULE)
+	$(eval INSTALL_LIST = $(TEST_GEN_MODS_DIR)) $(INSTALL_SINGLE_RULE)
 	$(eval INSTALL_LIST = $(wildcard config settings)) $(INSTALL_SINGLE_RULE)
 endef
 
@@ -156,7 +166,7 @@  define CLEAN
 	$(RM) -r $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES) $(EXTRA_CLEAN)
 endef
 
-clean:
+clean: $(if $(TEST_GEN_MODS_DIR),clean_mods_dir)
 	$(CLEAN)
 
 # Enables to extend CFLAGS and LDFLAGS from command line, e.g.
@@ -187,4 +197,4 @@  $(OUTPUT)/%:%.S
 	$(LINK.S) $^ $(LDLIBS) -o $@
 endif
 
-.PHONY: run_tests all clean install emit_tests
+.PHONY: run_tests all clean install emit_tests gen_mods_dir clean_mods_dir