Re: [PATCH v4 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event

[Babu Moger]

Hi Reinette,

On 10/12/26 21:16, Reinette Chatre wrote:
> Hi Babu,
>
> On 10/12/26 6:02 PM, Moger, Babu wrote:
> > On 10/12/2026 1:44 PM, Reinette Chatre wrote:
> > > On 10/9/26 4:44 PM, Babu Moger wrote:
>
> ...
>
> > > > @@ -1611,6 +1627,19 @@ ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf,
> > > >                mbm_cntr_free_all(r, d);
> > > >                resctrl_reset_rmid_all(r, d);
> > > >            }
> > > > +
> > > > +        /*
> > > > +         * Counters were freed above, so both new groups (via mkdir) and
> > > > +         * the groups that already exist need assignments. Groups created
> > > > +         * while in "default" mode have no counter assigned, including the
> > > > +         * default group created when resctrl is mounted. Assign counters
> > >
> > > This patch refers three times to how "default group" receives counters during auto assignment.
> > >
> > > How important is it that the default group is assigned counters when switching to "mbm_event" mode?
> > >
> > > This patch implies that auto assignment will always include the default group but that cannot be
> > > guaranteed since the default group is always at the end of the rdt_all_groups list. This means
> > > that when traversing rdt_all_groups in resctrl_assign_cntrs_allrdtgrp(), whether default group
> > > receives counters would depend on how many other CTRL_MON and MON groups there are.
> >
> > Good point. I recreated the problem. It is not always guaranteed that the default group gets the counter with the current logic.
> >
> > > > +         * to them so that enabling the mode leaves the same assignments
> > > > +         * that mkdir would have made.
> > >
> > > Related to previous comment is that the changelog mentions "same per-group state that auto-assignment
> > > would have produced" that repeats a similat claim made above that states "same assignments that mkdir
> > > would have made". This behavior assumes that a CTRL_MON group is created with all its MON groups before
> > > the next CTRL_MON group is created *and* that enough counters are available for all groups. If
> > > there are not enough counters then the most recent CTRL_MON groups will receive counters first potentially
> > > leaving the first "mkdir" created CTRL_MON groups without counters.
> > >
> > > It is not clear to me whether default group requires counters. Separately, I do not see whether resctrl
> > > can or should support the same ordering as what "mkdir" would have produced. If these are not actually
> > > requirements then the patch is fine but the comments, documentation change, and changelog need to change
> > > to match the code.
> > >
> > > If the default group does require counters then we either need to change how groups
> > > are added to rdt_all_groups or how resctrl_assign_cntrs_allrdtgrp() traverses the list.
> >
> >
> > We have always treated the default group as a special case. I think
>
> In general I think the special treatment results from the default group
> always being present. It is not clear to me that this implies that the default
> group needs to have counters assigned when switching to mbm_event mode.
>
>
> > we should ensure that the default group gets counters before any
> > other group. How about adding logic to give the default group
> > priority when assigning counters?
>
> I do not find there to be a strong motivation for this assignment but it
> should be fine to establish such expectation at this point. Just to confirm, you
> explicitly only consider the default group and not also the monitoring groups
> belonging to the default group?

It is just the default group. Not the monitoring group under the default 
group.

> > How about something like this?
> >
> > diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> > index 9e705eafe873..5f3711d78a19 100644
> > --- a/fs/resctrl/monitor.c
> > +++ b/fs/resctrl/monitor.c
> > @@ -1339,8 +1339,10 @@ static void resctrl_assign_cntrs_allrdtgrp(void)
> >   {
> >          struct rdtgroup *prgrp, *crgrp;
> >
> > +       rdtgroup_assign_cntrs(&rdtgroup_default);
> >          list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
> > -               rdtgroup_assign_cntrs(prgrp);
> > +               if(prgrp != &rdtgroup_default)
> > +                       rdtgroup_assign_cntrs(prgrp);
> >
> >                  list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list)
> >                          rdtgroup_assign_cntrs(crgrp);
>
> ok. If you go this route, please add a small comment to highlight why the list
> traversal is done in this unexpected way. If you do so, please take care with
> potential duplicate comments between this function and its caller.

ok.

> I do not have a preference about which groups need counters assigned when
> switching to mbm_event mode. Please just ensure that the documentation (resctrl.rst,
> changelog, code comments) match the behavior and explains why.

Ok.
Thanks
Babu