Hello,
The aim of this series is to make resctrl_tests run by using
kselftest framework.
- I modify Makefile of resctrl_test and Makefile of selftest, to
build/run resctrl_tests by using kselftest framework.
- I set the limited time for resctrl_tests to 120 seconds, to ensure the
resctrl_tests finish in limited time.
- When resctrl file system 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
executing ctrl+c.
Difference from v1:
- I change the order of patches according to Reinette's review.
- "LDLIBS + = -lnuma" has no dependencies on this patch series, delete
it from [PATCH v2 2/5].
- I separate the license info of Makefile into a new patch [PATCH v2
3/5].
- I separate "limited time" into a new patch [PATCH v2 4/5].
(There is no change in [PATCH v2 1/5] and [PATCH v2 5/5])
In addition, I think 120s is not a problem since some tests have longer
timeout (e.g. net test is 300s), please let me know if this is wrong
Thanks,
Shaopeng Tan (5):
selftests/resctrl: Kill the child process created by fork() when the
SIGTERM signal comes
selftests/resctrl: Make resctrl_tests run using kselftest framework
selftests/resctrl: Add license to resctrl_test Makefile
selftests/resctrl: Change default limited time to 120 seconds for
resctrl_tests
selftests/resctrl: Return KSFT_SKIP(4) if resctrlfile system is not
supported or resctrl is not run as root
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/resctrl/Makefile | 20 +++++++------------
.../testing/selftests/resctrl/resctrl_tests.c | 4 ++--
tools/testing/selftests/resctrl/resctrl_val.c | 1 +
tools/testing/selftests/resctrl/settings | 1 +
5 files changed, 12 insertions(+), 15 deletions(-)
create mode 100644 tools/testing/selftests/resctrl/settings
--
2.27.0
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 <[email protected]>
---
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
This commit enables resctrl_tests to be built/run in kselftest framework.
Build/run resctrl_tests by building/running all tests of kselftest, or by using
the "TARGETS" variable on the make command line to specify resctrl_tests.
This commit modified the Makefile of kernel kselftest set and
the Makefile of resctrl_tests.
Signed-off-by: Shaopeng Tan <[email protected]>
---
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..c9e8540fc594 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
+EXTRA_SOURCES := $(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): $(EXTRA_SOURCES)
+include ../lib.mk
--
2.27.0
This commit changed the default limited time(45s) to 120 seconds for
resctrl_tests by adding "setting" file. Since 45 seconds may not be
enough on some machine environments.
Signed-off-by: Shaopeng Tan <[email protected]>
---
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
To unify the return code of resctrl_tests with the return code of
selftest set, return KSFT_SKIP (4) if resctrl file system is not
supported or resctrl is not run as root.
Signed-off-by: Shaopeng Tan <[email protected]>
---
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
Add Software Package Data Exchange license information to resctrl_test Makefile.
Signed-off-by: Shaopeng Tan <[email protected]>
---
tools/testing/selftests/resctrl/Makefile | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/testing/selftests/resctrl/Makefile b/tools/testing/selftests/resctrl/Makefile
index c9e8540fc594..adfd92145e12 100644
--- a/tools/testing/selftests/resctrl/Makefile
+++ b/tools/testing/selftests/resctrl/Makefile
@@ -1,3 +1,5 @@
+#SPDX-License-Identifier: GPL-2.0
+
CFLAGS += -g -Wall -O2 -D_FORTIFY_SOURCE=2
TEST_GEN_PROGS := resctrl_tests
--
2.27.0
Hi,
Friendly ping for a review.
>
> Hello,
>
> The aim of this series is to make resctrl_tests run by using kselftest framework.
> - I modify Makefile of resctrl_test and Makefile of selftest, to
> build/run resctrl_tests by using kselftest framework.
> - I set the limited time for resctrl_tests to 120 seconds, to ensure the
> resctrl_tests finish in limited time.
> - When resctrl file system 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
> executing ctrl+c.
>
> Difference from v1:
> - I change the order of patches according to Reinette's review.
> - "LDLIBS + = -lnuma" has no dependencies on this patch series, delete
> it from [PATCH v2 2/5].
> - I separate the license info of Makefile into a new patch [PATCH v2
> 3/5].
> - I separate "limited time" into a new patch [PATCH v2 4/5].
> (There is no change in [PATCH v2 1/5] and [PATCH v2 5/5]) In addition, I think
> 120s is not a problem since some tests have longer timeout (e.g. net test is
> 300s), please let me know if this is wrong
>
> Thanks,
>
> Shaopeng Tan (5):
> selftests/resctrl: Kill the child process created by fork() when the
> SIGTERM signal comes
> selftests/resctrl: Make resctrl_tests run using kselftest framework
> selftests/resctrl: Add license to resctrl_test Makefile
> selftests/resctrl: Change default limited time to 120 seconds for
> resctrl_tests
> selftests/resctrl: Return KSFT_SKIP(4) if resctrlfile system is not
> supported or resctrl is not run as root
>
> tools/testing/selftests/Makefile | 1 +
> tools/testing/selftests/resctrl/Makefile | 20 +++++++------------
> .../testing/selftests/resctrl/resctrl_tests.c | 4 ++--
> tools/testing/selftests/resctrl/resctrl_val.c | 1 +
> tools/testing/selftests/resctrl/settings | 1 +
> 5 files changed, 12 insertions(+), 15 deletions(-) create mode 100644
> tools/testing/selftests/resctrl/settings
>
> --
> 2.27.0
Hi Shaopeng Tan,
On 12/13/2021 2:01 AM, Shaopeng Tan wrote:
> 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
SIGTEM -> SIGTERM ?
> "timeout --foregroup <seconds>" command.
foregroup?
This is a bit confusing though since it mentions that the "timeout" utility
is called after the test times out. Perhaps you can just describe that, if
present, the test is run using the timeout utility and it will
send SIGTERM to the test upon timeout.
> In resctrl_tests, fork() is used to create a child process.
> This commit ensures child process to be killed before parent process
Especially since I know you are planning more changes in the x86 area
where this is enforced more strictly, please do get into the habit of
describing your changes in imperative mood. Please search for
"This patch" in Documentation/process/submitting-patches.rst (your
usage of "This commit" is equivalent to "This patch") for more details.
Please address this in all your patches where "This commit" is frequently
used.
> exiting if SIGTERM signal comes.
>
> Signed-off-by: Shaopeng Tan <[email protected]>
> ---
> 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;
The change looks good. Since this snippet is preceded with a comment
that describes its usage you could also update it with the expanded
use of the kselftest framework.
Reinette
Hi Shaopeng Tan,
On 12/13/2021 2:01 AM, Shaopeng Tan wrote:
> This commit enables resctrl_tests to be built/run in kselftest framework.
(This commit)
> Build/run resctrl_tests by building/running all tests of kselftest, or by using
> the "TARGETS" variable on the make command line to specify resctrl_tests.
Please review the feedback I provided to your first version. I do not
see the changelog improvements that describe how a user may use the
kselftest framework to run the resctrl tests nor the requested information
on how existing workflows are impacted. Going back through my previous review
feedback to ensure that all is addressed is very time consuming and causes
considerable delay when needing to review newer versions.
tools/testing/selftests/resctrl/README should be updated also
> This commit modified the Makefile of kernel kselftest set and
> the Makefile of resctrl_tests.
This is clear from the diffstat. You could elaborate why the changes were
needed instead and do so without the "This commit" usage.
>
> Signed-off-by: Shaopeng Tan <[email protected]>
> ---
> 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..c9e8540fc594 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
> +EXTRA_SOURCES := $(wildcard *.c)
Why is the meaning of "EXTRA_SOURCES" (i.e. what is "extra"?) and
why is "SRCS" no longer sufficient?
>
> -$(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): $(EXTRA_SOURCES)
> +include ../lib.mk
Reinette
Hi Shaopeng Tan,
On 12/13/2021 2:01 AM, Shaopeng Tan wrote:
> Add Software Package Data Exchange license information to resctrl_test Makefile.
>
Considering the patch series, why is this change needed to be able to run resctrl
tests from within the kselftest framework?
> Signed-off-by: Shaopeng Tan <[email protected]>
> ---
> tools/testing/selftests/resctrl/Makefile | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tools/testing/selftests/resctrl/Makefile b/tools/testing/selftests/resctrl/Makefile
> index c9e8540fc594..adfd92145e12 100644
> --- a/tools/testing/selftests/resctrl/Makefile
> +++ b/tools/testing/selftests/resctrl/Makefile
> @@ -1,3 +1,5 @@
> +#SPDX-License-Identifier: GPL-2.0
> +
Please see Documentation/process/license-rules.rst and examples from
other Makefiles in this area.
> CFLAGS += -g -Wall -O2 -D_FORTIFY_SOURCE=2
>
> TEST_GEN_PROGS := resctrl_tests
Reinette
Hi Shaopeng Tan,
On 12/13/2021 2:01 AM, Shaopeng Tan wrote:
> This commit changed the default limited time(45s) to 120 seconds for
(This commit)
> resctrl_tests by adding "setting" file. Since 45 seconds may not be
> enough on some machine environments.
This sounds very imprecise when you say "may not be enough". From what I
understand you have already encountered systems where this is not enough
so you have established that 45 seconds is too short. The next question is,
how was 120 seconds determined and how do you know it will be enough?
>
> Signed-off-by: Shaopeng Tan <[email protected]>
> ---
> 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
Reinette
Hi Shaopeng Tan,
Subject line still with typo. Also, what does "resctrl is not run as root"
mean? Do you mean "resctrl_tests"? The subject line can be made more succinct
while the details are moved to the commit message.
On 12/13/2021 2:01 AM, Shaopeng Tan wrote:
> To unify the return code of resctrl_tests with the return code of
> selftest set, return KSFT_SKIP (4) if resctrl file system is not
> supported or resctrl is not run as root.
Please review the feedback I provided to your first version. I do not
see the explanation of how changing ksft_exit_fail_msg() to
ksft_exit_skip() accomplishes the goal of unifying the return code.
It surely is not a good use of my time to review patches and have the
feedback ignored.
>
> Signed-off-by: Shaopeng Tan <[email protected]>
> ---
> 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();
>
Reinette
Hi Reinette,
> On 12/13/2021 2:01 AM, Shaopeng Tan wrote:
> > This commit changed the default limited time(45s) to 120 seconds for
>
> (This commit)
>
> > resctrl_tests by adding "setting" file. Since 45 seconds may not be
> > enough on some machine environments.
>
> This sounds very imprecise when you say "may not be enough". From what I
> understand you have already encountered systems where this is not enough so
> you have established that 45 seconds is too short. The next question is, how
> was 120 seconds determined and how do you know it will be enough?
It took about 68 seconds in my environment (Intel(R) Xeon(R) Gold 6254 CPU @ 3.10GHz).
This test accesses a fixed size(250M) memory by default settings
which is used when run in kselftest framework.
I think that the execution time does not change significantly in different environments.
So, I roughly doubled the execution time and get 120s, which I set to the limited time.
I think it is enough for any environments.
If 120s are also not enough, user can freely set the limited time by "setting" file.
Thanks for your feedback of this patch series,
I will correct them in a few days.
> > Signed-off-by: Shaopeng Tan <[email protected]>
> > ---
> > 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
Best regards,
Tan Shaopeng
Hi Tan Shaopeng,
On 1/20/2022 11:59 PM, [email protected] wrote:
> Hi Reinette,
>
>> On 12/13/2021 2:01 AM, Shaopeng Tan wrote:
>>> This commit changed the default limited time(45s) to 120 seconds for
>>
>> (This commit)
>>
>>> resctrl_tests by adding "setting" file. Since 45 seconds may not be
>>> enough on some machine environments.
>>
>> This sounds very imprecise when you say "may not be enough". From what I
>> understand you have already encountered systems where this is not enough so
>> you have established that 45 seconds is too short. The next question is, how
>> was 120 seconds determined and how do you know it will be enough?
>
> It took about 68 seconds in my environment (Intel(R) Xeon(R) Gold 6254 CPU @ 3.10GHz).
> This test accesses a fixed size(250M) memory by default settings
> which is used when run in kselftest framework.
> I think that the execution time does not change significantly in different environments.
> So, I roughly doubled the execution time and get 120s, which I set to the limited time.
> I think it is enough for any environments.
> If 120s are also not enough, user can freely set the limited time by "setting" file.
>
How about something like:
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.
Reinette
Hi Reinette,
> On 1/20/2022 11:59 PM, [email protected] wrote:
> > Hi Reinette,
> >
> >> On 12/13/2021 2:01 AM, Shaopeng Tan wrote:
> >>> This commit changed the default limited time(45s) to 120 seconds for
> >>
> >> (This commit)
> >>
> >>> resctrl_tests by adding "setting" file. Since 45 seconds may not be
> >>> enough on some machine environments.
> >>
> >> This sounds very imprecise when you say "may not be enough". From
> >> what I understand you have already encountered systems where this is
> >> not enough so you have established that 45 seconds is too short. The
> >> next question is, how was 120 seconds determined and how do you know it
> will be enough?
> >
> > It took about 68 seconds in my environment (Intel(R) Xeon(R) Gold 6254 CPU
> @ 3.10GHz).
> > This test accesses a fixed size(250M) memory by default settings which
> > is used when run in kselftest framework.
> > I think that the execution time does not change significantly in different
> environments.
> > So, I roughly doubled the execution time and get 120s, which I set to the
> limited time.
> > I think it is enough for any environments.
> > If 120s are also not enough, user can freely set the limited time by "setting"
> file.
> >
>
> How about something like:
>
> 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.
Thanks for your feedback.
I will use it for commit log in next version.
Best regards,
Tan Shaopeng