Re: [PATCH v2] x86/resctrl: Disallow mongroup rename on MPAM

[Reinette Chatre]

Hi Peter,

On 12/5/24 7:38 AM, Peter Newman wrote:
> Moving a monitoring group to a different parent control assumes that the

Should "parent control" perhaps be "parent control group"?

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

I think it is potentially confusing how the changelog jumps between
different architectural acronyms without introduction and
without noting how these architectural acronyms relate to resctrl.

Consider something like below:

	On x86 the hardware monitoring id (the x86 RMID) is independent
	from the hardware control id (the x86 CLOSID). On Arm the
	hardware monitoring id (the MPAM PMG) is dependent on the
	hardware control id (the MPAM PARTID). resctrl associates the
	hardware monitoring id with a resctrl monitoring group using
	the x86 "rmid" term and associates the hardware control id with
	the resctrl control group using the x86 "closid" term.

	User space can move a monitoring group to a different parent
	control group. This results in the monitoring group's control id
	changed to the new parent control group's id. This is safe to do
	on x86 because the monitoring and control ids are independent, but
	not safe on Arm where the ids are dependent. On Arm the destination
	control	group may not have the original monitoring id available for
	use, and if it does, the monitoring counters associated with the new
	control	and monitoring id pair will not reflect	the original monitoring
	group's	data.

	Use resctrl_arch_rmid_idx_encode() to detect if a change in
	the control group id impacts the monitoring id and prevent
	moving a monitor group to a new control group if it does.

Please do correct and feel free to improve.

> 
> Signed-off-by: Peter Newman <peternewman@xxxxxxxxxx>
> ---

It may be helpful to add in the maintainer notes that there is no
Fixes: tag needed because MPAM is not yet supported and thus this issue
cannot be encountered at this time.

> v1->v2:
>  - separated out from earlier series
>  - fixed capitalization in error message
> 
> [v1] https://lore.kernel.org/lkml/20240325172707.73966-4-peternewman@xxxxxxxxxx/
> 
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index d906a1cd84917..8c77496b090cd 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -3888,6 +3888,19 @@ static int rdtgroup_rename(struct kernfs_node *kn,
>  		goto out;
>  	}
>  
> +	/*
> +	 * If changing the CLOSID impacts the RMID, this operation is not
> +	 * supported.
> +	 */
> +	if (resctrl_arch_rmid_idx_encode(rdtgrp->mon.parent->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");
> +		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
> 
> base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37

The change looks good to me.

Thank you.

Reinette