The benchmark command handling (-b) in resctrl selftests is overly complicated code. This series turns the benchmark command immutable to preserve it for all selftests and improves benchmark command related error handling.
This series also ends up removing the strcpy() calls which were pointed out earlier.
v3: - Removed DEFAULT_SPAN_STR for real and the duplicated copy of defines that made to v2 likely due to my incorrect conflict resolutions
v2: - Added argument length check into patch 1/7 - Updated also -b line in help message. - Document -b argument related "algorithm" - Use asprintf() to convert defined constant int to string - Improved changelog texts - Added \n to ksft_exit_fail_msg() call messages. - Print DEFAULT_SPAN with %u instead of %zu to avoid need to cast it
Ilpo Järvinen (7): selftests/resctrl: Ensure the benchmark commands fits to its array selftests/resctrl: Correct benchmark command help selftests/resctrl: Remove bw_report and bm_type from main() selftests/resctrl: Simplify span lifetime selftests/resctrl: Make benchmark command const and build it with pointers selftests/resctrl: Remove ben_count variable selftests/resctrl: Cleanup benchmark argument parsing
tools/testing/selftests/resctrl/cache.c | 5 +- tools/testing/selftests/resctrl/cat_test.c | 13 +- tools/testing/selftests/resctrl/cmt_test.c | 34 +++-- tools/testing/selftests/resctrl/mba_test.c | 4 +- tools/testing/selftests/resctrl/mbm_test.c | 7 +- tools/testing/selftests/resctrl/resctrl.h | 17 ++- .../testing/selftests/resctrl/resctrl_tests.c | 120 +++++++++--------- tools/testing/selftests/resctrl/resctrl_val.c | 10 +- 8 files changed, 120 insertions(+), 90 deletions(-)
Benchmark command is copied into an array in the stack. The array is BENCHMARK_ARGS items long but the command line could try to provide a longer command. Argument size is also fixed by BENCHMARK_ARG_SIZE (63 bytes of space after fitting the terminating \0 character) and user could have inputted argument longer than that.
Return error in case the benchmark command does not fit to the space allocated for it.
Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test") Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com --- tools/testing/selftests/resctrl/resctrl_tests.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index d511daeb6851..1e464ebeac47 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -255,9 +255,14 @@ int main(int argc, char **argv) return ksft_exit_skip("Not running as root. Skipping...\n");
if (has_ben) { + if (argc - ben_ind >= BENCHMARK_ARGS - 1) + ksft_exit_fail_msg("Too long benchmark command.\n"); + /* Extract benchmark command from command line. */ for (i = ben_ind; i < argc; i++) { benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i]; + if (strlen(argv[i]) >= BENCHMARK_ARG_SIZE - 1) + ksft_exit_fail_msg("Too long benchmark command argument.\n"); sprintf(benchmark_cmd[i - ben_ind], "%s", argv[i]); } benchmark_cmd[ben_count] = NULL;
Benchmark command must be the last argument because it consumes all the remaining arguments but help misleadingly shows it as the first argument. The benchmark command is also shown in quotes but it does not match with the code.
Correct -b argument place in the help message and remove the quotes. Tweak also how the options are presented by using ... notation.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com --- tools/testing/selftests/resctrl/resctrl_tests.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 1e464ebeac47..de99923fa2e0 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -52,8 +52,8 @@ int get_vendor(void)
static void cmd_help(void) { - printf("usage: resctrl_tests [-h] [-b "benchmark_cmd [options]"] [-t test list] [-n no_of_bits]\n"); - printf("\t-b benchmark_cmd [options]: run specified benchmark for MBM, MBA and CMT\n"); + printf("usage: resctrl_tests [-h] [-t test list] [-n no_of_bits] [-b benchmark_cmd [option]...]\n"); + printf("\t-b benchmark_cmd [option]...: run specified benchmark for MBM, MBA and CMT\n"); printf("\t default benchmark is builtin fill_buf\n"); printf("\t-t test list: run tests specified in the test list, "); printf("e.g. -t mbm,mba,cmt,cat\n");
Hi Ilpo,
On 8/23/2023 6:15 AM, Ilpo Järvinen wrote:
Benchmark command must be the last argument because it consumes all the remaining arguments but help misleadingly shows it as the first argument. The benchmark command is also shown in quotes but it does not match with the code.
Correct -b argument place in the help message and remove the quotes. Tweak also how the options are presented by using ... notation.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com
Thank you.
Reviewed-by: Reinette Chatre reinette.chatre@intel.com
Reinette
bw_report is always set to "reads" and bm_type is set to "fill_buf" but is never used.
Set bw_report directly to "reads" in MBA/MBM test and remove bm_type.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Reviewed-by: Reinette Chatre reinette.chatre@intel.com --- tools/testing/selftests/resctrl/mba_test.c | 4 ++-- tools/testing/selftests/resctrl/mbm_test.c | 4 ++-- tools/testing/selftests/resctrl/resctrl.h | 4 ++-- .../testing/selftests/resctrl/resctrl_tests.c | 18 +++++++----------- 4 files changed, 13 insertions(+), 17 deletions(-)
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c index 4d2f145804b8..094424d835d0 100644 --- a/tools/testing/selftests/resctrl/mba_test.c +++ b/tools/testing/selftests/resctrl/mba_test.c @@ -141,7 +141,7 @@ void mba_test_cleanup(void) remove(RESULT_FILE_NAME); }
-int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd) +int mba_schemata_change(int cpu_no, char **benchmark_cmd) { struct resctrl_val_param param = { .resctrl_val = MBA_STR, @@ -149,7 +149,7 @@ int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd) .mongrp = "m1", .cpu_no = cpu_no, .filename = RESULT_FILE_NAME, - .bw_report = bw_report, + .bw_report = "reads", .setup = mba_setup }; int ret; diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c index c7de6f5977f6..3e4a800e0e40 100644 --- a/tools/testing/selftests/resctrl/mbm_test.c +++ b/tools/testing/selftests/resctrl/mbm_test.c @@ -109,7 +109,7 @@ void mbm_test_cleanup(void) remove(RESULT_FILE_NAME); }
-int mbm_bw_change(size_t span, int cpu_no, char *bw_report, char **benchmark_cmd) +int mbm_bw_change(size_t span, int cpu_no, char **benchmark_cmd) { struct resctrl_val_param param = { .resctrl_val = MBM_STR, @@ -118,7 +118,7 @@ int mbm_bw_change(size_t span, int cpu_no, char *bw_report, char **benchmark_cmd .span = span, .cpu_no = cpu_no, .filename = RESULT_FILE_NAME, - .bw_report = bw_report, + .bw_report = "reads", .setup = mbm_setup }; int ret; diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 838d1a438f33..f3446ac664c2 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -98,10 +98,10 @@ 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 memflush, int op, bool once); 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); +int mbm_bw_change(size_t span, int cpu_no, char **benchmark_cmd); void tests_cleanup(void); void mbm_test_cleanup(void); -int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd); +int mba_schemata_change(int cpu_no, char **benchmark_cmd); void mba_test_cleanup(void); int get_cbm_mask(char *cache_type, char *cbm_mask); int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size); diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index de99923fa2e0..af0d32abf08e 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -70,8 +70,7 @@ void tests_cleanup(void) cat_test_cleanup(); }
-static void run_mbm_test(char **benchmark_cmd, size_t span, - int cpu_no, char *bw_report) +static void run_mbm_test(char **benchmark_cmd, size_t span, int cpu_no) { int res;
@@ -88,7 +87,7 @@ static void run_mbm_test(char **benchmark_cmd, size_t span, goto umount; }
- res = mbm_bw_change(span, cpu_no, bw_report, benchmark_cmd); + res = mbm_bw_change(span, cpu_no, benchmark_cmd); 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"); @@ -97,7 +96,7 @@ static void run_mbm_test(char **benchmark_cmd, size_t span, umount_resctrlfs(); }
-static void run_mba_test(char **benchmark_cmd, int cpu_no, char *bw_report) +static void run_mba_test(char **benchmark_cmd, int cpu_no) { int res;
@@ -114,7 +113,7 @@ static void run_mba_test(char **benchmark_cmd, int cpu_no, char *bw_report) goto umount; }
- res = mba_schemata_change(cpu_no, bw_report, benchmark_cmd); + res = mba_schemata_change(cpu_no, benchmark_cmd); ksft_test_result(!res, "MBA: schemata change\n");
umount: @@ -174,9 +173,9 @@ static void run_cat_test(int cpu_no, int no_of_bits) int main(int argc, char **argv) { bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true; - char *benchmark_cmd[BENCHMARK_ARGS], bw_report[64], bm_type[64]; char benchmark_cmd_area[BENCHMARK_ARGS][BENCHMARK_ARG_SIZE]; int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0; + char *benchmark_cmd[BENCHMARK_ARGS]; int ben_ind, ben_count, tests = 0; size_t span = 250 * MB; bool cat_test = true; @@ -279,9 +278,6 @@ int main(int argc, char **argv) benchmark_cmd[5] = NULL; }
- sprintf(bw_report, "reads"); - sprintf(bm_type, "fill_buf"); - if (!check_resctrlfs_support()) return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n");
@@ -293,10 +289,10 @@ int main(int argc, char **argv) ksft_set_plan(tests ? : 4);
if (mbm_test) - run_mbm_test(benchmark_cmd, span, cpu_no, bw_report); + run_mbm_test(benchmark_cmd, span, cpu_no);
if (mba_test) - run_mba_test(benchmark_cmd, cpu_no, bw_report); + run_mba_test(benchmark_cmd, cpu_no);
if (cmt_test) run_cmt_test(benchmark_cmd, cpu_no);
struct resctrl_val_param contains span member. resctrl_val(), however, never uses it because the value of span is embedded into the default benchmark command and parsed from it by run_benchmark().
Remove span from resctrl_val_param. Provide DEFAULT_SPAN for the code that needs it. CMT and CAT tests communicate span that is different from the DEFAULT_SPAN between their internal functions which is converted into passing it directly as a parameter.
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 | 5 +++-- tools/testing/selftests/resctrl/cat_test.c | 13 +++++++------ tools/testing/selftests/resctrl/cmt_test.c | 11 ++++++----- tools/testing/selftests/resctrl/mbm_test.c | 5 ++--- tools/testing/selftests/resctrl/resctrl.h | 8 ++++---- tools/testing/selftests/resctrl/resctrl_tests.c | 9 ++++----- 6 files changed, 26 insertions(+), 25 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c index d3cbb829ff6a..a0318bd3a63d 100644 --- a/tools/testing/selftests/resctrl/cache.c +++ b/tools/testing/selftests/resctrl/cache.c @@ -205,10 +205,11 @@ int measure_cache_vals(struct resctrl_val_param *param, int bm_pid) * cache_val: execute benchmark and measure LLC occupancy resctrl * and perf cache miss for the benchmark * @param: parameters passed to cache_val() + * @span: buffer size for the benchmark * * Return: 0 on success. non-zero on failure. */ -int cat_val(struct resctrl_val_param *param) +int cat_val(struct resctrl_val_param *param, size_t span) { int memflush = 1, operation = 0, ret = 0; char *resctrl_val = param->resctrl_val; @@ -245,7 +246,7 @@ int cat_val(struct resctrl_val_param *param) if (ret) break;
- if (run_fill_buf(param->span, memflush, operation, true)) { + if (run_fill_buf(span, memflush, operation, true)) { fprintf(stderr, "Error-running fill buffer\n"); ret = -1; goto pe_close; diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 3848dfb46aba..97b87285ab2a 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -41,7 +41,7 @@ static int cat_setup(struct resctrl_val_param *p) return ret; }
-static int check_results(struct resctrl_val_param *param) +static int check_results(struct resctrl_val_param *param, size_t span) { char *token_array[8], temp[512]; unsigned long sum_llc_perf_miss = 0; @@ -76,7 +76,7 @@ static int check_results(struct resctrl_val_param *param) fclose(fp); no_of_bits = count_bits(param->mask);
- return show_cache_info(sum_llc_perf_miss, no_of_bits, param->span / 64, + return show_cache_info(sum_llc_perf_miss, no_of_bits, span / 64, MAX_DIFF, MAX_DIFF_PERCENT, runs - 1, get_vendor() == ARCH_INTEL, false); } @@ -96,6 +96,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) char cbm_mask[256]; int count_of_bits; char pipe_message; + size_t span;
/* Get default cbm mask for L3/L2 cache */ ret = get_cbm_mask(cache_type, cbm_mask); @@ -140,7 +141,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) /* Set param values for parent thread which will be allocated bitmask * with (max_bits - n) bits */ - param.span = cache_size * (count_of_bits - n) / count_of_bits; + span = cache_size * (count_of_bits - n) / count_of_bits; strcpy(param.ctrlgrp, "c2"); strcpy(param.mongrp, "m2"); strcpy(param.filename, RESULT_FILE_NAME2); @@ -162,7 +163,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) param.mask = l_mask_1; strcpy(param.ctrlgrp, "c1"); strcpy(param.mongrp, "m1"); - param.span = cache_size * n / count_of_bits; + span = cache_size * n / count_of_bits; strcpy(param.filename, RESULT_FILE_NAME1); param.num_of_runs = 0; param.cpu_no = sibling_cpu_no; @@ -176,9 +177,9 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
remove(param.filename);
- ret = cat_val(¶m); + ret = cat_val(¶m, span); if (ret == 0) - ret = check_results(¶m); + ret = check_results(¶m, span);
if (bm_pid == 0) { /* Tell parent that child is ready */ diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index cb2197647c6c..9d8e38e995ef 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -27,7 +27,7 @@ static int cmt_setup(struct resctrl_val_param *p) return 0; }
-static int check_results(struct resctrl_val_param *param, int no_of_bits) +static int check_results(struct resctrl_val_param *param, size_t span, int no_of_bits) { char *token_array[8], temp[512]; unsigned long sum_llc_occu_resc = 0; @@ -58,7 +58,7 @@ static int check_results(struct resctrl_val_param *param, int no_of_bits) } fclose(fp);
- return show_cache_info(sum_llc_occu_resc, no_of_bits, param->span, + return show_cache_info(sum_llc_occu_resc, no_of_bits, span, MAX_DIFF, MAX_DIFF_PERCENT, runs - 1, true, true); } @@ -74,6 +74,7 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) unsigned long long_mask; char cbm_mask[256]; int count_of_bits; + size_t span; int ret;
if (!validate_resctrl_feature_request(CMT_STR)) @@ -105,13 +106,13 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) .cpu_no = cpu_no, .filename = RESULT_FILE_NAME, .mask = ~(long_mask << n) & long_mask, - .span = cache_size * n / count_of_bits, .num_of_runs = 0, .setup = cmt_setup, };
+ span = cache_size * n / count_of_bits; if (strcmp(benchmark_cmd[0], "fill_buf") == 0) - sprintf(benchmark_cmd[1], "%zu", param.span); + sprintf(benchmark_cmd[1], "%zu", span);
remove(RESULT_FILE_NAME);
@@ -119,7 +120,7 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) if (ret) goto out;
- ret = check_results(¶m, n); + ret = check_results(¶m, span, n);
out: cmt_test_cleanup(); diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c index 3e4a800e0e40..b830fc84338b 100644 --- a/tools/testing/selftests/resctrl/mbm_test.c +++ b/tools/testing/selftests/resctrl/mbm_test.c @@ -109,13 +109,12 @@ void mbm_test_cleanup(void) remove(RESULT_FILE_NAME); }
-int mbm_bw_change(size_t span, int cpu_no, char **benchmark_cmd) +int mbm_bw_change(int cpu_no, char **benchmark_cmd) { struct resctrl_val_param param = { .resctrl_val = MBM_STR, .ctrlgrp = "c1", .mongrp = "m1", - .span = span, .cpu_no = cpu_no, .filename = RESULT_FILE_NAME, .bw_report = "reads", @@ -129,7 +128,7 @@ int mbm_bw_change(size_t span, int cpu_no, char **benchmark_cmd) if (ret) goto out;
- ret = check_results(span); + ret = check_results(DEFAULT_SPAN);
out: mbm_test_cleanup(); diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index f3446ac664c2..bcd0d2060f81 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -38,6 +38,8 @@
#define END_OF_TESTS 1
+#define DEFAULT_SPAN (250 * MB) + #define PARENT_EXIT(err_msg) \ do { \ perror(err_msg); \ @@ -52,7 +54,6 @@ * @ctrlgrp: Name of the control monitor group (con_mon grp) * @mongrp: Name of the monitor group (mon grp) * @cpu_no: CPU number to which the benchmark would be binded - * @span: Memory bytes accessed in each benchmark iteration * @filename: Name of file to which the o/p should be written * @bw_report: Bandwidth report type (reads vs writes) * @setup: Call back function to setup test environment @@ -62,7 +63,6 @@ struct resctrl_val_param { char ctrlgrp[64]; char mongrp[64]; int cpu_no; - size_t span; char filename[64]; char *bw_report; unsigned long mask; @@ -98,7 +98,7 @@ 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 memflush, int op, bool once); int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param); -int mbm_bw_change(size_t span, int cpu_no, char **benchmark_cmd); +int mbm_bw_change(int cpu_no, char **benchmark_cmd); void tests_cleanup(void); void mbm_test_cleanup(void); int mba_schemata_change(int cpu_no, char **benchmark_cmd); @@ -108,7 +108,7 @@ int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size); void ctrlc_handler(int signum, siginfo_t *info, void *ptr); int signal_handler_register(void); void signal_handler_unregister(void); -int cat_val(struct resctrl_val_param *param); +int cat_val(struct resctrl_val_param *param, size_t span); void cat_test_cleanup(void); int cat_perf_miss_val(int cpu_no, int no_of_bits, char *cache_type); int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd); diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index af0d32abf08e..d4ce8592466c 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -70,7 +70,7 @@ void tests_cleanup(void) cat_test_cleanup(); }
-static void run_mbm_test(char **benchmark_cmd, size_t span, int cpu_no) +static void run_mbm_test(char **benchmark_cmd, int cpu_no) { int res;
@@ -87,7 +87,7 @@ static void run_mbm_test(char **benchmark_cmd, size_t span, int cpu_no) goto umount; }
- res = mbm_bw_change(span, cpu_no, benchmark_cmd); + res = mbm_bw_change(cpu_no, benchmark_cmd); 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"); @@ -177,7 +177,6 @@ int main(int argc, char **argv) int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0; char *benchmark_cmd[BENCHMARK_ARGS]; int ben_ind, ben_count, tests = 0; - size_t span = 250 * MB; bool cat_test = true;
for (i = 0; i < argc; i++) { @@ -271,7 +270,7 @@ int main(int argc, char **argv) benchmark_cmd[i] = benchmark_cmd_area[i];
strcpy(benchmark_cmd[0], "fill_buf"); - sprintf(benchmark_cmd[1], "%zu", span); + sprintf(benchmark_cmd[1], "%u", DEFAULT_SPAN); strcpy(benchmark_cmd[2], "1"); strcpy(benchmark_cmd[3], "0"); strcpy(benchmark_cmd[4], "false"); @@ -289,7 +288,7 @@ int main(int argc, char **argv) ksft_set_plan(tests ? : 4);
if (mbm_test) - run_mbm_test(benchmark_cmd, span, cpu_no); + run_mbm_test(benchmark_cmd, cpu_no);
if (mba_test) run_mba_test(benchmark_cmd, cpu_no);
Benchmark command is used in multiple tests so it should not be mutated by the tests but CMT test alters span argument. Due to the order of tests (CMT test runs last), mutating the span argument in CMT test does not trigger any real problems currently.
Mark benchmark_cmd strings as const and setup the benchmark command using pointers. Because the benchmark command becomes const, the input arguments can be used directly. Besides being simpler, using the input arguments directly also removes the internal size restriction.
CMT test has to create a copy of the benchmark command before altering the benchmark command.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com --- tools/testing/selftests/resctrl/cmt_test.c | 25 +++++++-- tools/testing/selftests/resctrl/mba_test.c | 2 +- tools/testing/selftests/resctrl/mbm_test.c | 2 +- tools/testing/selftests/resctrl/resctrl.h | 11 ++-- .../testing/selftests/resctrl/resctrl_tests.c | 56 ++++++++++--------- tools/testing/selftests/resctrl/resctrl_val.c | 10 +++- 6 files changed, 67 insertions(+), 39 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index 9d8e38e995ef..cf2f5e92dea6 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -68,14 +68,17 @@ void cmt_test_cleanup(void) remove(RESULT_FILE_NAME); }
-int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) +int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd) { + const char * const *cmd = benchmark_cmd; + const char *new_cmd[BENCHMARK_ARGS]; unsigned long cache_size = 0; unsigned long long_mask; + char *span_str = NULL; char cbm_mask[256]; int count_of_bits; size_t span; - int ret; + int ret, i;
if (!validate_resctrl_feature_request(CMT_STR)) return -1; @@ -111,12 +114,23 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) };
span = cache_size * n / count_of_bits; - if (strcmp(benchmark_cmd[0], "fill_buf") == 0) - sprintf(benchmark_cmd[1], "%zu", span); + + if (strcmp(cmd[0], "fill_buf") == 0) { + /* Duplicate the command to be able to replace span in it */ + for (i = 0; benchmark_cmd[i]; i++) + new_cmd[i] = benchmark_cmd[i]; + new_cmd[i] = NULL; + + ret = asprintf(&span_str, "%zu", span); + if (ret < 0) + return -1; + new_cmd[1] = span_str; + cmd = new_cmd; + }
remove(RESULT_FILE_NAME);
- ret = resctrl_val(benchmark_cmd, ¶m); + ret = resctrl_val(cmd, ¶m); if (ret) goto out;
@@ -124,6 +138,7 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
out: cmt_test_cleanup(); + free(span_str);
return ret; } diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c index 094424d835d0..cf8284dadcb2 100644 --- a/tools/testing/selftests/resctrl/mba_test.c +++ b/tools/testing/selftests/resctrl/mba_test.c @@ -141,7 +141,7 @@ void mba_test_cleanup(void) remove(RESULT_FILE_NAME); }
-int mba_schemata_change(int cpu_no, char **benchmark_cmd) +int mba_schemata_change(int cpu_no, const char * const *benchmark_cmd) { struct resctrl_val_param param = { .resctrl_val = MBA_STR, diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c index b830fc84338b..1ae131a2e246 100644 --- a/tools/testing/selftests/resctrl/mbm_test.c +++ b/tools/testing/selftests/resctrl/mbm_test.c @@ -109,7 +109,7 @@ void mbm_test_cleanup(void) remove(RESULT_FILE_NAME); }
-int mbm_bw_change(int cpu_no, char **benchmark_cmd) +int mbm_bw_change(int cpu_no, const char * const *benchmark_cmd) { struct resctrl_val_param param = { .resctrl_val = MBM_STR, diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index bcd0d2060f81..32d23e665697 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -6,6 +6,7 @@ #include <math.h> #include <errno.h> #include <sched.h> +#include <stdint.h> #include <stdlib.h> #include <unistd.h> #include <string.h> @@ -38,6 +39,8 @@
#define END_OF_TESTS 1
+#define BENCHMARK_ARGS 64 + #define DEFAULT_SPAN (250 * MB)
#define PARENT_EXIT(err_msg) \ @@ -97,11 +100,11 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp, 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 memflush, int op, bool once); -int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param); -int mbm_bw_change(int cpu_no, char **benchmark_cmd); +int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *param); +int mbm_bw_change(int cpu_no, const char * const *benchmark_cmd); void tests_cleanup(void); void mbm_test_cleanup(void); -int mba_schemata_change(int cpu_no, char **benchmark_cmd); +int mba_schemata_change(int cpu_no, const char *const *benchmark_cmd); void mba_test_cleanup(void); int get_cbm_mask(char *cache_type, char *cbm_mask); int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size); @@ -111,7 +114,7 @@ void signal_handler_unregister(void); int cat_val(struct resctrl_val_param *param, size_t span); void cat_test_cleanup(void); int cat_perf_miss_val(int cpu_no, int no_of_bits, char *cache_type); -int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd); +int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd); unsigned int count_bits(unsigned long n); void cmt_test_cleanup(void); int get_core_sibling(int cpu_no); diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index d4ce8592466c..84a37bf67306 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -10,9 +10,6 @@ */ #include "resctrl.h"
-#define BENCHMARK_ARGS 64 -#define BENCHMARK_ARG_SIZE 64 - static int detect_vendor(void) { FILE *inf = fopen("/proc/cpuinfo", "r"); @@ -70,7 +67,7 @@ void tests_cleanup(void) cat_test_cleanup(); }
-static void run_mbm_test(char **benchmark_cmd, int cpu_no) +static void run_mbm_test(const char **benchmark_cmd, int cpu_no) { int res;
@@ -96,7 +93,7 @@ static void run_mbm_test(char **benchmark_cmd, int cpu_no) umount_resctrlfs(); }
-static void run_mba_test(char **benchmark_cmd, int cpu_no) +static void run_mba_test(const char **benchmark_cmd, int cpu_no) { int res;
@@ -120,7 +117,7 @@ static void run_mba_test(char **benchmark_cmd, int cpu_no) umount_resctrlfs(); }
-static void run_cmt_test(char **benchmark_cmd, int cpu_no) +static void run_cmt_test(const char **benchmark_cmd, int cpu_no) { int res;
@@ -173,11 +170,13 @@ static void run_cat_test(int cpu_no, int no_of_bits) int main(int argc, char **argv) { bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true; - char benchmark_cmd_area[BENCHMARK_ARGS][BENCHMARK_ARG_SIZE]; int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0; - char *benchmark_cmd[BENCHMARK_ARGS]; + const char *benchmark_cmd[BENCHMARK_ARGS]; int ben_ind, ben_count, tests = 0; + char *span_str = NULL; bool cat_test = true; + char *skip_reason; + int ret;
for (i = 0; i < argc; i++) { if (strcmp(argv[i], "-b") == 0) { @@ -257,31 +256,31 @@ int main(int argc, char **argv) ksft_exit_fail_msg("Too long benchmark command.\n");
/* Extract benchmark command from command line. */ - for (i = ben_ind; i < argc; i++) { - benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i]; - if (strlen(argv[i]) >= BENCHMARK_ARG_SIZE - 1) - ksft_exit_fail_msg("Too long benchmark command argument.\n"); - sprintf(benchmark_cmd[i - ben_ind], "%s", argv[i]); - } + for (i = 0; i < argc - ben_ind; i++) + benchmark_cmd[i] = argv[i + ben_ind]; benchmark_cmd[ben_count] = NULL; } else { /* If no benchmark is given by "-b" argument, use fill_buf. */ - for (i = 0; i < 5; i++) - benchmark_cmd[i] = benchmark_cmd_area[i]; - - strcpy(benchmark_cmd[0], "fill_buf"); - sprintf(benchmark_cmd[1], "%u", DEFAULT_SPAN); - strcpy(benchmark_cmd[2], "1"); - strcpy(benchmark_cmd[3], "0"); - strcpy(benchmark_cmd[4], "false"); + benchmark_cmd[0] = "fill_buf"; + ret = asprintf(&span_str, "%u", DEFAULT_SPAN); + if (ret < 0) + ksft_exit_fail_msg("Out of memory!\n"); + benchmark_cmd[1] = span_str; + benchmark_cmd[2] = "1"; + benchmark_cmd[3] = "0"; + benchmark_cmd[4] = "false"; benchmark_cmd[5] = NULL; }
- if (!check_resctrlfs_support()) - return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n"); + if (!check_resctrlfs_support()) { + skip_reason = "resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n"; + goto free_span; + }
- if (umount_resctrlfs()) - return ksft_exit_skip("resctrl FS unmount failed.\n"); + if (umount_resctrlfs()) { + skip_reason = "resctrl FS unmount failed.\n"; + goto free_span; + }
filter_dmesg();
@@ -299,5 +298,10 @@ int main(int argc, char **argv) if (cat_test) run_cat_test(cpu_no, no_of_bits);
+ free(span_str); ksft_finished(); + +free_span: + free(span_str); + return ksft_exit_skip(skip_reason); } diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index f0f6c5f6e98b..51963a6f2186 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -629,7 +629,7 @@ measure_vals(struct resctrl_val_param *param, unsigned long *bw_resc_start) * * Return: 0 on success. non-zero on failure. */ -int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) +int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *param) { char *resctrl_val = param->resctrl_val; unsigned long bw_resc_start = 0; @@ -710,7 +710,13 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) if (ret) goto out;
- value.sival_ptr = benchmark_cmd; + /* + * The cast removes constness but nothing mutates benchmark_cmd within + * the context of this process. At the receiving process, it becomes + * argv, which is mutable, on exec() but that's after fork() so it + * doesn't matter for the process running the tests. + */ + value.sival_ptr = (void *)benchmark_cmd;
/* Taskset benchmark to specified cpu */ ret = taskset_benchmark(bm_pid, param->cpu_no);
Hi Ilpo,
On 8/23/2023 6:15 AM, Ilpo Järvinen wrote: ...
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index bcd0d2060f81..32d23e665697 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -6,6 +6,7 @@ #include <math.h> #include <errno.h> #include <sched.h> +#include <stdint.h>
What does this header provide?
#include <stdlib.h> #include <unistd.h> #include <string.h> @@ -38,6 +39,8 @@ #define END_OF_TESTS 1 +#define BENCHMARK_ARGS 64
#define DEFAULT_SPAN (250 * MB) #define PARENT_EXIT(err_msg) \ @@ -97,11 +100,11 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp, 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 memflush, int op, bool once); -int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param); -int mbm_bw_change(int cpu_no, char **benchmark_cmd); +int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *param); +int mbm_bw_change(int cpu_no, const char * const *benchmark_cmd); void tests_cleanup(void); void mbm_test_cleanup(void); -int mba_schemata_change(int cpu_no, char **benchmark_cmd); +int mba_schemata_change(int cpu_no, const char *const *benchmark_cmd);
Could you please use consistent spacing ("char * const" vs "char *const")?
void mba_test_cleanup(void); int get_cbm_mask(char *cache_type, char *cbm_mask); int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size); @@ -111,7 +114,7 @@ void signal_handler_unregister(void); int cat_val(struct resctrl_val_param *param, size_t span); void cat_test_cleanup(void); int cat_perf_miss_val(int cpu_no, int no_of_bits, char *cache_type); -int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd); +int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd); unsigned int count_bits(unsigned long n); void cmt_test_cleanup(void); int get_core_sibling(int cpu_no); diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index d4ce8592466c..84a37bf67306 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -10,9 +10,6 @@ */ #include "resctrl.h" -#define BENCHMARK_ARGS 64 -#define BENCHMARK_ARG_SIZE 64
static int detect_vendor(void) { FILE *inf = fopen("/proc/cpuinfo", "r"); @@ -70,7 +67,7 @@ void tests_cleanup(void) cat_test_cleanup(); } -static void run_mbm_test(char **benchmark_cmd, int cpu_no) +static void run_mbm_test(const char **benchmark_cmd, int cpu_no) { int res; @@ -96,7 +93,7 @@ static void run_mbm_test(char **benchmark_cmd, int cpu_no) umount_resctrlfs(); } -static void run_mba_test(char **benchmark_cmd, int cpu_no) +static void run_mba_test(const char **benchmark_cmd, int cpu_no) { int res; @@ -120,7 +117,7 @@ static void run_mba_test(char **benchmark_cmd, int cpu_no) umount_resctrlfs(); } -static void run_cmt_test(char **benchmark_cmd, int cpu_no) +static void run_cmt_test(const char **benchmark_cmd, int cpu_no) { int res;
Could you please elaborate why the above functions have "const char **" instead of "const char * const *"?
@@ -173,11 +170,13 @@ static void run_cat_test(int cpu_no, int no_of_bits) int main(int argc, char **argv) { bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
- char benchmark_cmd_area[BENCHMARK_ARGS][BENCHMARK_ARG_SIZE]; int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
- char *benchmark_cmd[BENCHMARK_ARGS];
- const char *benchmark_cmd[BENCHMARK_ARGS]; int ben_ind, ben_count, tests = 0;
- char *span_str = NULL; bool cat_test = true;
- char *skip_reason;
- int ret;
for (i = 0; i < argc; i++) { if (strcmp(argv[i], "-b") == 0) { @@ -257,31 +256,31 @@ int main(int argc, char **argv) ksft_exit_fail_msg("Too long benchmark command.\n"); /* Extract benchmark command from command line. */
for (i = ben_ind; i < argc; i++) {benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i];if (strlen(argv[i]) >= BENCHMARK_ARG_SIZE - 1)ksft_exit_fail_msg("Too long benchmark command argument.\n");sprintf(benchmark_cmd[i - ben_ind], "%s", argv[i]);}
for (i = 0; i < argc - ben_ind; i++)benchmark_cmd[i] = argv[i + ben_ind];
for (i = 0; i < 5; i++)benchmark_cmd[i] = benchmark_cmd_area[i];strcpy(benchmark_cmd[0], "fill_buf");sprintf(benchmark_cmd[1], "%u", DEFAULT_SPAN);strcpy(benchmark_cmd[2], "1");strcpy(benchmark_cmd[3], "0");strcpy(benchmark_cmd[4], "false");
benchmark_cmd[0] = "fill_buf";ret = asprintf(&span_str, "%u", DEFAULT_SPAN);if (ret < 0)ksft_exit_fail_msg("Out of memory!\n");benchmark_cmd[1] = span_str;benchmark_cmd[2] = "1";benchmark_cmd[3] = "0";benchmark_cmd[4] = "false";
- if (!check_resctrlfs_support())
return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n");
- if (!check_resctrlfs_support()) {
skip_reason = "resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n";goto free_span;- }
- if (umount_resctrlfs())
return ksft_exit_skip("resctrl FS unmount failed.\n");
- if (umount_resctrlfs()) {
skip_reason = "resctrl FS unmount failed.\n";goto free_span;- }
filter_dmesg(); @@ -299,5 +298,10 @@ int main(int argc, char **argv) if (cat_test) run_cat_test(cpu_no, no_of_bits);
- free(span_str); ksft_finished();
+free_span:
- free(span_str);
- return ksft_exit_skip(skip_reason);
}
This is a tricky one. If I understand correctly this goto target makes some assumptions about the state (no test plan created yet) and exit reason (it has to be skipped). A temporary variable is also thrown into the mix. Can this not be simplified by moving the snippet where benchmark_cmd[] is initialized to fill_buf to be just before the tests are run? Perhaps right before ksft_set_plan()? This may be an easier move to consider when the changes in patch 7 are taken into account.
Reinette
On Tue, 29 Aug 2023, Reinette Chatre wrote:
On 8/23/2023 6:15 AM, Ilpo Järvinen wrote: ...
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index bcd0d2060f81..32d23e665697 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -97,11 +100,11 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp, 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 memflush, int op, bool once); -int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param); -int mbm_bw_change(int cpu_no, char **benchmark_cmd); +int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *param); +int mbm_bw_change(int cpu_no, const char * const *benchmark_cmd); void tests_cleanup(void); void mbm_test_cleanup(void); -int mba_schemata_change(int cpu_no, char **benchmark_cmd); +int mba_schemata_change(int cpu_no, const char *const *benchmark_cmd);
Could you please use consistent spacing ("char * const" vs "char *const")?
@@ -120,7 +117,7 @@ static void run_mba_test(char **benchmark_cmd, int cpu_no) umount_resctrlfs(); } -static void run_cmt_test(char **benchmark_cmd, int cpu_no) +static void run_cmt_test(const char **benchmark_cmd, int cpu_no) { int res;
Could you please elaborate why the above functions have "const char **" instead of "const char * const *"?
Thanks for the review!
Sure. I'll make them consistent.
@@ -173,11 +170,13 @@ static void run_cat_test(int cpu_no, int no_of_bits) int main(int argc, char **argv) { bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
- char benchmark_cmd_area[BENCHMARK_ARGS][BENCHMARK_ARG_SIZE]; int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
- char *benchmark_cmd[BENCHMARK_ARGS];
- const char *benchmark_cmd[BENCHMARK_ARGS]; int ben_ind, ben_count, tests = 0;
- char *span_str = NULL; bool cat_test = true;
- char *skip_reason;
- int ret;
for (i = 0; i < argc; i++) { if (strcmp(argv[i], "-b") == 0) { @@ -257,31 +256,31 @@ int main(int argc, char **argv) ksft_exit_fail_msg("Too long benchmark command.\n"); /* Extract benchmark command from command line. */
for (i = ben_ind; i < argc; i++) {benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i];if (strlen(argv[i]) >= BENCHMARK_ARG_SIZE - 1)ksft_exit_fail_msg("Too long benchmark command argument.\n");sprintf(benchmark_cmd[i - ben_ind], "%s", argv[i]);}
for (i = 0; i < argc - ben_ind; i++)benchmark_cmd[i] = argv[i + ben_ind];
for (i = 0; i < 5; i++)benchmark_cmd[i] = benchmark_cmd_area[i];strcpy(benchmark_cmd[0], "fill_buf");sprintf(benchmark_cmd[1], "%u", DEFAULT_SPAN);strcpy(benchmark_cmd[2], "1");strcpy(benchmark_cmd[3], "0");strcpy(benchmark_cmd[4], "false");
benchmark_cmd[0] = "fill_buf";ret = asprintf(&span_str, "%u", DEFAULT_SPAN);if (ret < 0)ksft_exit_fail_msg("Out of memory!\n");benchmark_cmd[1] = span_str;benchmark_cmd[2] = "1";benchmark_cmd[3] = "0";benchmark_cmd[4] = "false";
- if (!check_resctrlfs_support())
return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n");
- if (!check_resctrlfs_support()) {
skip_reason = "resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n";goto free_span;- }
- if (umount_resctrlfs())
return ksft_exit_skip("resctrl FS unmount failed.\n");
- if (umount_resctrlfs()) {
skip_reason = "resctrl FS unmount failed.\n";goto free_span;- }
filter_dmesg(); @@ -299,5 +298,10 @@ int main(int argc, char **argv) if (cat_test) run_cat_test(cpu_no, no_of_bits);
- free(span_str); ksft_finished();
+free_span:
- free(span_str);
- return ksft_exit_skip(skip_reason);
}
This is a tricky one. If I understand correctly this goto target makes some assumptions about the state (no test plan created yet) and exit reason (it has to be skipped). A temporary variable is also thrown into the mix.
So in the end the symmetry proved to be not as simple as was depicted earlier but "tricky"... I tried to warn about this and it's why I wished to avoid the allocation entirely. Without allocation, there would have not been need for the temporary variable nor adjusting the control flow with that label.
Can this not be simplified by moving the snippet where benchmark_cmd[] is initialized to fill_buf to be just before the tests are run? Perhaps right before ksft_set_plan()?
So I throw a temporary variable into the mix (has_ben) to keep track when benchmark_cmd needs to be initialized to the default command? It doesn't play well with what I've in queue after this when user parameters are collected into a struct which is initialized to default value by a helper function before any argument processing. That is, initializing the parameters to defaults needs to be split before and after the parameter parsing code.
This may be an easier move to consider when the changes in patch 7 are taken into account.
Perhaps you could consider accepting my earlier approach which avoided the allocation entirely now that you've seen where this leads to? ...At least you should understand my reasoning for that on much deeper level now.
Hi Ilpo,
On 8/30/2023 1:59 AM, Ilpo Järvinen wrote:
On Tue, 29 Aug 2023, Reinette Chatre wrote:
This is a tricky one. If I understand correctly this goto target makes some assumptions about the state (no test plan created yet) and exit reason (it has to be skipped). A temporary variable is also thrown into the mix.
So in the end the symmetry proved to be not as simple as was depicted earlier but "tricky"... I tried to warn about this and it's why I wished to avoid the allocation entirely. Without allocation, there would have not been need for the temporary variable nor adjusting the control flow with that label.
hmmm ... I do not see why an allocation forces the use of a temporary variable and a change in control (more below).
Can this not be simplified by moving the snippet where benchmark_cmd[] is initialized to fill_buf to be just before the tests are run? Perhaps right before ksft_set_plan()?
So I throw a temporary variable into the mix (has_ben) to keep track when benchmark_cmd needs to be initialized to the default command? It doesn't play well with what I've in queue after this when user parameters are collected into a struct which is initialized to default value by a helper function before any argument processing. That is, initializing the parameters to defaults needs to be split before and after the parameter parsing code.
No new temporary variable is needed. Of course, I do not have insight into what is further down in your queue but based on this work I do think it can be simplified. Since code is easier to consider, the snippet below applies on top of this series and shows what I was proposing:
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index ae9001ef7b0a..8033eabb9aa8 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -170,11 +170,10 @@ static void run_cat_test(int cpu_no, int no_of_bits) int main(int argc, char **argv) { bool mbm_test = true, mba_test = true, cmt_test = true; + const char *benchmark_cmd[BENCHMARK_ARGS] = {}; int c, cpu_no = 1, i, no_of_bits = 0; - const char *benchmark_cmd[BENCHMARK_ARGS]; char *span_str = NULL; bool cat_test = true; - char *skip_reason; int tests = 0; int ret;
@@ -247,17 +246,6 @@ int main(int argc, char **argv) } }
- /* If no benchmark is given by "-b" argument, use fill_buf. */ - benchmark_cmd[0] = "fill_buf"; - ret = asprintf(&span_str, "%u", DEFAULT_SPAN); - if (ret < 0) - ksft_exit_fail_msg("Out of memory!\n"); - benchmark_cmd[1] = span_str; - benchmark_cmd[2] = "1"; - benchmark_cmd[3] = "0"; - benchmark_cmd[4] = "false"; - benchmark_cmd[5] = NULL; - last_arg:
ksft_print_header(); @@ -267,23 +255,30 @@ int main(int argc, char **argv) * 1. We write to resctrl FS * 2. We execute perf commands */ - if (geteuid() != 0) { - skip_reason = "Not running as root. Skipping...\n"; - goto free_span; - } + if (geteuid() != 0) + return ksft_exit_skip("Not running as root. Skipping...\n");
- if (!check_resctrlfs_support()) { - skip_reason = "resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n"; - goto free_span; - } + if (!check_resctrlfs_support()) + return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n");
- if (umount_resctrlfs()) { - skip_reason = "resctrl FS unmount failed.\n"; - goto free_span; - } + if (umount_resctrlfs()) + return ksft_exit_skip("resctrl FS unmount failed.\n");
filter_dmesg();
+ if (!benchmark_cmd[0]) { + /* If no benchmark is given by "-b" argument, use fill_buf. */ + benchmark_cmd[0] = "fill_buf"; + ret = asprintf(&span_str, "%u", DEFAULT_SPAN); + if (ret < 0) + ksft_exit_fail_msg("Out of memory!\n"); + benchmark_cmd[1] = span_str; + benchmark_cmd[2] = "1"; + benchmark_cmd[3] = "0"; + benchmark_cmd[4] = "false"; + benchmark_cmd[5] = NULL; + } + ksft_set_plan(tests ? : 4);
if (mbm_test) @@ -300,8 +295,4 @@ int main(int argc, char **argv)
free(span_str); ksft_finished(); - -free_span: - free(span_str); - return ksft_exit_skip(skip_reason); }
On Wed, 30 Aug 2023, Reinette Chatre wrote:
On 8/30/2023 1:59 AM, Ilpo Järvinen wrote:
On Tue, 29 Aug 2023, Reinette Chatre wrote:
This is a tricky one. If I understand correctly this goto target makes some assumptions about the state (no test plan created yet) and exit reason (it has to be skipped). A temporary variable is also thrown into the mix.
So in the end the symmetry proved to be not as simple as was depicted earlier but "tricky"... I tried to warn about this and it's why I wished to avoid the allocation entirely. Without allocation, there would have not been need for the temporary variable nor adjusting the control flow with that label.
hmmm ... I do not see why an allocation forces the use of a temporary variable and a change in control (more below).
Can this not be simplified by moving the snippet where benchmark_cmd[] is initialized to fill_buf to be just before the tests are run? Perhaps right before ksft_set_plan()?
So I throw a temporary variable into the mix (has_ben) to keep track when benchmark_cmd needs to be initialized to the default command? It doesn't play well with what I've in queue after this when user parameters are collected into a struct which is initialized to default value by a helper function before any argument processing. That is, initializing the parameters to defaults needs to be split before and after the parameter parsing code.
No new temporary variable is needed. Of course, I do not have insight into what is further down in your queue but based on this work I do think it can be simplified. Since code is easier to consider, the snippet below applies on top of this series and shows what I was proposing:
Okay, I'll use the approach you proposed. It'll prevent moving all initialization of the default values before the argument parsing code which I would have wanted to do (in a patch that is not part of this series but it's not an end of the world).
ben_count is only used to write the terminator for the list. It is enough to use i from the loop so no need for another variable.
Remove ben_count variable as it is not needed.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Reviewed-by: Reinette Chatre reinette.chatre@intel.com --- tools/testing/selftests/resctrl/resctrl_tests.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 84a37bf67306..94516d1f4307 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -172,7 +172,7 @@ int main(int argc, char **argv) bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true; int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0; const char *benchmark_cmd[BENCHMARK_ARGS]; - int ben_ind, ben_count, tests = 0; + int ben_ind, tests = 0; char *span_str = NULL; bool cat_test = true; char *skip_reason; @@ -181,7 +181,6 @@ int main(int argc, char **argv) for (i = 0; i < argc; i++) { if (strcmp(argv[i], "-b") == 0) { ben_ind = i + 1; - ben_count = argc - ben_ind; argc_new = ben_ind - 1; has_ben = true; break; @@ -258,7 +257,7 @@ int main(int argc, char **argv) /* Extract benchmark command from command line. */ for (i = 0; i < argc - ben_ind; i++) benchmark_cmd[i] = argv[i + ben_ind]; - benchmark_cmd[ben_count] = NULL; + benchmark_cmd[i] = NULL; } else { /* If no benchmark is given by "-b" argument, use fill_buf. */ benchmark_cmd[0] = "fill_buf";
Benchmark argument is handled by custom argument parsing code which is more complicated than it needs to be.
Process benchmark argument within the normal getopt() handling and drop entirely unnecessary ben_ind and has_ben variables. If -b is not given, setup the default benchmark command right after the switch statement and make -b to goto over it while it terminates the getopt() loop.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Reviewed-by: Reinette Chatre reinette.chatre@intel.com --- .../testing/selftests/resctrl/resctrl_tests.c | 71 ++++++++++--------- 1 file changed, 36 insertions(+), 35 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 94516d1f4307..ae9001ef7b0a 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -169,28 +169,35 @@ static void run_cat_test(int cpu_no, int no_of_bits)
int main(int argc, char **argv) { - bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true; - int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0; + bool mbm_test = true, mba_test = true, cmt_test = true; + int c, cpu_no = 1, i, no_of_bits = 0; const char *benchmark_cmd[BENCHMARK_ARGS]; - int ben_ind, tests = 0; char *span_str = NULL; bool cat_test = true; char *skip_reason; + int tests = 0; int ret;
- for (i = 0; i < argc; i++) { - if (strcmp(argv[i], "-b") == 0) { - ben_ind = i + 1; - argc_new = ben_ind - 1; - has_ben = true; - break; - } - } - - while ((c = getopt(argc_new, argv, "ht:b:n:p:")) != -1) { + while ((c = getopt(argc, argv, "ht:b:n:p:")) != -1) { char *token;
switch (c) { + case 'b': + /* + * First move optind back to the (first) optarg and + * then build the benchmark command using the + * remaining arguments. + */ + optind--; + if (argc - optind >= BENCHMARK_ARGS - 1) + ksft_exit_fail_msg("Too long benchmark command"); + + /* Extract benchmark command from command line. */ + for (i = 0; i < argc - optind; i++) + benchmark_cmd[i] = argv[i + optind]; + benchmark_cmd[i] = NULL; + + goto last_arg; case 't': token = strtok(optarg, ",");
@@ -240,6 +247,19 @@ int main(int argc, char **argv) } }
+ /* If no benchmark is given by "-b" argument, use fill_buf. */ + benchmark_cmd[0] = "fill_buf"; + ret = asprintf(&span_str, "%u", DEFAULT_SPAN); + if (ret < 0) + ksft_exit_fail_msg("Out of memory!\n"); + benchmark_cmd[1] = span_str; + benchmark_cmd[2] = "1"; + benchmark_cmd[3] = "0"; + benchmark_cmd[4] = "false"; + benchmark_cmd[5] = NULL; + +last_arg: + ksft_print_header();
/* @@ -247,28 +267,9 @@ int main(int argc, char **argv) * 1. We write to resctrl FS * 2. We execute perf commands */ - if (geteuid() != 0) - return ksft_exit_skip("Not running as root. Skipping...\n"); - - if (has_ben) { - if (argc - ben_ind >= BENCHMARK_ARGS - 1) - ksft_exit_fail_msg("Too long benchmark command.\n"); - - /* Extract benchmark command from command line. */ - for (i = 0; i < argc - ben_ind; i++) - benchmark_cmd[i] = argv[i + ben_ind]; - benchmark_cmd[i] = NULL; - } else { - /* If no benchmark is given by "-b" argument, use fill_buf. */ - benchmark_cmd[0] = "fill_buf"; - ret = asprintf(&span_str, "%u", DEFAULT_SPAN); - if (ret < 0) - ksft_exit_fail_msg("Out of memory!\n"); - benchmark_cmd[1] = span_str; - benchmark_cmd[2] = "1"; - benchmark_cmd[3] = "0"; - benchmark_cmd[4] = "false"; - benchmark_cmd[5] = NULL; + if (geteuid() != 0) { + skip_reason = "Not running as root. Skipping...\n"; + goto free_span; }
if (!check_resctrlfs_support()) {
Hi,
On 2023-08-23 at 16:15:56 +0300, Ilpo Järvinen wrote:
Benchmark argument is handled by custom argument parsing code which is more complicated than it needs to be.
Process benchmark argument within the normal getopt() handling and drop entirely unnecessary ben_ind and has_ben variables. If -b is not given, setup the default benchmark command right after the switch statement and make -b to goto over it while it terminates the getopt() loop.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Reviewed-by: Reinette Chatre reinette.chatre@intel.com
.../testing/selftests/resctrl/resctrl_tests.c | 71 ++++++++++--------- 1 file changed, 36 insertions(+), 35 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 94516d1f4307..ae9001ef7b0a 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -169,28 +169,35 @@ static void run_cat_test(int cpu_no, int no_of_bits)
int main(int argc, char **argv) {
- bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
- int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
- bool mbm_test = true, mba_test = true, cmt_test = true;
- int c, cpu_no = 1, i, no_of_bits = 0; const char *benchmark_cmd[BENCHMARK_ARGS];
- int ben_ind, tests = 0; char *span_str = NULL; bool cat_test = true; char *skip_reason;
- int tests = 0; int ret;
- for (i = 0; i < argc; i++) {
if (strcmp(argv[i], "-b") == 0) {ben_ind = i + 1;argc_new = ben_ind - 1;has_ben = true;break;}- }
- while ((c = getopt(argc_new, argv, "ht:b:n:p:")) != -1) {
while ((c = getopt(argc, argv, "ht:b:n:p:")) != -1) { char *token;
switch (c) {
case 'b':/** First move optind back to the (first) optarg and* then build the benchmark command using the* remaining arguments.*/optind--;if (argc - optind >= BENCHMARK_ARGS - 1)ksft_exit_fail_msg("Too long benchmark command");
Isn't this condition off by two?
I did some testing and the maximum amount of benchmark arguments is 62 while the array of const char* has 64 spaces. Is it supposed to have less than the maximum capacity?
Wouldn't something like this be more valid with BENCHMARK_ARGS equal to 64? : if (argc - optind > BENCHMARK_ARGS)
On Tue, 29 Aug 2023, Maciej Wieczór-Retman wrote:
On 2023-08-23 at 16:15:56 +0300, Ilpo Järvinen wrote:
Benchmark argument is handled by custom argument parsing code which is more complicated than it needs to be.
Process benchmark argument within the normal getopt() handling and drop entirely unnecessary ben_ind and has_ben variables. If -b is not given, setup the default benchmark command right after the switch statement and make -b to goto over it while it terminates the getopt() loop.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Reviewed-by: Reinette Chatre reinette.chatre@intel.com
.../testing/selftests/resctrl/resctrl_tests.c | 71 ++++++++++--------- 1 file changed, 36 insertions(+), 35 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 94516d1f4307..ae9001ef7b0a 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -169,28 +169,35 @@ static void run_cat_test(int cpu_no, int no_of_bits)
int main(int argc, char **argv) {
- bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
- int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
- bool mbm_test = true, mba_test = true, cmt_test = true;
- int c, cpu_no = 1, i, no_of_bits = 0; const char *benchmark_cmd[BENCHMARK_ARGS];
- int ben_ind, tests = 0; char *span_str = NULL; bool cat_test = true; char *skip_reason;
- int tests = 0; int ret;
- for (i = 0; i < argc; i++) {
if (strcmp(argv[i], "-b") == 0) {ben_ind = i + 1;argc_new = ben_ind - 1;has_ben = true;break;}- }
- while ((c = getopt(argc_new, argv, "ht:b:n:p:")) != -1) {
while ((c = getopt(argc, argv, "ht:b:n:p:")) != -1) { char *token;
switch (c) {
case 'b':/** First move optind back to the (first) optarg and* then build the benchmark command using the* remaining arguments.*/optind--;if (argc - optind >= BENCHMARK_ARGS - 1)ksft_exit_fail_msg("Too long benchmark command");Isn't this condition off by two?
I did some testing and the maximum amount of benchmark arguments is 62 while the array of const char* has 64 spaces. Is it supposed to have less than the maximum capacity?
Wouldn't something like this be more valid with BENCHMARK_ARGS equal to 64? : if (argc - optind > BENCHMARK_ARGS)
Certainly not off by two as the array must be NULL terminated but it seems to be off-by-one (to the safe direction), yes.
On 2023-08-29 at 16:04:29 +0300, Ilpo Järvinen wrote:
On Tue, 29 Aug 2023, Maciej Wieczór-Retman wrote:
On 2023-08-23 at 16:15:56 +0300, Ilpo Järvinen wrote:
Benchmark argument is handled by custom argument parsing code which is more complicated than it needs to be.
Process benchmark argument within the normal getopt() handling and drop entirely unnecessary ben_ind and has_ben variables. If -b is not given, setup the default benchmark command right after the switch statement and make -b to goto over it while it terminates the getopt() loop.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Reviewed-by: Reinette Chatre reinette.chatre@intel.com
.../testing/selftests/resctrl/resctrl_tests.c | 71 ++++++++++--------- 1 file changed, 36 insertions(+), 35 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 94516d1f4307..ae9001ef7b0a 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -169,28 +169,35 @@ static void run_cat_test(int cpu_no, int no_of_bits)
int main(int argc, char **argv) {
- bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
- int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
- bool mbm_test = true, mba_test = true, cmt_test = true;
- int c, cpu_no = 1, i, no_of_bits = 0; const char *benchmark_cmd[BENCHMARK_ARGS];
- int ben_ind, tests = 0; char *span_str = NULL; bool cat_test = true; char *skip_reason;
- int tests = 0; int ret;
- for (i = 0; i < argc; i++) {
if (strcmp(argv[i], "-b") == 0) {ben_ind = i + 1;argc_new = ben_ind - 1;has_ben = true;break;}- }
- while ((c = getopt(argc_new, argv, "ht:b:n:p:")) != -1) {
while ((c = getopt(argc, argv, "ht:b:n:p:")) != -1) { char *token;
switch (c) {
case 'b':/** First move optind back to the (first) optarg and* then build the benchmark command using the* remaining arguments.*/optind--;if (argc - optind >= BENCHMARK_ARGS - 1)ksft_exit_fail_msg("Too long benchmark command");Isn't this condition off by two?
I did some testing and the maximum amount of benchmark arguments is 62 while the array of const char* has 64 spaces. Is it supposed to have less than the maximum capacity?
Wouldn't something like this be more valid with BENCHMARK_ARGS equal to 64? : if (argc - optind > BENCHMARK_ARGS)
Certainly not off by two as the array must be NULL terminated but it seems to be off-by-one (to the safe direction), yes.
Sorry, yes, off by one, now I can see the NULL just after the loop.
Hello Ilpo,
I reviewed and tested this patch series, it looks fine.
Reviewed-by:tan.shaopeng@jp.fujitsu.com Tested-by:tan.shaopeng@jp.fujitsu.com
Best regards, Shaopeng TAN
linux-kselftest-mirror@lists.linaro.org
-
Ilpo Järvinen -
Maciej Wieczór-Retman -
Reinette Chatre -
Shaopeng Tan (Fujitsu)