[v5,12/19] selftests/resctrl: Remove "malloc_and_init_memory" param from run_fill_buf()
Commit Message
run_fill_buf()'s malloc_and_init_memory parameter is always 1. There's
also duplicated memory init code for malloc_and_init_memory == 0 case
in fill_buf() which is unused.
Remove the malloc_and_init_memory parameter and the duplicated mem init
code.
While at it, fix also a typo in run_fill_buf() prototype's argument.
Co-developed-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
---
tools/testing/selftests/resctrl/cache.c | 6 ++--
tools/testing/selftests/resctrl/fill_buf.c | 28 +++----------------
tools/testing/selftests/resctrl/resctrl.h | 3 +-
.../testing/selftests/resctrl/resctrl_tests.c | 13 ++++-----
tools/testing/selftests/resctrl/resctrlfs.c | 12 ++++----
5 files changed, 19 insertions(+), 43 deletions(-)
Comments
On 7/17/23 07:15, Ilpo Järvinen wrote:
> run_fill_buf()'s malloc_and_init_memory parameter is always 1. There's
> also duplicated memory init code for malloc_and_init_memory == 0 case
> in fill_buf() which is unused.
>
> Remove the malloc_and_init_memory parameter and the duplicated mem init
> code.
>
> While at it, fix also a typo in run_fill_buf() prototype's argument.
>
> Co-developed-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> tools/testing/selftests/resctrl/cache.c | 6 ++--
> tools/testing/selftests/resctrl/fill_buf.c | 28 +++----------------
> tools/testing/selftests/resctrl/resctrl.h | 3 +-
> .../testing/selftests/resctrl/resctrl_tests.c | 13 ++++-----
> tools/testing/selftests/resctrl/resctrlfs.c | 12 ++++----
> 5 files changed, 19 insertions(+), 43 deletions(-)
>
> diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> index 53df66814cd6..63b551c44f1d 100644
> --- a/tools/testing/selftests/resctrl/cache.c
> +++ b/tools/testing/selftests/resctrl/cache.c
> @@ -210,7 +210,7 @@ int measure_cache_vals(struct resctrl_val_param *param, int bm_pid)
> */
> int cat_val(struct resctrl_val_param *param)
> {
> - int malloc_and_init_memory = 1, memflush = 1, operation = 0, ret = 0;
> + int memflush = 1, operation = 0, ret = 0;
> char *resctrl_val = param->resctrl_val;
> pid_t bm_pid;
>
> @@ -247,8 +247,8 @@ int cat_val(struct resctrl_val_param *param)
> if (ret)
> break;
>
> - if (run_fill_buf(param->span, malloc_and_init_memory,
> - memflush, operation, resctrl_val)) {
> + if (run_fill_buf(param->span, memflush, operation,
> + resctrl_val)) {
> fprintf(stderr, "Error-running fill buffer\n");
> ret = -1;
> goto pe_close;
> diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
> index 785cbd8d0148..d8f5505eb9e6 100644
> --- a/tools/testing/selftests/resctrl/fill_buf.c
> +++ b/tools/testing/selftests/resctrl/fill_buf.c
> @@ -138,36 +138,18 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr,
> return 0;
> }
>
> -static int
> -fill_cache(size_t buf_size, int malloc_and_init, int memflush,
> - int op, char *resctrl_val)
> +static int fill_cache(size_t buf_size, int memflush, int op, char *resctrl_val)
> {
> unsigned char *start_ptr, *end_ptr;
> - unsigned long long i;
> int ret;
>
> - if (malloc_and_init)
> - start_ptr = malloc_and_init_memory(buf_size);
> - else
> - start_ptr = malloc(buf_size);
> -
> + start_ptr = malloc_and_init_memory(buf_size);
> if (!start_ptr)
> return -1;
>
> startptr = start_ptr;
> end_ptr = start_ptr + buf_size;
>
> - /*
> - * It's better to touch the memory once to avoid any compiler
> - * optimizations
> - */
> - if (!malloc_and_init) {
> - for (i = 0; i < buf_size; i++)
> - *start_ptr++ = (unsigned char)rand();
> - }
> -
> - start_ptr = startptr;
> -
> /* Flush the memory before using to avoid "cache hot pages" effect */
> if (memflush)
> mem_flush(start_ptr, buf_size);
> @@ -188,14 +170,12 @@ fill_cache(size_t buf_size, int malloc_and_init, int memflush,
> return 0;
> }
>
> -int run_fill_buf(size_t span, int malloc_and_init_memory, int memflush, int op,
> - char *resctrl_val)
> +int run_fill_buf(size_t span, int memflush, int op, char *resctrl_val)
> {
> size_t cache_size = span;
> int ret;
>
> - ret = fill_cache(cache_size, malloc_and_init_memory, memflush, op,
> - resctrl_val);
> + ret = fill_cache(cache_size, memflush, op, resctrl_val);
> if (ret) {
> printf("\n Error in fill cache\n");
> return -1;
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 52068ceea956..3054cc4ef0e3 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -97,8 +97,7 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
> char *resctrl_val);
> int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu,
> int group_fd, unsigned long flags);
> -int run_fill_buf(size_t span, int malloc_and_init_memory, int memflush, int op,
> - char *resctrl_va);
> +int run_fill_buf(size_t span, int memflush, int op, char *resctrl_val);
> int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param);
> int mbm_bw_change(size_t span, int cpu_no, char *bw_report, char **benchmark_cmd);
> void tests_cleanup(void);
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index bf0cadab36b0..125ed0ca11e3 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -89,7 +89,7 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, size_t span,
> }
>
> if (!has_ben)
> - sprintf(benchmark_cmd[5], "%s", MBA_STR);
> + sprintf(benchmark_cmd[4], "%s", MBA_STR);
> res = mbm_bw_change(span, cpu_no, bw_report, benchmark_cmd);
> ksft_test_result(!res, "MBM: bw change\n");
> if ((get_vendor() == ARCH_INTEL) && res)
> @@ -141,7 +141,7 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
> }
>
> if (!has_ben)
> - sprintf(benchmark_cmd[5], "%s", CMT_STR);
> + sprintf(benchmark_cmd[4], "%s", CMT_STR);
> res = cmt_resctrl_val(cpu_no, 5, benchmark_cmd);
> ksft_test_result(!res, "CMT: test\n");
> if ((get_vendor() == ARCH_INTEL) && res)
> @@ -267,16 +267,15 @@ int main(int argc, char **argv)
> benchmark_cmd[ben_count] = NULL;
> } else {
> /* If no benchmark is given by "-b" argument, use fill_buf. */
> - for (i = 0; i < 6; i++)
> + for (i = 0; i < 5; i++)
> benchmark_cmd[i] = benchmark_cmd_area[i];
>
> strcpy(benchmark_cmd[0], "fill_buf");
> sprintf(benchmark_cmd[1], "%zu", span);
> strcpy(benchmark_cmd[2], "1");
> - strcpy(benchmark_cmd[3], "1");
> - strcpy(benchmark_cmd[4], "0");
> - strcpy(benchmark_cmd[5], "");
> - benchmark_cmd[6] = NULL;
> + strcpy(benchmark_cmd[3], "0");
> + strcpy(benchmark_cmd[4], "");
> + benchmark_cmd[5] = NULL;
> }
Lots of copying - can the above be simplified. Also seeing this
warning:
WARNING: Prefer strscpy over strcpy - see: https://github.com/KSPP/linux/issues/88
#233: FILE: tools/testing/selftests/resctrl/resctrl_tests.c:276:
+ strcpy(benchmark_cmd[3], "0");
WARNING: Prefer strscpy over strcpy - see: https://github.com/KSPP/linux/issues/88
#234: FILE: tools/testing/selftests/resctrl/resctrl_tests.c:277:
+ strcpy(benchmark_cmd[4], "");
total: 0 errors, 2 warnings, 142 lines checked
I am applying the patch set to linux-kselftest next - please fix
the aboe and send a patch on top of linux-kselftest next
thanks,
-- Shuah
Hi Shuah,
On 7/25/2023 7:49 AM, Shuah Khan wrote:
> WARNING: Prefer strscpy over strcpy - see: https://github.com/KSPP/linux/issues/88
> #233: FILE: tools/testing/selftests/resctrl/resctrl_tests.c:276:
> + strcpy(benchmark_cmd[3], "0");
>
> WARNING: Prefer strscpy over strcpy - see: https://github.com/KSPP/linux/issues/88
> #234: FILE: tools/testing/selftests/resctrl/resctrl_tests.c:277:
> + strcpy(benchmark_cmd[4], "");
>
> total: 0 errors, 2 warnings, 142 lines checked
>
> I am applying the patch set to linux-kselftest next - please fix
> the aboe and send a patch on top of linux-kselftest next
Is strscpy() available to userspace? I found lib/strscpy_kunit.c
that makes me think this is currently only available to kernel code.
Reinette
On Tue, 25 Jul 2023, Reinette Chatre wrote:
> Hi Shuah,
>
> On 7/25/2023 7:49 AM, Shuah Khan wrote:
> > WARNING: Prefer strscpy over strcpy - see: https://github.com/KSPP/linux/issues/88
> > #233: FILE: tools/testing/selftests/resctrl/resctrl_tests.c:276:
> > + strcpy(benchmark_cmd[3], "0");
> >
> > WARNING: Prefer strscpy over strcpy - see: https://github.com/KSPP/linux/issues/88
> > #234: FILE: tools/testing/selftests/resctrl/resctrl_tests.c:277:
> > + strcpy(benchmark_cmd[4], "");
> >
> > total: 0 errors, 2 warnings, 142 lines checked
> >
> > I am applying the patch set to linux-kselftest next - please fix
> > the aboe and send a patch on top of linux-kselftest next
>
> Is strscpy() available to userspace? I found lib/strscpy_kunit.c
> that makes me think this is currently only available to kernel code.
It's not available and I've done so little userspace programming in
the recent years (and even less string manipulation in userspace C)
I've no idea what would be the best way to replace it.
However, I've a few patches which will cleanup the benchmark command
handling that include removal of these strcpy()s. Those changes looked
separate enough from the rest I can send it independent of that CAT
rewrite which would have been the next entry in my pending resctrl
selftest patches queue.
On 7/25/23 09:26, Reinette Chatre wrote:
> Hi Shuah,
>
> On 7/25/2023 7:49 AM, Shuah Khan wrote:
>> WARNING: Prefer strscpy over strcpy - see: https://github.com/KSPP/linux/issues/88
>> #233: FILE: tools/testing/selftests/resctrl/resctrl_tests.c:276:
>> + strcpy(benchmark_cmd[3], "0");
>>
>> WARNING: Prefer strscpy over strcpy - see: https://github.com/KSPP/linux/issues/88
>> #234: FILE: tools/testing/selftests/resctrl/resctrl_tests.c:277:
>> + strcpy(benchmark_cmd[4], "");
>>
>> total: 0 errors, 2 warnings, 142 lines checked
>>
>> I am applying the patch set to linux-kselftest next - please fix
>> the aboe and send a patch on top of linux-kselftest next
>
> Is strscpy() available to userspace? I found lib/strscpy_kunit.c
> that makes me think this is currently only available to kernel code.
>
Very likely. You can ignore the comment. I already applied the
series anyway.
thanks,
-- Shuah
On 8/7/2023 11:47 AM, Shuah Khan wrote:
>
> Very likely. You can ignore the comment. I already applied the
> series anyway.
Thank you very much Shuah.
Reinette
@@ -210,7 +210,7 @@ int measure_cache_vals(struct resctrl_val_param *param, int bm_pid)
*/
int cat_val(struct resctrl_val_param *param)
{
- int malloc_and_init_memory = 1, memflush = 1, operation = 0, ret = 0;
+ int memflush = 1, operation = 0, ret = 0;
char *resctrl_val = param->resctrl_val;
pid_t bm_pid;
@@ -247,8 +247,8 @@ int cat_val(struct resctrl_val_param *param)
if (ret)
break;
- if (run_fill_buf(param->span, malloc_and_init_memory,
- memflush, operation, resctrl_val)) {
+ if (run_fill_buf(param->span, memflush, operation,
+ resctrl_val)) {
fprintf(stderr, "Error-running fill buffer\n");
ret = -1;
goto pe_close;
@@ -138,36 +138,18 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr,
return 0;
}
-static int
-fill_cache(size_t buf_size, int malloc_and_init, int memflush,
- int op, char *resctrl_val)
+static int fill_cache(size_t buf_size, int memflush, int op, char *resctrl_val)
{
unsigned char *start_ptr, *end_ptr;
- unsigned long long i;
int ret;
- if (malloc_and_init)
- start_ptr = malloc_and_init_memory(buf_size);
- else
- start_ptr = malloc(buf_size);
-
+ start_ptr = malloc_and_init_memory(buf_size);
if (!start_ptr)
return -1;
startptr = start_ptr;
end_ptr = start_ptr + buf_size;
- /*
- * It's better to touch the memory once to avoid any compiler
- * optimizations
- */
- if (!malloc_and_init) {
- for (i = 0; i < buf_size; i++)
- *start_ptr++ = (unsigned char)rand();
- }
-
- start_ptr = startptr;
-
/* Flush the memory before using to avoid "cache hot pages" effect */
if (memflush)
mem_flush(start_ptr, buf_size);
@@ -188,14 +170,12 @@ fill_cache(size_t buf_size, int malloc_and_init, int memflush,
return 0;
}
-int run_fill_buf(size_t span, int malloc_and_init_memory, int memflush, int op,
- char *resctrl_val)
+int run_fill_buf(size_t span, int memflush, int op, char *resctrl_val)
{
size_t cache_size = span;
int ret;
- ret = fill_cache(cache_size, malloc_and_init_memory, memflush, op,
- resctrl_val);
+ ret = fill_cache(cache_size, memflush, op, resctrl_val);
if (ret) {
printf("\n Error in fill cache\n");
return -1;
@@ -97,8 +97,7 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
char *resctrl_val);
int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu,
int group_fd, unsigned long flags);
-int run_fill_buf(size_t span, int malloc_and_init_memory, int memflush, int op,
- char *resctrl_va);
+int run_fill_buf(size_t span, int memflush, int op, char *resctrl_val);
int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param);
int mbm_bw_change(size_t span, int cpu_no, char *bw_report, char **benchmark_cmd);
void tests_cleanup(void);
@@ -89,7 +89,7 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, size_t span,
}
if (!has_ben)
- sprintf(benchmark_cmd[5], "%s", MBA_STR);
+ sprintf(benchmark_cmd[4], "%s", MBA_STR);
res = mbm_bw_change(span, cpu_no, bw_report, benchmark_cmd);
ksft_test_result(!res, "MBM: bw change\n");
if ((get_vendor() == ARCH_INTEL) && res)
@@ -141,7 +141,7 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
}
if (!has_ben)
- sprintf(benchmark_cmd[5], "%s", CMT_STR);
+ sprintf(benchmark_cmd[4], "%s", CMT_STR);
res = cmt_resctrl_val(cpu_no, 5, benchmark_cmd);
ksft_test_result(!res, "CMT: test\n");
if ((get_vendor() == ARCH_INTEL) && res)
@@ -267,16 +267,15 @@ int main(int argc, char **argv)
benchmark_cmd[ben_count] = NULL;
} else {
/* If no benchmark is given by "-b" argument, use fill_buf. */
- for (i = 0; i < 6; i++)
+ for (i = 0; i < 5; i++)
benchmark_cmd[i] = benchmark_cmd_area[i];
strcpy(benchmark_cmd[0], "fill_buf");
sprintf(benchmark_cmd[1], "%zu", span);
strcpy(benchmark_cmd[2], "1");
- strcpy(benchmark_cmd[3], "1");
- strcpy(benchmark_cmd[4], "0");
- strcpy(benchmark_cmd[5], "");
- benchmark_cmd[6] = NULL;
+ strcpy(benchmark_cmd[3], "0");
+ strcpy(benchmark_cmd[4], "");
+ benchmark_cmd[5] = NULL;
}
sprintf(bw_report, "reads");
@@ -302,7 +302,7 @@ int taskset_benchmark(pid_t bm_pid, int cpu_no)
*/
void run_benchmark(int signum, siginfo_t *info, void *ucontext)
{
- int operation, ret, malloc_and_init_memory, memflush;
+ int operation, ret, memflush;
char **benchmark_cmd;
char resctrl_val[64];
size_t span;
@@ -321,13 +321,11 @@ void run_benchmark(int signum, siginfo_t *info, void *ucontext)
if (strcmp(benchmark_cmd[0], "fill_buf") == 0) {
/* Execute default fill_buf benchmark */
span = strtoul(benchmark_cmd[1], NULL, 10);
- malloc_and_init_memory = atoi(benchmark_cmd[2]);
- memflush = atoi(benchmark_cmd[3]);
- operation = atoi(benchmark_cmd[4]);
- sprintf(resctrl_val, "%s", benchmark_cmd[5]);
+ memflush = atoi(benchmark_cmd[2]);
+ operation = atoi(benchmark_cmd[3]);
+ sprintf(resctrl_val, "%s", benchmark_cmd[4]);
- if (run_fill_buf(span, malloc_and_init_memory, memflush,
- operation, resctrl_val))
+ if (run_fill_buf(span, memflush, operation, resctrl_val))
fprintf(stderr, "Error in running fill buffer\n");
} else {
/* Execute specified benchmark */