Re: [PATCH 2/3] selftests/resctrl: Simplify cleanup in ctrl-c handler

From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Maciej Wieczor-Retman <maciej.wieczor-retman@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
Date: Tue, 20 Feb 2024 16:21:32 +0200 (EET)	[thread overview]
Message-ID: <b7b1aace-66f2-ece8-f8ba-7e323e04fa70@linux.intel.com> (raw)
In-Reply-To: <uck6k4upde5642uarjjyoeyt76ju7f3ympqbnugnv3ufaw65k2@wdh2chxqsooo>

[-- Attachment #1: Type: text/plain, Size: 2444 bytes --]

On Tue, 20 Feb 2024, Maciej Wieczor-Retman wrote:

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

Why cannot the pointer be const too? The signal handler is not supposed to 
modify the contents of the resctrl_test struct.

There are two types of constness in C. One (the most commonly used one) 
relates to immutability of the contents of the struct the pointer (or char 
*) points to while the other enforces the pointer itself to remain 
immutable. Usually, the former is what is useful and it's what you get 
when you naturally write "const struct".

> >> +	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?

Yes, that's what I'm after here. Lets keep it internal to the signal 
handling code.

-- 
 i.