Re: [PATCH v3 4/5] selftests/resctrl: Cleanup properly when an error occurs in CAT test

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.

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

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?

} remove(param.filename); ret = cat_val(&param);

• if (ret)

• return ret;
  

• ret = check_results(&param);
• if (ret)

• return ret;
  

• if (ret == 0)

• ret = check_results(&param);
  

Why not use a goto in error case to do umount_resctrlfs() instead of changing the conditionals?

if (bm_pid == 0) { /* Tell parent that child is ready */ @@ -201,7 +212,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) sizeof(pipe_message)) { close(pipefd[1]); perror("# failed signaling parent process");

• 	return errno;
  

  }

close(pipefd[1]); @@ -226,5 +236,5 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) if (bm_pid) umount_resctrlfs();

• return 0;

• return ret; }

With these changes made:

Reviewed-by: Shuah Khan skhan@linuxfoundation.org

thanks, -- Shuah