Received-SPF: pass (google.com: domain of linux-kernel+bounces-76364-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 22 Feb 2024 11:51:29 +0100
From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
To: Ilpo =?utf-8?B?SsOkcnZpbmVu?= <ilpo.jarvinen@linux.intel.com>
CC: Fenghua Yu <fenghua.yu@intel.com>, Reinette Chatre
	<reinette.chatre@intel.com>, Shuah Khan <shuah@kernel.org>, LKML
	<linux-kernel@vger.kernel.org>, <linux-kselftest@vger.kernel.org>
Subject: Re: [PATCH v2 2/3] selftests/resctrl: Simplify cleanup in ctrl-c
 handler
Message-ID: <cl55dahbdccoikbqguqam3bbfyq2nwinuixpqwkn2fumeybhjj@gp5owhup6sgz>
References: <cover.1708596015.git.maciej.wieczor-retman@intel.com>
 <3a6639cd338a706bd2ed341aa6c717e1e79cc1f0.1708596015.git.maciej.wieczor-retman@intel.com>
 <c472e478-eaf1-0003-cf2d-c861e620fce2@linux.intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <c472e478-eaf1-0003-cf2d-c861e620fce2@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?Rvw4bJmBPP0O89f5GifXiQPElPv1vW8EVtiGKMLtEtRtZSnsK/NnJwWhiG?=
 =?iso-8859-1?Q?UvmgCynRN1J+iaqLLn7FIdRGUmuMm8oGtSqi1zbHd/sGDyAG/RqV5lblF2?=
 =?iso-8859-1?Q?XNgXL/zshTxAfNX4bTK6k8mlaqYALAJjmhUlli0bTgSPleqz66pYTqTbMc?=
 =?iso-8859-1?Q?H90BNQtHvZ2m9yChza5MUn8goI5VDd8UScWUtCzGsjBuyTNigxZByyxd0L?=
 =?iso-8859-1?Q?X/qaiy5oINGdXzLLWscI+qDp+FpYitjZCfgekZW98yVuZmTQUk8C5aWx/w?=
 =?iso-8859-1?Q?YvEngZ64bYDe5j1Yu80mQaav4+POUkf0+KEgiZW+ds0QUwNfztvQuCPXva?=
 =?iso-8859-1?Q?U92tUhqtO4OaFzyFU1sSqYoQpfoSkHTzZxWA5Y9P+p4lTzmPZX/FJ1/IeZ?=
 =?iso-8859-1?Q?dUqhuGkj+0sKrTVRi38y+6ztLu0rPCRhT9pL+FS4CUu1woRxGo4J7wsk2M?=
 =?iso-8859-1?Q?BwXZuHCJWLRFKvEa/2hPn3u6/23X4SBwzclU9Oe6Aj+yVT37c503Db3V2P?=
 =?iso-8859-1?Q?GdqtTTbj3tcwwBxxgAtsHYZNXQ0RKkVyaVDrs8b4jW8UsmWbyRnJw1TLiE?=
 =?iso-8859-1?Q?CnM1jMdNlm1uZszxpafJA/cCx1FgdzNDFN4igXapIHyN9SXF/e6th7sj0g?=
 =?iso-8859-1?Q?vduHdV31+q8wwBLK2MIS+RiaON4+sc3Scy8KvhdWOuQ6xfbIm45DbaNK8/?=
 =?iso-8859-1?Q?JezCcYUAbBL2MztBwZPB5FvrEn2+2AGl9cuj/Uyi/WhN0PkydlSwKdwtYS?=
 =?iso-8859-1?Q?jYpf0+vL77lxJtH6m4Y/eZ3aOeBFpIbPu8cesi2WrNDXARmC+p5siLzt0y?=
 =?iso-8859-1?Q?oRSSVtlbgW9aseA8j+X/tD9uHK35GADVy0BYrA4pH3A6VYizkriHDJeRer?=
 =?iso-8859-1?Q?tIW2YOoA/vOd6elWrE+BMvy353/95f6/eDoJwL8bgrCnl0AruO80R2CRst?=
 =?iso-8859-1?Q?nZN5EK89WdQAbb9AOpCjuLVwofrHYeugX89aNbT97wX6Xknxzv1b+LxCkH?=
 =?iso-8859-1?Q?gcFICPJURdmxOHF3H8tjkobMvLeaGN4itf/WnKaIzmWqY0tWDYYArFctn6?=
 =?iso-8859-1?Q?nQKIfbv5LGvnZdltxvVOQFRbZgI77oVn9ko5kn5XTB230oRl2Znd/cc1mZ?=
 =?iso-8859-1?Q?luzndjn0UvxuCuhNm4pdcseYQ6AE38XQpiSDnaus0cYL+2cc1Gk0gAjCa+?=
 =?iso-8859-1?Q?ImcMN9QMJNBHhaO3EDq30elwP/Or7lMNz7LaunMl6SQ7aRno3NFAm7KOaR?=
 =?iso-8859-1?Q?SEiFrViojTEJyOMlPvtnrXLg/lP2NODb1J9N6lBMN1iSZxF4LBHYa/1Jzy?=
 =?iso-8859-1?Q?24J0maBz6wxit7OZP1Ujl4e1pWkfgVGeQwyApvs8RjITXWxyttKXgLaepr?=
 =?iso-8859-1?Q?tn1EphCTN3d8Q2CC2VkYr+JWkEpJ54/Uv6MSLvmJ4EOc0sAbu7gWQ2Ms3Q?=
 =?iso-8859-1?Q?LxisFBe/FbxXtyTY6UZss0vhmDWN1zjV03TBx76P86XPWe7FHEOUGPpIrJ?=
 =?iso-8859-1?Q?776Wm/k5hO2UH+/gnCPgzdQfb0IPVI00kW6plxERJXFKxuldo1HSyBSb9N?=
 =?iso-8859-1?Q?7Oo9+FD7LvLyljG0wgEHuL3ed/NvGNJ/VMxazZn4iU7OeWWUQFPZBra366?=
 =?iso-8859-1?Q?aGVQYPsFUOuFdk2BICV6iAd8Kki/OHoPfe2YV3xubcdIAE67KPqUXJGheu?=
 =?iso-8859-1?Q?XdOcpDM97SnmNqe8t9g=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5b279d88-7cb2-4276-049c-08dc33943eb5
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6231.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Feb 2024 10:51:38.7134
 (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: x25aUuC6T2zGX5xZ6jt6pNmJxjA/swLE0bilmDN9sPm8Z6wHAPVWA/zmcdVUQ/pyyPsP6ljBR+/r1KXW6iUfZNogYM3Wwprt4NfcwJvm970=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV8PR11MB8605
X-OriginatorOrg: intel.com

On 2024-02-22 at 12:12:44 +0200, Ilpo J?rvinen wrote:
>On Thu, 22 Feb 2024, Maciej Wieczor-Retman wrote:
>
>> Ctrl-c handler isn't aware of what test is currently running. Because of
>> that it executes all cleanups even if they aren't necessary. Since the
>> ctrl-c handler uses the sa_sigaction system no parameters can be passed
>> to it as function arguments.
>> 
>> Add a global variable to make ctrl-c handler aware of the currently run
>> test and only execute the correct cleanup callback.
>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> ---
>> Changelog v2:
>> - Remove tests_cleanup() from resctrl.h.
>> - Make current_test a const pointer only inside resctrl_val.c. (Ilpo)
>> 
>>  tools/testing/selftests/resctrl/resctrl.h       |  3 +--
>>  tools/testing/selftests/resctrl/resctrl_tests.c | 14 +++-----------
>>  tools/testing/selftests/resctrl/resctrl_val.c   |  6 ++++--
>>  3 files changed, 8 insertions(+), 15 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>> index 0f49df4961ea..826783b29c9d 100644
>> --- a/tools/testing/selftests/resctrl/resctrl.h
>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>> @@ -153,7 +153,6 @@ int resctrl_val(const struct resctrl_test *test,
>>  		const struct user_params *uparams,
>>  		const char * const *benchmark_cmd,
>>  		struct resctrl_val_param *param);
>> -void tests_cleanup(void);
>>  void mbm_test_cleanup(void);
>>  void mba_test_cleanup(void);
>>  unsigned long create_bit_mask(unsigned int start, unsigned int len);
>> @@ -162,7 +161,7 @@ int get_full_cbm(const char *cache_type, unsigned long *mask);
>>  int get_mask_no_shareable(const char *cache_type, unsigned long *mask);
>>  int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size);
>>  void ctrlc_handler(int signum, siginfo_t *info, void *ptr);
>> -int signal_handler_register(void);
>> +int signal_handler_register(const struct resctrl_test *test);
>>  void signal_handler_unregister(void);
>>  void cat_test_cleanup(void);
>>  unsigned int count_bits(unsigned long n);
>> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
>> index 75fc49ba3efb..161f5365b4f0 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
>> @@ -75,19 +75,11 @@ static void cmd_help(void)
>>  	printf("\t-h: help\n");
>>  }
>>  
>> -void tests_cleanup(void)
>> -{
>> -	mbm_test_cleanup();
>> -	mba_test_cleanup();
>> -	cmt_test_cleanup();
>> -	cat_test_cleanup();
>> -}
>> -
>> -static int test_prepare(void)
>> +static int test_prepare(const struct resctrl_test *test)
>>  {
>>  	int res;
>>  
>> -	res = signal_handler_register();
>> +	res = signal_handler_register(test);
>>  	if (res) {
>>  		ksft_print_msg("Failed to register signal handler\n");
>>  		return res;
>> @@ -130,7 +122,7 @@ static void run_single_test(const struct resctrl_test *test, const struct user_p
>>  
>>  	ksft_print_msg("Starting %s test ...\n", test->name);
>>  
>> -	if (test_prepare()) {
>> +	if (test_prepare(test)) {
>>  		ksft_exit_fail_msg("Abnormal failure when preparing for the test\n");
>>  		return;
>>  	}
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
>> index 5a49f07a6c85..d572815436f3 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_val.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
>> @@ -62,6 +62,7 @@ struct imc_counter_config {
>>  static char mbm_total_path[1024];
>>  static int imcs;
>>  static struct imc_counter_config imc_counters_config[MAX_IMCS][2];
>> +const struct resctrl_test *current_test;
>
>static const struct

Okay, I'll add it.

>
>>  void membw_initialize_perf_event_attr(int i, int j)
>>  {
>> @@ -472,7 +473,7 @@ void ctrlc_handler(int signum, siginfo_t *info, void *ptr)
>>  	if (bm_pid)
>>  		kill(bm_pid, SIGKILL);
>>  	umount_resctrlfs();
>> -	tests_cleanup();
>> +	current_test->cleanup();
>
>These calls should have if (current_test->cleanup()) guard. Isn't the 
>non-contiguous already test w/o the cleanup function?

Yes, I remembered to put the check in the main test function but I forgot to put
the check here too, thanks.

I'll just resend the corrected version today.

>
>Other than those two, this looked okay.
>
>-- 
> i.
>

-- 
Kind regards
Maciej Wiecz?r-Retman