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];
> 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);
> }
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
