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

[Ilpo Järvinen]

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:
> >>> 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? 

I'll change to that.

> > 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, &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.
> > 
> 
> 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 think I'll use similar resolution to this as CMT test does, it has an 
extra variable which is NULL in when -b is provided so free() is no-op
on that path. Then I can use asprintf().

-- 
 i.