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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4621 bytes --]

On Thu, 17 Aug 2023, Reinette Chatre wrote:
> 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"

Oh, so it simply meant code without memory leaks :-).

> - 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.

That's, unfortunately, what I needed to do even if it resulted in less 
clean code when I, in a later patch that is not part of this series, 
added a function the setup the default parameters into user parameters 
struct. main() will now pass that span_str for it to do "symmetrical" 
free inside main().

> For example, if memory
> is allocated at the beginning of main(), then it is freed on exit of main(),

you make it sound easier than the reality is. There's no singular point 
that is "exit of main()". It has way too many exit paths because of how 
selftests framework works. It doesn't give you back control when you ask 
it to exit the tests.

You'll see how complicated this gets once we get to the user parameters 
structure patch but I'll use asprintf()+free() for now ;-). We can revisit 
this discussion if you feel like it when we get to that patch.

...And to think this all is because C cannot easily make known constant 
int -> string conversion w/o some runtime code.

> 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.

Of course.

-- 
 i.