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

[Shaopeng Tan (Fujitsu)]

Hello Reinette,

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

Thanks for your explanation. I understand

Best regards,
Shaopeng TAN