[PATCH] selftests: Install settings files to fix TIMEOUT failures

[PATCH] selftests: Install settings files to fix TIMEOUT failures

[Michael Ellerman]

Commit 852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second
timeout per test") added a 45 second timeout for tests, and also added
a way for tests to customise the timeout via a settings file.

For example the ftrace tests take multiple minutes to run, so they
were given longer in commit b43e78f65b1d ("tracing/selftests: Turn off
timeout setting").

This works when the tests are run from the source tree. However if the
tests are installed with "make -C tools/testing/selftests install",
the settings files are not copied into the install directory. When the
tests are then run from the install directory the longer timeouts are
not applied and the tests timeout incorrectly.

So add the settings files to TEST_FILES of the appropriate Makefiles
to cause the settings files to be installed using the existing install
logic.

Fixes: 852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second timeout per test")
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 tools/testing/selftests/ftrace/Makefile    | 2 +-
 tools/testing/selftests/livepatch/Makefile | 2 ++
 tools/testing/selftests/net/mptcp/Makefile | 2 ++
 tools/testing/selftests/rseq/Makefile      | 2 ++
 tools/testing/selftests/rtc/Makefile       | 2 ++
 5 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/ftrace/Makefile b/tools/testing/selftests/ftrace/Makefile
index cd1f5b3a7774..d6e106fbce11 100644
--- a/tools/testing/selftests/ftrace/Makefile
+++ b/tools/testing/selftests/ftrace/Makefile
@@ -2,7 +2,7 @@
 all:
 
 TEST_PROGS := ftracetest
-TEST_FILES := test.d
+TEST_FILES := test.d settings
 EXTRA_CLEAN := $(OUTPUT)/logs/*
 
 include ../lib.mk
diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile
index 3876d8d62494..1acc9e1fa3fb 100644
--- a/tools/testing/selftests/livepatch/Makefile
+++ b/tools/testing/selftests/livepatch/Makefile
@@ -8,4 +8,6 @@ TEST_PROGS := \
 	test-state.sh \
 	test-ftrace.sh
 
+TEST_FILES := settings
+
 include ../lib.mk
diff --git a/tools/testing/selftests/net/mptcp/Makefile b/tools/testing/selftests/net/mptcp/Makefile
index 93de52016dde..ba450e62dc5b 100644
--- a/tools/testing/selftests/net/mptcp/Makefile
+++ b/tools/testing/selftests/net/mptcp/Makefile
@@ -8,6 +8,8 @@ TEST_PROGS := mptcp_connect.sh
 
 TEST_GEN_FILES = mptcp_connect
 
+TEST_FILES := settings
+
 EXTRA_CLEAN := *.pcap
 
 include ../../lib.mk
diff --git a/tools/testing/selftests/rseq/Makefile b/tools/testing/selftests/rseq/Makefile
index d6469535630a..f1053630bb6f 100644
--- a/tools/testing/selftests/rseq/Makefile
+++ b/tools/testing/selftests/rseq/Makefile
@@ -19,6 +19,8 @@ TEST_GEN_PROGS_EXTENDED = librseq.so
 
 TEST_PROGS = run_param_test.sh
 
+TEST_FILES := settings
+
 include ../lib.mk
 
 $(OUTPUT)/librseq.so: rseq.c rseq.h rseq-*.h
diff --git a/tools/testing/selftests/rtc/Makefile b/tools/testing/selftests/rtc/Makefile
index de9c8566672a..90fa1a346908 100644
--- a/tools/testing/selftests/rtc/Makefile
+++ b/tools/testing/selftests/rtc/Makefile
@@ -6,4 +6,6 @@ TEST_GEN_PROGS = rtctest
 
 TEST_GEN_PROGS_EXTENDED = setdate
 
+TEST_FILES := settings
+
 include ../lib.mk
-- 
2.21.1

Re: [PATCH] selftests: Install settings files to fix TIMEOUT failures

[Joe Lawrence]

On 2/19/20 11:42 PM, Michael Ellerman wrote:
> Commit 852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second
> timeout per test") added a 45 second timeout for tests, and also added
> a way for tests to customise the timeout via a settings file.
> 
> For example the ftrace tests take multiple minutes to run, so they
> were given longer in commit b43e78f65b1d ("tracing/selftests: Turn off
> timeout setting").
> 
> This works when the tests are run from the source tree. However if the
> tests are installed with "make -C tools/testing/selftests install",
> the settings files are not copied into the install directory. When the
> tests are then run from the install directory the longer timeouts are
> not applied and the tests timeout incorrectly.
> 
> So add the settings files to TEST_FILES of the appropriate Makefiles
> to cause the settings files to be installed using the existing install
> logic.
> 
> Fixes: 852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second timeout per test")
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>   tools/testing/selftests/ftrace/Makefile    | 2 +-
>   tools/testing/selftests/livepatch/Makefile | 2 ++
>   tools/testing/selftests/net/mptcp/Makefile | 2 ++
>   tools/testing/selftests/rseq/Makefile      | 2 ++
>   tools/testing/selftests/rtc/Makefile       | 2 ++
>   5 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/ftrace/Makefile b/tools/testing/selftests/ftrace/Makefile
> index cd1f5b3a7774..d6e106fbce11 100644
> --- a/tools/testing/selftests/ftrace/Makefile
> +++ b/tools/testing/selftests/ftrace/Makefile
> @@ -2,7 +2,7 @@
>   all:
>   
>   TEST_PROGS := ftracetest
> -TEST_FILES := test.d
> +TEST_FILES := test.d settings
>   EXTRA_CLEAN := $(OUTPUT)/logs/*
>   
>   include ../lib.mk
> diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile
> index 3876d8d62494..1acc9e1fa3fb 100644
> --- a/tools/testing/selftests/livepatch/Makefile
> +++ b/tools/testing/selftests/livepatch/Makefile
> @@ -8,4 +8,6 @@ TEST_PROGS := \
>   	test-state.sh \
>   	test-ftrace.sh
>   
> +TEST_FILES := settings
> +
>   include ../lib.mk
> diff --git a/tools/testing/selftests/net/mptcp/Makefile b/tools/testing/selftests/net/mptcp/Makefile
> index 93de52016dde..ba450e62dc5b 100644
> --- a/tools/testing/selftests/net/mptcp/Makefile
> +++ b/tools/testing/selftests/net/mptcp/Makefile
> @@ -8,6 +8,8 @@ TEST_PROGS := mptcp_connect.sh
>   
>   TEST_GEN_FILES = mptcp_connect
>   
> +TEST_FILES := settings
> +
>   EXTRA_CLEAN := *.pcap
>   
>   include ../../lib.mk
> diff --git a/tools/testing/selftests/rseq/Makefile b/tools/testing/selftests/rseq/Makefile
> index d6469535630a..f1053630bb6f 100644
> --- a/tools/testing/selftests/rseq/Makefile
> +++ b/tools/testing/selftests/rseq/Makefile
> @@ -19,6 +19,8 @@ TEST_GEN_PROGS_EXTENDED = librseq.so
>   
>   TEST_PROGS = run_param_test.sh
>   
> +TEST_FILES := settings
> +
>   include ../lib.mk
>   
>   $(OUTPUT)/librseq.so: rseq.c rseq.h rseq-*.h
> diff --git a/tools/testing/selftests/rtc/Makefile b/tools/testing/selftests/rtc/Makefile
> index de9c8566672a..90fa1a346908 100644
> --- a/tools/testing/selftests/rtc/Makefile
> +++ b/tools/testing/selftests/rtc/Makefile
> @@ -6,4 +6,6 @@ TEST_GEN_PROGS = rtctest
>   
>   TEST_GEN_PROGS_EXTENDED = setdate
>   
> +TEST_FILES := settings
> +
>   include ../lib.mk
> 

Looks good to me,

Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

-- Joe

Re: [PATCH] selftests: Install settings files to fix TIMEOUT failures

[Shuah Khan]

On 2/20/20 3:25 PM, Joe Lawrence wrote:
> On 2/19/20 11:42 PM, Michael Ellerman wrote:
>> Commit 852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second
>> timeout per test") added a 45 second timeout for tests, and also added
>> a way for tests to customise the timeout via a settings file.
>>
>> For example the ftrace tests take multiple minutes to run, so they
>> were given longer in commit b43e78f65b1d ("tracing/selftests: Turn off
>> timeout setting").
>>
>> This works when the tests are run from the source tree. However if the
>> tests are installed with "make -C tools/testing/selftests install",
>> the settings files are not copied into the install directory. When the
>> tests are then run from the install directory the longer timeouts are
>> not applied and the tests timeout incorrectly.
>>
>> So add the settings files to TEST_FILES of the appropriate Makefiles
>> to cause the settings files to be installed using the existing install
>> logic.
>>
>> Fixes: 852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second 
>> timeout per test")
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>>   tools/testing/selftests/ftrace/Makefile    | 2 +-
>>   tools/testing/selftests/livepatch/Makefile | 2 ++
>>   tools/testing/selftests/net/mptcp/Makefile | 2 ++
>>   tools/testing/selftests/rseq/Makefile      | 2 ++
>>   tools/testing/selftests/rtc/Makefile       | 2 ++
>>   5 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/ftrace/Makefile 
>> b/tools/testing/selftests/ftrace/Makefile
>> index cd1f5b3a7774..d6e106fbce11 100644
>> --- a/tools/testing/selftests/ftrace/Makefile
>> +++ b/tools/testing/selftests/ftrace/Makefile
>> @@ -2,7 +2,7 @@
>>   all:
>>   TEST_PROGS := ftracetest
>> -TEST_FILES := test.d
>> +TEST_FILES := test.d settings
>>   EXTRA_CLEAN := $(OUTPUT)/logs/*
>>   include ../lib.mk
>> diff --git a/tools/testing/selftests/livepatch/Makefile 
>> b/tools/testing/selftests/livepatch/Makefile
>> index 3876d8d62494..1acc9e1fa3fb 100644
>> --- a/tools/testing/selftests/livepatch/Makefile
>> +++ b/tools/testing/selftests/livepatch/Makefile
>> @@ -8,4 +8,6 @@ TEST_PROGS := \
>>       test-state.sh \
>>       test-ftrace.sh
>> +TEST_FILES := settings
>> +
>>   include ../lib.mk
>> diff --git a/tools/testing/selftests/net/mptcp/Makefile 
>> b/tools/testing/selftests/net/mptcp/Makefile
>> index 93de52016dde..ba450e62dc5b 100644
>> --- a/tools/testing/selftests/net/mptcp/Makefile
>> +++ b/tools/testing/selftests/net/mptcp/Makefile
>> @@ -8,6 +8,8 @@ TEST_PROGS := mptcp_connect.sh
>>   TEST_GEN_FILES = mptcp_connect
>> +TEST_FILES := settings
>> +
>>   EXTRA_CLEAN := *.pcap
>>   include ../../lib.mk
>> diff --git a/tools/testing/selftests/rseq/Makefile 
>> b/tools/testing/selftests/rseq/Makefile
>> index d6469535630a..f1053630bb6f 100644
>> --- a/tools/testing/selftests/rseq/Makefile
>> +++ b/tools/testing/selftests/rseq/Makefile
>> @@ -19,6 +19,8 @@ TEST_GEN_PROGS_EXTENDED = librseq.so
>>   TEST_PROGS = run_param_test.sh
>> +TEST_FILES := settings
>> +
>>   include ../lib.mk
>>   $(OUTPUT)/librseq.so: rseq.c rseq.h rseq-*.h
>> diff --git a/tools/testing/selftests/rtc/Makefile 
>> b/tools/testing/selftests/rtc/Makefile
>> index de9c8566672a..90fa1a346908 100644
>> --- a/tools/testing/selftests/rtc/Makefile
>> +++ b/tools/testing/selftests/rtc/Makefile
>> @@ -6,4 +6,6 @@ TEST_GEN_PROGS = rtctest
>>   TEST_GEN_PROGS_EXTENDED = setdate
>> +TEST_FILES := settings
>> +
>>   include ../lib.mk
>>
> 
> Looks good to me,
> 
> Acked-by: Joe Lawrence <joe.lawrence@redhat.com>
> 

Thanks for Ack. I already applied it to kselftest fixes branch.

https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/log/?h=fixes

thanks,
-- Shuah

Re: [PATCH] selftests: Install settings files to fix TIMEOUT failures

[Kees Cook]

On Thu, Feb 20, 2020 at 03:42:41PM +1100, Michael Ellerman wrote:
> Commit 852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second
> timeout per test") added a 45 second timeout for tests, and also added
> a way for tests to customise the timeout via a settings file.
> 
> For example the ftrace tests take multiple minutes to run, so they
> were given longer in commit b43e78f65b1d ("tracing/selftests: Turn off
> timeout setting").
> 
> This works when the tests are run from the source tree. However if the
> tests are installed with "make -C tools/testing/selftests install",
> the settings files are not copied into the install directory. When the
> tests are then run from the install directory the longer timeouts are
> not applied and the tests timeout incorrectly.

Eek, yes, nice catch.

> So add the settings files to TEST_FILES of the appropriate Makefiles
> to cause the settings files to be installed using the existing install
> logic.

Instead, shouldn't lib.mk "notice" the settings file and automatically
include it in TEST_FILES instead of having to do this in each separate
Makefile?

-Kees

-- 
Kees Cook

Re: [PATCH] selftests: Install settings files to fix TIMEOUT failures

[Shuah Khan]

On 2/20/20 3:51 PM, Kees Cook wrote:
> On Thu, Feb 20, 2020 at 03:42:41PM +1100, Michael Ellerman wrote:
>> Commit 852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second
>> timeout per test") added a 45 second timeout for tests, and also added
>> a way for tests to customise the timeout via a settings file.
>>
>> For example the ftrace tests take multiple minutes to run, so they
>> were given longer in commit b43e78f65b1d ("tracing/selftests: Turn off
>> timeout setting").
>>
>> This works when the tests are run from the source tree. However if the
>> tests are installed with "make -C tools/testing/selftests install",
>> the settings files are not copied into the install directory. When the
>> tests are then run from the install directory the longer timeouts are
>> not applied and the tests timeout incorrectly.
> 
> Eek, yes, nice catch.
> 
>> So add the settings files to TEST_FILES of the appropriate Makefiles
>> to cause the settings files to be installed using the existing install
>> logic.
> 
> Instead, shouldn't lib.mk "notice" the settings file and automatically
> include it in TEST_FILES instead of having to do this in each separate
> Makefile?
> 

Let's keep it custom per test for now.

thanks,
-- Shuah

Re: [PATCH] selftests: Install settings files to fix TIMEOUT failures

[Michael Ellerman]

Shuah Khan <skhan@linuxfoundation.org> writes:
> On 2/20/20 3:51 PM, Kees Cook wrote:
>> On Thu, Feb 20, 2020 at 03:42:41PM +1100, Michael Ellerman wrote:
>>> Commit 852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second
>>> timeout per test") added a 45 second timeout for tests, and also added
>>> a way for tests to customise the timeout via a settings file.
>>>
>>> For example the ftrace tests take multiple minutes to run, so they
>>> were given longer in commit b43e78f65b1d ("tracing/selftests: Turn off
>>> timeout setting").
>>>
>>> This works when the tests are run from the source tree. However if the
>>> tests are installed with "make -C tools/testing/selftests install",
>>> the settings files are not copied into the install directory. When the
>>> tests are then run from the install directory the longer timeouts are
>>> not applied and the tests timeout incorrectly.
>> 
>> Eek, yes, nice catch.
>> 
>>> So add the settings files to TEST_FILES of the appropriate Makefiles
>>> to cause the settings files to be installed using the existing install
>>> logic.
>> 
>> Instead, shouldn't lib.mk "notice" the settings file and automatically
>> include it in TEST_FILES instead of having to do this in each separate
>> Makefile?
>> 
>
> Let's keep it custom per test for now.

Yeah that seems less magical.

FWIW the patch below does work, so it's a small patch, but I'm not sure
it's worth the added complexity vs just listing the file in the few
tests that need it.

cheers


diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 1c8a1963d03f..82086c6ad5e7 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -20,6 +20,10 @@ 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))
 
+ifeq ($(wildcard settings),settings)
+       TEST_FILES += settings
+endif
+
 ifdef KSFT_KHDR_INSTALL
 top_srcdir ?= ../../../..
 include $(top_srcdir)/scripts/subarch.include

Re: [PATCH] selftests: Install settings files to fix TIMEOUT failures

[Shuah Khan]

On 2/20/20 10:12 PM, Michael Ellerman wrote:
> Shuah Khan <skhan@linuxfoundation.org> writes:
>> On 2/20/20 3:51 PM, Kees Cook wrote:
>>> On Thu, Feb 20, 2020 at 03:42:41PM +1100, Michael Ellerman wrote:
>>>> Commit 852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second
>>>> timeout per test") added a 45 second timeout for tests, and also added
>>>> a way for tests to customise the timeout via a settings file.
>>>>
>>>> For example the ftrace tests take multiple minutes to run, so they
>>>> were given longer in commit b43e78f65b1d ("tracing/selftests: Turn off
>>>> timeout setting").
>>>>
>>>> This works when the tests are run from the source tree. However if the
>>>> tests are installed with "make -C tools/testing/selftests install",
>>>> the settings files are not copied into the install directory. When the
>>>> tests are then run from the install directory the longer timeouts are
>>>> not applied and the tests timeout incorrectly.
>>>
>>> Eek, yes, nice catch.
>>>
>>>> So add the settings files to TEST_FILES of the appropriate Makefiles
>>>> to cause the settings files to be installed using the existing install
>>>> logic.
>>>
>>> Instead, shouldn't lib.mk "notice" the settings file and automatically
>>> include it in TEST_FILES instead of having to do this in each separate
>>> Makefile?
>>>
>>
>> Let's keep it custom per test for now.
> 
> Yeah that seems less magical.
> 
> FWIW the patch below does work, so it's a small patch, but I'm not sure
> it's worth the added complexity vs just listing the file in the few
> tests that need it.

I am not seeing a whole lot of benefit in doing this in lib.mk
Test authors have to think about which files to include in
TEST_FILES and add it to Makefile anyway.

My second reason is I would like authors to think about the settings
as opposed to just duplicate from another test. This timeout changes
the kselftest run-time and behavior.

I am also finding making features generic has downsides to it unless
we are smart about it. We have several overrides already for things
that made perfect sense to make generic.

> diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> index 1c8a1963d03f..82086c6ad5e7 100644
> --- a/tools/testing/selftests/lib.mk
> +++ b/tools/testing/selftests/lib.mk
> @@ -20,6 +20,10 @@ 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))
>   
> +ifeq ($(wildcard settings),settings)
> +       TEST_FILES += settings
> +endif
> +
>   ifdef KSFT_KHDR_INSTALL
>   top_srcdir ?= ../../../..
>   include $(top_srcdir)/scripts/subarch.include
> 

Thanks for the patch though. We can consider adding it a later time, if
we see value in making this generic.

thanks,
-- Shuah