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@xxxxxxxxxxxxxxx>
> >>> Cc: <stable@xxxxxxxxxxxxxxx>
> >>> ---
> >>> 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();
> >>> ksft_exit_fail_msg("Failed to mount resctrl FS\n");
> >>> return;
> >>> }
> >>> @@ -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();
> >>> ksft_exit_fail_msg("Failed to mount resctrl FS\n");
> >>> return;
> >>> }
> >>> @@ -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.
--
i.
