Re: [PATCH 5/7] selftests/resctrl: Use pointers to build benchmark cmd and make it const

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, &param);

• ret = resctrl_val(cmd, &param); 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