Hi Shaopeng and Shuah,
On 11/2/2022 2:41 AM, Shuah Khan wrote:
> On 11/1/22 03:43, Shaopeng Tan wrote:
>> After creating a child process with fork() in CAT test, if there is
>> an error occurs or such as a SIGINT signal is received, the parent
>> process will be terminated immediately, but the child process will not
>> be killed and also umount_resctrlfs() will not be called.
>>
>> Add a signal handler like other tests to kill child process, umount
>> resctrlfs, cleanup result files, etc. when an error occurs.
>>
>> Signed-off-by: Shaopeng Tan <tan.shaopeng@xxxxxxxxxxxxxx>
>> ---
>> tools/testing/selftests/resctrl/cat_test.c | 28 +++++++++++++++-------
>> 1 file changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
>> index 6a8306b0a109..5f81817f4366 100644
>> --- a/tools/testing/selftests/resctrl/cat_test.c
>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>> @@ -98,12 +98,21 @@ void cat_test_cleanup(void)
>> remove(RESULT_FILE_NAME2);
>> }
>> +static void ctrl_handler(int signo)
>> +{
>> + kill(bm_pid, SIGKILL);
>> + umount_resctrlfs();
>> + tests_cleanup();
>> + ksft_print_msg("Ending\n\n");
>
> Is there a reason to print this message? Remove it unless it serves
> a purpose.
This function appears to be a duplicate of existing
resctrl_val.c:ctrlc_handler(). Could the duplication be avoided
instead of refining the copy?
>
>> +
>> + exit(EXIT_SUCCESS);
>> +}
>> +
>> int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>> {
>> unsigned long l_mask, l_mask_1;
>> int ret, pipefd[2], sibling_cpu_no;
>> char pipe_message;
>> - pid_t bm_pid;
>
> Odd. bm_pid is used below - why remove it here?
There is a global bm_pid in resctrl_val.c that is made available
via extern in resctrl.h. This is what causes this code to still
compile but I would also like to learn why moving to that is
desired as a change here. I expect such a big change to get a
mention in the commit message.
>
>> cache_size = 0;
>> @@ -181,17 +190,19 @@ 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 {
>> + /* set up ctrl-c handler */
>> + if (signal(SIGINT, ctrl_handler) == SIG_ERR ||
>> + signal(SIGHUP, ctrl_handler) == SIG_ERR ||
>> + signal(SIGTERM, ctrl_handler) == SIG_ERR)
>> + printf("Failed to catch SIGNAL!\n");
>
> Is perror() more appropriate here?
Should we be using signal() at all? "man signal" reads:
"WARNING: the behavior of signal() varies across UNIX versions,
and has also varied historically across different versions of Linux.
Avoid its use: use sigaction(2) instead."
"Failed to catch SIGNAL" also seems unclear to me. This is
where a signal handler is set up, the signal for which the handler
is installed has not arrived.
>
>> }
>> remove(param.filename);
>> ret = cat_val(¶m);
>> - if (ret)
>> - return ret;
>> -
>> - ret = check_results(¶m);
>> - if (ret)
>> - return ret;
>> + if (ret == 0)
>> + ret = check_results(¶m);
>
> Why not use a goto in error case to do umount_resctrlfs() instead of changing
> the conditionals?
My understanding is the code that follows is needed to
synchronize the exits between the parent and child. It is the parent
that will run umount_resctrlfs() and it should only do so
after the child is done. A goto by the parent may thus cause
umount_resctrlfs() to be run while the child still relies on it while
a goto by the child may cause the parent not to receive the message
that the child is complete.
Reinette
