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

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

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

On Tue, 19 Mar 2024, Reinette Chatre wrote:
> 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@linux.intel.com>
> > ---
> >  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.

Ah, fixed it now. They seemed to came directly from the old code.

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

Okay, I can look into this too. I've not looked too deeply why the 
difference between MBA and MBM test is there.

-- 
 i.