Re: [PATCH v2 2/2] selftests/resctrl: Move run_benchmark() to a more fitting file

[Maciej Wieczór-Retman]

Hi,

On 2023-08-30 at 13:51:29 -0700, Reinette Chatre wrote:
>Hi Maciej,
>
>On 8/28/2023 2:56 AM, Wieczor-Retman, Maciej wrote:
>> Resctrlfs.c file contains mostly functions that interact in some way
>> with resctrl FS entries while functions inside resctrl_val.c deal with
>> measurements and benchmarking.
>> 
>> Run_benchmark() function is located in resctrlfs.c file even though it's
>> purpose is not interacting with the resctrl FS but to execute cache
>> checking logic.
>
>It looks like your editor may be automatically capitalize first words
>of sentences like Resctrlfs.c and Run_benchmark() above.

I'll fix it.

>Also please note that when using () to indicate a function it is not
>necessary to say it is a function. For example above can just be:
>"run_benchmark() is located ..." ... similarly you can just say
>"resctrlfs.c contains ...".

Thanks for the tip, will apply it from now on.

>> 
>> Move run_benchmark() to resctrl_val.c just before resctrl_val() function
>> that makes use of run_benchmark().
>> 
>> Remove return comment from kernel-doc since the function is type void.
>> 
>> Changelog v2:
>> - Add dots at the end of patch msg sentences.
>> - Remove "Return: void" from run_benchmark() kernel-doc comment.
>> 
>
>same comment about changelog.

It'll be fixed next time.

>> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@xxxxxxxxx>
>> ---
>>  tools/testing/selftests/resctrl/resctrl_val.c | 50 ++++++++++++++++++
>>  tools/testing/selftests/resctrl/resctrlfs.c   | 52 -------------------
>>  2 files changed, 50 insertions(+), 52 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
>> index f0f6c5f6e98b..5c8dc0a7bab9 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_val.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
>> @@ -621,6 +621,56 @@ measure_vals(struct resctrl_val_param *param, unsigned long *bw_resc_start)
>>  	return 0;
>>  }
>>  
>> +/*
>> + * run_benchmark - Run a specified benchmark or fill_buf (default benchmark)
>> + *		   in specified signal. Direct benchmark stdio to /dev/null.
>> + * @signum:	signal number
>> + * @info:	signal info
>> + * @ucontext:	user context in signal handling
>> + */
>> +void run_benchmark(int signum, siginfo_t *info, void *ucontext)
>
>Can run_benchmark() now be made static and its declaration removed from
>the header file?

Thanks for noticing. Yes, from my side it seems turning it into static
is okay. I tried it out on Ilpo's branches that I know he's currently
working on and there were no errors either.

-- 
Kind regards
Maciej Wieczór-Retman