Hello,
The aim of this patch series is to improve the resctrl selftest.
Without these fixes, some unnecessary processing will be executed
and test results will be confusing.
There is no behavior change in test themselves.
[patch 1] Make write_schemata() run to set up shemata with 100% allocation
on first run in MBM test.
[patch 2] The MBA test result message is always output as "ok",
make output message to be "not ok" if MBA check result is failed.
[patch 3] When a child process is created by fork(), the buffer of the
parent process is also copied. Flush the buffer before
executing fork().
[patch 4] Add a signal handler to cleanup properly before exiting the
parent process if there is an error occurs after creating
a child process with fork() in the CAT test.
[patch 5] Before exiting each test CMT/CAT/MBM/MBA, clear test result
files function cat/cmt/mbm/mba_test_cleanup() are called
twice. Delete once.
This patch series is based on Linux v6.1-rc5
Difference from v3:
[patch 2]
Rename "failed" to "ret" to avoid confusion.
[patch 4]
- Use sigaction(2) instead of signal().
- Add a description of using global bm_pid in commit message.
- Add comments to clarify why let the child continue to its
infinite loop after the write() failed.
[patch 5]
Ensure to run cat/cmt/mbm/mba_test_cleanup() to clear test result
file before return if an error occurs.
Pervious versions of this series:
[v1] https://lore.kernel.org/lkml/[email protected]/
[v2] https://lore.kernel.org/lkml/[email protected]/
[v3] https://lore.kernel.org/lkml/[email protected]/
Shaopeng Tan (5):
selftests/resctrl: Fix set up schemata with 100% allocation on first
run in MBM test
selftests/resctrl: Return MBA check result and make it to output
message
selftests/resctrl: Flush stdout file buffer before executing fork()
selftests/resctrl: Cleanup properly when an error occurs in CAT test
selftests/resctrl: Remove duplicate codes that clear each test result
file
tools/testing/selftests/resctrl/cat_test.c | 31 +++++++++++++------
tools/testing/selftests/resctrl/cmt_test.c | 7 ++---
tools/testing/selftests/resctrl/mba_test.c | 23 +++++++-------
tools/testing/selftests/resctrl/mbm_test.c | 20 ++++++------
.../testing/selftests/resctrl/resctrl_tests.c | 4 ---
tools/testing/selftests/resctrl/resctrl_val.c | 1 +
tools/testing/selftests/resctrl/resctrlfs.c | 5 ++-
7 files changed, 50 insertions(+), 41 deletions(-)
--
2.27.0
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 <[email protected]>
Signed-off-by: Shaopeng Tan <[email protected]>
---
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");
}
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);
if (bm_pid == 0) {
/* Tell parent that child is ready */
@@ -199,9 +207,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 +236,5 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
if (bm_pid)
umount_resctrlfs();
- return 0;
+ return ret;
}
--
2.27.0
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,
while ensuring cleanup properly even when errors occur in each test.
Signed-off-by: Shaopeng Tan <[email protected]>
---
tools/testing/selftests/resctrl/cmt_test.c | 7 +++----
tools/testing/selftests/resctrl/mba_test.c | 7 +++----
tools/testing/selftests/resctrl/mbm_test.c | 7 +++----
tools/testing/selftests/resctrl/resctrl_tests.c | 4 ----
4 files changed, 9 insertions(+), 16 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index 8968e36db99d..617ee95d272d 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -133,13 +133,12 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
ret = resctrl_val(benchmark_cmd, ¶m);
if (ret)
- return ret;
+ goto out;
ret = check_results(¶m, n);
- if (ret)
- return ret;
+out:
cmt_test_cleanup();
- return 0;
+ return ret;
}
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index e3a473976d74..b948938a3b4d 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -160,13 +160,12 @@ int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd)
ret = resctrl_val(benchmark_cmd, ¶m);
if (ret)
- return ret;
+ goto out;
ret = check_results();
- if (ret)
- return ret;
+out:
mba_test_cleanup();
- return 0;
+ return ret;
}
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 6d550f012829..040ca1f9c173 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -134,13 +134,12 @@ int mbm_bw_change(int span, int cpu_no, char *bw_report, char **benchmark_cmd)
ret = resctrl_val(benchmark_cmd, ¶m);
if (ret)
- return ret;
+ goto out;
ret = check_results(span);
- if (ret)
- return ret;
+out:
mbm_test_cleanup();
- return 0;
+ return ret;
}
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();
}
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();
}
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();
}
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();
}
int main(int argc, char **argv)
--
2.27.0
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: Shuah Khan <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
Signed-off-by: Shaopeng Tan <[email protected]>
---
tools/testing/selftests/resctrl/mba_test.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 1a1bdb6180cf..e3a473976d74 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -51,10 +51,10 @@ 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;
+ bool ret = false;
ksft_print_msg("Results are displayed in (MB)\n");
/* Memory bandwidth from 100% down to 10% */
@@ -90,13 +90,15 @@ static void show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
ksft_print_msg("avg_bw_imc: %lu\n", avg_bw_imc);
ksft_print_msg("avg_bw_resc: %lu\n", avg_bw_resc);
if (avg_diff_per > MAX_DIFF_PERCENT)
- failed = true;
+ ret = true;
}
ksft_print_msg("%s Check schemata change using MBA\n",
- failed ? "Fail:" : "Pass:");
- if (failed)
+ ret ? "Fail:" : "Pass:");
+ if (ret)
ksft_print_msg("At least one test failed\n");
+
+ return ret;
}
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)
--
2.27.0
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: Shuah Khan <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
Signed-off-by: Shaopeng Tan <[email protected]>
---
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]);
--
2.27.0
Hi Shaopeng,
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 <[email protected]>
> Signed-off-by: Shaopeng Tan <[email protected]>
> ---
> 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
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.
Reinette
Hi Shaopeng,
On 11/16/2022 5:05 PM, 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,
> while ensuring cleanup properly even when errors occur in each test.
>
> Signed-off-by: Shaopeng Tan <[email protected]>
Thank you very much.
Reviewed-by: Reinette Chatre <[email protected]>
Reinette
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 <[email protected]>
> > Signed-off-by: Shaopeng Tan <[email protected]>
> > ---
> > 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.
> 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,
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.
Best regards,
Shaopeng Tan
Hi Shaopeng,
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 <[email protected]>
>>> Signed-off-by: Shaopeng Tan <[email protected]>
>>> ---
>>> 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.
>> 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?
> 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.
Reinette
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 <[email protected]>
> >>> Signed-off-by: Shaopeng Tan <[email protected]>
> >>> ---
> >>> 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(¶m);
- if (ret)
- return ret;
-
- ret = check_results(¶m);
- if (ret)
- return ret;
+ if (ret == 0)
+ ret = check_results(¶m);
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
Hi Shaopeng,
On 11/30/2022 12:32 AM, Shaopeng Tan (Fujitsu) wrote:
> 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);
> +}
> +
Could you please elaborate why this is necessary?
Reinette
Hi Reinette,
> On 11/30/2022 12:32 AM, Shaopeng Tan (Fujitsu) wrote:
>
> > 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);
> > +}
> > +
>
> Could you please elaborate why this is necessary?
If enter "ctrl-c" when running "resctrl_tests -t cat",
SIGINT will be sent to all processes (parent&child).
At this time, the child process receives a SIGINT signal, but does not take any action.
In this case the parent process may not call ctrlc_handler() as expected.
Therefore, ctrlc_handler_child() is necessary.
It may be better to ignore the signal, then code can be simple as follows.
----
if (bm_pid == 0) {
param.mask = l_mask_1;
strcpy(param.ctrlgrp, "c1");
strcpy(param.mongrp, "m1");
param.span = cache_size * n / count_of_bits;
strcpy(param.filename, RESULT_FILE_NAME1);
param.num_of_runs = 0;
param.cpu_no = sibling_cpu_no;
/* Ignore the signal,and wait to be cleaned up by the parent process */
sigfillset(&sigact.sa_mask);
sigact.sa_handler = SIG_IGN;
//sigact.sa_sigaction = ctrlc_handler_child; //delete
if (sigaction(SIGINT, &sigact, NULL) ||
sigaction(SIGTERM, &sigact, NULL) ||
sigaction(SIGHUP, &sigact, NULL))
perror("# sigaction");
} else {
----
Best regards,
Shaopeng Tan
Hi Shaopeng,
On 12/1/2022 12:20 AM, Shaopeng Tan (Fujitsu) wrote:
> Hi Reinette,
>
>> On 11/30/2022 12:32 AM, Shaopeng Tan (Fujitsu) wrote:
>>
>>> 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);
>>> +}
>>> +
>>
>> Could you please elaborate why this is necessary?
>
> If enter "ctrl-c" when running "resctrl_tests -t cat",
> SIGINT will be sent to all processes (parent&child).
>
> At this time, the child process receives a SIGINT signal, but does not take any action.
> In this case the parent process may not call ctrlc_handler() as expected.
Apologies, but I am not able to follow. My understanding is that the
ideal in working an failing case is for the parent to kill the child.
Could you please elaborate why the ctrlc_handler() may not be called?
> Therefore, ctrlc_handler_child() is necessary.
>
> It may be better to ignore the signal, then code can be simple as follows.
> ----
> if (bm_pid == 0) {
> param.mask = l_mask_1;
> strcpy(param.ctrlgrp, "c1");
> strcpy(param.mongrp, "m1");
> param.span = cache_size * n / count_of_bits;
> strcpy(param.filename, RESULT_FILE_NAME1);
> param.num_of_runs = 0;
> param.cpu_no = sibling_cpu_no;
> /* Ignore the signal,and wait to be cleaned up by the parent process */
> sigfillset(&sigact.sa_mask);
> sigact.sa_handler = SIG_IGN;
> //sigact.sa_sigaction = ctrlc_handler_child; //delete
> if (sigaction(SIGINT, &sigact, NULL) ||
> sigaction(SIGTERM, &sigact, NULL) ||
> sigaction(SIGHUP, &sigact, NULL))
> perror("# sigaction");
> } else {
Reinette
Hi Reinette
> On 12/1/2022 12:20 AM, Shaopeng Tan (Fujitsu) wrote:
> > Hi Reinette,
> >
> >> On 11/30/2022 12:32 AM, Shaopeng Tan (Fujitsu) wrote:
> >>
> >>> 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);
> >>> +}
> >>> +
> >>
> >> Could you please elaborate why this is necessary?
> >
> > If enter "ctrl-c" when running "resctrl_tests -t cat", SIGINT will be
> > sent to all processes (parent&child).
> >
> > At this time, the child process receives a SIGINT signal, but does not take any
> action.
> > In this case the parent process may not call ctrlc_handler() as expected.
>
> Apologies, but I am not able to follow. My understanding is that the ideal in
> working an failing case is for the parent to kill the child.
> Could you please elaborate why the ctrlc_handler() may not be called?
Apologies for the late replay.
The problem is that at the time of running CAT test,
previous ctrlc_handler from MBM/MBA/CMT test will be inherited to child process.
Let me explain in detail:
In resctrl_tests,the default run order of the tests is MBM->MBA->CMT->CAT.
When running MBM, MBA, CMT, signal handler(ctrlc_handler) was set to the parent process.
After these tests, when fork() is executed in CAT,
the signal handler set by MBM/MBA/CMT is inherited by the parent&child process of CAT.
At this time, if "ctrl+c" SIGINT is sent to parent&child process,
according to the inherited signal handler,
the child process may kill parent process before parent process kills child process.
Therefore, when running all tests(MBM->MBA->CMT->CAT),
signal handler of child process need to be overridden in CAT.
Also, when running CAT test only,
since there are no signal handler that can be inherited from other tests,
signal handler of parent process need to be set.
> > Therefore, ctrlc_handler_child() is necessary.
> >
> > It may be better to ignore the signal, then code can be simple as follows.
> > ----
> > if (bm_pid == 0) {
> > param.mask = l_mask_1;
> > strcpy(param.ctrlgrp, "c1");
> > strcpy(param.mongrp, "m1");
> > param.span = cache_size * n / count_of_bits;
> > strcpy(param.filename, RESULT_FILE_NAME1);
> > param.num_of_runs = 0;
> > param.cpu_no = sibling_cpu_no;
> > /* Ignore the signal,and wait to be cleaned up by the parent
> process */
> > sigfillset(&sigact.sa_mask);
> > sigact.sa_handler = SIG_IGN;
> > //sigact.sa_sigaction = ctrlc_handler_child; //delete
> > if (sigaction(SIGINT, &sigact, NULL) ||
> > sigaction(SIGTERM, &sigact, NULL) ||
> > sigaction(SIGHUP, &sigact, NULL))
> > perror("# sigaction");
> > } else {
Best regards,
Shaopeng
Hi Shaopeng,
On 12/16/2022 12:20 AM, Shaopeng Tan (Fujitsu) wrote:
> Hi Reinette
>
>> On 12/1/2022 12:20 AM, Shaopeng Tan (Fujitsu) wrote:
>>> Hi Reinette,
>>>
>>>> On 11/30/2022 12:32 AM, Shaopeng Tan (Fujitsu) wrote:
>>>>
>>>>> 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);
>>>>> +}
>>>>> +
>>>>
>>>> Could you please elaborate why this is necessary?
>>>
>>> If enter "ctrl-c" when running "resctrl_tests -t cat", SIGINT will be
>>> sent to all processes (parent&child).
>>>
>>> At this time, the child process receives a SIGINT signal, but does not take any
>> action.
>>> In this case the parent process may not call ctrlc_handler() as expected.
>>
>> Apologies, but I am not able to follow. My understanding is that the ideal in
>> working an failing case is for the parent to kill the child.
>> Could you please elaborate why the ctrlc_handler() may not be called?
>
> Apologies for the late replay.
>
> The problem is that at the time of running CAT test,
> previous ctrlc_handler from MBM/MBA/CMT test will be inherited to child process.
>
> Let me explain in detail:
> In resctrl_tests,the default run order of the tests is MBM->MBA->CMT->CAT.
> When running MBM, MBA, CMT, signal handler(ctrlc_handler) was set to the parent process.
> After these tests, when fork() is executed in CAT,
> the signal handler set by MBM/MBA/CMT is inherited by the parent&child process of CAT.
> At this time, if "ctrl+c" SIGINT is sent to parent&child process,
> according to the inherited signal handler,
> the child process may kill parent process before parent process kills child process.
> Therefore, when running all tests(MBM->MBA->CMT->CAT),
> signal handler of child process need to be overridden in CAT.
Thank you for the additional details. I do not think that this should be handled
in the CAT test. The CAT test should not need to work around leftovers from the other
tests, that will just leave us with harder code to maintain. Instead, I think this should
be addressed in resctrl_val() where the signal handler is set for the
MBM/MBA/CMT tests. That is, when the test is complete and the test case
specific signal handler no longer needed (after test itself kills the child and
handler thus no longer needed), the signal handler should be reset to SIG_DFL.
This should give the CAT test a clean slate to work with.
>
> Also, when running CAT test only,
> since there are no signal handler that can be inherited from other tests,
> signal handler of parent process need to be set.
Yes, this is clear. It is the child signal handler that was confusing to me.
Reinette