[PATCH 0/3] selftests/resctrl: Add resctrl_tests into kselftest set

[PATCH 0/3] selftests/resctrl: Add resctrl_tests into kselftest set

[Shaopeng Tan]

Hello,

Resctrl_tests is in the kselftest directory, but it cannot use kselftest 
framework. The aim of this series is to make resctrl_tests run by using 
kselftest framework and to fix some bug/setting of resctrl_tests when 
use kselftest framework.

In kselftest framework, we can build/run resctrl_tests by build/run 
all tests of kselftest, and we also can use the "TARGETS" variable 
on the make command line to specify resctrl_tests to build/run.

To ensure the resctrl_tests finish in limited time(which is specified 
by timeout command), set the limited time for resctrl_tests to 120 seconds.
When resctrl filesystem is not supported or resctrl_tests is not run as 
root, return skip code of kselftest.  If it is not finish in limited time, 
terminate resctrl_tests same as when executing ctrl+c.

Thanks,

Tan, Shaopeng (3):
  selftests/resctrl: Make resctrl_tests run using kselftest framework
  selftests/resctrl: Return KSFT_SKIP(4) if resctrl filessystem is not
    supported or resctrl is not run as root
  selftests/resctrl: Kill the child process created by fork() when the
    SIGTERM signal comes

 tools/testing/selftests/Makefile                |  1 +
 tools/testing/selftests/resctrl/Makefile        | 21 +++++++++------------
 tools/testing/selftests/resctrl/resctrl_tests.c |  4 ++--
 tools/testing/selftests/resctrl/resctrl_val.c   |  1 +
 tools/testing/selftests/resctrl/settings        |  1 +
 5 files changed, 14 insertions(+), 14 deletions(-)
 create mode 100644 tools/testing/selftests/resctrl/settings

-- 
1.8.3.1

[PATCH 1/3] selftests/resctrl: Make resctrl_tests run using kselftest framework

[Shaopeng Tan]

From: "Tan, Shaopeng" <tan.shaopeng@jp.fujitsu.com>

This commit enables kselftest to be built/run in resctrl framework.
Build/run resctrl_tests by build/run all tests of kselftest, or by using
the "TARGETS" variable on the make command line to specify resctrl_tests.
To make resctrl_tests run using kselftest framework, this commit modified
the Makefile of kernel kselftest set and the Makefile of resctrl_tests.
To ensure the resctrl_tests finish in limited time, this commit changed
the default limited time(45s) to 120 seconds for resctrl_tests by adding
"setting" file.

Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
 tools/testing/selftests/Makefile         |  1 +
 tools/testing/selftests/resctrl/Makefile | 21 +++++++++------------
 tools/testing/selftests/resctrl/settings |  1 +
 3 files changed, 11 insertions(+), 12 deletions(-)
 create mode 100644 tools/testing/selftests/resctrl/settings

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index c852eb4..7df397c 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -51,6 +51,7 @@ TARGETS += proc
 TARGETS += pstore
 TARGETS += ptrace
 TARGETS += openat2
+TARGETS += resctrl
 TARGETS += rlimits
 TARGETS += rseq
 TARGETS += rtc
diff --git a/tools/testing/selftests/resctrl/Makefile b/tools/testing/selftests/resctrl/Makefile
index 6bcee2e..3786cbb 100644
--- a/tools/testing/selftests/resctrl/Makefile
+++ b/tools/testing/selftests/resctrl/Makefile
@@ -1,17 +1,14 @@
-CC = $(CROSS_COMPILE)gcc
-CFLAGS = -g -Wall -O2 -D_FORTIFY_SOURCE=2
-SRCS=$(wildcard *.c)
-OBJS=$(SRCS:.c=.o)
+# SPDX-License-Identifier: GPL-2.0
+# Makefile for resctrl selftests
 
-all: resctrl_tests
+CFLAGS += -g -Wall -O2 -D_FORTIFY_SOURCE=2
+LDLIBS += -lnuma
 
-$(OBJS): $(SRCS)
-	$(CC) $(CFLAGS) -c $(SRCS)
+TEST_GEN_PROGS := resctrl_tests
+EXTRA_SOURCES := $(wildcard *.c)
 
