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

30 Nov 2022

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

    

2025

2024

2023

2022

2021

2020

2019

2018

2017

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