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