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-rc3
Difference from v2:
Moved [PATCH v2 3/4] to the last and insert patch 4 before it.
[patch 1] Fixed the typo miss in the changelog and initialized
*p(resctrl_val_param) before use it. And since there was no
MBM processing in write_schemata(), added it.
[patch 4] A signal handler is introduced in this patch. With this patch,
patch 5 clear duplicate code cat/cmt/mbm/mba_test_cleanup()
without falling into the indicated trap.
https://lore.kernel.org/lkml/[email protected]/
Pervious versions of this series:
[v1] https://lore.kernel.org/lkml/[email protected]/
[v2] 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 | 29 +++++++++++++------
tools/testing/selftests/resctrl/mba_test.c | 8 ++---
tools/testing/selftests/resctrl/mbm_test.c | 13 +++++----
.../testing/selftests/resctrl/resctrl_tests.c | 4 ---
tools/testing/selftests/resctrl/resctrl_val.c | 1 +
tools/testing/selftests/resctrl/resctrlfs.c | 5 +++-
6 files changed, 36 insertions(+), 24 deletions(-)
--
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: Reinette Chatre <[email protected]>
Signed-off-by: Shaopeng Tan <[email protected]>
---
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;
}
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
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 <[email protected]>
---
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");
+
+ 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;
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");
}
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 */
@@ -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;
}
--
2.27.0
There is a comment "Set up shemata with 100% allocation on the first run"
in function mbm_setup(), but there is an increment bug and the condition
"num_of_runs == 0" will never be met and write_schemata() will never be
called to set schemata to 100%. Even if write_schemata() is called in MBM
test, since it is not supported for MBM test it does not set the schemata.
This is currently fine because resctrl_val_parm->mum_resctrlfs is always 1
and umount/mount will be run in each test to set the schemata to 100%.
To support the usage when MBM test does not unmount/remount resctrl
filesystem before the test starts, fix to call write_schemata() and
set schemata properly when the function is called for the first time.
Also, remove static local variable 'num_of_runs' because this is not
needed as there is resctrl_val_param->num_of_runs which should be used
instead like in cat_setup().
Signed-off-by: Shaopeng Tan <[email protected]>
---
tools/testing/selftests/resctrl/mbm_test.c | 13 +++++++------
tools/testing/selftests/resctrl/resctrlfs.c | 4 +++-
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 8392e5c55ed0..6d550f012829 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -89,23 +89,24 @@ static int check_results(int span)
static int mbm_setup(int num, ...)
{
struct resctrl_val_param *p;
- static int num_of_runs;
va_list param;
int ret = 0;
- /* Run NUM_OF_RUNS times */
- if (num_of_runs++ >= NUM_OF_RUNS)
- return -1;
-
va_start(param, num);
p = va_arg(param, struct resctrl_val_param *);
va_end(param);
+ /* Run NUM_OF_RUNS times */
+ if (p->num_of_runs >= NUM_OF_RUNS)
+ return -1;
+
/* Set up shemata with 100% allocation on the first run. */
- if (num_of_runs == 0)
+ if (p->num_of_runs == 0)
ret = write_schemata(p->ctrlgrp, "100", p->cpu_no,
p->resctrl_val);
+ p->num_of_runs++;
+
return ret;
}
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 6f543e470ad4..8546bc9f1786 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -498,6 +498,7 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
FILE *fp;
if (strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) &&
+ strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) &&
strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) &&
strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
return -ENOENT;
@@ -523,7 +524,8 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) ||
!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata);
- if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)))
+ if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) ||
+ !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)))
sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata);
fp = fopen(controlgroup, "w");
--
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: 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
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 <[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]);
Good find. Looks good to me.
Reviewed-by: Shuah Khan <[email protected]>
thanks,
-- Shuah
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 <[email protected]>
> Signed-off-by: Shaopeng Tan <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> ---
> 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(¶m);
> - if (ret)
> - return ret;
> -
> - ret = check_results(¶m);
> - if (ret)
> - return ret;
> + if (ret == 0)
> + ret = check_results(¶m);
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 <[email protected]>
thanks,
-- Shuah
Hi Shaopeng 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 <[email protected]>
>> ---
>> 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?
>
>> +
>> + 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.
>
>> 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.
>
>> }
>> 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);
>
> 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.
Reinette
Hi Shaopeng,
On 11/1/2022 2:43 AM, 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
I find the above hard to read. How about "..., if an error occurs or
a signal such as SIGINT is received, ..."
> 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 <[email protected]>
> ---
> tools/testing/selftests/resctrl/cat_test.c | 28 +++++++++++++++-------
> 1 file changed, 19 insertions(+), 9 deletions(-)
>
...
> @@ -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.
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?
Could you please add a comment to the code to clarify this flow?
Thank you very much
Reinette
Hi Shaopeng,
On 11/1/2022 2:43 AM, Shaopeng Tan wrote:
> There is a comment "Set up shemata with 100% allocation on the first run"
> in function mbm_setup(), but there is an increment bug and the condition
> "num_of_runs == 0" will never be met and write_schemata() will never be
> called to set schemata to 100%. Even if write_schemata() is called in MBM
> test, since it is not supported for MBM test it does not set the schemata.
> This is currently fine because resctrl_val_parm->mum_resctrlfs is always 1
> and umount/mount will be run in each test to set the schemata to 100%.
>
> To support the usage when MBM test does not unmount/remount resctrl
> filesystem before the test starts, fix to call write_schemata() and
> set schemata properly when the function is called for the first time.
>
> Also, remove static local variable 'num_of_runs' because this is not
> needed as there is resctrl_val_param->num_of_runs which should be used
> instead like in cat_setup().
>
> Signed-off-by: Shaopeng Tan <[email protected]>
Thank you very much.
Reviewed-by: Reinette Chatre <[email protected]>
Reinette
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 <[email protected]>
> >> ---
> >> 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(¶m);
> >> - if (ret)
> >> - return ret;
> >> -
> >> - ret = check_results(¶m);
> >> - if (ret)
> >> - return ret;
> >> + if (ret == 0)
> >> + ret = check_results(¶m);
> >
> > 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