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

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

[Shaopeng Tan]

Hello,

The aim of this series is to make resctrl_tests run by using
kselftest framework.
- I modify resctrl_test Makefile and kselftest Makefile,
  to enable build/run resctrl_tests by using kselftest framework.
  Of course, users can also build/run resctrl_tests without
  using framework as before.
- I change the default limited time for resctrl_tests to 120 seconds, to
  ensure the resctrl_tests finish in limited time on different environments.
- When resctrl file system is not supported by environment or
  resctrl_tests is not run as root, return skip code of kselftest framework.
- If resctrl_tests does not finish in limited time, terminate it as
  same as executing ctrl+c that kills parent process and child process.

Difference from v2:
- I reworte changelog of this patch series.
- I added how to use framework to run resctrl to README. [PATCH v3 2/5]
- License has no dependencies on this patch series, I separated from it this patch series to another patch.
https://lore.kernel.org/lkml/20211213100154.180599-1-tan.shaopeng@jp.fujitsu.com/

With regard to the limited time, I think 120s is not a problem since some tests have a longer
timeout (e.g. net test is 300s). Please let me know if this is wrong.

Thanks,

Shaopeng Tan (5):
  selftests/resctrl: Kill child process before parent process terminates
    if SIGTERM is received
  selftests/resctrl: Make resctrl_tests run using kselftest framework
  selftests/resctrl: Update README about using kselftest framework to
    build/run resctrl_tests
  selftests/resctrl: Change the default limited time to 120 seconds
  selftests/resctrl: Fix resctrl_tests' return code to work with
    selftest framework

 tools/testing/selftests/Makefile              |  1 +
 tools/testing/selftests/resctrl/Makefile      | 20 ++++-------
 tools/testing/selftests/resctrl/README        | 34 +++++++++++++++++++
 .../testing/selftests/resctrl/resctrl_tests.c |  4 +--
 tools/testing/selftests/resctrl/resctrl_val.c |  1 +
 tools/testing/selftests/resctrl/settings      |  1 +
 6 files changed, 45 insertions(+), 16 deletions(-)
 create mode 100644 tools/testing/selftests/resctrl/settings

-- 
2.27.0

[PATCH v3 1/5] selftests/resctrl: Kill child process before parent process terminates if SIGTERM is received

[Shaopeng Tan]

In kselftest framework, a sub test is run using the timeout utility
and it will send SIGTERM to the test upon timeout.

In resctrl_tests, a child process is created by fork() to
run benchmark but SIGTERM is not set in sigaction().
If SIGTERM signal is received, the parent process will be killed,
but the child process still exists.

kill child process before parent process terminates
if SIGTERM signal is received.

Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
Some important feedbacks from v1&v2 are addressed as follows:

- Change the order so that current patch 3/3 becomes 1/3. Since without
  the SIGTERM fix, the test would hang if run from the kselftest framework.
  => I changed the order and the SIGTERM fix now becomes patch [1/5].

- Describe that the test is run using the timeout utility and
  it will send SIGTERM to the test upon timeout.
  => I updated the changelog to include this information.

- Describe changes in imperative mood, and address this in all patches.
  See Documentation/process/submitting-patches.rst for more details.
  => I described all my patches' changelog in imperative mood and
     deleted "This commit".

- +	    sigaction(SIGTERM, &sigact, NULL) ||
  This snippet is preceded with a comment that describes its usage
  you could also update it with the expanded use of the kselftest framework.
  => I don't think it is necessary to add other comments.
     Since the current comment already states "Register CTRL-C handler for parent,
     as it has to kill benchmark before exiting", So, when SIGTERM comes,
     the benchmark(child process) should be killed before parent process terminates,
     but it was missing.

 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 95224345c78e..b32b96356ec7 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;
-- 
2.27.0

[PATCH v3 2/5] selftests/resctrl: Make resctrl_tests run using kselftest framework

[Shaopeng Tan]

In kselftest framework, all tests can be build/run at a time,
and a sub test also can be build/run individually. As follows:
$ make -C tools/testing/selftests run_tests
$ make -C tools/testing/selftests TARGETS=ptrace run_tests

However, resctrl_tests cannot be run using kselftest framework,
users have to change directory to tools/testing/selftests/resctrl/,
run "make" to build executable file "resctrl_tests",
and run "sudo ./resctrl_tests" to execute the test.

To build/run resctrl_tests using kselftest framework.
Modify tools/testing/selftests/Makefile
and tools/testing/selftests/resctrl/Makefile.

Even after this change, users can still build/run resctrl_tests
without using framework as before.

Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
Some important feedbacks from v1&v2 are addressed as follows:

- The changelog mentions that changes were made to the resctrl
  selftest Makefile but it does not describe what the change accomplish
  or why they are needed.
  => By changing the Makefile, resctrl_tests can use kselftest
     framework like other sub tests. I described this in changelog.

- The changelog did not describe how a user may use the kselftest
  framework to run the resctrl tests nor the requested information
  on how existing workflows are impacted.
  => I described how to build/run resctrl_tests with kselftest framework,
     and described the existing workflows are not impacted that users can
     build/run resctrl_tests without using kselftest framework as before.

- tools/testing/selftests/resctrl/README should be updated.
  => I separate the update of README to a new patch.[patch v3 3/5]

- Why is the meaning of "EXTRA_SOURCES" (i.e. what is "extra"?) and
  why is "SRCS" no longer sufficient?
  => I referred to other Makefiles, and found "SRCS" is better
     than "EXTRA_SOURCES". So, I updated it to use "SRCS".

 tools/testing/selftests/Makefile         |  1 +
 tools/testing/selftests/resctrl/Makefile | 20 ++++++--------------
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index c852eb40c4f7..7df397c6893c 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 6bcee2ec91a9..de26638540ba 100644
--- a/tools/testing/selftests/resctrl/Makefile
+++ b/tools/testing/selftests/resctrl/Makefile
@@ -1,17 +1,9 @@
-CC = $(CROSS_COMPILE)gcc
-CFLAGS = -g -Wall -O2 -D_FORTIFY_SOURCE=2
-SRCS=$(wildcard *.c)
-OBJS=$(SRCS:.c=.o)
+CFLAGS += -g -Wall -O2 -D_FORTIFY_SOURCE=2
 
-all: resctrl_tests
+TEST_GEN_PROGS := resctrl_tests
+SRCS := $(wildcard *.c)
 
-$(OBJS): $(SRCS)
-	$(CC) $(CFLAGS) -c $(SRCS)
+all: $(TEST_GEN_PROGS)
 
-resctrl_tests: $(OBJS)
-	$(CC) $(CFLAGS) -o $@ $^
-
-.PHONY: clean
-
-clean:
-	$(RM) $(OBJS) resctrl_tests
+$(TEST_GEN_PROGS): $(SRCS)
+include ../lib.mk
-- 
2.27.0

[PATCH v3 3/5] selftests/resctrl: Update README about using kselftest framework to build/run resctrl_tests

[Shaopeng Tan]

In this patch series, I make restrl_tests build/run using kselftest
framework, but some users do not known how to build/run resctrl_tests
using kseltest framework.

Add manual of how to make resctrl_tests build/run
using kselftest framework into README.

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

diff --git a/tools/testing/selftests/resctrl/README b/tools/testing/selftests/resctrl/README
index 3d2bbd4fa3aa..a0dd459049b7 100644
--- a/tools/testing/selftests/resctrl/README
+++ b/tools/testing/selftests/resctrl/README
@@ -12,9 +12,43 @@ Allocation test on Intel RDT hardware. More tests will be added in the future.
 And the test suit can be extended to cover AMD QoS and ARM MPAM hardware
 as well.
 