-resctrl_tests: $(OBJS)
-	$(CC) $(CFLAGS) -o $@ $^
+all: $(TEST_GEN_PROGS)
 
-.PHONY: clean
+$(TEST_GEN_PROGS): $(EXTRA_SOURCES)
 
-clean:
-	$(RM) $(OBJS) resctrl_tests
+include ../lib.mk
diff --git a/tools/testing/selftests/resctrl/settings b/tools/testing/selftests/resctrl/settings
new file mode 100644
index 0000000..6091b45
--- /dev/null
+++ b/tools/testing/selftests/resctrl/settings
@@ -0,0 +1 @@
+timeout=120
-- 
1.8.3.1

[PATCH 2/3] selftests/resctrl: Return KSFT_SKIP(4) if resctrl filessystem is not supported or resctrl is not run as root

[Shaopeng Tan]

From: "Tan, Shaopeng" <tan.shaopeng@jp.fujitsu.com>

To unify the return code of resctrl_tests with the return code of
selftest set, return KSFT_SKIP (4) if resctrl filessystem is not
supported or resctrl is not run as root.

Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
 tools/testing/selftests/resctrl/resctrl_tests.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 973f09a..3be0895 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -205,7 +205,7 @@ int main(int argc, char **argv)
 	 * 2. We execute perf commands
 	 */
 	if (geteuid() != 0)
-		return ksft_exit_fail_msg("Not running as root, abort testing.\n");
+		return ksft_exit_skip("Not running as root, abort testing.\n");
 
 	/* Detect AMD vendor */
 	detect_amd();
@@ -235,7 +235,7 @@ int main(int argc, char **argv)
 	sprintf(bm_type, "fill_buf");
 
 	if (!check_resctrlfs_support())
-		return ksft_exit_fail_msg("resctrl FS does not exist\n");
+		return ksft_exit_skip("resctrl FS does not exist\n");
 
 	filter_dmesg();
 
-- 
1.8.3.1

[PATCH 3/3] selftests/resctrl: Kill the child process created by fork() when the SIGTERM signal comes

[Shaopeng Tan]

From: "Tan, Shaopeng" <tan.shaopeng@jp.fujitsu.com>

