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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 22 Mar 2024 14:22:19 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Reinette Chatre <reinette.chatre@...el.com>
cc: linux-kselftest@...r.kernel.org, Shuah Khan <shuah@...nel.org>, 
    Babu Moger <babu.moger@....com>, 
    Maciej Wieczór-Retman <maciej.wieczor-retman@...el.com>, 
    Fenghua Yu <fenghua.yu@...el.com>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 08/13] selftests/resctrl: Add ->init() callback into
 resctrl_val_param

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