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

["Shaopeng Tan (Fujitsu)" <tan.shaopeng@fujitsu.com>]

Hi Reinette,

> On 11/24/2022 12:17 AM, Shaopeng Tan (Fujitsu) wrote:
> > Hi Reinette,
> >
> >> On 11/16/2022 5:05 PM, 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. if an error occurs in parent
> >>> process. To use ctrlc_handler() of other tests to kill child
> >>> process(bm_pid), using global bm_pid instead of local bm_pid.
> >>>
> >>> Wait for child process to be killed if an error occurs in child process.
> >>>
> >>> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
> >>> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> >>> ---
> >>>  tools/testing/selftests/resctrl/cat_test.c | 30
> >> ++++++++++++++--------
> >>>  1 file changed, 20 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/tools/testing/selftests/resctrl/cat_test.c
> >> b/tools/testing/selftests/resctrl/cat_test.c
> >>> index 6a8306b0a109..1f8f5cf94e95 100644
> >>> --- a/tools/testing/selftests/resctrl/cat_test.c
> >>> +++ b/tools/testing/selftests/resctrl/cat_test.c
> >>> @@ -100,10 +100,10 @@ void cat_test_cleanup(void)
> >>>
> >>>  int cat_perf_miss_val(int cpu_no, int n, char *cache_type)  {
> >>> +	struct sigaction sigact;
> >>>  	unsigned long l_mask, l_mask_1;
> >>>  	int ret, pipefd[2], sibling_cpu_no;
> >>>  	char pipe_message;
> >>> -	pid_t bm_pid;
> >>>
> >>>  	cache_size = 0;
> >>>
> >>> @@ -181,17 +181,25 @@ 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 {
> >>> +		/*
> >>> +		 * Register CTRL-C handler for parent, as it has to kill
> >>> +		 * child process before exiting
> >>> +		 */
> >>> +		sigact.sa_sigaction = ctrlc_handler;
> >>> +		sigemptyset(&sigact.sa_mask);
> >>> +		sigact.sa_flags = SA_SIGINFO;
> >>> +		if (sigaction(SIGINT, &sigact, NULL) ||
> >>> +		    sigaction(SIGTERM, &sigact, NULL) ||
> >>> +		    sigaction(SIGHUP, &sigact, NULL))
> >>> +			perror("# sigaction");
> >>
> >> Why keep going at this point?
> >>
> >> I tried this change but I was not able to trigger ctrlc_handler(). It
> >
> > I tested this change using kselftest framework, In this case, the
> > timeout command sent a SIGTERM signal, and then ctrlc_handler() was
> > triggered.
> > Since the handling of SIGINT and SIGHUP signals is overridden in
> > run_fill_buf(),
> > ctrl_handler() may be called if ctrl-c is received.
> 
> I tested this by running "resctrl_tests -t cat" and when doing so this change
> does not behave as described.

Yes, the fix of v4 cannot satisfy "resctrl_tests -t cat"".
I will add new fix in next version.

> >> seems that the handler is changed soon after (see
> >> cat_val()->run_fill_buf()) and ctrl_handler() (note the subtle name
> >> difference) is run instead when a SIGINT is received. The value of
> >> ctrl_handler() is not clear to me though, and it could even be argued
> >> that it is broken because it starts with
> >> free(startptr) and startptr would likely already be free'd at this
> >> point without resetting its value to NULL.
> >>
> >> From what I understand ctrl_handler() and its installation from
> >> run_fill_buf() could be removed so that it does not override what is
> >> being done with this change. Otherwise, from what I can tell, this
> >> new handler will never run.
> >
> > I think removing ctrl_handler() is fine.
> > In CAT test, it overrides ctrlc_handler().
> > In other tests(CMT,MBA,MBM), the child process used ctrl_handler() to
> > cleanup itself,
> 
> Is that explicit cleanup required? All I can see is free(startptr) and that pointer
> would usually be invalid as I mentioned earlier. If the process crashes while
> running fill_cache() and thus not get a chance to run free(startptr) then the OS
> would release the memory, no?

Sorry, my explanation was not clear.
I agree with you, I think removing ctrl_handler() is OK.

> > but the parent process cleanup the child process with ctrlc_handler()
> properly.
> > Also, avoid using signal().
> >  fill_buf.c：run_fill_buf()
> >  201         /* set up ctrl-c handler */
> >  202         if (signal(SIGINT, ctrl_handler) == SIG_ERR)
> >  203                 printf("Failed to catch SIGINT!\n");
> >
> > I removed ctrl_handler() and ran resctrl_tests with and without the kselftest
> framework.
> > There is no problem.
> 
> Thank you. I only used the CAT test when I found the issue.

Removing ctrl_handler() is only part of the fix in the next version(v5).
All fixes as follows.

--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -98,12 +98,17 @@ void cat_test_cleanup(void)
        remove(RESULT_FILE_NAME2);
 }

+static void ctrlc_handler_child(int signum, siginfo_t *info, void *ptr)
+{
+       exit(EXIT_SUCCESS);
+}
+
 int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
 {
+       struct sigaction sigact;
        unsigned long l_mask, l_mask_1;
        int ret, pipefd[2], sibling_cpu_no;
        char pipe_message;
-       pid_t bm_pid;

        cache_size = 0;

@@ -181,17 +186,33 @@ 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;
+
+               sigfillset(&sigact.sa_mask);
+               sigact.sa_sigaction = ctrlc_handler_child;
+               sigact.sa_flags = SA_SIGINFO;
+               if (sigaction(SIGINT, &sigact, NULL) ||
+                   sigaction(SIGTERM, &sigact, NULL) ||
+                   sigaction(SIGHUP, &sigact, NULL))
+                       perror("# sigaction");
+       } else {
+               /*
+                * Register CTRL-C handler for parent, as it has to kill
+                * child process before exiting
+                */
+               sigact.sa_sigaction = ctrlc_handler;
+               sigemptyset(&sigact.sa_mask);
+               sigact.sa_flags = SA_SIGINFO;
+               if (sigaction(SIGINT, &sigact, NULL) ||
+                   sigaction(SIGTERM, &sigact, NULL) ||
+                   sigaction(SIGHUP, &sigact, NULL))
+                       perror("# sigaction");
        }

        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);

        if (bm_pid == 0) {
                /* Tell parent that child is ready */
@@ -199,9 +220,11 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
                pipe_message = 1;
                if (write(pipefd[1], &pipe_message, sizeof(pipe_message)) <
                    sizeof(pipe_message)) {
-                       close(pipefd[1]);
+                       /*
+                        * Just print the error message.
+                        * Let while(1) run and wait for itself to be killed.
+                        */
                        perror("# failed signaling parent process");
-                       return errno;
                }

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

-       return 0;
+       return ret;
 }
diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
index 56ccbeae0638..322c6812e15c 100644
--- a/tools/testing/selftests/resctrl/fill_buf.c
+++ b/tools/testing/selftests/resctrl/fill_buf.c
@@ -33,14 +33,6 @@ static void sb(void)
 #endif
 }

-static void ctrl_handler(int signo)
-{
-       free(startptr);
-       printf("\nEnding\n");
-       sb();
-       exit(EXIT_SUCCESS);
-}
-
 static void cl_flush(void *p)
 {
 #if defined(__i386) || defined(__x86_64)
@@ -198,12 +190,6 @@ int run_fill_buf(unsigned long span, int malloc_and_init_memory,
        unsigned long long cache_size = span;
        int ret;

-       /* set up ctrl-c handler */
-       if (signal(SIGINT, ctrl_handler) == SIG_ERR)
-               printf("Failed to catch SIGINT!\n");
-       if (signal(SIGHUP, ctrl_handler) == SIG_ERR)
-               printf("Failed to catch SIGHUP!\n");
-
        ret = fill_cache(cache_size, malloc_and_init_memory, memflush, op,
                         resctrl_val);
        if (ret) {


Best regards,
Shaopeng Tan