Re: [PATCH v4 10/16] selftests/resctrl: Add ->init() callback into resctrl_val_param

[Reinette Chatre <reinette.chatre@xxxxxxxxx>]

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:

...

> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 5dc3def70669..d3fbb957309d 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -86,7 +86,8 @@ struct resctrl_test {
>    * @mongrp:		Name of the monitor group (mon grp)
>    * @filename:		Name of file to which the o/p should be written
>    * @bw_report:		Bandwidth report type (reads vs writes)
> - * @setup:		Call back function to setup test environment
> + * @init:		Callback function to initialize test environment
> + * @setup:		Callback function to setup per test run environment

hmmm ... ok ... this unrelated change could sneak in with the stretch argument of
making the text consistent.

>    * @measure:		Callback that performs the measurement (a single test)
>    */
>   struct resctrl_val_param {
> @@ -97,6 +98,8 @@ struct resctrl_val_param {
>   	char		*bw_report;
>   	unsigned long	mask;
>   	int		num_of_runs;
> +	int		(*init)(const struct resctrl_val_param *param,
> +				int domain_id);
>   	int		(*setup)(const struct resctrl_test *test,
>   				 const struct user_params *uparams,
>   				 struct resctrl_val_param *param);
> @@ -149,8 +152,11 @@ unsigned char *alloc_buffer(size_t buf_size, int memflush);
>   void mem_flush(unsigned char *buf, size_t buf_size);
>   void fill_cache_read(unsigned char *buf, size_t buf_size, bool once);
>   int run_fill_buf(size_t buf_size, int memflush, int op, bool once);
> +int initialize_mem_bw_imc(void);
>   int measure_mem_bw(const struct user_params *uparams,
>   		   struct resctrl_val_param *param, pid_t bm_pid);
> +void initialize_mem_bw_resctrl(const struct resctrl_val_param *param,
> +			       int domain_id);
>   int resctrl_val(const struct resctrl_test *test,
>   		const struct user_params *uparams,
>   		const char * const *benchmark_cmd,
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index 76ae89e1aea3..23a82bd547a3 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -23,18 +23,6 @@
>   #define CON_MBM_LOCAL_BYTES_PATH		\
>   	"%s/%s/mon_data/mon_L3_%02d/mbm_local_bytes"
>   
> -#define CON_MON_LCC_OCCUP_PATH		\
> -	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
> -
> -#define CON_LCC_OCCUP_PATH		\
> -	"%s/%s/mon_data/mon_L3_%02d/llc_occupancy"
> -
> -#define MON_LCC_OCCUP_PATH		\
> -	"%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
> -
> -#define LCC_OCCUP_PATH			\
> -	"%s/mon_data/mon_L3_%02d/llc_occupancy"
> -
>   struct membw_read_format {
>   	__u64 value;         /* The value of the event */
>   	__u64 time_enabled;  /* if PERF_FORMAT_TOTAL_TIME_ENABLED */
> @@ -268,7 +256,7 @@ static int num_of_imcs(void)
>   	return count;
>   }
>   
> -static int initialize_mem_bw_imc(void)
> +int initialize_mem_bw_imc(void)
>   {
>   	int imc, j;
>   
> @@ -430,24 +418,18 @@ static int get_mem_bw_imc(char *bw_report, float *bw_imc)
>   
>   /*
>    * initialize_mem_bw_resctrl:	Appropriately populate "mbm_total_path"
> - * @ctrlgrp:			Name of the control monitor group (con_mon grp)
> - * @domain_id:			Domain ID (cache ID; for MB, L3 cache ID)
> + * @param:	parameters passed to resctrl_val()

Please let description consistently start with upper case.

> + * @domain_id:	Domain ID (cache ID; for MB, L3 cache ID)
>    */
> -static void initialize_mem_bw_resctrl(const char *ctrlgrp, int domain_id)
> +void initialize_mem_bw_resctrl(const struct resctrl_val_param *param,
> +			       int domain_id)
>   {
>   	sprintf(mbm_total_path, CON_MBM_LOCAL_BYTES_PATH, RESCTRL_PATH,
> -		ctrlgrp, domain_id);
> +		param->ctrlgrp, domain_id);
>   }

With typo fixed:

| Reviewed-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>

Reinette