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.
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: Use pointers to build benchmark cmd and make it const 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 | 32 +++++-- tools/testing/selftests/resctrl/mba_test.c | 4 +- tools/testing/selftests/resctrl/mbm_test.c | 7 +- tools/testing/selftests/resctrl/resctrl.h | 22 +++-- .../testing/selftests/resctrl/resctrl_tests.c | 88 ++++++++----------- tools/testing/selftests/resctrl/resctrl_val.c | 10 ++- 8 files changed, 98 insertions(+), 83 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.
Return error in case the benchmark command does not fit to its array.
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 | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index d511daeb6851..eef9e02516ad 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -255,6 +255,9 @@ 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"); + /* Extract benchmark command from command line. */ for (i = ben_ind; i < argc; i++) { benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i];
Hi Ilpo,
On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
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.
Return error in case the benchmark command does not fit to its array.
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 | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index d511daeb6851..eef9e02516ad 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -255,6 +255,9 @@ 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");
I think there are two potential issues here: too many arguments and too long arguments. Current code can handle 64 (63 with last required to be NULL) arguments each expected to be 64 bytes (63 to end with \0). The above fix looks to be handling the first issue, with this the error message could be more accurate if it refers to the number of arguments instead. It looks to me as though the latter issue still needs to be handled. I understand that this becomes unnecessary via the refactor in following patches but I expect that this fix needs to be backported (cc. stable also) and thus it may benefit from a precision added to the sprintf() that follows the snippet below?
/* Extract benchmark command from command line. */ for (i = ben_ind; i < argc; i++) { benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i];
Reinette
ps. Unless you have an updated email address that works, could you please remove Sai's email from future submissions?
On Mon, 14 Aug 2023, Reinette Chatre wrote:
On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
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.
Return error in case the benchmark command does not fit to its array.
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 | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index d511daeb6851..eef9e02516ad 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -255,6 +255,9 @@ 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");I think there are two potential issues here: too many arguments and too long arguments. Current code can handle 64 (63 with last required to be NULL) arguments each expected to be 64 bytes (63 to end with \0). The above fix looks to be handling the first issue, with this the error message could be more accurate if it refers to the number of arguments instead. It looks to me as though the latter issue still needs to be handled. I understand that this becomes unnecessary via the refactor in following patches but I expect that this fix needs to be backported (cc. stable also) and thus it may benefit from a precision added to the sprintf() that follows the snippet below?
Thanks for taking a look, yes, both are problems. Third problem which still remains is if "fill_buf" is the first argument of -b, the arguments are not validated to match the formatting used internally (I guess it might be intentional to allow overriding the internal fill_buf arguments but just the validation is lacking).
I'll add strlen() check instead of using sprintf() in order to properly fail rather than silently truncating the arguments.
/* Extract benchmark command from command line. */ for (i = ben_ind; i < argc; i++) { benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i];Reinette
ps. Unless you have an updated email address that works, could you please remove Sai's email from future submissions?
It's auto-added by git send-email machinery. I guess I can try to make an exception to my usual workflow by sending only to manually specified To addresses (if I remember). Perhaps one day I'll write a tool to filter out the addresses from git send-email generated ones but as is I don't have one.
Hi Ilpo,
On 8/15/2023 2:10 AM, Ilpo Järvinen wrote:
ps. Unless you have an updated email address that works, could you please remove Sai's email from future submissions?
It's auto-added by git send-email machinery. I guess I can try to make an exception to my usual workflow by sending only to manually specified To addresses (if I remember). Perhaps one day I'll write a tool to filter out the addresses from git send-email generated ones but as is I don't have one.
Which git send-email machinery are you referring to? If I understand correctly it does not automatically pick addresses but you can provide custom commands to it that can do it.
Reinette
On Tue, 15 Aug 2023, Reinette Chatre wrote:
On 8/15/2023 2:10 AM, Ilpo Järvinen wrote:
ps. Unless you have an updated email address that works, could you please remove Sai's email from future submissions?
It's auto-added by git send-email machinery. I guess I can try to make an exception to my usual workflow by sending only to manually specified To addresses (if I remember). Perhaps one day I'll write a tool to filter out the addresses from git send-email generated ones but as is I don't have one.
Which git send-email machinery are you referring to? If I understand correctly it does not automatically pick addresses but you can provide custom commands to it that can do it.
Ah sorry, it is actually scripts/get_maintainer.pl automation I use with git send-email to figure out where to send the patches besides the --to & --cc entries I provided. For this patch, get_maintainer.pl returns this list:
Fenghua Yu fenghua.yu@intel.com (supporter:RDT - RESOURCE ALLOCATION,blamed_fixes:1/1=100%) Reinette Chatre reinette.chatre@intel.com (supporter:RDT - RESOURCE ALLOCATION) Shuah Khan shuah@kernel.org (maintainer:KERNEL SELFTEST FRAMEWORK,blamed_fixes:1/1=100%) Babu Moger babu.moger@amd.com (blamed_fixes:1/1=100%) Sai Praneeth Prakhya sai.praneeth.prakhya@intel.com (blamed_fixes:1/1=100%) linux-kernel@vger.kernel.org (open list:RDT - RESOURCE ALLOCATION) linux-kselftest@vger.kernel.org (open list:KERNEL SELFTEST FRAMEWORK)
...which includes Sai's address (not much I can do about that, it's immutably crafted into git history that those lines were once touched by Sai). I've thought of writing yet another wrapper to filter out known failing addresses but until that's done, either I need to (remember to) manually send the series w/o get_maintainer.pl automation or accept a few failures here and there.
Hi Ilpo,
On 8/15/2023 11:32 PM, Ilpo Järvinen wrote:
Ah sorry, it is actually scripts/get_maintainer.pl automation I use with git send-email to figure out where to send the patches besides the --to & --cc entries I provided. For this patch, get_maintainer.pl returns this list:
Fenghua Yu fenghua.yu@intel.com (supporter:RDT - RESOURCE ALLOCATION,blamed_fixes:1/1=100%) Reinette Chatre reinette.chatre@intel.com (supporter:RDT - RESOURCE ALLOCATION) Shuah Khan shuah@kernel.org (maintainer:KERNEL SELFTEST FRAMEWORK,blamed_fixes:1/1=100%) Babu Moger babu.moger@amd.com (blamed_fixes:1/1=100%) Sai Praneeth Prakhya sai.praneeth.prakhya@intel.com (blamed_fixes:1/1=100%) linux-kernel@vger.kernel.org (open list:RDT - RESOURCE ALLOCATION) linux-kselftest@vger.kernel.org (open list:KERNEL SELFTEST FRAMEWORK)
...which includes Sai's address (not much I can do about that, it's immutably crafted into git history that those lines were once touched by Sai). I've thought of writing yet another wrapper to filter out known failing addresses but until that's done, either I need to (remember to) manually send the series w/o get_maintainer.pl automation or accept a few failures here and there.
You can append Sai's address to .get_maintainer.ignore.
Reinette
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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index eef9e02516ad..559868b16079 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -52,7 +52,7 @@ 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("usage: resctrl_tests [-h] [-t test list] [-n no_of_bits] [-b benchmark_cmd [option]...]\n"); printf("\t-b benchmark_cmd [options]: 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, ");
Hi Ilpo,
On 8/8/2023 2:16 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
tools/testing/selftests/resctrl/resctrl_tests.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index eef9e02516ad..559868b16079 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -52,7 +52,7 @@ 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("usage: resctrl_tests [-h] [-t test list] [-n no_of_bits] [-b benchmark_cmd [option]...]\n"); printf("\t-b benchmark_cmd [options]: run specified benchmark for MBM, MBA and CMT\n");
Considering it was documented goal of patch to use "..." notation, should it be done consistently by changing above line also?
printf("\t default benchmark is builtin fill_buf\n"); printf("\t-t test list: run tests specified in the test list, ");
Reinette
On Mon, 14 Aug 2023, Reinette Chatre wrote:
Hi Ilpo,
On 8/8/2023 2:16 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
tools/testing/selftests/resctrl/resctrl_tests.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index eef9e02516ad..559868b16079 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -52,7 +52,7 @@ 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("usage: resctrl_tests [-h] [-t test list] [-n no_of_bits] [-b benchmark_cmd [option]...]\n"); printf("\t-b benchmark_cmd [options]: run specified benchmark for MBM, MBA and CMT\n");
Considering it was documented goal of patch to use "..." notation, should it be done consistently by changing above line also?
Yes, I'll do that.
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 --- 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 559868b16079..3dcd15a4b813 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; @@ -277,9 +276,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");
@@ -291,10 +287,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);
Hi Ilpo,
On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
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
Reinette
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 --- 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 3dcd15a4b813..903167a192d7 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++) { @@ -269,7 +268,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], "%zu", (size_t)DEFAULT_SPAN); strcpy(benchmark_cmd[2], "1"); strcpy(benchmark_cmd[3], "0"); strcpy(benchmark_cmd[4], "false"); @@ -287,7 +286,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);
Hi Ilpo,
On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
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
If I understand correctly the custom benchmark has always been required to operate on the same span as the default benchmark in order for the results to be measured accurately.
Thank you for eliminating unnecessary code.
Reviewed-by: Reinette Chatre reinette.chatre@intel.com
Reinette
Benchmark parameter uses fixed-size buffers in stack which is slightly dangerous. As benchmark command is used in multiple tests, it should not be mutated by the tests. Due to the order of tests, 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. As span is constant in main(), just provide the default span also as string to be used in setting up the default fill_buf argument so no malloc() is required for it.
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 | 23 ++++++++++--- tools/testing/selftests/resctrl/mba_test.c | 2 +- tools/testing/selftests/resctrl/mbm_test.c | 2 +- tools/testing/selftests/resctrl/resctrl.h | 16 ++++++--- .../testing/selftests/resctrl/resctrl_tests.c | 33 ++++++++----------- tools/testing/selftests/resctrl/resctrl_val.c | 10 ++++-- 6 files changed, 54 insertions(+), 32 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index 9d8e38e995ef..a40e12c3b1a7 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -68,14 +68,16 @@ 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 *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 +113,22 @@ 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); + /* Duplicate the command to be able to replace span in it */ + for (i = 0; benchmark_cmd[i]; i++) + cmd[i] = benchmark_cmd[i]; + cmd[i] = NULL; + + if (strcmp(cmd[0], "fill_buf") == 0) { + span_str = malloc(SIZE_MAX_DECIMAL_SIZE); + if (!span_str) + return -1; + snprintf(span_str, SIZE_MAX_DECIMAL_SIZE, "%zu", span); + cmd[1] = span_str; + }
remove(RESULT_FILE_NAME);
- ret = resctrl_val(benchmark_cmd, ¶m); + ret = resctrl_val(cmd, ¶m); if (ret) goto out;
@@ -124,6 +136,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..ddb1e83a3a64 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,7 +39,14 @@
#define END_OF_TESTS 1
+#define BENCHMARK_ARGS 64 + +/* Approximate %zu max length */ +#define SIZE_MAX_DECIMAL_SIZE (sizeof(SIZE_MAX) * 8 / 3 + 2) + +/* Define default span both as integer and string, these should match */ #define DEFAULT_SPAN (250 * MB) +#define DEFAULT_SPAN_STR "262144000"
#define PARENT_EXIT(err_msg) \ do { \ @@ -97,11 +105,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 +119,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 903167a192d7..74a10abeb01d 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -10,8 +10,9 @@ */ #include "resctrl.h"
-#define BENCHMARK_ARGS 64 -#define BENCHMARK_ARG_SIZE 64 +/* Define default span both as integer and string, these should match */ +#define DEFAULT_SPAN (250 * MB) +#define DEFAULT_SPAN_STR "262144000"
static int detect_vendor(void) { @@ -70,7 +71,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 +97,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 +121,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,9 +174,8 @@ 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; bool cat_test = true;
@@ -257,21 +257,16 @@ int main(int argc, char **argv) ksft_exit_fail_msg("Too long benchmark command");
/* Extract benchmark command from command line. */ - for (i = ben_ind; i < argc; i++) { - benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i]; - 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], "%zu", (size_t)DEFAULT_SPAN); - strcpy(benchmark_cmd[2], "1"); - strcpy(benchmark_cmd[3], "0"); - strcpy(benchmark_cmd[4], "false"); + benchmark_cmd[0] = "fill_buf"; + benchmark_cmd[1] = DEFAULT_SPAN_STR; + benchmark_cmd[2] = "1"; + benchmark_cmd[3] = "0"; + benchmark_cmd[4] = "false"; benchmark_cmd[5] = NULL; }
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/8/2023 2:16 AM, Ilpo Järvinen wrote:
Benchmark parameter uses fixed-size buffers in stack which is slightly dangerous. As benchmark command is used in multiple tests, it should
Could you please be specific with issues with current implementation? The term "slightly dangerous" is vague.
not be mutated by the tests. Due to the order of tests, 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. As span is constant in main(), just provide the default span also as string to be used in setting up the default fill_buf argument so no malloc() is required for it.
What is wrong with using malloc()?
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 | 23 ++++++++++--- tools/testing/selftests/resctrl/mba_test.c | 2 +- tools/testing/selftests/resctrl/mbm_test.c | 2 +- tools/testing/selftests/resctrl/resctrl.h | 16 ++++++--- .../testing/selftests/resctrl/resctrl_tests.c | 33 ++++++++----------- tools/testing/selftests/resctrl/resctrl_val.c | 10 ++++-- 6 files changed, 54 insertions(+), 32 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index 9d8e38e995ef..a40e12c3b1a7 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -68,14 +68,16 @@ 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 *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 +113,22 @@ 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);
- /* Duplicate the command to be able to replace span in it */
- for (i = 0; benchmark_cmd[i]; i++)
cmd[i] = benchmark_cmd[i];- cmd[i] = NULL;
- if (strcmp(cmd[0], "fill_buf") == 0) {
span_str = malloc(SIZE_MAX_DECIMAL_SIZE);if (!span_str)return -1;snprintf(span_str, SIZE_MAX_DECIMAL_SIZE, "%zu", span);
Have you considered asprintf()?
cmd[1] = span_str;- }
It looks to me that array only needs to be duplicated if the default benchmark is used?
remove(RESULT_FILE_NAME);
- ret = resctrl_val(benchmark_cmd, ¶m);
- ret = resctrl_val(cmd, ¶m); if (ret) goto out;
...
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index bcd0d2060f81..ddb1e83a3a64 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,7 +39,14 @@ #define END_OF_TESTS 1 +#define BENCHMARK_ARGS 64
+/* Approximate %zu max length */ +#define SIZE_MAX_DECIMAL_SIZE (sizeof(SIZE_MAX) * 8 / 3 + 2)
+/* Define default span both as integer and string, these should match */ #define DEFAULT_SPAN (250 * MB) +#define DEFAULT_SPAN_STR "262144000"
I think above hardcoding can be eliminated by using asprintf()? This does allocate memory though so I would like to understand why one goal is to not dynamically allocate memory.
#define PARENT_EXIT(err_msg) \ do { \
Reinette
On Mon, 14 Aug 2023, Reinette Chatre wrote:
Hi Ilpo,
On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
Benchmark parameter uses fixed-size buffers in stack which is slightly dangerous. As benchmark command is used in multiple tests, it should
Could you please be specific with issues with current implementation? The term "slightly dangerous" is vague.
I've reworded this so this fragment no longer remains here because the earlier patch got changes so the dangerous part is no longer there.
not be mutated by the tests. Due to the order of tests, 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. As span is constant in main(), just provide the default span also as string to be used in setting up the default fill_buf argument so no malloc() is required for it.
What is wrong with using malloc()?
Nothing. I think you slightly misunderstood what I meant here.
The main challenge is not malloc() itself but keeping track of what memory has been dynamically allocated, which is simple if nothing has been malloc()ed. With the const benchmark command and default span, there's no need to malloc(), thus I avoid it to keep things simpler on the free() side.
I've tried to reword the entire changelog, please check the v2 changelog once I post it.
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 | 23 ++++++++++--- tools/testing/selftests/resctrl/mba_test.c | 2 +- tools/testing/selftests/resctrl/mbm_test.c | 2 +- tools/testing/selftests/resctrl/resctrl.h | 16 ++++++--- .../testing/selftests/resctrl/resctrl_tests.c | 33 ++++++++----------- tools/testing/selftests/resctrl/resctrl_val.c | 10 ++++-- 6 files changed, 54 insertions(+), 32 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index 9d8e38e995ef..a40e12c3b1a7 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -68,14 +68,16 @@ 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 *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 +113,22 @@ 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);
- /* Duplicate the command to be able to replace span in it */
- for (i = 0; benchmark_cmd[i]; i++)
cmd[i] = benchmark_cmd[i];- cmd[i] = NULL;
- if (strcmp(cmd[0], "fill_buf") == 0) {
span_str = malloc(SIZE_MAX_DECIMAL_SIZE);if (!span_str)return -1;snprintf(span_str, SIZE_MAX_DECIMAL_SIZE, "%zu", span);Have you considered asprintf()?
Changed to asprintf() now.
cmd[1] = span_str;- }
It looks to me that array only needs to be duplicated if the default benchmark is used?
While it's true, another aspect is how that affects the code flow. If I make that change, the benchmark command could come from two different places which is now avoided. IMHO, the current approach is simpler to understand even if it does the unnecessary copy of a few pointers.
But please let me know if you still prefer the other way around so I can change to that.
remove(RESULT_FILE_NAME);
- ret = resctrl_val(benchmark_cmd, ¶m);
- ret = resctrl_val(cmd, ¶m); if (ret) goto out;
...
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index bcd0d2060f81..ddb1e83a3a64 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,7 +39,14 @@ #define END_OF_TESTS 1 +#define BENCHMARK_ARGS 64
+/* Approximate %zu max length */ +#define SIZE_MAX_DECIMAL_SIZE (sizeof(SIZE_MAX) * 8 / 3 + 2)
+/* Define default span both as integer and string, these should match */ #define DEFAULT_SPAN (250 * MB) +#define DEFAULT_SPAN_STR "262144000"
I think above hardcoding can be eliminated by using asprintf()? This does allocate memory though so I would like to understand why one goal is to not dynamically allocate memory.
Because it's simpler on the _free() side_. If there's no allocation, no free() is needed.
Only challenge that remains is the int -> string conversion for the default span which can be either done like in the patch or using some preprocessor trickery to convert the number to string. If you prefer the latter, I can change to that so it's not hardcoded both as int and string.
Hi Ilpo,
On 8/15/2023 2:42 AM, Ilpo Järvinen wrote:
On Mon, 14 Aug 2023, Reinette Chatre wrote:
Hi Ilpo,
On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
Benchmark parameter uses fixed-size buffers in stack which is slightly dangerous. As benchmark command is used in multiple tests, it should
Could you please be specific with issues with current implementation? The term "slightly dangerous" is vague.
I've reworded this so this fragment no longer remains here because the earlier patch got changes so the dangerous part is no longer there.
not be mutated by the tests. Due to the order of tests, 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. As span is constant in main(), just provide the default span also as string to be used in setting up the default fill_buf argument so no malloc() is required for it.
What is wrong with using malloc()?
Nothing. I think you slightly misunderstood what I meant here.
The main challenge is not malloc() itself but keeping track of what memory has been dynamically allocated, which is simple if nothing has been malloc()ed. With the const benchmark command and default span, there's no need to malloc(), thus I avoid it to keep things simpler on the free() side.
Keeping things symmetrical helps.
I've tried to reword the entire changelog, please check the v2 changelog once I post it.
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 | 23 ++++++++++--- tools/testing/selftests/resctrl/mba_test.c | 2 +- tools/testing/selftests/resctrl/mbm_test.c | 2 +- tools/testing/selftests/resctrl/resctrl.h | 16 ++++++--- .../testing/selftests/resctrl/resctrl_tests.c | 33 ++++++++----------- tools/testing/selftests/resctrl/resctrl_val.c | 10 ++++-- 6 files changed, 54 insertions(+), 32 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index 9d8e38e995ef..a40e12c3b1a7 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -68,14 +68,16 @@ 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 *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 +113,22 @@ 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);
- /* Duplicate the command to be able to replace span in it */
- for (i = 0; benchmark_cmd[i]; i++)
cmd[i] = benchmark_cmd[i];- cmd[i] = NULL;
- if (strcmp(cmd[0], "fill_buf") == 0) {
span_str = malloc(SIZE_MAX_DECIMAL_SIZE);if (!span_str)return -1;snprintf(span_str, SIZE_MAX_DECIMAL_SIZE, "%zu", span);Have you considered asprintf()?
Changed to asprintf() now.
cmd[1] = span_str;- }
It looks to me that array only needs to be duplicated if the default benchmark is used?
While it's true, another aspect is how that affects the code flow. If I make that change, the benchmark command could come from two different places which is now avoided. IMHO, the current approach is simpler to understand even if it does the unnecessary copy of a few pointers.
cmd provided to resctrl_val() can point to original buffer or modified buffer. What is wrong with a pointer possibly pointing to two different locations?
But please let me know if you still prefer the other way around so I can change to that.
Your motivation for this approach is not clear to me.
remove(RESULT_FILE_NAME);
- ret = resctrl_val(benchmark_cmd, ¶m);
- ret = resctrl_val(cmd, ¶m); if (ret) goto out;
...
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index bcd0d2060f81..ddb1e83a3a64 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,7 +39,14 @@ #define END_OF_TESTS 1 +#define BENCHMARK_ARGS 64
+/* Approximate %zu max length */ +#define SIZE_MAX_DECIMAL_SIZE (sizeof(SIZE_MAX) * 8 / 3 + 2)
+/* Define default span both as integer and string, these should match */ #define DEFAULT_SPAN (250 * MB) +#define DEFAULT_SPAN_STR "262144000"
I think above hardcoding can be eliminated by using asprintf()? This does allocate memory though so I would like to understand why one goal is to not dynamically allocate memory.
Because it's simpler on the _free() side_. If there's no allocation, no free() is needed.
Only challenge that remains is the int -> string conversion for the default span which can be either done like in the patch or using some preprocessor trickery to convert the number to string. If you prefer the latter, I can change to that so it's not hardcoded both as int and string.
This manual int->string sounds like the trickery to me and can be avoided by just using asprintf(). I understand that no free() is needed when no memory is allocated but it looks to me as though these allocations can be symmetrical - allocate the memory before the tests are run and free it after?
Reinette
On Tue, 15 Aug 2023, Reinette Chatre wrote:
On 8/15/2023 2:42 AM, Ilpo Järvinen wrote:
On Mon, 14 Aug 2023, Reinette Chatre wrote:
On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
Benchmark parameter uses fixed-size buffers in stack which is slightly dangerous. As benchmark command is used in multiple tests, it should
Could you please be specific with issues with current implementation? The term "slightly dangerous" is vague.
I've reworded this so this fragment no longer remains here because the earlier patch got changes so the dangerous part is no longer there.
not be mutated by the tests. Due to the order of tests, 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. As span is constant in main(), just provide the default span also as string to be used in setting up the default fill_buf argument so no malloc() is required for it.
What is wrong with using malloc()?
Nothing. I think you slightly misunderstood what I meant here.
The main challenge is not malloc() itself but keeping track of what memory has been dynamically allocated, which is simple if nothing has been malloc()ed. With the const benchmark command and default span, there's no need to malloc(), thus I avoid it to keep things simpler on the free() side.
Keeping things symmetrical helps.
I've tried to reword the entire changelog, please check the v2 changelog once I post it.
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 | 23 ++++++++++--- tools/testing/selftests/resctrl/mba_test.c | 2 +- tools/testing/selftests/resctrl/mbm_test.c | 2 +- tools/testing/selftests/resctrl/resctrl.h | 16 ++++++--- .../testing/selftests/resctrl/resctrl_tests.c | 33 ++++++++----------- tools/testing/selftests/resctrl/resctrl_val.c | 10 ++++-- 6 files changed, 54 insertions(+), 32 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index 9d8e38e995ef..a40e12c3b1a7 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -68,14 +68,16 @@ 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 *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 +113,22 @@ 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);
- /* Duplicate the command to be able to replace span in it */
- for (i = 0; benchmark_cmd[i]; i++)
cmd[i] = benchmark_cmd[i];- cmd[i] = NULL;
- if (strcmp(cmd[0], "fill_buf") == 0) {
span_str = malloc(SIZE_MAX_DECIMAL_SIZE);if (!span_str)return -1;snprintf(span_str, SIZE_MAX_DECIMAL_SIZE, "%zu", span);Have you considered asprintf()?
Changed to asprintf() now.
cmd[1] = span_str;- }
It looks to me that array only needs to be duplicated if the default benchmark is used?
While it's true, another aspect is how that affects the code flow. If I make that change, the benchmark command could come from two different places which is now avoided. IMHO, the current approach is simpler to understand even if it does the unnecessary copy of a few pointers.
cmd provided to resctrl_val() can point to original buffer or modified buffer. What is wrong with a pointer possibly pointing to two different locations?
I'll change to that.
But please let me know if you still prefer the other way around so I can change to that.
Your motivation for this approach is not clear to me.
remove(RESULT_FILE_NAME);
- ret = resctrl_val(benchmark_cmd, ¶m);
- ret = resctrl_val(cmd, ¶m); if (ret) goto out;
...
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index bcd0d2060f81..ddb1e83a3a64 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,7 +39,14 @@ #define END_OF_TESTS 1 +#define BENCHMARK_ARGS 64
+/* Approximate %zu max length */ +#define SIZE_MAX_DECIMAL_SIZE (sizeof(SIZE_MAX) * 8 / 3 + 2)
+/* Define default span both as integer and string, these should match */ #define DEFAULT_SPAN (250 * MB) +#define DEFAULT_SPAN_STR "262144000"
I think above hardcoding can be eliminated by using asprintf()? This does allocate memory though so I would like to understand why one goal is to not dynamically allocate memory.
Because it's simpler on the _free() side_. If there's no allocation, no free() is needed.
Only challenge that remains is the int -> string conversion for the default span which can be either done like in the patch or using some preprocessor trickery to convert the number to string. If you prefer the latter, I can change to that so it's not hardcoded both as int and string.
This manual int->string sounds like the trickery to me and can be avoided by just using asprintf(). I understand that no free() is needed when no memory is allocated but it looks to me as though these allocations can be symmetrical - allocate the memory before the tests are run and free it after?
It could be symmetrical but that means I'll be doing unnecessary alloc if -b is provided which I assume you're against given your comment on always creating copy of cmd in CMT test's case.
I think I'll use similar resolution to this as CMT test does, it has an extra variable which is NULL in when -b is provided so free() is no-op on that path. Then I can use asprintf().
Hi Ilpo,
On 8/16/2023 12:13 AM, Ilpo Järvinen wrote:
On Tue, 15 Aug 2023, Reinette Chatre wrote:
On 8/15/2023 2:42 AM, Ilpo Järvinen wrote:
On Mon, 14 Aug 2023, Reinette Chatre wrote:
On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
...
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index bcd0d2060f81..ddb1e83a3a64 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,7 +39,14 @@ #define END_OF_TESTS 1 +#define BENCHMARK_ARGS 64
+/* Approximate %zu max length */ +#define SIZE_MAX_DECIMAL_SIZE (sizeof(SIZE_MAX) * 8 / 3 + 2)
+/* Define default span both as integer and string, these should match */ #define DEFAULT_SPAN (250 * MB) +#define DEFAULT_SPAN_STR "262144000"
I think above hardcoding can be eliminated by using asprintf()? This does allocate memory though so I would like to understand why one goal is to not dynamically allocate memory.
Because it's simpler on the _free() side_. If there's no allocation, no free() is needed.
Only challenge that remains is the int -> string conversion for the default span which can be either done like in the patch or using some preprocessor trickery to convert the number to string. If you prefer the latter, I can change to that so it's not hardcoded both as int and string.
This manual int->string sounds like the trickery to me and can be avoided by just using asprintf(). I understand that no free() is needed when no memory is allocated but it looks to me as though these allocations can be symmetrical - allocate the memory before the tests are run and free it after?
It could be symmetrical but that means I'll be doing unnecessary alloc if -b is provided which I assume you're against given your comment on always creating copy of cmd in CMT test's case.
I seemed to have lost track here ... could you please elaborate where the unnecessary alloc will be?
I think I'll use similar resolution to this as CMT test does, it has an extra variable which is NULL in when -b is provided so free() is no-op on that path. Then I can use asprintf().
Reinette
On Wed, 16 Aug 2023, Reinette Chatre wrote:
On 8/16/2023 12:13 AM, Ilpo Järvinen wrote:
On Tue, 15 Aug 2023, Reinette Chatre wrote:
On 8/15/2023 2:42 AM, Ilpo Järvinen wrote:
On Mon, 14 Aug 2023, Reinette Chatre wrote:
On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
...
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index bcd0d2060f81..ddb1e83a3a64 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,7 +39,14 @@ #define END_OF_TESTS 1 +#define BENCHMARK_ARGS 64
+/* Approximate %zu max length */ +#define SIZE_MAX_DECIMAL_SIZE (sizeof(SIZE_MAX) * 8 / 3 + 2)
+/* Define default span both as integer and string, these should match */ #define DEFAULT_SPAN (250 * MB) +#define DEFAULT_SPAN_STR "262144000"
I think above hardcoding can be eliminated by using asprintf()? This does allocate memory though so I would like to understand why one goal is to not dynamically allocate memory.
Because it's simpler on the _free() side_. If there's no allocation, no free() is needed.
Only challenge that remains is the int -> string conversion for the default span which can be either done like in the patch or using some preprocessor trickery to convert the number to string. If you prefer the latter, I can change to that so it's not hardcoded both as int and string.
This manual int->string sounds like the trickery to me and can be avoided by just using asprintf(). I understand that no free() is needed when no memory is allocated but it looks to me as though these allocations can be symmetrical - allocate the memory before the tests are run and free it after?
It could be symmetrical but that means I'll be doing unnecessary alloc if -b is provided which I assume you're against given your comment on always creating copy of cmd in CMT test's case.
I seemed to have lost track here ... could you please elaborate where the unnecessary alloc will be?
If there's what you call "symmetry", it implies the code always does alloc. However, the logic in main() is such that when -b is provided, no default benchmark command needs to be assigned, so no alloc for span is necessary. Thus, there either is unnecessary alloc with -b or _no symmetry_.
But I've already converted to asprintf() so no need to continue this discussion.
I think I'll use similar resolution to this as CMT test does, it has an extra variable which is NULL in when -b is provided so free() is no-op on that path. Then I can use asprintf().
Reinette
Hi Ilpo,
On 8/17/2023 1:32 AM, Ilpo Järvinen wrote:
On Wed, 16 Aug 2023, Reinette Chatre wrote:
On 8/16/2023 12:13 AM, Ilpo Järvinen wrote:
On Tue, 15 Aug 2023, Reinette Chatre wrote:
On 8/15/2023 2:42 AM, Ilpo Järvinen wrote:
On Mon, 14 Aug 2023, Reinette Chatre wrote:
On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
...
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > index bcd0d2060f81..ddb1e83a3a64 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,7 +39,14 @@ > > #define END_OF_TESTS 1 > > +#define BENCHMARK_ARGS 64 > + > +/* Approximate %zu max length */ > +#define SIZE_MAX_DECIMAL_SIZE (sizeof(SIZE_MAX) * 8 / 3 + 2) > + > +/* Define default span both as integer and string, these should match */ > #define DEFAULT_SPAN (250 * MB) > +#define DEFAULT_SPAN_STR "262144000"
I think above hardcoding can be eliminated by using asprintf()? This does allocate memory though so I would like to understand why one goal is to not dynamically allocate memory.
Because it's simpler on the _free() side_. If there's no allocation, no free() is needed.
Only challenge that remains is the int -> string conversion for the default span which can be either done like in the patch or using some preprocessor trickery to convert the number to string. If you prefer the latter, I can change to that so it's not hardcoded both as int and string.
This manual int->string sounds like the trickery to me and can be avoided by just using asprintf(). I understand that no free() is needed when no memory is allocated but it looks to me as though these allocations can be symmetrical - allocate the memory before the tests are run and free it after?
It could be symmetrical but that means I'll be doing unnecessary alloc if -b is provided which I assume you're against given your comment on always creating copy of cmd in CMT test's case.
I seemed to have lost track here ... could you please elaborate where the unnecessary alloc will be?
If there's what you call "symmetry", it implies the code always does alloc. However, the logic in main() is such that when -b is provided, no
No. Symmetry does not mean "always alloc" - what I attempted to covey was that tracking allocations become easier if the memory is freed in code that is symmetrical to where the memory is allocated. For example, if memory is allocated at the beginning of main(), then it is freed on exit of main(), or if there is a "test_resources_alloc()" that is called before a test is run then there could be a "test_resources_free()" that is called after a test is run.
default benchmark command needs to be assigned, so no alloc for span is necessary. Thus, there either is unnecessary alloc with -b or _no symmetry_.
But I've already converted to asprintf() so no need to continue this discussion.
Please note that asprintf() allocates memory that needs to be freed.
Reinette
On Thu, 17 Aug 2023, Reinette Chatre wrote:
On 8/17/2023 1:32 AM, Ilpo Järvinen wrote:
On Wed, 16 Aug 2023, Reinette Chatre wrote:
On 8/16/2023 12:13 AM, Ilpo Järvinen wrote:
On Tue, 15 Aug 2023, Reinette Chatre wrote:
On 8/15/2023 2:42 AM, Ilpo Järvinen wrote:
On Mon, 14 Aug 2023, Reinette Chatre wrote: > On 8/8/2023 2:16 AM, Ilpo Järvinen wrote: >> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h >> index bcd0d2060f81..ddb1e83a3a64 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,7 +39,14 @@ >> >> #define END_OF_TESTS 1 >> >> +#define BENCHMARK_ARGS 64 >> + >> +/* Approximate %zu max length */ >> +#define SIZE_MAX_DECIMAL_SIZE (sizeof(SIZE_MAX) * 8 / 3 + 2) >> + >> +/* Define default span both as integer and string, these should match */ >> #define DEFAULT_SPAN (250 * MB) >> +#define DEFAULT_SPAN_STR "262144000" > > I think above hardcoding can be eliminated by using asprintf()? This > does allocate memory though so I would like to understand why one > goal is to not dynamically allocate memory.
Because it's simpler on the _free() side_. If there's no allocation, no free() is needed.
Only challenge that remains is the int -> string conversion for the default span which can be either done like in the patch or using some preprocessor trickery to convert the number to string. If you prefer the latter, I can change to that so it's not hardcoded both as int and string.
This manual int->string sounds like the trickery to me and can be avoided by just using asprintf(). I understand that no free() is needed when no memory is allocated but it looks to me as though these allocations can be symmetrical - allocate the memory before the tests are run and free it after?
It could be symmetrical but that means I'll be doing unnecessary alloc if -b is provided which I assume you're against given your comment on always creating copy of cmd in CMT test's case.
I seemed to have lost track here ... could you please elaborate where the unnecessary alloc will be?
If there's what you call "symmetry", it implies the code always does alloc. However, the logic in main() is such that when -b is provided, no
No. Symmetry does not mean "always alloc"
Oh, so it simply meant code without memory leaks :-).
- what I attempted to covey was
that tracking allocations become easier if the memory is freed in code that is symmetrical to where the memory is allocated.
That's, unfortunately, what I needed to do even if it resulted in less clean code when I, in a later patch that is not part of this series, added a function the setup the default parameters into user parameters struct. main() will now pass that span_str for it to do "symmetrical" free inside main().
For example, if memory is allocated at the beginning of main(), then it is freed on exit of main(),
you make it sound easier than the reality is. There's no singular point that is "exit of main()". It has way too many exit paths because of how selftests framework works. It doesn't give you back control when you ask it to exit the tests.
You'll see how complicated this gets once we get to the user parameters structure patch but I'll use asprintf()+free() for now ;-). We can revisit this discussion if you feel like it when we get to that patch.
...And to think this all is because C cannot easily make known constant int -> string conversion w/o some runtime code.
or if there is a "test_resources_alloc()" that is called before a test is run then there could be a "test_resources_free()" that is called after a test is run.
default benchmark command needs to be assigned, so no alloc for span is necessary. Thus, there either is unnecessary alloc with -b or _no symmetry_.
But I've already converted to asprintf() so no need to continue this discussion.
Please note that asprintf() allocates memory that needs to be freed.
Of course.
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 --- 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 74a10abeb01d..81c2ed299e6f 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -176,13 +176,12 @@ 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; bool cat_test = true;
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; @@ -259,7 +258,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";
Hi Ilpo,
Just a nitpick ... this patch is standing out among the rest by not starting the subject with a capital letter.
On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
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
tools/testing/selftests/resctrl/resctrl_tests.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Reinette Chatre reinette.chatre@intel.com
Reinette
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 --- .../testing/selftests/resctrl/resctrl_tests.c | 56 +++++++++---------- 1 file changed, 25 insertions(+), 31 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 81c2ed299e6f..a437aaa69cc5 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -173,25 +173,27 @@ 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; bool cat_test = true; + int tests = 0;
- 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': + optind--; /* Back to optarg */ + 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, ",");
@@ -241,6 +243,16 @@ int main(int argc, char **argv) } }
+ /* If no benchmark is given by "-b" argument, use fill_buf. */ + benchmark_cmd[0] = "fill_buf"; + benchmark_cmd[1] = DEFAULT_SPAN_STR; + benchmark_cmd[2] = "1"; + benchmark_cmd[3] = "0"; + benchmark_cmd[4] = "false"; + benchmark_cmd[5] = NULL; + +last_arg: + ksft_print_header();
/* @@ -251,24 +263,6 @@ int main(int argc, char **argv) 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"); - - /* 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"; - benchmark_cmd[1] = DEFAULT_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");
Hi Ilpo,
On 8/8/2023 2:16 AM, 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
.../testing/selftests/resctrl/resctrl_tests.c | 56 +++++++++---------- 1 file changed, 25 insertions(+), 31 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 81c2ed299e6f..a437aaa69cc5 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -173,25 +173,27 @@ 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; bool cat_test = true;
- int tests = 0;
- 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':optind--; /* Back to optarg */
The above tab usage is not clear. Also, tail comments in code has been found to disturb reading to the point that x86 contributors are explicitly asked to refrain from using it. Perhaps rather a short summary of algorithm before the code starts?
Apart from this style nitpick it looks good to me. After that is handled you can add:
Reviewed-by: Reinette Chatre reinette.chatre@intel.com
Reinette
linux-kselftest-mirror@lists.linaro.org
-
Ilpo Järvinen -
Reinette Chatre