| Message ID | 20220216022641.2998318-1-tan.shaopeng@jp.fujitsu.com |
|---|---|
| Headers | show |
| Series | selftests/resctrl: Add resctrl_tests into kselftest set | expand |
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
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
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
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
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
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
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
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