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@jp.fujitsu.com
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