Re: [PATCH v2] fs/resctrl: Disallow monitor group rename when the RMID changes

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Jinjie,

Thank you very much for picking this fix up.

For the subject, since the RMID actually never changes, could it instead be:
	fs/resctrl: Disallow monitor group rename when the RMID index changes

On 11/5/26 4:35 AM, Jinjie Ruan wrote:
> From: Peter Newman <peternewman@google.com>
> 
> A monitor group can be renamed into the mon_groups directory of another

"renamed" -> "moved"?

(Since this patch would still support rename of MPAM monitor groups.)

> control group to move its tasks to that control group's allocation
> while keeping their monitoring data. Where the RMID is only meaningful
> together with the CLOSID, as on arm64 MPAM where the RMID is a PMG that
> extends the PARTID, this move does not work. The rename succeeds, but:
> 
> - The group's monitoring data is reset. Its counters are identified by
>   the CLOSID and RMID together, so after the move the group reads the
>   counters of the new CLOSID instead of the ones its tasks used.

Does this not also mean that the the monitor group is assigned an RMID
index that is never allocated to it? Sounds like this would cause issues
when the monitor group is removed and resctrl tries to free that RMID.

> 
> - The tasks are not moved. mongrp_reparent() sets the new parent and
>   CLOSID of the group before rdt_move_group_tasks() looks for the tasks
>   to update, and the search then matches on the new CLOSID, which none
>   of the tasks carries yet. The tasks keep the old CLOSID and the old
>   control group's allocation, and the group does not count them.
> 
> The move has never worked on these systems, so rejecting it does not
> take away behavior that an existing user can rely on.

Could this snippet please instead motivate why the issues with monitor
group move are not/cannot be fixed to help explain why this reject
is needed instead?

> 
> Reject the move with -EOPNOTSUPP when it changes the RMID index of the
> group, and document the restriction in resctrl.rst. Where the RMID does
> not depend on the CLOSID, such as on x86, the move keeps the same index
> and is still allowed.


> 
> Fixes: 3e9b35823aab ("arm_mpam: resctrl: Add rmid index helpers")
> Signed-off-by: Peter Newman <peternewman@google.com>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Cc: <stable@vger.kernel.org> # 7.1.x

Looks like 3e9b35823aab was added in 7.1 so the range is not needed?

> Link: https://lore.kernel.org/lkml/20241205153845.394714-1-peternewman@google.com/
> Link: https://lore.kernel.org/lkml/20261031101052.1692230-1-ruanjinjie@huawei.com/

The changelog contains no references to these Link: entries? What are they for?
Looks like first is an earlier version of the patch so does not look to be needed
as part of permanent changelog related to this patch?

Please see: "Ordering of commit tags" in Documentation/process/maintainer-tip.rst
on how changelog can refer to Link: tags.

The second Link: seems to be pointing to the decision that this user space request
needs to be rejected instead of fixed. Looks like the relevant message is

	https://lore.kernel.org/lkml/3a637631-c8ec-4e26-a03c-5ac80d16fab3@arm.com/

you could use similar language as recent commit 
	7782a6a8dfc0 ("x86,fs/resctrl: Keep mbm_assign_mode in default mode at boot")

That points to the decision in a "Link:" tag and then uses text in changelog like:
	"... as decided in the thread at the Link tag." 

Above is not actually what Documentation/process/maintainer-tip.rst documents but it
is what x86 maintainer changed it to after the original commit followed
Documentation/process/maintainer-tip.rst, so I guess either may be ok.


> ---


> ---
>  Documentation/filesystems/resctrl.rst |  7 ++++---
>  fs/resctrl/rdtgroup.c                 | 13 +++++++++++++
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index b52795e03303..9ae34aace0ff 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -552,9 +552,10 @@ will automatically remove all MON groups below it.
>  Moving MON group directories to a new parent CTRL_MON group is supported
>  for the purpose of changing the resource allocations of a MON group
>  without impacting its monitoring data or assigned tasks. This operation
> -is not allowed for MON groups which monitor CPUs. No other move
> -operation is currently allowed other than simply renaming a CTRL_MON or
> -MON group.
> +is not allowed for MON groups which monitor CPUs, and fails with
> +-EOPNOTSUPP on systems where the RMID depends on the CLOSID. No other
> +move operation is currently allowed other than simply renaming a
> +CTRL_MON or MON group.

merging the discussion in 
 https://lore.kernel.org/lkml/113c13ba-e1de-44b1-9a6f-4bd94ecbd71e@arm.com/

To avoid user needing to understand the RMID dependency, how about:

---8<---
@@ -566,10 +566,10 @@ will automatically remove all MON groups below it.
 
 Moving MON group directories to a new parent CTRL_MON group is supported
 for the purpose of changing the resource allocations of a MON group
-without impacting its monitoring data or assigned tasks. This operation
-is not allowed for MON groups which monitor CPUs. No other move
-operation is currently allowed other than simply renaming a CTRL_MON or
-MON group.
+without impacting its monitoring data or assigned tasks. This operation is
+not allowed for MON groups which monitor CPUs, or when the monitoring data
+or assigned tasks would be impacted. No other move operation is currently
+allowed other than simply renaming a CTRL_MON or MON group.
 
 All groups contain the following files:
---8<---

>  
>  All groups contain the following files:
>  
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index e6b64a5c9302..3c002b711024 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -4398,6 +4398,19 @@ static int rdtgroup_rename(struct kernfs_node *kn,
>  		goto out;
>  	}
>  
> +	/*
> +	 * Moving the group changes the CLOSID. Where the RMID is only
> +	 * meaningful together with the CLOSID, the RMID index changes and the
> +	 * monitor state cannot follow the move.
> +	 */
> +	if (rdtgrp->mon.parent != new_prdtgrp && resctrl_arch_mon_capable() &&

The above two checks look unnecessary:
	"rdtgrp->mon.parent != new_prdtgrp" - this check is used to ensure that
	these systems can still just rename existing monitor groups. In this scenario
	the CLOSID will be compared against itself and the two encode() calls will
	return the same index, no?

	"resctrl_arch_mon_capable()" - earlier in function the source is required
	to be a MON group, which can only be possible if the system supports
	monitoring.

> +	    resctrl_arch_rmid_idx_encode(rdtgrp->closid, rdtgrp->mon.rmid) !=
> +	    resctrl_arch_rmid_idx_encode(new_prdtgrp->closid, rdtgrp->mon.rmid)) {
> +		rdt_last_cmd_puts("Changing parent control group not supported\n");

Could this use same terminology as resctrl.rst to help user match documented
behavior with the error message? For example,

		"Moving a MON group to a new CTRL_MON group is not supported\n"
		

> +		ret = -EOPNOTSUPP;
> +		goto out;
> +	}
> +
>  	/*
>  	 * If the MON group is monitoring CPUs, the CPUs must be assigned to the
>  	 * current parent CTRL_MON group and therefore cannot be assigned to

Reinette