Received-SPF: pass (google.com: domain of linux-kernel+bounces-73152-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 20 Feb 2024 15:08:03 +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 2/3] selftests/resctrl: Simplify cleanup in ctrl-c handler
Message-ID: <uck6k4upde5642uarjjyoeyt76ju7f3ympqbnugnv3ufaw65k2@wdh2chxqsooo>
References: <cover.1708434017.git.maciej.wieczor-retman@intel.com>
 <8c4fcfb6b4e38a0f0e400be88ecf1af0d20e12e7.1708434017.git.maciej.wieczor-retman@intel.com>
 <5f251bcd-a343-bb6e-a947-7605dc59f9ea@linux.intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <5f251bcd-a343-bb6e-a947-7605dc59f9ea@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?zmO3FydwVbOuB/3tB4Uc6H3JadeVYwzrtDqbVAzVnTPxsHWkmGM1nRxZob?=
 =?iso-8859-1?Q?AAARQY7KB10fEGrZO8h9wyhHVAFiOZct4KGRF7cqKgt47q1ZjCjLjSgUEx?=
 =?iso-8859-1?Q?HA0Qf7JG0cN9jtfE46hGKtPBndlmhO0Sdw263VeW3npHXXoMx/N49w7Dkc?=
 =?iso-8859-1?Q?OtjCHUwnuY8JkSTeFBQOxqBMSP7xItuBJkLYYlRVUST85ix/lL7U9w+k+v?=
 =?iso-8859-1?Q?jisitESVUFCT+baH43OIExSAM1T7Ib6YtwRp6bO6d6IZmG65VWSkBT3T5S?=
 =?iso-8859-1?Q?0IDPtaBMzHkiiF47vNYWs1MrVKHbDvlpHqENakEmEeFHL0M0iqa8eh/qX9?=
 =?iso-8859-1?Q?dPuoIyWL58+U8kogGkgZuy52FIbtl4h2dctX1pBx6P8L06LqtlBvDA4lAz?=
 =?iso-8859-1?Q?Hurb2NGM29O2y2VC9P9XB9avq1RmEvibcSigUVh0OmdvmE/60hK40V28rg?=
 =?iso-8859-1?Q?5FQ9oZ2dfp2Bk85a9bVm43sm6Yr6g2W83jOoBlAmuTBqzn2LXUOCkwWLIv?=
 =?iso-8859-1?Q?MhJ/kyYPNg/0QapH4EpqtYwmdxRt0kke3uYR8+SSlVxRY6JaEmwCZOCtrQ?=
 =?iso-8859-1?Q?wiaMJuI8T26rQYmJNWWaxmIsRoGxFK6TP90IytIAwQVmmS8xenau+sYbvg?=
 =?iso-8859-1?Q?eXfUnoYutg2DFKwRpZhDJ1SKn81fioyuoWV7eahtSN3CTQab+GKX+BB7bH?=
 =?iso-8859-1?Q?gqwS1X1HxKISHTCFbgcg/3EMKT0lDBEvV5xQx4v/OrR42dXY5v5YGNlgKN?=
 =?iso-8859-1?Q?kr1twha9LualjIp1u8jI4vP7yHOnMTARQhXEJUif0PZsGIM44v9BuFPPho?=
 =?iso-8859-1?Q?ruZajybk4c9KEqFNpVQoeeWlrCud34jH1A/U89WxE6pLwiA4CgmNEfiQmX?=
 =?iso-8859-1?Q?RGeNp0bR8H706DFMRSulpD3ZbyjBRFt1dlgJjDB3twvryj7WcQFnLRZ6mv?=
 =?iso-8859-1?Q?6E0uyfDHdqWBIna5nhS55qqEJOOsAE/Feu7xmsI4JBma/7br8GUnJ2Q+aD?=
 =?iso-8859-1?Q?frQExhGyf+ZLwMRZvm499AWLcVMMxfxocEmyDFC17MgU00BqbMA+QacCC4?=
 =?iso-8859-1?Q?v1ClWjtQIRotaGwfgOwhl9pqYc7CYScX12K7uixcjgvHX9SN65nE2eapb4?=
 =?iso-8859-1?Q?kuVFs6yvAqAkVWKRwKkOfvX1Qrcee5wQAjdaTXN59Dd2gkWdjZTYZcZS74?=
 =?iso-8859-1?Q?iRg0eKGADYQLFRZlOI/ytDM97OrbOmRTTqAeYPQnAUUL82clZr7fy6j51h?=
 =?iso-8859-1?Q?++hRMeCIuMjqLTEfUqAjQtTVfgb5Acz6aw2Aaft0JtKgRgOXwBYgO1Riy3?=
 =?iso-8859-1?Q?gQon5NXHQLAiEaqCRKfe1rBrK1bPusa9yUJefq+65dQX99UrlH/Ckkp+7b?=
 =?iso-8859-1?Q?XZsuXbj+YTU59zETH1CQqvtm1GXIAKVZ3r76x/oTaue09XMMCscWTR7C+M?=
 =?iso-8859-1?Q?aHHk3pUcnZUyK8sXCRR6FTTEEzm/EMv79q8CwvO6vt/XORcDsGho8X27DL?=
 =?iso-8859-1?Q?SqoV9d+LWxQLmYMgdd8qOLdmL1rU8QYVT5r61C/0dRyNhwV6DTzS2iJBXz?=
 =?iso-8859-1?Q?lMysUVhRSgR6Gw7NvJG0Gi71y7L51ILp/wDFVMenB3iG69AZJI/bd4r0e4?=
 =?iso-8859-1?Q?9mFXLEyRzxyVdbabAPaZ/rrk2oBcbEdZ53SxukskeLZfzHCE5aHlNgbiy7?=
 =?iso-8859-1?Q?NHTd3ujFemcHu/j9c/A=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2d700e24-8545-42cc-c498-08dc321d601b
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6231.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Feb 2024 14:08:12.9471
 (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: itpdDqh1HLEHaFXviaZWPJkqJcQYZkIOTZW3Uc6m+HaNj7bYV1FOGCoI8pAjFo1CrqRs8SdZouK64GIZuYqtPXpXaSkpA1XYjjyngnrY/JQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB7599
X-OriginatorOrg: intel.com

On 2024-02-20 at 15:45:23 +0200, Ilpo J?rvinen wrote:
>On Tue, 20 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 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>
>> ---
>>  tools/testing/selftests/resctrl/resctrl.h     |  2 ++
>>  .../testing/selftests/resctrl/resctrl_tests.c | 20 +++++++++----------
>>  tools/testing/selftests/resctrl/resctrl_val.c |  2 +-
>>  3 files changed, 13 insertions(+), 11 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>> index 0f49df4961ea..79b45cbeb628 100644
>> --- a/tools/testing/selftests/resctrl/resctrl.h
>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>> @@ -128,6 +128,8 @@ extern pid_t bm_pid, ppid;
>>  
>>  extern char llc_occup_path[1024];
>>  
>> +extern struct resctrl_test current_test;
>
>Why this is not just a pointer?

I tried making this as a pointer but the 'test' in test_prepare() is of type
'const struct resctrl_test *' and there are warnings about dropping the const
modifier.

>
>> +
>>  int get_vendor(void);
>>  bool check_resctrlfs_support(void);
>>  int filter_dmesg(void);
>> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
>> index 75fc49ba3efb..b17f7401892c 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
>> @@ -14,6 +14,12 @@
>>  static volatile int sink_target;
>>  volatile int *value_sink = &sink_target;
>>  
>> +/*
>> + * Set during test preparation for the cleanup function pointer used in
>> + * ctrl-c sa_sigaction
>> + */
>> +struct resctrl_test current_test;
>> +
>>  static struct resctrl_test *resctrl_tests[] = {
>>  	&mbm_test,
>>  	&mba_test,
>> @@ -75,18 +81,12 @@ 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();
>> -}
>
>This should be removed from resctrl.h too.

Thanks, forgot that it was there too.

>
>> -
>> -static int test_prepare(void)
>> +static int test_prepare(const struct resctrl_test *test)
>>  {
>>  	int res;
>>  
>> +	current_test = *test;
>
>I'd prefer to keep this internal to signal handling functions so that 
>either the struct resctrl_test or just the cleanup handler is passed 
>to signal_handler_register().

Okay, would moving this assignment to signal_handler_register() be okay then?

>
>It'd also allow current_test (or the cleanup function ptr) to be static.
>
>-- 
> i.
>

-- 
Kind regards
Maciej Wiecz?r-Retman