Re: [PATCH v5 RESEND 3/7] fs/resctrl: Clear the CDP state after the cleanup

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

Hi Jinjie,

On 11/10/26 5:39 AM, Jinjie Ruan wrote:
> On MPAM a CLOSID is translated into the PARTIDs that the hardware
> configuration uses. Without CDP, CLOSID c is PARTID c; with CDP it is
> PARTIDs 2c and 2c+1. resctrl_unmount() clears the CDP state with
> rdt_disable_ctx() before resctrl_fs_teardown() removes the groups, so
> the teardown translates the CLOSIDs differently from when the groups
> were set up.

This behavior is not unique to MPAM. Above makes this sound like
architecture specific behavior that makes one wonder why this needs
to be fixed in resctrl filesystem

> 
> For example, rdtgroup_unassign_cntrs() unassigns and resets the MBM
> counters of each group through resctrl_arch_config_cntr(). On MPAM that
> helper maps a counter to a monitor with the index
> resctrl_get_config_index() returns for the CDP state. A counter assigned

Please take care not to narrate the code. Instead just describe the flow
on a high level to help understand the change without needing reader to
refer to the code much.

> while CDP was enabled is mapped to the monitor for PARTIDs 2c and 2c+1.
> With CDP cleared, MPAM removes the mapping of PARTID c and resets the
> monitor for PARTID c instead, so the mappings of PARTIDs 2c and 2c+1 are
> left behind. The helper had an empty body on MPAM until the ABMC support
> implemented it, which is when the teardown started to program the
> mapping with the cleared state.
> 
> Clear the CDP state after resctrl_fs_teardown(), so the teardown runs
> with the CDP state the configuration was created under.
> 
> x86 is not affected as its RMIDs and counters do not depend on the
> CLOSID.

I think x86 is lucky here to not have encountered this issue and this fix
applies to it just as much, adding to motivation for this needing to be
included in the filesystem. Below is an example of a changelog that aims
to capture all the comments, you need not use verbatim and are welcome to
improve (and please correct) it:

	To support CDP, resctrl halves the number of CLOSIDs available to
	resource groups and gives each remaining CLOSID two hardware
	configurations instead of one, distinguished by index: 2 * CLOSID for
	data and 2 * CLOSID + 1 for code. Without CDP a CLOSID has a single
	configuration, at index CLOSID. Every architecture computes these
	indices the same way.

	Unmounting clears the CDP state before it undoes what the groups were
	given, so the cleanup computes index CLOSID where the group was set up
	with 2 * CLOSID and 2 * CLOSID + 1. The group's two configurations are
	left in place and index CLOSID is cleared instead, which with CDP
	enabled is a configuration of a different CLOSID. The architecture may
	keep this state for as long as the resource exists.

	Only MPAM can reach this today. Of everything the cleanup has to undo,
	MPAM addresses only the MBM counter assignment by the translated CLOSID,
	and it only began doing so when it implemented assignable counters. x86
	addresses its counters by RMID alone.

	Clear the CDP state after the cleanup, so that the cleanup runs with the
	CDP state the groups were set up under.

> 
> Fixes: 6e0e538a75ce ("arm_mpam: resctrl: Add resctrl_arch_config_cntr() for ABMC use")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Cc: <stable@vger.kernel.org> # 7.3+

Looks like 6e0e538a75ce landed in 7.3 so the stable constraint looks unnecessary?

> ---
Patch looks good to me.

Thank you very much.

Reinette