On 11/1/22 03:43, Shaopeng Tan wrote:

Since MBA check result is not returned, the MBA test result message is always output as "ok" regardless of whether the MBA check result is true or false.

Make output message to be "not ok" if MBA check result is failed.

Reviewed-by: Reinette Chatre reinette.chatre@intel.com Signed-off-by: Shaopeng Tan tan.shaopeng@jp.fujitsu.com

---

tools/testing/selftests/resctrl/mba_test.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c index 1a1bdb6180cf..5d14802add4d 100644 --- a/tools/testing/selftests/resctrl/mba_test.c +++ b/tools/testing/selftests/resctrl/mba_test.c @@ -51,7 +51,7 @@ static int mba_setup(int num, ...) return 0; } -static void show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc) +static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc) { int allocation, runs; bool failed = false; @@ -97,6 +97,8 @@ static void show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc) failed ? "Fail:" : "Pass:"); if (failed) ksft_print_msg("At least one test failed\n");

• return failed;

Rename "failed" to "ret" - naming the variable "failed" is confusing.

} static int check_results(void) @@ -132,9 +134,7 @@ static int check_results(void) fclose(fp);

• show_mba_info(bw_imc, bw_resc);
• return 0;

• return show_mba_info(bw_imc, bw_resc); }

void mba_test_cleanup(void)

With that change,

Reviewed-by: Shuah Khan skhan@linuxfoundation.org

thanks, -- Shuah

On 11/1/22 03:43, Shaopeng Tan wrote:

When a process has buffered output, a child process created by fork() will also copy buffered output. When using kselftest framework, the output (resctrl test result message) will be printed multiple times.

Add fflush() to flush out the buffered output before executing fork().

Reviewed-by: Reinette Chatre reinette.chatre@intel.com Signed-off-by: Shaopeng Tan tan.shaopeng@jp.fujitsu.com

---

tools/testing/selftests/resctrl/cat_test.c | 1 + tools/testing/selftests/resctrl/resctrl_val.c | 1 + tools/testing/selftests/resctrl/resctrlfs.c | 1 + 3 files changed, 3 insertions(+)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 1c5e90c63254..6a8306b0a109 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -167,6 +167,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) return errno; }

• fflush(stdout); bm_pid = fork();

/* Set param values for child thread which will be allocated bitmask diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index b32b96356ec7..6948843bf995 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -629,6 +629,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) * Fork to start benchmark, save child's pid so that it can be killed * when needed */

• fflush(stdout); bm_pid = fork(); if (bm_pid == -1) { perror("# Unable to fork");

diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index 8546bc9f1786..d95688298469 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -678,6 +678,7 @@ int filter_dmesg(void) perror("pipe"); return ret; }

• fflush(stdout); pid = fork(); if (pid == 0) { close(pipefds[0]);

Good find. Looks good to me.

Reviewed-by: Shuah Khan skhan@linuxfoundation.org

thanks, -- Shuah

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

Hi Reinette 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?

Yes, it's a duplicate of existing resctrl_val.c:ctrlc_handler(). I will try to use resctrl_val.c:ctrlc_handler() in next version patch series.

...

...

+    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.

Yes. I used global bm_pid, since bm_pid is used in ctrl_handler() before this function cat_perf_miss_val(). I will add a description to 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.

I will use sigaction(2) and perror().

...

...

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

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.

Yes, the code that follows is needed to synchronize the exits between the parent and child.

...

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

It looks like pipefd[1] will be closed twice if the write() failed.

This close(pipefd[1]); should also be removed.

It does look strange to let the child continue to its infinite loop after the write() failed. I assume that it is because the parent will also be stuck and the new ctrl_handler() is expected to unblock both?

If a SIGNAL is received, ctrl_handler() will be called to kill the child process and exit parent process. If no SIGNAL is received, the parent process will kill the child process. (by else{kill(bm_pid, SIGKILL);})

Could you please add a comment to the code to clarify this flow?

I will add comments here.

Best regards, Shaopeng Tan

Hi Shaopeng,

On 11/1/2022 2:43 AM, Shaopeng Tan wrote:

Before exiting each test function(run_cmt/cat/mbm/mba_test()), test results("ok","not ok") are printed by ksft_test_result() and then temporary result files are cleaned by function cmt/cat/mbm/mba_test_cleanup(). However, before running ksft_test_result(), function cmt/cat/mbm/mba_test_cleanup() has been run in each test function as follows: cmt_resctrl_val() cat_perf_miss_val() mba_schemata_change() mbm_bw_change()

Remove duplicate codes that clear each test result file.

Signed-off-by: Shaopeng Tan tan.shaopeng@jp.fujitsu.com

tools/testing/selftests/resctrl/resctrl_tests.c | 4 ---- 1 file changed, 4 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index df0d8d8526fc..8732cf736528 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -88,7 +88,6 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span, ksft_test_result(!res, "MBM: bw change\n"); if ((get_vendor() == ARCH_INTEL) && res) ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");

• mbm_test_cleanup();

}

From what I can tell this still seem to suffer from the problem where the test files may not be cleaned. With the removal of mbm_test_cleanup() the cleanup is now expected to be done in mbm_bw_change().

Note that:

mbm_bw_change() { ...

ret = resctrl_val(benchmark_cmd, &param); if (ret) return ret; /* Test results stored in file */

ret = check_results(span); if (ret) return ret; <== Return without cleaning test result file

mbm_test_cleanup(); <== Test result file cleaned only when test passed.

return 0; }

static void run_mba_test(bool has_ben, char **benchmark_cmd, int span, @@ -107,7 +106,6 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span, sprintf(benchmark_cmd[1], "%d", span); res = mba_schemata_change(cpu_no, bw_report, benchmark_cmd); ksft_test_result(!res, "MBA: schemata change\n");

• mba_test_cleanup();

}

mba_schemata_change() has the same pattern as mbm_bw_change() so test result files may linger when test fails.

static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no) @@ -126,7 +124,6 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no) ksft_test_result(!res, "CMT: test\n"); if ((get_vendor() == ARCH_INTEL) && res) ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");

• cmt_test_cleanup();

}

Same pattern again.

static void run_cat_test(int cpu_no, int no_of_bits) @@ -142,7 +139,6 @@ static void run_cat_test(int cpu_no, int no_of_bits) res = cat_perf_miss_val(cpu_no, no_of_bits, "L3"); ksft_test_result(!res, "CAT: test\n");

• cat_test_cleanup();

}

Patch 4 makes this work. Thanks for doing that.

Reinette