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

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Shaopeng,

On 9/28/2023 1:10 AM, Shaopeng Tan (Fujitsu) wrote:
>> On 9/15/2023 8:44 AM, Ilpo Järvinen wrote:

...

>>> +static void run_mbm_test(const char * const *benchmark_cmd, int
>>> +cpu_no) {
>>> +	int res;
>>> +
>>> +	ksft_print_msg("Starting MBM BW change ...\n");
>>> +
>>> +	if (test_prepare())
>>> +		return;
>>>
>>
>> I am not sure about this. With this exit the kselftest machinery is not aware of
>> the test passing or failing. I wonder if there should not rather be a "goto" here
>> that triggers ksft_test_result()? This needs some more thought though. First,
>> with this change test_prepare() officially gains responsibility to determine if a
>> failure is transient (just a single test
>> fails) or permanent (no use trying any other tests if this fails). For the former it
>> would then be up to the caller to call ksft_test_result() and for the latter
>> test_prepare() will call ksft_exit_fail_msg().
>> Second, that SNC warning may be an inconvenience with a new goto. Here it
>> may be ok to print that message before the test failure?
> 
> If a failure may be permanent, it may be best to detect it before running all tests, rather than in test_prepare().
> Now some detections are completed before running all tests. For example:
> 273         if (geteuid() != 0)
> 274                 return ksft_exit_skip("Not running as root. Skipping...\n");
> 275
> 276         if (!check_resctrlfs_support())
> 277                 return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n");
> 278
> 279         if (umount_resctrlfs())
> 280                 return ksft_exit_skip("resctrl FS unmount failed.\n");
> 

You are correct that the tests should aim to detect as early as possible if
no test has a chance of succeeding. This is covered in the checks you mention.
The purpose of test_prepare()/test_cleanup() pair is to perform actions that
should be done for every test. For example, resctrl is mounted before each
test and unmounted after each test. Since these actions are required to be done
for every test it cannot be a single call before all tests are run.

It may be possible to add a test_prepare() directly followed by a test_cleanup()
before any test is run to be more explicit about early detection but that
does not seem necessary considering the checks would be done anyway when the
first test is run. Even when doing so it would not eliminate the need for 
test_prepare()/test_cleanup() to form part of every test run and needing to exit
if, for example, a previous test triggered a fault preventing resctrl from
being mounted.

Reinette