Re: [PATCH] fs/resctrl: Unmount resctrl with the CDP state it was mounted with

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

Hi Jinjie,

On 10/31/26 2:46 AM, Jinjie Ruan wrote:
> Busy RMIDs of a monitor domain are normally released by the limbo handler
> once their cache occupancy drops below the threshold. The handler keeps
> running after resctrl is unmounted, and  each RMID left in limbo retains
> the CLOSID it was allocated with.
> 
> When reading the counter for an RMID in limbo, the architecture translates
> the CLOSID associated with that RMID into a PARTID. On arm64 MPAM with CDP
> enabled, CLOSID c maps to data PARTID 2 * c and code PARTID 2 * c + 1,
> so the same CLOSID selects a different PARTID depending on the CDP state.
> resctrl_unmount() clears the CDP state before resctrl_fs_teardown() frees
> the groups, so an RMID left in limbo is read using the CDP state of the
> next mount rather than the state it was allocated under. A CLOSID from the
> upper half of the range then produces a PARTID beyond the MSC's range; the
> MSC raises an MPAM error interrupt, and mpam_disable() disables MPAM.

Could you please start the problem description with a brief description of the
scenario? As it is written it is not clear what is meant with "the next mount".

> 
> Move the CDP disable after the groups have been torn down, so every
> CLOSID freed on unmount is translated with the CDP state it was
> allocated under.

Above is a brief statement that appears unrelated to earlier problem description.
Does it aim to capture some other problems encountered with existing rdt_disable_ctx()
placement? I think MPAM is fortunate that it does not use CLOSID in 
resctrl_arch_reset_all_ctrls() but from what I can tell the per-resource group
rdtgroup_unassign_cntrs() may also end up leaving stale monitor configuration
behind?

> Keep the RMIDs in limbo instead of forcing them back
> to the free list, because a quick remount would otherwise reuse RMIDs
> whose counters are still dirty. Pause the limbo handler while the
> filesystem is unmounted. At mount, release any RMID left in limbo under
> a different CDP state without reading its counter, since its CLOSID no
> longer selects the same PARTID, and restart the handler.

Sounds reasonable.

> 
> x86 RMIDs are independent of the CLOSID, so the CDP state does not
> change which counter is read. Only arm64 MPAM systems are affected, and
> only when CDP is enabled with the cdp mount option (CDP depends on
> CONFIG_EXPERT).
> 
> Reproduced on QEMU:
> - mount resctrl, create four control groups
> - unmount, and mount again with -o cdp.
> 
> Before this patch, the limbo handler reads the counters for the limbo RMIDs

(nit: please never use the "this patch" term in a changelog - this is by definition
a patch and this term can be a trigger word for some).

> using the doubled PARTID:
> 
> 	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
> 
> After this patch the RMIDs are released before their counters are read, and
> the same sequence completes without an error.
> 
> Rebased on the below fix.
> 	https://lore.kernel.org/all/20261031063634.1678345-3-ruanjinjie@huawei.com/

Information like above rebase hint should not follow a change into the kernel
and is best left below the "---".

> 
> Fixes: 6789fb99282c ("arm_mpam: resctrl: Add CDP emulation")
> Suggested-by: Ben Horgan <ben.horgan@arm.com>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Cc: <stable@vger.kernel.org> # 7.3.x
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Reinette Chatre <reinette.chatre@intel.com>
> Cc: Dave Martin <Dave.Martin@arm.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Babu Moger <babu.moger@amd.com>
> ---
>  fs/resctrl/internal.h |  1 +
>  fs/resctrl/monitor.c  | 60 ++++++++++++++++++++++++++++++++++++++++---
>  fs/resctrl/rdtgroup.c | 31 +++++++++++++++++-----
>  3 files changed, 83 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index 1c85e06f09bf..ccae796a1489 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -373,6 +373,7 @@ void closid_free(int closid);
>  int setup_rmid_lru_list(void);
>  
>  void free_rmid_lru_list(void);
> +void resctrl_limbo_pause(void);
>  
>  int alloc_rmid(u32 closid);
>  
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 2002c4af1a5c..292f7f334b4c 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -70,6 +70,16 @@ static u32 *closid_num_dirty_rmid;
>   */
>  static unsigned int rmid_limbo_count;
>  
> +/*
> + * @limbo_paused - True if the filesystem is unmounted
> + *     The limbo handler must not read counters until the next mount has
> + *     set the CDP state up.

Is this new flag needed? The description "True if the filesystem is unmounted"
could be a hint that it could just use the "resctrl_mounted" flag
similar to how the MBM overflow handler uses it?


> + * @limbo_cdp - True if the RMIDs currently in limbo were freed with
> + *     CDP enabled.
> + */
> +static bool limbo_paused;
> +static bool limbo_cdp;
> +
>  /*
>   * @rmid_entry - The entry in the limbo and free lists.
>   */
> @@ -350,6 +360,8 @@ static void add_rmid_to_limbo(struct rmid_entry *entry)
>  		entry->busy++;
>  	}
>  
> +	if (!rmid_limbo_count)
> +		limbo_cdp = resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3);