In kselftest framework there is a limited time for each sub test,
when the time limit comes SIGTEM signal will be sent to sub test by
"timeout --foregroup <seconds>" command.
In resctrl_tests, fork() is used to create a child process.
This commit ensures child process to be killed before parent process
exiting if SIGTERM signal comes.

Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
 tools/testing/selftests/resctrl/resctrl_val.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 9522434..b32b963 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -678,6 +678,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
 	sigemptyset(&sigact.sa_mask);
 	sigact.sa_flags = SA_SIGINFO;
 	if (sigaction(SIGINT, &sigact, NULL) ||
+	    sigaction(SIGTERM, &sigact, NULL) ||
 	    sigaction(SIGHUP, &sigact, NULL)) {
 		perror("# sigaction");
 		ret = errno;
-- 
1.8.3.1

RE: [PATCH 0/3] selftests/resctrl: Add resctrl_tests into kselftest set

[tan.shaopeng]

Hi,

Ping... any comments&advice about adding resctrl_tests into kselftest set?

Best regards
Tan Shaopeng
 
> Hello,
> 
> Resctrl_tests is in the kselftest directory, but it cannot use kselftest framework.
> The aim of this series is to make resctrl_tests run by using kselftest framework
> and to fix some bug/setting of resctrl_tests when use kselftest framework.
> 
> In kselftest framework, we can build/run resctrl_tests by build/run all tests of
> kselftest, and we also can use the "TARGETS" variable on the make command
> line to specify resctrl_tests to build/run.
> 
> To ensure the resctrl_tests finish in limited time(which is specified by timeout
> command), set the limited time for resctrl_tests to 120 seconds.
> When resctrl filesystem is not supported or resctrl_tests is not run as root,
> return skip code of kselftest.  If it is not finish in limited time, terminate
> resctrl_tests same as when executing ctrl+c.
> 
> Thanks,
> 
> Tan, Shaopeng (3):
>   selftests/resctrl: Make resctrl_tests run using kselftest framework
>   selftests/resctrl: Return KSFT_SKIP(4) if resctrl filessystem is not
>     supported or resctrl is not run as root
>   selftests/resctrl: Kill the child process created by fork() when the
>     SIGTERM signal comes
> 
>  tools/testing/selftests/Makefile                |  1 +
>  tools/testing/selftests/resctrl/Makefile        | 21
> +++++++++------------
>  tools/testing/selftests/resctrl/resctrl_tests.c |  4 ++--
>  tools/testing/selftests/resctrl/resctrl_val.c   |  1 +
>  tools/testing/selftests/resctrl/settings        |  1 +
>  5 files changed, 14 insertions(+), 14 deletions(-)  create mode 100644
> tools/testing/selftests/resctrl/settings
> 
> --
> 1.8.3.1

Re: [PATCH 2/3] selftests/resctrl: Return KSFT_SKIP(4) if resctrl filessystem is not supported or resctrl is not run as root

[Reinette Chatre]

Hi Shaopeng Tan,

(subject line and commit message: filessystem -> file system)

On 11/10/2021 1:33 AM, Shaopeng Tan wrote:
> From: "Tan, Shaopeng" <tan.shaopeng@jp.fujitsu.com>
> 
> To unify the return code of resctrl_tests with the return code of
> selftest set, return KSFT_SKIP (4) if resctrl filessystem is not
> supported or resctrl is not run as root.

Could you please elaborate how changing ksft_exit_fail_msg() to 
ksft_exit_skip() accomplishes the goal of unifying the return code? 
What is wrong with using ksft_exit_fail_msg()?

Reinette

Re: [PATCH 1/3] selftests/resctrl: Make resctrl_tests run using kselftest framework

[Reinette Chatre]

Hi Shaopeng Tan,

On 11/10/2021 1:33 AM, Shaopeng Tan wrote:
> From: "Tan, Shaopeng" <tan.shaopeng@jp.fujitsu.com>
> 
> This commit enables kselftest to be built/run in resctrl framework.
> Build/run resctrl_tests by build/run all tests of kselftest, or by using
> the "TARGETS" variable on the make command line to specify resctrl_tests.
> To make resctrl_tests run using kselftest framework, this commit modified
> the Makefile of kernel kselftest set and the Makefile of resctrl_tests.

The above sentence mentions that changes were made to the resctrl 
selftest Makefile but it does not describe what the change accomplish or 
why they are needed. Could you please elaborate?

> To ensure the resctrl_tests finish in limited time, this commit changed
> the default limited time(45s) to 120 seconds for resctrl_tests by adding
> "setting" file.

How is changing the timeout related to the resctrl framework changes? Is 
it not a separate change?

Thank you

Reinette

RE: [PATCH 2/3] selftests/resctrl: Return KSFT_SKIP(4) if resctrl filessystem is not supported or resctrl is not run as root

[tan.shaopeng]

Hi Reinette
 
> (subject line and commit message: filessystem -> file system)
Thanks.
 
> On 11/10/2021 1:33 AM, Shaopeng Tan wrote:
> > From: "Tan, Shaopeng" <tan.shaopeng@jp.fujitsu.com>
> >
> > To unify the return code of resctrl_tests with the return code of
> > selftest set, return KSFT_SKIP (4) if resctrl filessystem is not
> > supported or resctrl is not run as root.
> 
> Could you please elaborate how changing ksft_exit_fail_msg() to
> ksft_exit_skip() accomplishes the goal of unifying the return code?
> What is wrong with using ksft_exit_fail_msg()?

In selftest framwork, 
if a test need root privileges, but it is run as user privileges,
the test result will counted as a SKIP item, instead of a FAIL item.

For example, 
(1)tools/testing/selftests/mqueue/mq_open_tests.c
267         if (getuid() != 0) 
268                 ksft_exit_skip("Not running as root, but almost all tests "
269                         "require root in order to modify\nsystem settings.  "
270                         "Exiting.\n"); 

(2)tools/testing/selftests/bpf/test_kmod.sh
  5 ksft_skip=4
  6 
  7 msg="skip all tests:"
  8 if [ "$(id -u)" != "0" ]; then
  9         echo $msg please run this as root >&2
 10         exit $ksft_skip
 11 fi

Regards, 
Shaopeng Tan

RE: [PATCH 1/3] selftests/resctrl: Make resctrl_tests run using kselftest framework

[tan.shaopeng]

Hi Reinette

> On 11/10/2021 1:33 AM, Shaopeng Tan wrote:
> > From: "Tan, Shaopeng" <tan.shaopeng@jp.fujitsu.com>
> >
> > This commit enables kselftest to be built/run in resctrl framework.
> > Build/run resctrl_tests by build/run all tests of kselftest, or by
> > using the "TARGETS" variable on the make command line to specify
> resctrl_tests.
> > To make resctrl_tests run using kselftest framework, this commit
> > modified the Makefile of kernel kselftest set and the Makefile of
> resctrl_tests.
> 
> The above sentence mentions that changes were made to the resctrl selftest
> Makefile but it does not describe what the change accomplish or why they are
> needed. Could you please elaborate?

Before these changes of Makefile, when we run resctrl test,
we need to goto tools/testing/selftests/resctrl/ directory,
run "make" to build executable file "resctrl_tests",
and run "sudo ./resctrl_tests" to execute the test.

With this patch, we can resctrl test in selftest framwork as follow.
Run all tests include resctrl:
 $ make -C tools/testing/selftests run_tests
Run a subset(resctrl) of selftests:
 $ make -C tools/testing/selftests TARGETS=resctrl run_tests

Linux Kernel Selftests :
https://www.kernel.org/doc/html/latest/dev-tools/kselftest.html 

> > To ensure the resctrl_tests finish in limited time, this commit
> > changed the default limited time(45s) to 120 seconds for resctrl_tests
> > by adding "setting" file.
> 
> How is changing the timeout related to the resctrl framework changes? Is it not
> a separate change?

In selftest framwork, the default limited time of all tests is 45 seconds
which is specified by common file tools/testing/selftests/kselftest/runner.sh.
Each test can change the limited time individually by adding a "setting" 
file into its own directory. I changed the limited time of resctrl to120s 
because 45s was not enough in my environment.

Regards,
Shaopeng Tan

Re: [PATCH 1/3] selftests/resctrl: Make resctrl_tests run using kselftest framework

[Reinette Chatre]

Hi Shaopeng Tan,

On 11/30/2021 6:36 PM, tan.shaopeng@fujitsu.com wrote:
> Hi Reinette
> 
>> On 11/10/2021 1:33 AM, Shaopeng Tan wrote:
>>> From: "Tan, Shaopeng" <tan.shaopeng@jp.fujitsu.com>
>>>
>>> This commit enables kselftest to be built/run in resctrl framework.
>>> Build/run resctrl_tests by build/run all tests of kselftest, or by
>>> using the "TARGETS" variable on the make command line to specify
>> resctrl_tests.
>>> To make resctrl_tests run using kselftest framework, this commit
>>> modified the Makefile of kernel kselftest set and the Makefile of
>> resctrl_tests.
>>
>> The above sentence mentions that changes were made to the resctrl selftest
>> Makefile but it does not describe what the change accomplish or why they are
>> needed. Could you please elaborate?
> 
> Before these changes of Makefile, when we run resctrl test,
> we need to goto tools/testing/selftests/resctrl/ directory,
> run "make" to build executable file "resctrl_tests",
> and run "sudo ./resctrl_tests" to execute the test.
> 
> With this patch, we can resctrl test in selftest framwork as follow.
> Run all tests include resctrl:
>   $ make -C tools/testing/selftests run_tests
> Run a subset(resctrl) of selftests:
>   $ make -C tools/testing/selftests TARGETS=resctrl run_tests
> 
> Linux Kernel Selftests :
> https://www.kernel.org/doc/html/latest/dev-tools/kselftest.html

Understood and this is a reasonable change. What you wrote above would 
be a great addition to the changelog. I think it is also important to 
add to the changelog that the changes do not change the existing 
behavior and users can continue to build and run the tests as before.

Also, please follow the guidance found in "Separate your changes" in
Documentation/process/submitting-patches.rst: Logical changes should be 
in separate patches. This patch does too many things. Please consider:
1) the license addition is not relevant to this work
2) the new comment seems unnecessary as it states the obvious
3) where is the "LDLIBS += -lnuma" change coming from and why is it needed?