+resctrl_tests can be run with or without kselftest framework.
+
+USE KSELFTEST FRAMEWORK
+-----------------------
+
+BUILD
+-----
+
+Execute the following command in top level directory of the kernel source.
+
+Build resctrl:
+ $ make -C tools/testing/selftests TARGETS=resctrl
+
+Build all self tests:
+ $ make -C tools/testing/selftests
+
+RUN
+---
+
+Run resctrl:
+ $ make -C tools/testing/selftests TARGETS=resctrl run_tests
+
+Run all self tests:
+ $ make -C tools/testing/selftests run_tests
+
+Using kselftest framework, the ./resctrl_tests will be run without any parameters.
+
+More details about kselftest framework as follow.
+Documentation/dev-tools/kselftest.rst
+
+NOT USE KSELFTEST FRAMEWORK
+---------------------------
+
 BUILD
 -----
 
+Execute the following command in this directory(tools/testing/selftests/resctrl/).
 Run "make" to build executable file "resctrl_tests".
 
 RUN
-- 
2.27.0

[PATCH v3 4/5] selftests/resctrl: Change the default limited time to 120 seconds

[Shaopeng Tan]

When testing on a Intel(R) Xeon(R) Gold 6254 CPU @ 3.10GHz the resctrl
selftests fail due to timeout after exceeding the default time limit of
45 seconds. On this system the test takes about 68 seconds. 
Since the failing test by default accesses a fixed size of memory, the
execution time should not vary significantly between different environment.
A new default of 120 seconds should be sufficient yet easy to customize
with the introduction of the "settings" file for reference.

Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
From the perspective of the kselftest framework,
a rule of "Don't take too long" is a concern.
To get some better informed opinions from kselftest audience,
I highlighted this change in the cover letter.

I adopted most of Reinette's phrase from the discussion in patch v2
to explain why 120s is appropriate for this test.

 tools/testing/selftests/resctrl/settings | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 tools/testing/selftests/resctrl/settings

diff --git a/tools/testing/selftests/resctrl/settings b/tools/testing/selftests/resctrl/settings
new file mode 100644
index 000000000000..6091b45d226b
--- /dev/null
+++ b/tools/testing/selftests/resctrl/settings
@@ -0,0 +1 @@
+timeout=120
-- 
2.27.0

[PATCH v3 5/5] selftests/resctrl: Fix resctrl_tests' return code to work with selftest framework

[Shaopeng Tan]

In kselftest framework, if a sub test can not run by some reasons,
the test result should be marked as SKIP rather than FAIL.
Return KSFT_SKIP(4) instead of KSFT_FAIL(1) if resctrl_tests is not run
as root or it is run on a test environment which does not support resctrl.

 - ksft_exit_fail_msg(): returns KSFT_FAIL(1)
 - ksft_exit_skip(): returns KSFT_SKIP(4)

Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
Some important feedbacks from v1&v2 are addressed as follows:

- 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.
  => I highligted this content in changelog as follows.
     "In kselftest framework, if a test is not run by some reason,
     the test result is marked as SKIP rather than FAIL."

- The subject line can be made more succinct while the details are
  moved to the commit message.
  => I made subject line succinct and moved the details to changelog.

- How changing ksft_exit_fail_msg() to ksft_exit_skip() accomplishes
  the goal of unifying the return code.
  => In changelog, I explained why return code KSFT_SKIP(4) is needed
     in kselftest framework and the return code of each function.

 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 973f09a66e1e..3be0895c492b 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();
 
-- 
2.27.0

Re: [PATCH v3 1/5] selftests/resctrl: Kill child process before parent process terminates if SIGTERM is received

[Shuah Khan]

On 2/15/22 7:26 PM, Shaopeng Tan wrote:
> In kselftest framework, a sub test is run using the timeout utility
> and it will send SIGTERM to the test upon timeout.
> 
> In resctrl_tests, a child process is created by fork() to
> run benchmark but SIGTERM is not set in sigaction().
> If SIGTERM signal is received, the parent process will be killed,
> but the child process still exists.
> 
> kill child process before parent process terminates
> if SIGTERM signal is received.
> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
> Some important feedbacks from v1&v2 are addressed as follows:
> 
> - Change the order so that current patch 3/3 becomes 1/3. Since without
>    the SIGTERM fix, the test would hang if run from the kselftest framework.
>    => I changed the order and the SIGTERM fix now becomes patch [1/5].
> 
> - Describe that the test is run using the timeout utility and
>    it will send SIGTERM to the test upon timeout.
>    => I updated the changelog to include this information.
> 
> - Describe changes in imperative mood, and address this in all patches.
>    See Documentation/process/submitting-patches.rst for more details.
>    => I described all my patches' changelog in imperative mood and
>       deleted "This commit".
> 
> - +	    sigaction(SIGTERM, &sigact, NULL) ||
>    This snippet is preceded with a comment that describes its usage
>    you could also update it with the expanded use of the kselftest framework.
>    => I don't think it is necessary to add other comments.
>       Since the current comment already states "Register CTRL-C handler for parent,
>       as it has to kill benchmark before exiting", So, when SIGTERM comes,
>       the benchmark(child process) should be killed before parent process terminates,
>       but it was missing.
> 
>   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 95224345c78e..b32b96356ec7 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;
> 

This looks good to me.

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

Re: [PATCH v3 2/5] selftests/resctrl: Make resctrl_tests run using kselftest framework

[Shuah Khan]

On 2/15/22 7:26 PM, Shaopeng Tan wrote:
> In kselftest framework, all tests can be build/run at a time,
> and a sub test also can be build/run individually. As follows:
> $ make -C tools/testing/selftests run_tests
> $ make -C tools/testing/selftests TARGETS=ptrace run_tests
> 
> However, resctrl_tests cannot be run using kselftest framework,
> users have to change directory to tools/testing/selftests/resctrl/,
> run "make" to build executable file "resctrl_tests",
> and run "sudo ./resctrl_tests" to execute the test.
> 
> To build/run resctrl_tests using kselftest framework.
> Modify tools/testing/selftests/Makefile
> and tools/testing/selftests/resctrl/Makefile.
> 
> Even after this change, users can still build/run resctrl_tests
> without using framework as before.
> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
> Some important feedbacks from v1&v2 are addressed as follows:
> 
> - The changelog mentions that changes were made to the resctrl
>    selftest Makefile but it does not describe what the change accomplish
>    or why they are needed.
>    => By changing the Makefile, resctrl_tests can use kselftest
>       framework like other sub tests. I described this in changelog.
> 
> - The changelog did not describe how a user may use the kselftest
>    framework to run the resctrl tests nor the requested information
>    on how existing workflows are impacted.
>    => I described how to build/run resctrl_tests with kselftest framework,
>       and described the existing workflows are not impacted that users can
>       build/run resctrl_tests without using kselftest framework as before.
> 
> - tools/testing/selftests/resctrl/README should be updated.
>    => I separate the update of README to a new patch.[patch v3 3/5]
> 
> - Why is the meaning of "EXTRA_SOURCES" (i.e. what is "extra"?) and
>    why is "SRCS" no longer sufficient?
>    => I referred to other Makefiles, and found "SRCS" is better
>       than "EXTRA_SOURCES". So, I updated it to use "SRCS".
> 
>   tools/testing/selftests/Makefile         |  1 +
>   tools/testing/selftests/resctrl/Makefile | 20 ++++++--------------
>   2 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index c852eb40c4f7..7df397c6893c 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 6bcee2ec91a9..de26638540ba 100644
> --- a/tools/testing/selftests/resctrl/Makefile
> +++ b/tools/testing/selftests/resctrl/Makefile
> @@ -1,17 +1,9 @@
> -CC = $(CROSS_COMPILE)gcc
> -CFLAGS = -g -Wall -O2 -D_FORTIFY_SOURCE=2
> -SRCS=$(wildcard *.c)
> -OBJS=$(SRCS:.c=.o)
> +CFLAGS += -g -Wall -O2 -D_FORTIFY_SOURCE=2
>   
> -all: resctrl_tests
> +TEST_GEN_PROGS := resctrl_tests
> +SRCS := $(wildcard *.c)
>   
> -$(OBJS): $(SRCS)
> -	$(CC) $(CFLAGS) -c $(SRCS)
> +all: $(TEST_GEN_PROGS)
>   
> -resctrl_tests: $(OBJS)
> -	$(CC) $(CFLAGS) -o $@ $^
> -
> -.PHONY: clean
> -
> -clean:
> -	$(RM) $(OBJS) resctrl_tests
> +$(TEST_GEN_PROGS): $(SRCS)

