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@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.
>
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