When logical changes are isolated into separate patches it really makes 
the patches easier to understand.

>>> To ensure the resctrl_tests finish in limited time, this commit
>>> changed the default limited time(45s) to 120 seconds for resctrl_tests
>>> by adding "setting" file.
>>
>> How is changing the timeout related to the resctrl framework changes? Is it not
>> a separate change?
> 
> In selftest framwork, the default limited time of all tests is 45 seconds
> which is specified by common file tools/testing/selftests/kselftest/runner.sh.
> Each test can change the limited time individually by adding a "setting"
> file into its own directory. I changed the limited time of resctrl to120s
> because 45s was not enough in my environment.

Understood. My question was if this can be a separate change with its 
own patch? It seems to me that this deserves its own patch ... but 
actually it also raises an important issue that the resctrl tests are 
taking a long time.

I do see a rule for tests in Documentation/dev-tools/kselftest.rst: 
"Don't take too long". This may be a motivation _not_ to include the 
resctrl tests in the regular kselftest targets because when a user wants 
to run all tests on a system it needs to be quick and the resctrl tests 
are not quick.

Reinette

Re: [PATCH 2/3] selftests/resctrl: Return KSFT_SKIP(4) if resctrl filessystem is not supported or resctrl is not run as root

[Reinette Chatre]