This patch breaks the test build - the below use-cases fail

make kselftest-all TARGETS=resctrl
make -C  tools/testing/selftests/ TARGETS=resctrl

Also a simple make in tools/testing/selftests/resctr

thanks,
-- Shuah

Re: [PATCH v3 3/5] selftests/resctrl: Update README about using kselftest framework to build/run resctrl_tests

[Shuah Khan]

On 2/15/22 7:26 PM, Shaopeng Tan wrote:
> In this patch series, I make restrl_tests build/run using kselftest
> framework, but some users do not known how to build/run resctrl_tests
> using kseltest framework.
> 
> Add manual of how to make resctrl_tests build/run
> using kselftest framework into README.
> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>   tools/testing/selftests/resctrl/README | 34 ++++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/README b/tools/testing/selftests/resctrl/README
> index 3d2bbd4fa3aa..a0dd459049b7 100644
> --- a/tools/testing/selftests/resctrl/README
> +++ b/tools/testing/selftests/resctrl/README
> @@ -12,9 +12,43 @@ Allocation test on Intel RDT hardware. More tests will be added in the future.
>   And the test suit can be extended to cover AMD QoS and ARM MPAM hardware
>   as well.
>   
> +resctrl_tests can be run with or without kselftest framework.
> +
> +USE KSELFTEST FRAMEWORK
> +-----------------------
> +
> +BUILD
> +-----
> +
> +Execute the following command in top level directory of the kernel source.
> +
> +Build resctrl:
> + $ make -C tools/testing/selftests TARGETS=resctrl
> +
> +Build all self tests:
> + $ make -C tools/testing/selftests
> +
> +RUN
> +---
> +
> +Run resctrl:
> + $ make -C tools/testing/selftests TARGETS=resctrl run_tests
> +

==================================
> +Run all self tests:
> + $ make -C tools/testing/selftests run_tests
> +

Remove the above
This part is relevant to this test. This is already documented in kselftest
doc.
==================================

> +Using kselftest framework, the ./resctrl_tests will be run without any parameters.
> +
> +More details about kselftest framework as follow.
> +Documentation/dev-tools/kselftest.rst
> +
> +NOT USE KSELFTEST FRAMEWORK
> +---------------------------
> +
>   BUILD
>   -----
>   
> +Execute the following command in this directory(tools/testing/selftests/resctrl/).
>   Run "make" to build executable file "resctrl_tests".
>   
>   RUN
> 

Add information how long it takes to run this test in here.

thanks,
-- Shuah

Re: [PATCH v3 4/5] selftests/resctrl: Change the default limited time to 120 seconds

[Shuah Khan]

On 2/15/22 7:26 PM, Shaopeng Tan wrote:
> When testing on a Intel(R) Xeon(R) Gold 6254 CPU @ 3.10GHz the resctrl
> selftests fail due to timeout after exceeding the default time limit of
> 45 seconds. On this system the test takes about 68 seconds.
> Since the failing test by default accesses a fixed size of memory, the
> execution time should not vary significantly between different environment.
> A new default of 120 seconds should be sufficient yet easy to customize
> with the introduction of the "settings" file for reference.
> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>  From the perspective of the kselftest framework,
> a rule of "Don't take too long" is a concern.
> To get some better informed opinions from kselftest audience,
> I highlighted this change in the cover letter.
> 
> I adopted most of Reinette's phrase from the discussion in patch v2
> to explain why 120s is appropriate for this test.
> 
>   tools/testing/selftests/resctrl/settings | 1 +
>   1 file changed, 1 insertion(+)
>   create mode 100644 tools/testing/selftests/resctrl/settings
> 
> diff --git a/tools/testing/selftests/resctrl/settings b/tools/testing/selftests/resctrl/settings
> new file mode 100644
> index 000000000000..6091b45d226b
> --- /dev/null
> +++ b/tools/testing/selftests/resctrl/settings
> @@ -0,0 +1 @@
> +timeout=120
> 

This is fine.

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

Re: [PATCH v3 5/5] selftests/resctrl: Fix resctrl_tests' return code to work with selftest framework

[Shuah Khan]

On 2/15/22 7:26 PM, Shaopeng Tan wrote:
> In kselftest framework, if a sub test can not run by some reasons,
> the test result should be marked as SKIP rather than FAIL.
> Return KSFT_SKIP(4) instead of KSFT_FAIL(1) if resctrl_tests is not run
> as root or it is run on a test environment which does not support resctrl.
> 
>   - ksft_exit_fail_msg(): returns KSFT_FAIL(1)
>   - ksft_exit_skip(): returns KSFT_SKIP(4)
> 

Thank for making this change to skip.

> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
> Some important feedbacks from v1&v2 are addressed as follows:
> 
> - 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.
>    => I highligted this content in changelog as follows.
>       "In kselftest framework, if a test is not run by some reason,
>       the test result is marked as SKIP rather than FAIL."
> 
> - The subject line can be made more succinct while the details are
>    moved to the commit message.
>    => I made subject line succinct and moved the details to changelog.
> 
> - How changing ksft_exit_fail_msg() to ksft_exit_skip() accomplishes
>    the goal of unifying the return code.
>    => In changelog, I explained why return code KSFT_SKIP(4) is needed
>       in kselftest framework and the return code of each function.
> 
>   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 973f09a66e1e..3be0895c492b 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");

