| Message ID | 20230911111930.16088-2-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Fixes to failing tests | expand |
Hi Ilpo, 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(); > 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? Reinette
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(); > > 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.
Hi Ilpo, 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(); >>> 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? Reinette
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(); > >>> 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.
Hi Ilpo, On 9/14/2023 3:16 AM, Ilpo Järvinen wrote: > 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(); >>>>> 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. > hmmm ... I did not expect that a total refactoring would be needed. I was thinking about a change from this: testX(...) { res = signal_handler_register(); /* error handling */ res = mount_resctrlfs(); /* error handling */ /* test */ unmount_resctrlfs(); signal_handler_register(); } to this: int test_setup(...) { res = signal_handler_register(); /* error handling */ res = mount_resctrlfs(); /* error handling */ } void test_cleanup(...) { unmount_resctrlfs(); signal_handler_register(); } testX(...) { res = test_setup(..); /* error handling */ /* test */ test_cleanup(); } I expect this to also support the bigger refactoring. Reinette
On Thu, 14 Sep 2023, Reinette Chatre wrote: > On 9/14/2023 3:16 AM, Ilpo Järvinen wrote: > > 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(); > >>>>> 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. > > > > hmmm ... I did not expect that a total refactoring would be needed. > > I was thinking about a change from this: > > > testX(...) > { > > res = signal_handler_register(); > /* error handling */ > res = mount_resctrlfs(); > /* error handling */ > > /* test */ > > unmount_resctrlfs(); > signal_handler_register(); > > } > > > to this: > > > int test_setup(...) > { > res = signal_handler_register(); > /* error handling */ > res = mount_resctrlfs(); > /* error handling */ > } > > > void test_cleanup(...) > { > unmount_resctrlfs(); > signal_handler_register(); > } > > > testX(...) > { > > res = test_setup(..); > /* error handling */ > > /* test */ > > test_cleanup(); > } > > I expect this to also support the bigger refactoring. Okay, I'll do so then. However, having already written the generic run_single_test() function that is part of the generic test framework, I definitely don't feel those helpers would be that helpful for it. It more feels like they'd make the flow less obvious by adding those two extra calls there but that's of course matter of taste.
Hi Ilpo, On 9/14/2023 10:05 AM, Ilpo Järvinen wrote: > On Thu, 14 Sep 2023, Reinette Chatre wrote: >> On 9/14/2023 3:16 AM, Ilpo Järvinen wrote: >>> 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(); >>>>>>> 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. >>> >> >> hmmm ... I did not expect that a total refactoring would be needed. >> >> I was thinking about a change from this: >> >> >> testX(...) >> { >> >> res = signal_handler_register(); >> /* error handling */ >> res = mount_resctrlfs(); >> /* error handling */ >> >> /* test */ >> >> unmount_resctrlfs(); >> signal_handler_register(); >> >> } >> >> >> to this: >> >> >> int test_setup(...) >> { >> res = signal_handler_register(); >> /* error handling */ >> res = mount_resctrlfs(); >> /* error handling */ >> } >> >> >> void test_cleanup(...) >> { >> unmount_resctrlfs(); >> signal_handler_register(); >> } >> >> >> testX(...) >> { >> >> res = test_setup(..); >> /* error handling */ >> >> /* test */ >> >> test_cleanup(); >> } >> >> I expect this to also support the bigger refactoring. > > Okay, I'll do so then. > > However, having already written the generic run_single_test() function > that is part of the generic test framework, I definitely don't feel those > helpers would be that helpful for it. It more feels like they'd make the > flow less obvious by adding those two extra calls there but that's of > course matter of taste. Sounds like there is some room for improvement here, perhaps open coding the test_setup() and test_cleanup() helpers within run_single_test(). This is purely speculation on my part as I have not seen the code. Reinette
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(); } static void run_cmt_test(const char * const *benchmark_cmd, int cpu_no) @@ -123,8 +135,13 @@ static void run_cmt_test(const char * const *benchmark_cmd, int cpu_no) ksft_print_msg("Starting CMT test ...\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; } @@ -141,6 +158,7 @@ static void run_cmt_test(const char * const *benchmark_cmd, int cpu_no) umount: umount_resctrlfs(); + signal_handler_unregister(); } static void run_cat_test(int cpu_no, int no_of_bits) @@ -149,8 +167,13 @@ static void run_cat_test(int cpu_no, int no_of_bits) ksft_print_msg("Starting CAT test ...\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; } @@ -165,6 +188,7 @@ static void run_cat_test(int cpu_no, int no_of_bits) umount: umount_resctrlfs(); + signal_handler_unregister(); } int main(int argc, char **argv) diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index 51963a6f2186..a9fe61133119 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -468,7 +468,9 @@ pid_t bm_pid, ppid; void ctrlc_handler(int signum, siginfo_t *info, void *ptr) { - kill(bm_pid, SIGKILL); + /* Only kill child after bm_pid is set after fork() */ + if (bm_pid) + kill(bm_pid, SIGKILL); umount_resctrlfs(); tests_cleanup(); ksft_print_msg("Ending\n\n"); @@ -485,6 +487,8 @@ int signal_handler_register(void) struct sigaction sigact; int ret = 0; + bm_pid = 0; + sigact.sa_sigaction = ctrlc_handler; sigemptyset(&sigact.sa_mask); sigact.sa_flags = SA_SIGINFO; @@ -706,10 +710,6 @@ int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *par ksft_print_msg("Benchmark PID: %d\n", bm_pid); - ret = signal_handler_register(); - if (ret) - goto out; - /* * The cast removes constness but nothing mutates benchmark_cmd within * the context of this process. At the receiving process, it becomes @@ -721,19 +721,19 @@ int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *par /* Taskset benchmark to specified cpu */ ret = taskset_benchmark(bm_pid, param->cpu_no); if (ret) - goto unregister; + goto out; /* Write benchmark to specified control&monitoring grp in resctrl FS */ ret = write_bm_pid_to_resctrl(bm_pid, param->ctrlgrp, param->mongrp, resctrl_val); if (ret) - goto unregister; + goto out; if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) || !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) { ret = initialize_mem_bw_imc(); if (ret) - goto unregister; + goto out; initialize_mem_bw_resctrl(param->ctrlgrp, param->mongrp, param->cpu_no, resctrl_val); @@ -748,7 +748,7 @@ int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *par sizeof(pipe_message)) { perror("# failed reading message from child process"); close(pipefd[0]); - goto unregister; + goto out; } } close(pipefd[0]); @@ -757,7 +757,7 @@ int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *par if (sigqueue(bm_pid, SIGUSR1, value) == -1) { perror("# sigqueue SIGUSR1 to child"); ret = errno; - goto unregister; + goto out; } /* Give benchmark enough time to fully run */ @@ -786,8 +786,6 @@ int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *par } } -unregister: - signal_handler_unregister(); out: kill(bm_pid, SIGKILL);
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. 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(-)