Hi Shaopeng Tan,

On 11/30/2021 6:36 PM, tan.shaopeng@fujitsu.com wrote:
> Hi Reinette
>   
>> (subject line and commit message: filessystem -> file system)
> Thanks.
>   
>> On 11/10/2021 1:33 AM, Shaopeng Tan wrote:
>>> From: "Tan, Shaopeng" <tan.shaopeng@jp.fujitsu.com>
>>>
>>> To unify the return code of resctrl_tests with the return code of
>>> selftest set, return KSFT_SKIP (4) if resctrl filessystem is not
>>> supported or resctrl is not run as root.
>>
>> Could you please elaborate how changing ksft_exit_fail_msg() to
>> ksft_exit_skip() accomplishes the goal of unifying the return code?
>> What is wrong with using ksft_exit_fail_msg()?
> 
> In selftest framwork,
> if a test need root privileges, but it is run as user privileges,
> the test result will counted as a SKIP item, instead of a FAIL item.

Thank you for the details. I think it is important to highlight that a 
skipped test is marked as successful to not unnecessarily report a 
feature failure when there actually is a failure in the test environment.

Reinette

RE: [PATCH 1/3] selftests/resctrl: Make resctrl_tests run using kselftest framework

[tan.shaopeng]

Hi Reinette,
 
> On 11/30/2021 6:36 PM, tan.shaopeng@fujitsu.com wrote:
> > Hi Reinette
> >
> >> On 11/10/2021 1:33 AM, Shaopeng Tan wrote:
> >>> From: "Tan, Shaopeng" <tan.shaopeng@jp.fujitsu.com>
> >>>
> >>> This commit enables kselftest to be built/run in resctrl framework.
> >>> Build/run resctrl_tests by build/run all tests of kselftest, or by
> >>> using the "TARGETS" variable on the make command line to specify
> >> resctrl_tests.
> >>> To make resctrl_tests run using kselftest framework, this commit
> >>> modified the Makefile of kernel kselftest set and the Makefile of
> >> resctrl_tests.
> >>
> >> The above sentence mentions that changes were made to the resctrl
> >> selftest Makefile but it does not describe what the change accomplish
> >> or why they are needed. Could you please elaborate?
> >
> > Before these changes of Makefile, when we run resctrl test, we need to
> > goto tools/testing/selftests/resctrl/ directory, run "make" to build
> > executable file "resctrl_tests", and run "sudo ./resctrl_tests" to
> > execute the test.
> >
> > With this patch, we can resctrl test in selftest framwork as follow.
> > Run all tests include resctrl:
> >   $ make -C tools/testing/selftests run_tests Run a subset(resctrl) of
> > selftests:
> >   $ make -C tools/testing/selftests TARGETS=resctrl run_tests
> >
> > Linux Kernel Selftests :
> > https://www.kernel.org/doc/html/latest/dev-tools/kselftest.html
> 
> Understood and this is a reasonable change. What you wrote above would be a
> great addition to the changelog. I think it is also important to add to the
> changelog that the changes do not change the existing behavior and users can
> continue to build and run the tests as before.

