Re: [PATCH 1/5] selftests/resctrl: Extend signal handler coverage to unmount on receiving signal

From: Reinette Chatre <reinette.chatre@intel.com>
To: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: "Shuah Khan" <shuah@kernel.org>,
	"Shuah Khan" <skhan@linuxfoundation.org>,
	linux-kselftest@vger.kernel.org,
	"Maciej Wieczór-Retman" <maciej.wieczor-retman@intel.com>,
	LKML <linux-kernel@vger.kernel.org>,
	"Shaopeng Tan" <tan.shaopeng@jp.fujitsu.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH 1/5] selftests/resctrl: Extend signal handler coverage to unmount on receiving signal
Date: Thu, 14 Sep 2023 08:04:36 -0700	[thread overview]
Message-ID: <b32f4bd1-9e99-3daa-9d39-8f241b41170c@intel.com> (raw)
In-Reply-To: <9dfe761f-6779-44c6-c227-5175d53ac25@linux.intel.com>

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