Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <d2d94def-742d-7661-3632-b9f7e4996415@intel.com>
Date:   Tue, 26 Sep 2023 14:38:37 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.15.1
Subject: Re: [PATCH v2 1/6] selftests/resctrl: Extend signal handler coverage
 to unmount on receiving signal
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=c3=a4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        Shuah Khan <shuah@kernel.org>,
        Shuah Khan <skhan@linuxfoundation.org>,
        <linux-kselftest@vger.kernel.org>,
        =?UTF-8?Q?Maciej_Wiecz=c3=b3r-Retman?= 
        <maciej.wieczor-retman@intel.com>
CC:     LKML <linux-kernel@vger.kernel.org>,
        Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>,
        <stable@vger.kernel.org>
References: <20230915154438.82931-1-ilpo.jarvinen@linux.intel.com>
 <20230915154438.82931-2-ilpo.jarvinen@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230915154438.82931-2-ilpo.jarvinen@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?K0hudE1Ec1dXQnUvb0I5Tk5iMSt5bEZTWUlrMjdTZUNYSC82a2pDanlVaEdn?=
 =?utf-8?B?MEp4VERVWFA1LzAvWE1BWEpxamtSWjl0SXU1WE9lSlo2NUxvYTBBU3lDdjN5?=
 =?utf-8?B?SEVyTE1JYTBIalhxZmVmUFlVWmg1SFVwSDVKVE9SdE5QVjRjak1DU0tIUlY2?=
 =?utf-8?B?ZEVEU1RhaHlncEpZblp3TWNLRHVJWHpZL01nWmlJc09GUXdvZ3pNWlRMTzhp?=
 =?utf-8?B?OTFESTdaWklkWkFIdFJQdk52OTBQSFhPSG5OdmNPVEpUWGRmbjJ6ejlLZWFh?=
 =?utf-8?B?MGxXMVUrcWY3TkNCdmxUS2trT0RoTFNFVFhud1VKNlVVR0pNMkdVMWR1N0tQ?=
 =?utf-8?B?V1g4K2pyTHhvQXZZb2lsSkFlVjlEVmREWS84bjJudUE3RmdsTG51d2JBOHFZ?=
 =?utf-8?B?OU5CUE8xSUR6QTQ4bDNLR0dUbnZ3MVp5NlQ0eUdjd0tKdzNMYmhzOHhwLzA2?=
 =?utf-8?B?NG41bVB0bGlNY2ZjWWVxTTRBaXkwMXZ2NS9lcG5nelBGS29LSjNjNDA4amJ2?=
 =?utf-8?B?K2g0NEFsY0QySlo1ZWNjSjZCQVk0UUV0MVZpWkZVcnIzU1RpY3BibGMzZmNs?=
 =?utf-8?B?ODljdDRMWDZkc3BBRjQ3OUJLR3RsYnhaMGRPNHMzZGRLVDZSb3J3aWZVNU5m?=
 =?utf-8?B?OWc5TjJqWWJUUnUvUUhhdnFHblVXL2xXL05ZaG8yamd4clFmeXEvMythWlpL?=
 =?utf-8?B?dHk4emhhWGorUGFiOHBzVVZCUkNXYnhyYW1YaXBFMTkxOHcrT0Z5ZWNPTDVJ?=
 =?utf-8?B?SWo2bWFaVW5NS1hBRkREUEFMeUNOM2VEV2dSREdUS0JRRmdoU3pydmtYSzI2?=
 =?utf-8?B?QWIrdXZPSHNaT3lQbTIvVThpUVVQYjdmT0plSDFFSTg5d3pQTnd4ZDV0QTY0?=
 =?utf-8?B?OVQwYytpMWtwZ3VsT3RLRkVEdm5jUUpjVnNPem1neGNua1JDYzQxd3dzNkdB?=
 =?utf-8?B?c2hxUFU3ckFjVkdUVThjZm9zMGZPbWVuTVV6RmpIVkhPZkg5dERVMFExZGpE?=
 =?utf-8?B?ZFRPREZhd3dERnpyNWVKWXF2NXhyMTY4VzljaEpSWjE5eTVYbmNNeFp0VVJE?=
 =?utf-8?B?aHJ3VnY2bTFLeEU2V2N5RG9VdHlCQ3VSQkthd2dvRXdJVEtXTXNBdjBUMUNU?=
 =?utf-8?B?R1l1bXNEd2MxSGVTMlRJYXlLSTNZbkNBT2hhazFzNkdXU0xVaFpRNHdyekNI?=
 =?utf-8?B?UWFOR2t4M2V3MzR0SWtiMTFFY3RSYXFLUUhPSFRLZ1hQclB2VU9LRUtpbVAr?=
 =?utf-8?B?VEgremVuMW12VytxN1gvZzlDdWVDTWFBYkxBM3pMYWUyZ1FPN0VtaGswWDlQ?=
 =?utf-8?B?b2VVV2FudFJuS1NJZjdZOEJzTVhWQmhIcnhVWG1qRWZLT3Z2aUMrTGh1Y2ZE?=
 =?utf-8?B?Qy92dldEeUxQYjArVFYxVy9hMmNEZjV4cnpCTG10RlNkbDF6dnBmWGdSZWVQ?=
 =?utf-8?B?bXJzUVRUMk5xMGhtMHNKQWVBK1VSb1BJWHJyL0xVTmV6ZDM3RjRXaWFuaVM3?=
 =?utf-8?B?dGxxaVgzM0YwQ1U0b0twUHBUdzRQVmRUMHFBYngxeWRscXhCZVgzNkNtMTZ4?=
 =?utf-8?B?UUYwYTkvTU1xYUI4ei8vSFJIVm0zOU9hTjRUM1R4b2ZWanF2WEtXc0RGaDNE?=
 =?utf-8?B?NHYrRGZOWXZvWFZJNlg1dFBkT1RIUm5GMDZHTjRMV3B1ckEvd2ZrOXFBY0s5?=
 =?utf-8?B?eDFHOFZhMjhyZi90NUh5Ylhud1lqZEVWMm1GZThiQXNYVFBQMVRkTWQ2UUxC?=
 =?utf-8?B?Rk9sNjJpZjlnYzFxNlVyWkplLzUxendiS2RySTB4NCtIY09SZWFrZkNsc2t1?=
 =?utf-8?B?RFhhV25EZEw5T2xpTUtrVDlINUJKcStBelpWT0xHRjJrMDV0TXZXNDE5eWRi?=
 =?utf-8?B?MHZTVTdML2ZrS3lZeVR6MS81T0FvZGdaeHZpTjcrVkpQNWpmOWo5VC9vM3Bs?=
 =?utf-8?B?TDVwbjdMOCtsek1ERlZJaG45bXlOZXJGZGFIMTg3SktEZng3SElsUXp1MmlN?=
 =?utf-8?B?QXVFMDFmNlZBZkhRcnZUSmZrRE1memlMeituV3VGZGJRSFUvUTJmTzZCWWNE?=
 =?utf-8?B?ZDhIdCtWbVFoLzZxYlNpZ281bHFoL0ZadEF3TDNLeWsxMUo0RWNob0l5S2Vt?=
 =?utf-8?B?YWU4cHhPQS9xTTVYS0VkL0s0bFp1ZGxGcFk0SG5YYU9OcklWQUdsMzJOVDdW?=
 =?utf-8?B?dWc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bed16710-83a3-4629-5e4e-08dbbed8f30d
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Sep 2023 21:38:59.3783
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 8HYhpmxtnm/cCWM0xVRtvszo2Lqd2TLoPl+eWH4Ja+wNCDtv0JCMeKpUoSUkfQAS+a00xqqZwqHxlfGLW2yTW+khKAheIMx3xkIfRWO6onU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB7705
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Tue, 26 Sep 2023 21:24:06 -0700 (PDT)