Thanks for your advice.

> Also, please follow the guidance found in "Separate your changes" in
> Documentation/process/submitting-patches.rst: Logical changes should be
> in separate patches. This patch does too many things. Please consider:
> 1) the license addition is not relevant to this work
> 2) the new comment seems unnecessary as it states the obvious
> 3) where is the "LDLIBS += -lnuma" change coming from and why is it
> needed?

I'm sorry, I made a mistake.
"LDLIBs += -lnuma" is dependent on the following patch.
I will reorganize patch series to include the following patch and
separate Makefile changes appropriately.

https://lore.kernel.org/lkml/20211110082734.3184985-1-tan.shaopeng@jp.fujitsu.com/
[PATCH] selftests/resctrl: Skip MBM&CMT tests when Intel Sub-NUMA

> When logical changes are isolated into separate patches it really makes the
> patches easier to understand.

Thanks for your advice, I will separate patches.

> >>> To ensure the resctrl_tests finish in limited time, this commit
> >>> changed the default limited time(45s) to 120 seconds for
> >>> resctrl_tests by adding "setting" file.
> >>
> >> How is changing the timeout related to the resctrl framework changes?
> >> Is it not a separate change?
> >
> > In selftest framwork, the default limited time of all tests is 45
> > seconds which is specified by common file
> tools/testing/selftests/kselftest/runner.sh.
> > Each test can change the limited time individually by adding a "setting"
> > file into its own directory. I changed the limited time of resctrl
> > to120s because 45s was not enough in my environment.
> 
> Understood. My question was if this can be a separate change with its own
> patch? It seems to me that this deserves its own patch ... but actually it also
> raises an important issue that the resctrl tests are taking a long time.
> 
> I do see a rule for tests in Documentation/dev-tools/kselftest.rst:
> "Don't take too long". This may be a motivation _not_ to include the resctrl tests
> in the regular kselftest targets because when a user wants to run all tests on a
> system it needs to be quick and the resctrl tests are not quick.

I think 120s is not long, there are 6 tests with a limited time over 120s, 
for example, the limited time of net test is set 300s. 


Regards,
Shaopeng Tan

Re: [PATCH 1/3] selftests/resctrl: Make resctrl_tests run using kselftest framework

[Reinette Chatre]

Hi Shaopeng Tan,

On 12/2/2021 11:21 PM, tan.shaopeng@fujitsu.com wrote:
>> On 11/30/2021 6:36 PM, tan.shaopeng@fujitsu.com wrote:
>>>> On 11/10/2021 1:33 AM, Shaopeng Tan wrote:
>>>>> From: "Tan, Shaopeng" <tan.shaopeng@jp.fujitsu.com>

>>>>> To ensure the resctrl_tests finish in limited time, this commit
>>>>> changed the default limited time(45s) to 120 seconds for
>>>>> resctrl_tests by adding "setting" file.
>>>>
>>>> How is changing the timeout related to the resctrl framework changes?
>>>> Is it not a separate change?
>>>
>>> In selftest framwork, the default limited time of all tests is 45
>>> seconds which is specified by common file
>> tools/testing/selftests/kselftest/runner.sh.
>>> Each test can change the limited time individually by adding a "setting"
>>> file into its own directory. I changed the limited time of resctrl
>>> to120s because 45s was not enough in my environment.
>>
>> Understood. My question was if this can be a separate change with its own
>> patch? It seems to me that this deserves its own patch ... but actually it also
>> raises an important issue that the resctrl tests are taking a long time.
>>
>> I do see a rule for tests in Documentation/dev-tools/kselftest.rst:
>> "Don't take too long". This may be a motivation _not_ to include the resctrl tests
>> in the regular kselftest targets because when a user wants to run all tests on a
>> system it needs to be quick and the resctrl tests are not quick.
> 
> I think 120s is not long, there are 6 tests with a limited time over 120s,
> for example, the limited time of net test is set 300s.

