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