On Wed, 13 Sep 2023, Reinette Chatre wrote:
On 9/13/2023 3:01 AM, Ilpo Järvinen wrote:
On Tue, 12 Sep 2023, Reinette Chatre wrote:
On 9/11/2023 4:19 AM, Ilpo Järvinen wrote:
Unmounting resctrl FS has been moved into the per test functions in resctrl_tests.c by commit caddc0fbe495 ("selftests/resctrl: Move resctrl FS mount/umount to higher level"). In case a signal (SIGINT, SIGTERM, or SIGHUP) is received, the running selftest is aborted by ctrlc_handler() which then unmounts resctrl fs before exiting. The current section between signal_handler_register() and signal_handler_unregister(), however, does not cover the entire duration when resctrl FS is mounted.
Move signal_handler_register() and signal_handler_unregister() call into the test functions in resctrl_tests.c to properly unmount resctrl fs. Adjust child process kill() call in ctrlc_handler() to only be invoked if the child was already forked.
Thank you for catching this.
Fixes: caddc0fbe495 ("selftests/resctrl: Move resctrl FS mount/umount to higher level") Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Cc: stable@vger.kernel.org
tools/testing/selftests/resctrl/cat_test.c | 8 ------- .../testing/selftests/resctrl/resctrl_tests.c | 24 +++++++++++++++++++ tools/testing/selftests/resctrl/resctrl_val.c | 22 ++++++++--------- 3 files changed, 34 insertions(+), 20 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 97b87285ab2a..224ba8544d8a 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -167,12 +167,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) strcpy(param.filename, RESULT_FILE_NAME1); param.num_of_runs = 0; param.cpu_no = sibling_cpu_no;
- } else {
ret = signal_handler_register();if (ret) {kill(bm_pid, SIGKILL);goto out;}remove(param.filename); @@ -209,10 +203,8 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) } close(pipefd[0]); kill(bm_pid, SIGKILL);
signal_handler_unregister();-out: cat_test_cleanup(); return ret; diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 823672a20a43..3d66fbdc2df3 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -73,8 +73,13 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no) ksft_print_msg("Starting MBM BW change ...\n");
- res = signal_handler_register();
- if (res)
return;- res = mount_resctrlfs(); if (res) {
signal_handler_unregister();@@ -91,6 +96,7 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no) umount: umount_resctrlfs();
- signal_handler_unregister();
} static void run_mba_test(const char * const *benchmark_cmd, int cpu_no) @@ -99,8 +105,13 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no) ksft_print_msg("Starting MBA Schemata change ...\n");
- res = signal_handler_register();
- if (res)
return;- res = mount_resctrlfs(); if (res) {
signal_handler_unregister();@@ -115,6 +126,7 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no) umount: umount_resctrlfs();
- signal_handler_unregister();
}
This adds more duplicated code for every test. Have you considered a single test setup function that can be used to mount resctrl FS and setup the signal handler paired with a single test teardown function?
Yes. Consolidating all these is among my not-yet submitted patches. I just had to do a backport-friendly Fixes patch first for this.
Could you please help me understand how the duplicate calls are more backport friendly?
Hi,
It's simply because the refactoring that has to be done to be able to introduce the generalized test framework is much more invasive and far reaching than this patch. Essentially, all the call signatures of the test functions need to match and the feature checks need to be done in new per test functions too. This is the diffstat of those changes alone:
tools/testing/selftests/resctrl/cat_test.c | 21 +++-- tools/testing/selftests/resctrl/cmt_test.c | 26 +++-- tools/testing/selftests/resctrl/mba_test.c | 20 +++- tools/testing/selftests/resctrl/mbm_test.c | 20 +++- tools/testing/selftests/resctrl/resctrl.h | 43 ++++++++- tools/testing/selftests/resctrl/resctrl_tests.c | 220 +++++++++++++++---------------------------- tools/testing/selftests/resctrl/resctrlfs.c | 5 +
(tools/testing/selftests/resctrl/resctrl_tests.c --- part would be slightly less if I'd reorder this patch but that only 24 lines off as per diffstat of this patch).
But that's not all.... To be able to push the generalized test framework to stable, you need to also count in the benchmark cmd changes which worked towards making the call signatures identical. So here's the diffstat for that series for quick reference:
tools/testing/selftests/resctrl/cache.c | 5 +- tools/testing/selftests/resctrl/cat_test.c | 13 +-- tools/testing/selftests/resctrl/cmt_test.c | 34 ++++-- tools/testing/selftests/resctrl/mba_test.c | 4 +- tools/testing/selftests/resctrl/mbm_test.c | 7 +- tools/testing/selftests/resctrl/resctrl.h | 16 +-- .../testing/selftests/resctrl/resctrl_tests.c | 100 ++++++++---------- tools/testing/selftests/resctrl/resctrl_val.c | 10 +-
That's ~500 lines changed vs ~50 so it's a magnitude worse and much less localized.
And rest assured, I did not like introducing the duplicated calls any more than you do (I did not write the generalized test framework for nothing, after all) but the way taken in this patch seemed the most reasonable option under these circumstances.