I am not familiar with the specific kselftest requirements in this 
regard but the test duration is surely something that needs to be kept 
in mind. Consider the systems performing integration testing on kernels 
everywhere - running the kselftest framework is a reasonable thing to do 
and test delays that may seem palatable on an individual run may not be 
appropriate for all test infrastructures.

Needing to almost triple the needed time from the default time is a red 
flag and really deserves to be in its own patch with a motivation. I 
would also recommend highlighting this change in the cover letter. This 
will bring the issue to the attention of the kselftest audience who will 
provide a better informed opinion (whether they want a long running test 
as part of the default framework or not).

Reinette

RE: [PATCH 1/3] selftests/resctrl: Make resctrl_tests run using kselftest framework

[tan.shaopeng]

Hi Reinette,
 
> On 12/2/2021 11:21 PM, tan.shaopeng@fujitsu.com wrote:
> >> On 11/30/2021 6:36 PM, tan.shaopeng@fujitsu.com wrote:
> >>>> On 11/10/2021 1:33 AM, Shaopeng Tan wrote:
> >>>>> From: "Tan, Shaopeng" <tan.shaopeng@jp.fujitsu.com>
> 
> >>>>> To ensure the resctrl_tests finish in limited time, this commit
> >>>>> changed the default limited time(45s) to 120 seconds for
> >>>>> resctrl_tests by adding "setting" file.
> >>>>
> >>>> How is changing the timeout related to the resctrl framework changes?
> >>>> Is it not a separate change?
> >>>
> >>> In selftest framwork, the default limited time of all tests is 45
> >>> seconds which is specified by common file
> >> tools/testing/selftests/kselftest/runner.sh.
> >>> Each test can change the limited time individually by adding a "setting"
> >>> file into its own directory. I changed the limited time of resctrl
> >>> to120s because 45s was not enough in my environment.
> >>
> >> Understood. My question was if this can be a separate change with its
> >> own patch? It seems to me that this deserves its own patch ... but
> >> actually it also raises an important issue that the resctrl tests are taking a
> long time.
> >>
> >> I do see a rule for tests in Documentation/dev-tools/kselftest.rst:
> >> "Don't take too long". This may be a motivation _not_ to include the
> >> resctrl tests in the regular kselftest targets because when a user
> >> wants to run all tests on a system it needs to be quick and the resctrl tests
> are not quick.
> >
> > I think 120s is not long, there are 6 tests with a limited time over
> > 120s, for example, the limited time of net test is set 300s.
> 
> I am not familiar with the specific kselftest requirements in this regard but the
> test duration is surely something that needs to be kept in mind. Consider the
> systems performing integration testing on kernels everywhere - running the
> kselftest framework is a reasonable thing to do and test delays that may seem
> palatable on an individual run may not be appropriate for all test
> infrastructures.
> 
> Needing to almost triple the needed time from the default time is a red flag and
> really deserves to be in its own patch with a motivation. I would also
> recommend highlighting this change in the cover letter. This will bring the issue
> to the attention of the kselftest audience who will provide a better informed
> opinion (whether they want a long running test as part of the default framework
> or not).

Thanks for your advice.
I will separate the part about default limited time to a new patch.
In order to get some opinions about change default limited time,
I will add a description in the cover letter,
when posting the next version of this patch.

Regards,
Shaopeng Tan

Re: [PATCH 1/3] selftests/resctrl: Make resctrl_tests run using kselftest framework

[Reinette Chatre]

Hi Shaopeng Tan,

On 12/5/2021 10:57 PM, tan.shaopeng@fujitsu.com wrote:
> I will separate the part about default limited time to a new patch.
> In order to get some opinions about change default limited time,
> I will add a description in the cover letter,
> when posting the next version of this patch.

When you submit the next version, could you please change the order so 
that current patch 3/3 becomes 1/3? If I understand correctly, without 
the SIGTERM fix, the test would just hang if run from the kselftest 
framework. It would be better to have SIGTERM handled before attempting 
to run with the framework to ensure things keep working smoothly.

Thank you very much.

Reinette