Let's update the message to "This test must be run as root. Skipping..."
>   
>   	/* 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");

Update the message to read -

"resctrl FS doesn not exist. Enable X86_CPU_RESCTRL and PROC_CPU_RESCTRL config options"

>   
>   	filter_dmesg();
>   
> 

With these changes:

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

Re: [PATCH v3 0/5] selftests/resctrl: Add resctrl_tests into kselftest set

[Shuah Khan]

On 2/15/22 7:26 PM, Shaopeng Tan wrote:
> Hello,
> 
> The aim of this series is to make resctrl_tests run by using
> kselftest framework.
> - I modify resctrl_test Makefile and kselftest Makefile,
>    to enable build/run resctrl_tests by using kselftest framework.
>    Of course, users can also build/run resctrl_tests without
>    using framework as before.
> - I change the default limited time for resctrl_tests to 120 seconds, to
>    ensure the resctrl_tests finish in limited time on different environments.
> - When resctrl file system is not supported by environment or
>    resctrl_tests is not run as root, return skip code of kselftest framework.
> - If resctrl_tests does not finish in limited time, terminate it as
>    same as executing ctrl+c that kills parent process and child process.
> 
> Difference from v2:
> - I reworte changelog of this patch series.
> - I added how to use framework to run resctrl to README. [PATCH v3 2/5]
> - License has no dependencies on this patch series, I separated from it this patch series to another patch.
> https://lore.kernel.org/lkml/20211213100154.180599-1-tan.shaopeng@jp.fujitsu.com/
> 
> With regard to the limited time, I think 120s is not a problem since some tests have a longer
> timeout (e.g. net test is 300s). Please let me know if this is wrong.
> 
> Thanks,
> 
> Shaopeng Tan (5):
>    selftests/resctrl: Kill child process before parent process terminates
>      if SIGTERM is received
>    selftests/resctrl: Make resctrl_tests run using kselftest framework
>    selftests/resctrl: Update README about using kselftest framework to
>      build/run resctrl_tests
>    selftests/resctrl: Change the default limited time to 120 seconds
>    selftests/resctrl: Fix resctrl_tests' return code to work with
>      selftest framework
> 
>   tools/testing/selftests/Makefile              |  1 +
>   tools/testing/selftests/resctrl/Makefile      | 20 ++++-------
>   tools/testing/selftests/resctrl/README        | 34 +++++++++++++++++++
>   .../testing/selftests/resctrl/resctrl_tests.c |  4 +--
>   tools/testing/selftests/resctrl/resctrl_val.c |  1 +
>   tools/testing/selftests/resctrl/settings      |  1 +
>   6 files changed, 45 insertions(+), 16 deletions(-)
>   create mode 100644 tools/testing/selftests/resctrl/settings
> 

Reviewed the patches - patches 1/5, 4/5 & 5/5 don't depend on kselftest
framework improvements. 2/5 and 3/5 are.

Please reorder the patches - move 4/5 and 5/5 up and make 2/5 and 3/5
the last in this series. Also see comments on individual patches.

thanks,
-- Shuah

RE: [PATCH v3 2/5] selftests/resctrl: Make resctrl_tests run using kselftest framework

[tan.shaopeng]

Hi Khan,

> On 2/15/22 7:26 PM, Shaopeng Tan wrote:
> > In kselftest framework, all tests can be build/run at a time, and a
> > sub test also can be build/run individually. As follows:
> > $ make -C tools/testing/selftests run_tests $ make -C
> > tools/testing/selftests TARGETS=ptrace run_tests
> >
> > However, resctrl_tests cannot be run using kselftest framework, users
> > have to change directory to tools/testing/selftests/resctrl/, run
> > "make" to build executable file "resctrl_tests", and run "sudo
> > ./resctrl_tests" to execute the test.
> >
> > To build/run resctrl_tests using kselftest framework.
> > Modify tools/testing/selftests/Makefile and
> > tools/testing/selftests/resctrl/Makefile.
> >
> > Even after this change, users can still build/run resctrl_tests
> > without using framework as before.
> >
> > Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> > ---
> > Some important feedbacks from v1&v2 are addressed as follows:
> >
> > - The changelog mentions that changes were made to the resctrl
> >    selftest Makefile but it does not describe what the change accomplish
> >    or why they are needed.
> >    => By changing the Makefile, resctrl_tests can use kselftest
> >       framework like other sub tests. I described this in changelog.
> >
> > - The changelog did not describe how a user may use the kselftest
> >    framework to run the resctrl tests nor the requested information
> >    on how existing workflows are impacted.
> >    => I described how to build/run resctrl_tests with kselftest framework,
> >       and described the existing workflows are not impacted that users can
> >       build/run resctrl_tests without using kselftest framework as before.
> >
> > - tools/testing/selftests/resctrl/README should be updated.
> >    => I separate the update of README to a new patch.[patch v3 3/5]
> >
> > - Why is the meaning of "EXTRA_SOURCES" (i.e. what is "extra"?) and
> >    why is "SRCS" no longer sufficient?
> >    => I referred to other Makefiles, and found "SRCS" is better
> >       than "EXTRA_SOURCES". So, I updated it to use "SRCS".
> >
> >   tools/testing/selftests/Makefile         |  1 +
> >   tools/testing/selftests/resctrl/Makefile | 20 ++++++--------------
> >   2 files changed, 7 insertions(+), 14 deletions(-)
> >
> > diff --git a/tools/testing/selftests/Makefile
> > b/tools/testing/selftests/Makefile
> > index c852eb40c4f7..7df397c6893c 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 6bcee2ec91a9..de26638540ba 100644
> > --- a/tools/testing/selftests/resctrl/Makefile
> > +++ b/tools/testing/selftests/resctrl/Makefile
> > @@ -1,17 +1,9 @@
> > -CC = $(CROSS_COMPILE)gcc
> > -CFLAGS = -g -Wall -O2 -D_FORTIFY_SOURCE=2 -SRCS=$(wildcard *.c)
> > -OBJS=$(SRCS:.c=.o)
> > +CFLAGS += -g -Wall -O2 -D_FORTIFY_SOURCE=2
> >
> > -all: resctrl_tests
> > +TEST_GEN_PROGS := resctrl_tests
> > +SRCS := $(wildcard *.c)
> >
> > -$(OBJS): $(SRCS)
> > -	$(CC) $(CFLAGS) -c $(SRCS)
> > +all: $(TEST_GEN_PROGS)
> >
> > -resctrl_tests: $(OBJS)
> > -	$(CC) $(CFLAGS) -o $@ $^
> > -
> > -.PHONY: clean
> > -
> > -clean:
> > -	$(RM) $(OBJS) resctrl_tests
> > +$(TEST_GEN_PROGS): $(SRCS)
> 
> This patch breaks the test build - the below use-cases fail
> 
> make kselftest-all TARGETS=resctrl
> make -C  tools/testing/selftests/ TARGETS=resctrl
> 
> Also a simple make in tools/testing/selftests/resctr

Thanks for your feedbacks.
I applied these patches to the source below and built 
resctrl_tests successfully using above use-cases on x86/arm machine.
(1)
 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
 Tag: v5.16
(2)
 https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
 Tag: next-20220217

Could you tell me which kernel source you used to build
and what error message you got?

Best regards, 
Tan Shaopeng

Re: [PATCH v3 2/5] selftests/resctrl: Make resctrl_tests run using kselftest framework

[Shuah Khan]

On 2/22/22 12:55 AM, tan.shaopeng@fujitsu.com wrote:
> Hi Khan,
> 
>> On 2/15/22 7:26 PM, Shaopeng Tan wrote:
>>> In kselftest framework, all tests can be build/run at a time, and a
>>> sub test also can be build/run individually. As follows:
>>> $ make -C tools/testing/selftests run_tests $ make -C
>>> tools/testing/selftests TARGETS=ptrace run_tests
>>>
>>> However, resctrl_tests cannot be run using kselftest framework, users
>>> have to change directory to tools/testing/selftests/resctrl/, run
>>> "make" to build executable file "resctrl_tests", and run "sudo
>>> ./resctrl_tests" to execute the test.
>>>
>>> To build/run resctrl_tests using kselftest framework.
>>> Modify tools/testing/selftests/Makefile and
>>> tools/testing/selftests/resctrl/Makefile.
>>>
>>> Even after this change, users can still build/run resctrl_tests
>>> without using framework as before.
>>>
>>> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
>>> ---
>>> Some important feedbacks from v1&v2 are addressed as follows:
>>>
>>> - The changelog mentions that changes were made to the resctrl
>>>     selftest Makefile but it does not describe what the change accomplish
>>>     or why they are needed.
>>>     => By changing the Makefile, resctrl_tests can use kselftest
>>>        framework like other sub tests. I described this in changelog.
>>>
>>> - The changelog did not describe how a user may use the kselftest
>>>     framework to run the resctrl tests nor the requested information
>>>     on how existing workflows are impacted.
>>>     => I described how to build/run resctrl_tests with kselftest framework,
>>>        and described the existing workflows are not impacted that users can
>>>        build/run resctrl_tests without using kselftest framework as before.
>>>
>>> - tools/testing/selftests/resctrl/README should be updated.
>>>     => I separate the update of README to a new patch.[patch v3 3/5]
>>>
>>> - Why is the meaning of "EXTRA_SOURCES" (i.e. what is "extra"?) and
>>>     why is "SRCS" no longer sufficient?
>>>     => I referred to other Makefiles, and found "SRCS" is better
>>>        than "EXTRA_SOURCES". So, I updated it to use "SRCS".
>>>
>>>    tools/testing/selftests/Makefile         |  1 +
>>>    tools/testing/selftests/resctrl/Makefile | 20 ++++++--------------
>>>    2 files changed, 7 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/Makefile
>>> b/tools/testing/selftests/Makefile
>>> index c852eb40c4f7..7df397c6893c 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 6bcee2ec91a9..de26638540ba 100644
>>> --- a/tools/testing/selftests/resctrl/Makefile
>>> +++ b/tools/testing/selftests/resctrl/Makefile
>>> @@ -1,17 +1,9 @@
>>> -CC = $(CROSS_COMPILE)gcc
>>> -CFLAGS = -g -Wall -O2 -D_FORTIFY_SOURCE=2 -SRCS=$(wildcard *.c)
>>> -OBJS=$(SRCS:.c=.o)
>>> +CFLAGS += -g -Wall -O2 -D_FORTIFY_SOURCE=2
>>>
>>> -all: resctrl_tests
>>> +TEST_GEN_PROGS := resctrl_tests
>>> +SRCS := $(wildcard *.c)
>>>
>>> -$(OBJS): $(SRCS)
>>> -	$(CC) $(CFLAGS) -c $(SRCS)
>>> +all: $(TEST_GEN_PROGS)
>>>
>>> -resctrl_tests: $(OBJS)
>>> -	$(CC) $(CFLAGS) -o $@ $^
>>> -
>>> -.PHONY: clean
>>> -
>>> -clean:
>>> -	$(RM) $(OBJS) resctrl_tests
>>> +$(TEST_GEN_PROGS): $(SRCS)
>>
>> This patch breaks the test build - the below use-cases fail
>>
>> make kselftest-all TARGETS=resctrl
>> make -C  tools/testing/selftests/ TARGETS=resctrl
>>
>> Also a simple make in tools/testing/selftests/resctr
> 
> Thanks for your feedbacks.
> I applied these patches to the source below and built
> resctrl_tests successfully using above use-cases on x86/arm machine.
> (1)
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>   Tag: v5.16
> (2)
>   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
>   Tag: next-20220217
> 
> Could you tell me which kernel source you used to build
> and what error message you got?
> 

I tried this on Linux 5.17-rc4

thanks,
-- Shuah

RE: [PATCH v3 2/5] selftests/resctrl: Make resctrl_tests run using kselftest framework

[tan.shaopeng]

Hi Shuah,

> On 2/22/22 12:55 AM, tan.shaopeng@fujitsu.com wrote:
> > Hi Khan,
> >
> >> On 2/15/22 7:26 PM, Shaopeng Tan wrote:
> >>> In kselftest framework, all tests can be build/run at a time, and a
> >>> sub test also can be build/run individually. As follows:
> >>> $ make -C tools/testing/selftests run_tests $ make -C
> >>> tools/testing/selftests TARGETS=ptrace run_tests
> >>>
> >>> However, resctrl_tests cannot be run using kselftest framework,
> >>> users have to change directory to tools/testing/selftests/resctrl/,
> >>> run "make" to build executable file "resctrl_tests", and run "sudo
> >>> ./resctrl_tests" to execute the test.
> >>>
> >>> To build/run resctrl_tests using kselftest framework.
> >>> Modify tools/testing/selftests/Makefile and
> >>> tools/testing/selftests/resctrl/Makefile.
> >>>
> >>> Even after this change, users can still build/run resctrl_tests
> >>> without using framework as before.
> >>>
> >>> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> >>> ---
> >>> Some important feedbacks from v1&v2 are addressed as follows:
> >>>
> >>> - The changelog mentions that changes were made to the resctrl
> >>>     selftest Makefile but it does not describe what the change
> accomplish
> >>>     or why they are needed.
> >>>     => By changing the Makefile, resctrl_tests can use kselftest
> >>>        framework like other sub tests. I described this in changelog.
> >>>
> >>> - The changelog did not describe how a user may use the kselftest
> >>>     framework to run the resctrl tests nor the requested information
> >>>     on how existing workflows are impacted.
> >>>     => I described how to build/run resctrl_tests with kselftest
> framework,
> >>>        and described the existing workflows are not impacted that users
> can
> >>>        build/run resctrl_tests without using kselftest framework as
> before.
> >>>
> >>> - tools/testing/selftests/resctrl/README should be updated.
> >>>     => I separate the update of README to a new patch.[patch v3 3/5]
> >>>
> >>> - Why is the meaning of "EXTRA_SOURCES" (i.e. what is "extra"?) and
> >>>     why is "SRCS" no longer sufficient?
> >>>     => I referred to other Makefiles, and found "SRCS" is better
> >>>        than "EXTRA_SOURCES". So, I updated it to use "SRCS".
> >>>
> >>>    tools/testing/selftests/Makefile         |  1 +
> >>>    tools/testing/selftests/resctrl/Makefile | 20 ++++++--------------
> >>>    2 files changed, 7 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/tools/testing/selftests/Makefile
> >>> b/tools/testing/selftests/Makefile
> >>> index c852eb40c4f7..7df397c6893c 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 6bcee2ec91a9..de26638540ba 100644
> >>> --- a/tools/testing/selftests/resctrl/Makefile
> >>> +++ b/tools/testing/selftests/resctrl/Makefile
> >>> @@ -1,17 +1,9 @@
> >>> -CC = $(CROSS_COMPILE)gcc
> >>> -CFLAGS = -g -Wall -O2 -D_FORTIFY_SOURCE=2 -SRCS=$(wildcard *.c)
> >>> -OBJS=$(SRCS:.c=.o)
> >>> +CFLAGS += -g -Wall -O2 -D_FORTIFY_SOURCE=2
> >>>
> >>> -all: resctrl_tests
> >>> +TEST_GEN_PROGS := resctrl_tests
> >>> +SRCS := $(wildcard *.c)
> >>>
> >>> -$(OBJS): $(SRCS)
> >>> -	$(CC) $(CFLAGS) -c $(SRCS)
> >>> +all: $(TEST_GEN_PROGS)
> >>>
> >>> -resctrl_tests: $(OBJS)
> >>> -	$(CC) $(CFLAGS) -o $@ $^
> >>> -
> >>> -.PHONY: clean
> >>> -
> >>> -clean:
> >>> -	$(RM) $(OBJS) resctrl_tests
> >>> +$(TEST_GEN_PROGS): $(SRCS)
> >>
> >> This patch breaks the test build - the below use-cases fail
> >>
> >> make kselftest-all TARGETS=resctrl
> >> make -C  tools/testing/selftests/ TARGETS=resctrl
> >>
> >> Also a simple make in tools/testing/selftests/resctr
> >
> > Thanks for your feedbacks.
> > I applied these patches to the source below and built resctrl_tests
> > successfully using above use-cases on x86/arm machine.
> > (1)
> >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> >   Tag: v5.16
> > (2)
> >   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> >   Tag: next-20220217
> >
> > Could you tell me which kernel source you used to build and what error
> > message you got?
> >
> 
> I tried this on Linux 5.17-rc4

I tried these patches on Linux 5.17-rc4 with gcc version 8.4.1 
and resctrl_tests is still built successfully.

Could you tell me what error message you got when you built it?

Best regards,
Tan Shaopeng

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

[tan.shaopeng]

Hi Shuah,

> On 2/15/22 7:26 PM, Shaopeng Tan wrote:
> > Hello,
> >
> > The aim of this series is to make resctrl_tests run by using kselftest
> > framework.
> > - I modify resctrl_test Makefile and kselftest Makefile,
> >    to enable build/run resctrl_tests by using kselftest framework.
> >    Of course, users can also build/run resctrl_tests without
> >    using framework as before.
> > - I change the default limited time for resctrl_tests to 120 seconds, to
> >    ensure the resctrl_tests finish in limited time on different environments.
> > - When resctrl file system is not supported by environment or
> >    resctrl_tests is not run as root, return skip code of kselftest framework.
> > - If resctrl_tests does not finish in limited time, terminate it as
> >    same as executing ctrl+c that kills parent process and child process.
> >
> > Difference from v2:
> > - I reworte changelog of this patch series.
> > - I added how to use framework to run resctrl to README. [PATCH v3
> > 2/5]
> > - License has no dependencies on this patch series, I separated from it this
> patch series to another patch.
> > https://lore.kernel.org/lkml/20211213100154.180599-1-tan.shaopeng@jp.f
> > ujitsu.com/
> >
> > With regard to the limited time, I think 120s is not a problem since
> > some tests have a longer timeout (e.g. net test is 300s). Please let me know if
> this is wrong.
> >
> > Thanks,
> >
> > Shaopeng Tan (5):
> >    selftests/resctrl: Kill child process before parent process terminates
> >      if SIGTERM is received
> >    selftests/resctrl: Make resctrl_tests run using kselftest framework
> >    selftests/resctrl: Update README about using kselftest framework to
> >      build/run resctrl_tests
> >    selftests/resctrl: Change the default limited time to 120 seconds
> >    selftests/resctrl: Fix resctrl_tests' return code to work with
> >      selftest framework
> >
> >   tools/testing/selftests/Makefile              |  1 +
> >   tools/testing/selftests/resctrl/Makefile      | 20 ++++-------
> >   tools/testing/selftests/resctrl/README        | 34
> +++++++++++++++++++
> >   .../testing/selftests/resctrl/resctrl_tests.c |  4 +--
> >   tools/testing/selftests/resctrl/resctrl_val.c |  1 +
> >   tools/testing/selftests/resctrl/settings      |  1 +
> >   6 files changed, 45 insertions(+), 16 deletions(-)
> >   create mode 100644 tools/testing/selftests/resctrl/settings
> >
> 
> Reviewed the patches - patches 1/5, 4/5 & 5/5 don't depend on kselftest
> framework improvements. 2/5 and 3/5 are.
> 
> Please reorder the patches - move 4/5 and 5/5 up and make 2/5 and 3/5 the
> last in this series. Also see comments on individual patches.

Ok, I will reorder all patches as follows, so that independent patches come first
and Makefile related patches come last:
[PATCH 1/5] selftests/resctrl: Kill child process before parent process terminates if SIGTERM is received
[PATCH 4/5] selftests/resctrl: Change the default limited time to 120 seconds
[PATCH 5/5] selftests/resctrl: Fix resctrl_tests' return code to work with selftest framework
[PATCH 2/5] selftests/resctrl: Make resctrl_tests run using kselftest framework
[PATCH 3/5] selftests/resctrl: Update README about using kselftest framework to build/run resctrl_tests
[PATCH] selftests/resctrl: Add missing SPDX license to Makefile

Please let me know if I'm wrong.

Best regards,
Tan Shaopeng

Re: [PATCH v3 2/5] selftests/resctrl: Make resctrl_tests run using kselftest framework

[Shuah Khan]

On 2/25/22 1:02 AM, tan.shaopeng@fujitsu.com wrote:
> Hi Shuah,
> 
>> On 2/22/22 12:55 AM, tan.shaopeng@fujitsu.com wrote:
>>> Hi Khan,
>>>
>>>> On 2/15/22 7:26 PM, Shaopeng Tan wrote:
>>>>> In kselftest framework, all tests can be build/run at a time, and a
>>>>> sub test also can be build/run individually. As follows:
>>>>> $ make -C tools/testing/selftests run_tests $ make -C
>>>>> tools/testing/selftests TARGETS=ptrace run_tests
>>>>>
>>>>> However, resctrl_tests cannot be run using kselftest framework,
>>>>> users have to change directory to tools/testing/selftests/resctrl/,
>>>>> run "make" to build executable file "resctrl_tests", and run "sudo
>>>>> ./resctrl_tests" to execute the test.
>>>>>
>>>>> To build/run resctrl_tests using kselftest framework.
>>>>> Modify tools/testing/selftests/Makefile and
>>>>> tools/testing/selftests/resctrl/Makefile.
>>>>>
>>>>> Even after this change, users can still build/run resctrl_tests
>>>>> without using framework as before.
>>>>>
>>>>> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
>>>>> ---
>>>>> Some important feedbacks from v1&v2 are addressed as follows:
>>>>>
>>>>> - The changelog mentions that changes were made to the resctrl
>>>>>      selftest Makefile but it does not describe what the change
>> accomplish
>>>>>      or why they are needed.
>>>>>      => By changing the Makefile, resctrl_tests can use kselftest
>>>>>         framework like other sub tests. I described this in changelog.
>>>>>
>>>>> - The changelog did not describe how a user may use the kselftest
>>>>>      framework to run the resctrl tests nor the requested information
>>>>>      on how existing workflows are impacted.
>>>>>      => I described how to build/run resctrl_tests with kselftest
>> framework,
>>>>>         and described the existing workflows are not impacted that users
>> can
>>>>>         build/run resctrl_tests without using kselftest framework as
>> before.
>>>>>
>>>>> - tools/testing/selftests/resctrl/README should be updated.
>>>>>      => I separate the update of README to a new patch.[patch v3 3/5]
>>>>>
>>>>> - Why is the meaning of "EXTRA_SOURCES" (i.e. what is "extra"?) and
>>>>>      why is "SRCS" no longer sufficient?
>>>>>      => I referred to other Makefiles, and found "SRCS" is better
>>>>>         than "EXTRA_SOURCES". So, I updated it to use "SRCS".
>>>>>
>>>>>     tools/testing/selftests/Makefile         |  1 +
>>>>>     tools/testing/selftests/resctrl/Makefile | 20 ++++++--------------
>>>>>     2 files changed, 7 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/tools/testing/selftests/Makefile
>>>>> b/tools/testing/selftests/Makefile
>>>>> index c852eb40c4f7..7df397c6893c 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 6bcee2ec91a9..de26638540ba 100644
>>>>> --- a/tools/testing/selftests/resctrl/Makefile
>>>>> +++ b/tools/testing/selftests/resctrl/Makefile
>>>>> @@ -1,17 +1,9 @@
>>>>> -CC = $(CROSS_COMPILE)gcc
>>>>> -CFLAGS = -g -Wall -O2 -D_FORTIFY_SOURCE=2 -SRCS=$(wildcard *.c)
>>>>> -OBJS=$(SRCS:.c=.o)
>>>>> +CFLAGS += -g -Wall -O2 -D_FORTIFY_SOURCE=2
>>>>>
>>>>> -all: resctrl_tests
>>>>> +TEST_GEN_PROGS := resctrl_tests
>>>>> +SRCS := $(wildcard *.c)
>>>>>
>>>>> -$(OBJS): $(SRCS)
>>>>> -	$(CC) $(CFLAGS) -c $(SRCS)
>>>>> +all: $(TEST_GEN_PROGS)
>>>>>
>>>>> -resctrl_tests: $(OBJS)
>>>>> -	$(CC) $(CFLAGS) -o $@ $^
>>>>> -
>>>>> -.PHONY: clean
>>>>> -
>>>>> -clean:
>>>>> -	$(RM) $(OBJS) resctrl_tests
>>>>> +$(TEST_GEN_PROGS): $(SRCS)
>>>>
>>>> This patch breaks the test build - the below use-cases fail
>>>>
>>>> make kselftest-all TARGETS=resctrl
>>>> make -C  tools/testing/selftests/ TARGETS=resctrl
>>>>
>>>> Also a simple make in tools/testing/selftests/resctr
>>>
>>> Thanks for your feedbacks.
>>> I applied these patches to the source below and built resctrl_tests
>>> successfully using above use-cases on x86/arm machine.
>>> (1)
>>>    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>>>    Tag: v5.16
>>> (2)
>>>    https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
>>>    Tag: next-20220217
>>>
>>> Could you tell me which kernel source you used to build and what error
>>> message you got?
>>>
>>
>> I tried this on Linux 5.17-rc4
> 
> I tried these patches on Linux 5.17-rc4 with gcc version 8.4.1
> and resctrl_tests is still built successfully.
> 
> Could you tell me what error message you got when you built it?

Here it is:

make
gcc   resctrl_tests.o cache.c cat_test.c cmt_test.c fill_buf.c mba_test.c mbm_test.c resctrlfs.c resctrl_tests.c resctrl_val.c   -o resctrl_tests
/usr/bin/ld: /tmp/ccoarGr4.o:(.bss+0x0): multiple definition of `is_amd'; resctrl_tests.o:/linux/linux_5.17/tools/testing/selftests/resctrl/resctrl_tests.c:16: first defined here
/usr/bin/ld: /tmp/ccoarGr4.o: in function `detect_amd':
resctrl_tests.c:(.text+0x63b): multiple definition of `detect_amd'; resctrl_tests.o:/linux/linux_5.17/tools/testing/selftests/resctrl/resctrl_tests.c:19: first defined here
/usr/bin/ld: /tmp/ccoarGr4.o: in function `tests_cleanup':
resctrl_tests.c:(.text+0x780): multiple definition of `tests_cleanup'; resctrl_tests.o:/linux/linux_5.17/tools/testing/selftests/resctrl/resctrl_tests.c:50: first defined here
/usr/bin/ld: /tmp/ccoarGr4.o: in function `main':
resctrl_tests.c:(.text+0xadd): multiple definition of `main'; resctrl_tests.o:/linux/linux_5.17/tools/testing/selftests/resctrl/resctrl_tests.c:129: first defined here
collect2: error: ld returned 1 exit status
make: *** [<builtin>: resctrl_tests] Error 1

I have gcc (Ubuntu 11.2.0-7ubuntu2) 11.2.0

Take a look at the changes to
tools/testing/selftests/resctrl/Makefile

I don't think you need to make the changes you made. I would start
small with including lib.mk and work from there.

thanks,
-- Shuah

Re: [PATCH v3 0/5] selftests/resctrl: Add resctrl_tests into kselftest set

[Shuah Khan]

On 2/25/22 1:03 AM, tan.shaopeng@fujitsu.com wrote:
> Hi Shuah,
> 
>> On 2/15/22 7:26 PM, Shaopeng Tan wrote:
>>> Hello,
>>>
>>> The aim of this series is to make resctrl_tests run by using kselftest
>>> framework.
>>> - I modify resctrl_test Makefile and kselftest Makefile,
>>>     to enable build/run resctrl_tests by using kselftest framework.
>>>     Of course, users can also build/run resctrl_tests without
>>>     using framework as before.
>>> - I change the default limited time for resctrl_tests to 120 seconds, to
>>>     ensure the resctrl_tests finish in limited time on different environments.
>>> - When resctrl file system is not supported by environment or
>>>     resctrl_tests is not run as root, return skip code of kselftest framework.
>>> - If resctrl_tests does not finish in limited time, terminate it as
>>>     same as executing ctrl+c that kills parent process and child process.
>>>
>>> Difference from v2:
>>> - I reworte changelog of this patch series.
>>> - I added how to use framework to run resctrl to README. [PATCH v3
>>> 2/5]
>>> - License has no dependencies on this patch series, I separated from it this
>> patch series to another patch.
>>> https://lore.kernel.org/lkml/20211213100154.180599-1-tan.shaopeng@jp.f
>>> ujitsu.com/
>>>
>>> With regard to the limited time, I think 120s is not a problem since
>>> some tests have a longer timeout (e.g. net test is 300s). Please let me know if
>> this is wrong.
>>>
>>> Thanks,
>>>
>>> Shaopeng Tan (5):
>>>     selftests/resctrl: Kill child process before parent process terminates
>>>       if SIGTERM is received
>>>     selftests/resctrl: Make resctrl_tests run using kselftest framework
>>>     selftests/resctrl: Update README about using kselftest framework to
>>>       build/run resctrl_tests
>>>     selftests/resctrl: Change the default limited time to 120 seconds
>>>     selftests/resctrl: Fix resctrl_tests' return code to work with
>>>       selftest framework
>>>
>>>    tools/testing/selftests/Makefile              |  1 +
>>>    tools/testing/selftests/resctrl/Makefile      | 20 ++++-------
>>>    tools/testing/selftests/resctrl/README        | 34
>> +++++++++++++++++++
>>>    .../testing/selftests/resctrl/resctrl_tests.c |  4 +--
>>>    tools/testing/selftests/resctrl/resctrl_val.c |  1 +
>>>    tools/testing/selftests/resctrl/settings      |  1 +
>>>    6 files changed, 45 insertions(+), 16 deletions(-)
>>>    create mode 100644 tools/testing/selftests/resctrl/settings
>>>
>>
>> Reviewed the patches - patches 1/5, 4/5 & 5/5 don't depend on kselftest
>> framework improvements. 2/5 and 3/5 are.
>>
>> Please reorder the patches - move 4/5 and 5/5 up and make 2/5 and 3/5 the
>> last in this series. Also see comments on individual patches.
> 
> Ok, I will reorder all patches as follows, so that independent patches come first
> and Makefile related patches come last:
> [PATCH 1/5] selftests/resctrl: Kill child process before parent process terminates if SIGTERM is received
> [PATCH 4/5] selftests/resctrl: Change the default limited time to 120 seconds
> [PATCH 5/5] selftests/resctrl: Fix resctrl_tests' return code to work with selftest framework
> [PATCH 2/5] selftests/resctrl: Make resctrl_tests run using kselftest framework
> [PATCH 3/5] selftests/resctrl: Update README about using kselftest framework to build/run resctrl_tests
> [PATCH] selftests/resctrl: Add missing SPDX license to Makefile
> 
> Please let me know if I'm wrong.
> 

This split looks good to me.

thanks,
-- Shuah

RE: [PATCH v3 2/5] selftests/resctrl: Make resctrl_tests run using kselftest framework

[tan.shaopeng]

Hi Shuah,

> On 2/25/22 1:02 AM, tan.shaopeng@fujitsu.com wrote:
> > Hi Shuah,
> >
> >> On 2/22/22 12:55 AM, tan.shaopeng@fujitsu.com wrote:
> >>> Hi Khan,
> >>>
> >>>> On 2/15/22 7:26 PM, Shaopeng Tan wrote:
> >>>>> In kselftest framework, all tests can be build/run at a time, and
> >>>>> a sub test also can be build/run individually. As follows:
> >>>>> $ make -C tools/testing/selftests run_tests $ make -C
> >>>>> tools/testing/selftests TARGETS=ptrace run_tests
> >>>>>
> >>>>> However, resctrl_tests cannot be run using kselftest framework,
> >>>>> users have to change directory to
> >>>>> tools/testing/selftests/resctrl/, run "make" to build executable
> >>>>> file "resctrl_tests", and run "sudo ./resctrl_tests" to execute the test.
> >>>>>
> >>>>> To build/run resctrl_tests using kselftest framework.
> >>>>> Modify tools/testing/selftests/Makefile and
> >>>>> tools/testing/selftests/resctrl/Makefile.
> >>>>>
> >>>>> Even after this change, users can still build/run resctrl_tests
> >>>>> without using framework as before.
> >>>>>
> >>>>> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> >>>>> ---
> >>>>> Some important feedbacks from v1&v2 are addressed as follows:
> >>>>>
> >>>>> - The changelog mentions that changes were made to the resctrl
> >>>>>      selftest Makefile but it does not describe what the change
> >> accomplish
> >>>>>      or why they are needed.
> >>>>>      => By changing the Makefile, resctrl_tests can use kselftest
> >>>>>         framework like other sub tests. I described this in changelog.
> >>>>>
> >>>>> - The changelog did not describe how a user may use the kselftest
> >>>>>      framework to run the resctrl tests nor the requested information
> >>>>>      on how existing workflows are impacted.
> >>>>>      => I described how to build/run resctrl_tests with kselftest
> >> framework,
> >>>>>         and described the existing workflows are not impacted that
> >>>>> users
> >> can
> >>>>>         build/run resctrl_tests without using kselftest framework
> >>>>> as
> >> before.
> >>>>>
> >>>>> - tools/testing/selftests/resctrl/README should be updated.
> >>>>>      => I separate the update of README to a new patch.[patch v3
> >>>>> 3/5]
> >>>>>
> >>>>> - Why is the meaning of "EXTRA_SOURCES" (i.e. what is "extra"?) and
> >>>>>      why is "SRCS" no longer sufficient?
> >>>>>      => I referred to other Makefiles, and found "SRCS" is better
> >>>>>         than "EXTRA_SOURCES". So, I updated it to use "SRCS".
> >>>>>
> >>>>>     tools/testing/selftests/Makefile         |  1 +
> >>>>>     tools/testing/selftests/resctrl/Makefile | 20
> ++++++--------------
> >>>>>     2 files changed, 7 insertions(+), 14 deletions(-)
> >>>>>
> >>>>> diff --git a/tools/testing/selftests/Makefile
> >>>>> b/tools/testing/selftests/Makefile
> >>>>> index c852eb40c4f7..7df397c6893c 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 6bcee2ec91a9..de26638540ba 100644
> >>>>> --- a/tools/testing/selftests/resctrl/Makefile
> >>>>> +++ b/tools/testing/selftests/resctrl/Makefile
> >>>>> @@ -1,17 +1,9 @@
> >>>>> -CC = $(CROSS_COMPILE)gcc
> >>>>> -CFLAGS = -g -Wall -O2 -D_FORTIFY_SOURCE=2
> -SRCS=$(wildcard *.c)
> >>>>> -OBJS=$(SRCS:.c=.o)
> >>>>> +CFLAGS += -g -Wall -O2 -D_FORTIFY_SOURCE=2
> >>>>>
> >>>>> -all: resctrl_tests
> >>>>> +TEST_GEN_PROGS := resctrl_tests
> >>>>> +SRCS := $(wildcard *.c)
> >>>>>
> >>>>> -$(OBJS): $(SRCS)
> >>>>> -	$(CC) $(CFLAGS) -c $(SRCS)
> >>>>> +all: $(TEST_GEN_PROGS)
> >>>>>
> >>>>> -resctrl_tests: $(OBJS)
> >>>>> -	$(CC) $(CFLAGS) -o $@ $^
> >>>>> -
> >>>>> -.PHONY: clean
> >>>>> -
> >>>>> -clean:
> >>>>> -	$(RM) $(OBJS) resctrl_tests
> >>>>> +$(TEST_GEN_PROGS): $(SRCS)
> >>>>
> >>>> This patch breaks the test build - the below use-cases fail
> >>>>
> >>>> make kselftest-all TARGETS=resctrl
> >>>> make -C  tools/testing/selftests/ TARGETS=resctrl
> >>>>
> >>>> Also a simple make in tools/testing/selftests/resctr
> >>>
> >>> Thanks for your feedbacks.
> >>> I applied these patches to the source below and built resctrl_tests
> >>> successfully using above use-cases on x86/arm machine.
> >>> (1)
> >>>    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> >>>    Tag: v5.16
> >>> (2)
> >>>    https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> >>>    Tag: next-20220217
> >>>
> >>> Could you tell me which kernel source you used to build and what
> >>> error message you got?
> >>>
> >>
> >> I tried this on Linux 5.17-rc4
> >
> > I tried these patches on Linux 5.17-rc4 with gcc version 8.4.1 and
> > resctrl_tests is still built successfully.
> >
> > Could you tell me what error message you got when you built it?
> 
> Here it is:
> 
> make
> gcc   resctrl_tests.o cache.c cat_test.c cmt_test.c fill_buf.c mba_test.c
> mbm_test.c resctrlfs.c resctrl_tests.c resctrl_val.c   -o resctrl_tests
> /usr/bin/ld: /tmp/ccoarGr4.o:(.bss+0x0): multiple definition of `is_amd';
> resctrl_tests.o:/linux/linux_5.17/tools/testing/selftests/resctrl/resctrl_tests.c
> :16: first defined here
> /usr/bin/ld: /tmp/ccoarGr4.o: in function `detect_amd':
> resctrl_tests.c:(.text+0x63b): multiple definition of `detect_amd';
> resctrl_tests.o:/linux/linux_5.17/tools/testing/selftests/resctrl/resctrl_tests.c
> :19: first defined here
> /usr/bin/ld: /tmp/ccoarGr4.o: in function `tests_cleanup':
> resctrl_tests.c:(.text+0x780): multiple definition of `tests_cleanup';
> resctrl_tests.o:/linux/linux_5.17/tools/testing/selftests/resctrl/resctrl_tests.c
> :50: first defined here
> /usr/bin/ld: /tmp/ccoarGr4.o: in function `main':
> resctrl_tests.c:(.text+0xadd): multiple definition of `main';
> resctrl_tests.o:/linux/linux_5.17/tools/testing/selftests/resctrl/resctrl_tests.c
> :129: first defined here
> collect2: error: ld returned 1 exit status
> make: *** [<builtin>: resctrl_tests] Error 1

I think resctrl_tests.o is an old file you left before applying this patch. 
Please remove it before rebuilding.

Before applying this patch, the resctrl_tests.o file was generated and 
saved in the current directory(tools/testing/selftests/resctrl/).
After applying this patch, the resctrl_tests.o file was not generated and
the compilation and linking work is done at one time by gcc.
These errors were caused by the previously left resctrl_tests.o file.

> I have gcc (Ubuntu 11.2.0-7ubuntu2) 11.2.0
> 
> Take a look at the changes to
> tools/testing/selftests/resctrl/Makefile
> 
> I don't think you need to make the changes you made. I would start small with
> including lib.mk and work from there.

Thanks for your advice.
Only the following codes can build resctrl_tests.
 + TEST_GEN_PROGS := resctrl_tests
 + include ../lib.mk
 + $(OUTPUT)/resctrl_tests: $(wildcard *.c)
I will use these codes in next version(v4).

Best regards,
Tan Shaopeng