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@xxxxxxxxxxxxxxx>
>>> ---
>>> 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