Hi Ilpo,

On 9/15/2023 8:44 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() calls
> from per test files into resctrl_tests.c to properly unmount resctrl
> fs. In order to not add signal_handler_register()/unregister() n times,
> create helpers test_prepare() and test_cleanup().
> 
> 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 | 65 +++++++++++--------
>  tools/testing/selftests/resctrl/resctrl_val.c | 22 +++----
>  3 files changed, 48 insertions(+), 47 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..524ba83d7568 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -67,21 +67,41 @@ void tests_cleanup(void)
>  	cat_test_cleanup();
>  }
>  
> -static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no)
> +static int test_prepare()
>  {
>  	int res;
>  
> -	ksft_print_msg("Starting MBM BW change ...\n");
> +	res = signal_handler_register();
> +	if (res)
> +		return res;
>  
>  	res = mount_resctrlfs();
>  	if (res) {
> +		signal_handler_unregister();
>  		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
> -		return;
> +		return res;
>  	}
> +	return 0;
> +}
> +
> +static void test_cleanup()
> +{
> +	umount_resctrlfs();
> +	signal_handler_unregister();
> +}

Thank you for adding these.

> +
> +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 (!validate_resctrl_feature_request(MBM_STR) || (get_vendor() != ARCH_INTEL)) {
>  		ksft_test_result_skip("Hardware does not support MBM or MBM is disabled\n");
> -		goto umount;
> +		goto cleanup;
>  	}
>  
>  	res = mbm_bw_change(cpu_no, benchmark_cmd);
> @@ -89,8 +109,8 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no)
>  	if ((get_vendor() == ARCH_INTEL) && res)
>  		ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
>  
> -umount:
> -	umount_resctrlfs();
> +cleanup:
> +	test_cleanup();
>  }
>  
>  static void run_mba_test(const char * const *benchmark_cmd, int cpu_no)
> @@ -99,22 +119,19 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no)
>  
>  	ksft_print_msg("Starting MBA Schemata change ...\n");
>  
> -	res = mount_resctrlfs();
> -	if (res) {
> -		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
> +	if (test_prepare())
>  		return;
> -	}
>  
>  	if (!validate_resctrl_feature_request(MBA_STR) || (get_vendor() != ARCH_INTEL)) {
>  		ksft_test_result_skip("Hardware does not support MBA or MBA is disabled\n");
> -		goto umount;
> +		goto cleanup;
>  	}
>  
>  	res = mba_schemata_change(cpu_no, benchmark_cmd);
>  	ksft_test_result(!res, "MBA: schemata change\n");
>  
> -umount:
> -	umount_resctrlfs();
> +cleanup:
> +	test_cleanup();
>  }
>  
>  static void run_cmt_test(const char * const *benchmark_cmd, int cpu_no)
> @@ -123,15 +140,12 @@ static void run_cmt_test(const char * const *benchmark_cmd, int cpu_no)
>  
>  	ksft_print_msg("Starting CMT test ...\n");
>  
> -	res = mount_resctrlfs();
> -	if (res) {
> -		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
> +	if (test_prepare())
>  		return;
> -	}
>  
>  	if (!validate_resctrl_feature_request(CMT_STR)) {
>  		ksft_test_result_skip("Hardware does not support CMT or CMT is disabled\n");
> -		goto umount;
> +		goto cleanup;
>  	}
>  
>  	res = cmt_resctrl_val(cpu_no, 5, benchmark_cmd);
> @@ -139,8 +153,8 @@ static void run_cmt_test(const char * const *benchmark_cmd, int cpu_no)
>  	if ((get_vendor() == ARCH_INTEL) && res)
>  		ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
>  
> -umount:
> -	umount_resctrlfs();
> +cleanup:
> +	test_cleanup();
>  }
>  
>  static void run_cat_test(int cpu_no, int no_of_bits)
> @@ -149,22 +163,19 @@ static void run_cat_test(int cpu_no, int no_of_bits)
>  
>  	ksft_print_msg("Starting CAT test ...\n");
>  
> -	res = mount_resctrlfs();
> -	if (res) {
> -		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
> +	if (test_prepare())
>  		return;
> -	}
>  
>  	if (!validate_resctrl_feature_request(CAT_STR)) {
>  		ksft_test_result_skip("Hardware does not support CAT or CAT is disabled\n");
> -		goto umount;
> +		goto cleanup;
>  	}
>  
>  	res = cat_perf_miss_val(cpu_no, no_of_bits, "L3");
>  	ksft_test_result(!res, "CAT: test\n");
>  
> -umount:
> -	umount_resctrlfs();
> +cleanup:
> +	test_cleanup();
>  }
>  
>  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;
> +

Since this is an initialization fix in this area ... what
do you think of also initializing sigact? It could just be
a change to
	struct sigaction sigact = {};

This will prevent registering a signal handler with 
uninitialized sa_flags.

Reinette