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