CDP state cannot change after resctrl mount so this runtime assignment
on each RMID state change looks redundant. It could just be set once on
mount? More on this later but I also think the !rmid_limbo_count assumes
an MPAM underlying architecture since there the rmid_limbo_count will
always be reset on a state change during mount. Having limbo_cdp gated
on rmid_limbo_count being zero may thus not behave the same on all
architectures.

>  	rmid_limbo_count++;
>  	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
>  		closid_num_dirty_rmid[entry->closid]++;
> @@ -852,6 +864,13 @@ void cqm_handle_limbo(struct work_struct *work)
>  	 */
>  	mutex_lock(&rdtgroup_mutex);
>  
> +	/*
> +	 * The limbo state is kept across mounts, but can't be read while
> +	 * the filesystem is unmounted: the CDP state may have changed.
> +	 */
> +	if (limbo_paused)

Could this be if (!resctrl_mounted) instead? Compare with mbm_handle_overflow().

> +		goto out_unlock;
> +
>  	/*
>  	 * Ensure the worker is dedicated to a CPU as intended and not
>  	 * relocated by workqueue subsystem as part of CPU going offline.
> @@ -991,6 +1010,8 @@ void mbm_setup_overflow_handler(struct rdt_l3_mon_domain *dom, unsigned long del
>  int setup_rmid_lru_list(void)
>  {
>  	struct rmid_entry *entry = NULL;
> +	struct rdt_l3_mon_domain *d;
> +	struct rdt_resource *r;
>  	u32 idx_limit;
>  	u32 idx;
>  	int i;
> @@ -1001,11 +1022,37 @@ int setup_rmid_lru_list(void)
>  	/*
>  	 * Called on every mount, but the number of RMIDs cannot change
>  	 * after the first mount, so keep using the same set of rmid_ptrs[]
> -	 * until resctrl_exit(). Note that the limbo handler continues to
> -	 * access rmid_ptrs[] after resctrl is unmounted.
> +	 * until resctrl_exit(). The limbo state is kept too, so that dirty
> +	 * RMIDs aren't reused after a quick remount.
>  	 */
> -	if (rmid_ptrs)
> +	if (rmid_ptrs) {
> +		r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
> +
> +		/*
> +		 * An entry freed under a different CDP state can't be read
> +		 * with the new one: its CLOSID selects a different PARTID.
> +		 * Release those without reading them.

Please refrain from adding architecture specific comments in resctrl filesystem code
that aims to be architecture agnostic. Since MPAM support was added after initial resctrl
design the "CLOSID" and "RMID" terms are "baked into" resctrl fs with known 
translations added by architectures. 

> +		 */
> +		if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3) != limbo_cdp) {

Here I am not so sure since I am very unfamiliar with the MPAM driver. It looks
like the problematic path in MPAM is gated by a global "cdp_enabled" flag that
may be set when filesystem is mounted with -o cdpl2? Could this mean that
this test would not accurately capture whether MPAM driver uses the "cdp_enabled"
flows?

Apart from this, should this be gated by a 
	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
as well as a check that LLC occupancy is actually enabled?

(
 ah - looks like sashiko agrees, could you please also check the sashiko
 feedback at 
	https://sashiko.dev/#/patchset/20261031094616.1691026-1-ruanjinjie%40huawei.com
)


> +			list_for_each_entry(d, &r->mon_domains, hdr.list) {

list_for_each_entry_rcu()?

> +				if (has_busy_rmid(d))
> +					release_limbo_rmids(d);
> +			}
> +		}
> +
> +		limbo_paused = false;
> +
> +		/* Restart the handler for the domains that are still busy. */

Switching to resctrl_mounted flag would mean this needs to be started later, after
the flag is set. Doing so has the benefit of placing it with the code that starts
the MBM overflow handler that centralizes the workers.

Thinking about it more - there seems to be a lot of benefits with such a move and
may avoid needing to move when setup_rmid_lru_list() is called since removing the
entries that no longer have matching CDP state only needs to be done before
starting the worker so this can all be done together, after setting resctrl_mounted?

Could there perhaps be a new function that initializes limbo_cdp, releases limbo
RMIDs (if needed), and starts the limbo worker? This function can be called from rdt_get_tree()
after resctrl_mounted is set? I think this could simplify this implementation.


> +		if (resctrl_is_mon_event_enabled(QOS_L3_OCCUP_EVENT_ID)) {
> +			list_for_each_entry(d, &r->mon_domains, hdr.list) {

list_for_each_entry_rcu()?

> +				if (has_busy_rmid(d))
> +					cqm_setup_limbo_handler(d, CQM_LIMBOCHECK_INTERVAL,

Why is a delay needed here instead of scheduling it to run immediately? 
With this delay a user may unnecessarily need to wait a second after mounting resctrl
before new resource groups can be created?

> +								RESCTRL_PICK_ANY_CPU);
> +			}
> +		}
> +
>  		return 0;
> +	}
>  
>  	idx_limit = resctrl_arch_system_num_rmid_idx();
>  	rmid_ptrs = kzalloc_objs(struct rmid_entry, idx_limit);
Reinette