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

[Ilpo Järvinen]

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.

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.

But please let me know if you still prefer the other way around so I can 
change to that.

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

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.

-- 
 i.