Re: [PATCH v5 RESEND 5/7] fs/resctrl: Release the limbo RMIDs when the CDP state changes

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

Hi Jinjie,

On 11/10/26 5:39 AM, Jinjie Ruan wrote:
> On MPAM the counter of an RMID is selected by the PARTIDs of the CLOSID
> that owned it. Without CDP, CLOSID c is PARTID c; with CDP it is
> PARTIDs 2c and 2c+1. An RMID that stays in limbo across an unmount and
> a mount that changes the CDP state is read through the PARTIDs of the
> new state, which are not the PARTIDs its cache lines are tagged with.
> 
> With 5 PARTIDs:
> 
>   mount                  CDP off, CLOSID c -> PARTID c
>   mkdir g1..g4           CLOSIDs 1..4
>   umount                 RMIDs enter limbo with CLOSIDs 1..4
>   mount -o cdp           CDP on, CLOSID c -> PARTIDs 2c, 2c+1
>   limbo handler          reads CLOSID 2 as PARTID 5 > PARTID_MAX 4
>                          MSC error interrupt, mpam_disable()
> 
> The other direction is worse because nothing reports it. If resctrl is
> mounted with CDP and mounted again without it, CLOSID c is read as
> PARTID c, which is in range, so the limbo handler silently tests the
> occupancy of the PARTID of a different CLOSID and frees or keeps the
> RMIDs based on it.
> 
> When a mount changes the CDP state, release the RMIDs in limbo without
> reading them instead of restarting the limbo handler. Cache lines that

This is a resctrl filesystem patch. Please take care to make it clear when
the description refers to architecture specific behavior and when it describes
filesystem behavior that impacts all architectures. First two paragraphs
describe MPAM behavior. Above then jumps to resctrl filesystem behavior
but it describes the filesystem behavior specific to MPAM systems as the
new filesystem behavior for all architectures.

Specifically, above needs a caveat that this new resctrl filesystem behavior
is restricted to systems where RMID depends on CLOSID.

> still carry those RMIDs are accounted to the next user of each RMID.
> 
> The CDP state that matters is the one that changes the PARTIDs used by
> the monitors, which is not necessarily the CDP state of the L3
> resource. MPAM gives every CLOSID two PARTIDs in all MSCs while CDP is
> enabled on any resource, so enabling CDP on L2 alone also changes the
> PARTIDs the L3 monitors use. Add resctrl_arch_mon_cdp_enabled() to let
> the architecture report this. Only MPAM can be affected, so the RMIDs
> are only released where the RMID depends on the CLOSID. On x86 the
> RMID does not depend on the CLOSID, and resctrl_arch_mon_cdp_enabled()
> returns false.

nack. The helper name and value returned on x86 is the first hint that this
is not right. For x86 the value is not a statement about x86: CDP exists on
x86 and can be enabled, so a function called resctrl_arch_mon_cdp_enabled()
returning false reads as wrong until the comment above it explains that what
is really being reported is whether CDP changes the IDs the monitors use.

Every architecture now needs to match this. This looks more like forcing
every architecture to indicate whether it has the same quirk as MPAM.

If there is no way to handle this in MPAM driver then handling this should at
least be contained between resctrl filesystem and MPAM driver. This should not
be forced on all architectures.

I trust the MPAM folks to say whether it can or cannot be done in MPAM driver.
If this is not possible in MPAM driver then it looks like this needs to go back
to include the L2 CDP check as was done in previous version. With this check 
gated by IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) it does hint "MPAM"
but would also need a comment to explain why that is done.

> Reproduced on QEMU with the sequence above:
> 
>   mpam:__mpam_irq_handler: error irq from msc:1 'MSMONCFG_ID_RANGE', partid:5, pmg: 0, ris: 0
>   mpam:mpam_disable: MPAM disabled due to hardware error interrupt
> 
> With the change the sequence completes without an error.
> 
> Fixes: 6789fb99282c ("arm_mpam: resctrl: Add CDP emulation")
> Suggested-by: Ben Horgan <ben.horgan@arm.com>
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Cc: <stable@vger.kernel.org> # 7.1+

Is the "7.1+" constraint needed?


> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index f56d0e32fd06..87d744a168fb 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -70,6 +70,14 @@ static u32 *closid_num_dirty_rmid;
>   */
>  static unsigned int rmid_limbo_count;
>  
> +/*
> + * @limbo_cdp_enabled - Whether CDP changed the hardware IDs the monitors

"hardware IDs" should be invisible to resctrl filesystem. The filesystem and
architecture separation should ideally hide these details from resctrl filesystem.

How about something like below that also makes clear what "true" means:
	"True if the RMIDs in limbo were placed there while CDP was enabled."

> + *			used for a CLOSID when the RMIDs in limbo were freed.
> + *			Only set when CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID

"set" -> "used"? (to highlight that all reads and writes are gated by
CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)

> + *			is enabled.

"enabled" -> "defined" (to match surrounding language)


Please match the spacing of surrounding code by using spaces, not tabs.

> + */
> +static bool limbo_cdp_enabled;
> +
>  /*
>   * @rmid_entry - The entry in the limbo and free lists.
>   */
> @@ -1044,12 +1052,15 @@ int setup_rmid_lru_list(void)
>   * resctrl_limbo_resume() - Restart the limbo handler after a mount.
>   *
>   * The limbo handler stops while resctrl is unmounted. Restart it on every
> - * domain that still has RMIDs in limbo.
> + * domain that still has RMIDs in limbo. Where the RMID depends on the
> + * CLOSID and the mount changed the hardware IDs a CLOSID is monitored

What does "hardware IDs" refer to here? On the resctrl filesystem side I
think "RMID index" will be the accurate term that makes it clear what this
refers to.

> + * with, the RMIDs in limbo can no longer be read: release them instead.
>   */
>  void resctrl_limbo_resume(void)
>  {
>  	struct rdt_l3_mon_domain *d;
>  	struct rdt_resource *r;
> +	bool release = false;
>  
>  	lockdep_assert_cpus_held();
>  	lockdep_assert_held(&rdtgroup_mutex);
> @@ -1057,6 +1068,13 @@ void resctrl_limbo_resume(void)
>  	if (!resctrl_is_mon_event_enabled(QOS_L3_OCCUP_EVENT_ID))
>  		return;
>  
> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> +		bool cdp_enabled = resctrl_arch_mon_cdp_enabled();
> +
> +		release = cdp_enabled != limbo_cdp_enabled;
> +		limbo_cdp_enabled = cdp_enabled;
> +	}
> +
>  	r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
>  	list_for_each_entry_rcu(d, &r->mon_domains, hdr.list,
>  				lockdep_is_cpus_held()) {
> @@ -1068,7 +1086,10 @@ void resctrl_limbo_resume(void)
>  		 * it is queued again on a CPU picked now.
>  		 */
>  		cancel_delayed_work(&d->cqm_limbo);
> -		cqm_setup_limbo_handler(d, 0, RESCTRL_PICK_ANY_CPU);
> +		if (release)
> +			release_limbo_rmids(d);
> +		else
> +			cqm_setup_limbo_handler(d, 0, RESCTRL_PICK_ANY_CPU);
>  	}
>  }
>  

Reinette