Re: [PATCH v2 08/13] selftests/resctrl: Add ->init() callback into resctrl_val_param

[Reinette Chatre]

Hi Ilpo,

On 3/11/2024 6:52 AM, Ilpo Järvinen wrote:
> The struct resctrl_val_param is there to customize behavior inside
> resctrl_val() which is currently not used to full extent and there are
> number of strcmp()s for test name in resctrl_val done by resctrl_val().
> 
> Create ->init() hook into the struct resctrl_val_param to cleanly
> do per test initialization.
> 
> Remove also unused branches to setup paths and the related #defines.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> ---
>  tools/testing/selftests/resctrl/cmt_test.c    |  12 ++
>  tools/testing/selftests/resctrl/mba_test.c    |  24 +++-
>  tools/testing/selftests/resctrl/mbm_test.c    |  24 +++-
>  tools/testing/selftests/resctrl/resctrl.h     |   9 +-
>  tools/testing/selftests/resctrl/resctrl_val.c | 123 ++----------------
>  5 files changed, 75 insertions(+), 117 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> index 241c0b129b58..e79eca9346f3 100644
> --- a/tools/testing/selftests/resctrl/cmt_test.c
> +++ b/tools/testing/selftests/resctrl/cmt_test.c
> @@ -16,6 +16,17 @@
>  #define MAX_DIFF		2000000
>  #define MAX_DIFF_PERCENT	15
>  
> +#define CON_MON_LCC_OCCUP_PATH		\
> +	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
> +
> +static int set_cmt_path(const struct resctrl_val_param *param, int domain_id)
> +{
> +	sprintf(llc_occup_path,	CON_MON_LCC_OCCUP_PATH,	RESCTRL_PATH,

Strangely some tab characters sneaked in above.

> +		param->ctrlgrp, param->mongrp, domain_id);
> +
> +	return 0;
> +}
> +
>  static int cmt_setup(const struct resctrl_test *test,
>  		     const struct user_params *uparams,
>  		     struct resctrl_val_param *p)

..

> @@ -826,17 +729,11 @@ int resctrl_val(const struct resctrl_test *test,
>  	if (ret)
>  		goto out;
>  
> -	if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
> -	    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
> -		ret = initialize_mem_bw_imc();
> +	if (param->init) {
> +		ret = param->init(param, domain_id);
>  		if (ret)
>  			goto out;
> -
> -		initialize_mem_bw_resctrl(param->ctrlgrp, param->mongrp,
> -					  domain_id, resctrl_val);
> -	} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
> -		initialize_llc_occu_resctrl(param->ctrlgrp, param->mongrp,
> -					    domain_id, resctrl_val);
> +	}
>  
>  	/* Parent waits for child to be ready. */
>  	close(pipefd[1]);

I am trying to make sense of what these tests are trying to do and
it is starting to look like the monitor groups (as they are used here)
are unnecessary.

Looking at resctrl_val()->write_bm_pid_to_resctrl() I see that the
control group is created with "bm_pid" written to its "tasks" file
and then a monitor group is created as child of the control group
with the same "bm_pid" written to its "tasks" file.
This looks unnecessary to me because when the original control
group is created on a system that supports monitoring then that
control group would automatically be a monitoring group also. In
the resctrl kernel document these different groups are termed
"CTRL_MON" and "MON" groups respectively.

For example, if user creates control group "c1":
# mkdir /sys/fs/resctrl/c1
Then, automatically it would also be a monitoring group with
its monitoring data available from
/sys/fs/resctrl/c1/mon_data/mon_L3_XX/*

PIDs written to /sys/fs/resctrl/c1/tasks will have allocations
assigned in /sys/fs/resctrl/c1/schemata and monitoring data
/sys/fd/resctrl/c1/mon_data/mon_L3_XX/* ... it is not necessary
to create a separate monitoring group to collect that 
monitoring data.

This seems to be the discrepancy between the MBA and MBM test ...
if I understand correctly they end up needing the same data but
the one uses the data from the CTRL_MON group while the other
creates a redundant child MON group to read the same data
that is available from the CTRL_MON group.

Reinette