Re: [PATCH v1 3/6] x86/resctrl: Disallow mongroup rename on MPAM

[Peter Newman]

Hi Reinette,

On Fri, Dec 6, 2024 at 5:14 AM Reinette Chatre
<reinette.chatre@xxxxxxxxx> wrote:
>
> +Tony
>
> Hi Peter,
>
> On 12/5/24 7:03 AM, Peter Newman wrote:
> > On Sat, Apr 6, 2024 at 12:10 AM Peter Newman <peternewman@xxxxxxxxxx> wrote:
> >> On Thu, Apr 4, 2024 at 4:11 PM Reinette Chatre
> >> <reinette.chatre@xxxxxxxxx> wrote:
> >>>
> >>> Hi Peter,
> >>>
> >>> On 3/25/2024 10:27 AM, Peter Newman wrote:
> >>>> Moving a monitoring group to a different parent control assumes that the
> >>>> monitors will not be impacted. This is not the case on MPAM where the
> >>>> PMG is an extension of the PARTID.
> >>>>
> >>>> Detect this situation by requiring the change in CLOSID not to affect
> >>>> the result of resctrl_arch_rmid_idx_encode(), otherwise return
> >>>> -EOPNOTSUPP.
> >>>>
> >>>
> >>> Thanks for catching this. This seems out of place in this series. It sounds
> >>> more like an independent fix that should go in separately.
> >>
> >> I asserted in a comment in a patch later in the series that the
> >> mongroup parent pointer never changes on MPAM, then decided to follow
> >> up on whether it was actually true, so it's only here because this
> >> series depends on it. I'll post it again separately with the fix you
> >> requested below.
> >
> > I'm preparing to finally re-post this patch, but another related
> > question came up...
> >
> > It turns out the check added to mongroup rename in this patch is an
> > important property that the user needs in order to correctly interpret
> > the value returned by info/L3_MON/num_rmids.
> >
> > I had told some Google users to attempt to move a monitoring group to
> > a new parent to determine whether the monitoring groups are
> > independent of their parent ctrl_mon groups. This approach proved
> > unwieldy when used on a system where the maximum number of allowed
> > groups has already been created. (In their problem case, a
> > newly-created process wanted to determine this property independently
> > of the older process which had originally mounted resctrl.)
>
> Could you please elaborate why users need the information that monitoring
> groups are independent of their parent ctrl_mon group?
> I understood from [2] that Arm can be expected to support moving monitor
> groups so I am concerned about userspace building some assumptions like
> "if the monitoring groups are dependent on their parent ctrl_mon groups then
> monitor groups cannot be moved".

It's to make the high-level decision of whether ctrl_mon groups or
mon_groups are to be used as the direct container of a job's tasks.

Given the semantics of PARTIDs vs PMGs on MPAM, the ctrl_mon groups
will always be chosen as the container for tasks, so I don't have a
use case for moving a monitoring group on MPAM.

Perhaps determining whether moving a monitoring group is allowed in
order to decide whether to use the model that requires it is
backwards.

>
> >
> > Also, this information does not require an active rdtgroup pointer or
> > CLOSID/RMID. The following will also work:
> >
> >  resctrl_arch_rmid_idx_encode(0, 0) != resctrl_arch_rmid_idx_encode(1, 0);
> >
> > I propose adding a new info file returning the result of this
> > expression to be placed beside num_rmids. I would name it
> > "mon_id_includes_control_id", as it implies that the hardware
> > logically interprets the monitoring ID as concatenated with its parent
> > control ID. This tells the user whether num_rmids defines the number
> > of monitoring groups (and ctrl_mon groups) which can be created system
> > wide or whether it's one more than the number of monitoring groups
> > which can be created below every ctrl_mon group.
>
> Is the goal purely to guide interpretation of "info/L3_MON/num_rmids"?

Partially, I suppose. It is necessary to know how many monitoring
groups can be created.

>
> The "mon_id_includes_control_id" seems unique to a use case and if I understand
> correctly needs additional interpretation from user space to reach the actual
> data of interest, which I understand to be the number of monitor groups that
> can be created.
>
> A while ago James proposed [1] adding a new "num_groups" inside each "mon_groups"
> directory, on x86 it will be the same as num_rmids, on Arm it will be the maximum PMG bits.
> At a high level(*) I think something like this may be an intuitive way to address this
> issue. What do you think?

On ARM, it would mean num_groups child mon groups can be created,
while on x86, it would mean between 0 and num_groups child mon_groups
can be created.

In either case, all instances of the file would return the same value
at any time.

>
> (*) If considering this I do think it may be more intuitive to have the file
> be at the top level of the control group and be named "num_mon_groups" (or something better)
> to support the idea that it includes the CTRL_MON group self and not that all monitor groups
> are within the mon_groups directory.
>
> Also please keep in mind that during the discussions about moving monitoring groups
> there were some ideas of needing to provide additional information to user space about
> whether counters are reset on a monitor group move. I think that if we are starting
> to look at these random properties as files in resctrl (like "mon_id_includes_control_id"
> and maybe "counter_reset_on_monitor_group_move") then we should consider some alternatives
> to present these flags to prevent resctrl's info directory from turning into a mess.

In the meantime, num_rmids > num_closids will probably be a good
enough heuristic to correctly guess the ID model.

Thanks,
-Peter