Received-SPF: pass (google.com: domain of linux-kernel+bounces-111413-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Fri, 22 Mar 2024 14:22:19 +0200 (EET)
To: Reinette Chatre <reinette.chatre@intel.com>
cc: linux-kselftest@vger.kernel.org, Shuah Khan <shuah@kernel.org>, 
    Babu Moger <babu.moger@amd.com>, 
    =?ISO-8859-15?Q?Maciej_Wiecz=F3r-Retman?= <maciej.wieczor-retman@intel.com>, 
    Fenghua Yu <fenghua.yu@intel.com>, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 08/13] selftests/resctrl: Add ->init() callback into
 resctrl_val_param
In-Reply-To: <a3d11ae1-0ea2-44d1-953c-0e9296582865@intel.com>
Message-ID: <15f0b698-adcc-9e05-90f0-082f0477a618@linux.intel.com>
References: <20240311135230.7007-1-ilpo.jarvinen@linux.intel.com> <20240311135230.7007-9-ilpo.jarvinen@linux.intel.com> <a3d11ae1-0ea2-44d1-953c-0e9296582865@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323328-1967048135-1711110139=:1115"

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-1967048135-1711110139=:1115
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: QUOTED-PRINTABLE

On Tue, 19 Mar 2024, Reinette Chatre wrote:
> On 3/11/2024 6:52 AM, Ilpo J=C3=A4rvinen 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().
> >=20
> > Create ->init() hook into the struct resctrl_val_param to cleanly
> > do per test initialization.
> >=20
> > Remove also unused branches to setup paths and the related #defines.
> >=20
> > Signed-off-by: Ilpo J=C3=A4rvinen <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(-)
> >=20
> > 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=09=092000000
> >  #define MAX_DIFF_PERCENT=0915
> > =20
> > +#define CON_MON_LCC_OCCUP_PATH=09=09\
> > +=09"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
> > +
> > +static int set_cmt_path(const struct resctrl_val_param *param, int dom=
ain_id)
> > +{
> > +=09sprintf(llc_occup_path,=09CON_MON_LCC_OCCUP_PATH,=09RESCTRL_PATH,
>=20
> 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,
> >  =09if (ret)
> >  =09=09goto out;
> > =20
> > -=09if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
> > -=09    !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
> > -=09=09ret =3D initialize_mem_bw_imc();
> > +=09if (param->init) {
> > +=09=09ret =3D param->init(param, domain_id);
> >  =09=09if (ret)
> >  =09=09=09goto out;
> > -
> > -=09=09initialize_mem_bw_resctrl(param->ctrlgrp, param->mongrp,
> > -=09=09=09=09=09  domain_id, resctrl_val);
> > -=09} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
> > -=09=09initialize_llc_occu_resctrl(param->ctrlgrp, param->mongrp,
> > -=09=09=09=09=09    domain_id, resctrl_val);
> > +=09}
> > =20
> >  =09/* Parent waits for child to be ready. */
> >  =09close(pipefd[1]);
>=20
> 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.
>=20
> 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.
>=20
> 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/*
>=20
> 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=20
> monitoring data.
>=20
> 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=20
difference between MBA and MBM test is there.

--=20
 i.

--8323328-1967048135-1711110139=:1115--