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

[Reinette Chatre]

Hi Ilpo,

On 8/17/2023 1:32 AM, Ilpo Järvinen wrote:
> On Wed, 16 Aug 2023, Reinette Chatre wrote:
>> On 8/16/2023 12:13 AM, Ilpo Järvinen wrote:
>>> On Tue, 15 Aug 2023, Reinette Chatre wrote:
>>>> On 8/15/2023 2:42 AM, Ilpo Järvinen wrote:
>>>>> On Mon, 14 Aug 2023, Reinette Chatre wrote:
>>>>>>
>>>>>> On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
>>
>> ...
>>>>>>> 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?
>>>
>>> It could be symmetrical but that means I'll be doing unnecessary alloc if 
>>> -b is provided which I assume you're against given your comment on always 
>>> creating copy of cmd in CMT test's case.
>>
>> I seemed to have lost track here ... could you please elaborate where the
>> unnecessary alloc will be?
> 
> If there's what you call "symmetry", it implies the code always does 
> alloc. However, the logic in main() is such that when -b is provided, no 

No. Symmetry does not mean "always alloc" - what I attempted to covey was
that tracking allocations become easier if the memory is freed in code
that is symmetrical to where the memory is allocated. For example, if memory
is allocated at the beginning of main(), then it is freed on exit of main(),
or if there is a "test_resources_alloc()" that is called before a test is
run then there could be a "test_resources_free()" that is called
after a test is run.

> default benchmark command needs to be assigned, so no alloc for span is 
> necessary. Thus, there either is unnecessary alloc with -b or _no 
> symmetry_.
> 
> But I've already converted to asprintf() so no need to continue this 
> discussion.

Please note that asprintf() allocates memory that needs to be freed.

Reinette