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

[Jinjie Ruan <ruanjinjie@huawei.com>]

在 2026/11/7 6:52, Reinette Chatre 写道:
> Hi Jinjie,

Hi Reinette,

> 
> On 11/4/26 11:09 PM, Jinjie Ruan wrote:
>> resctrl can be mounted without CDP, unmounted, and mounted again with
>> CDP enabled. A group's RMID is not returned to the free list when the
>> group is removed: it stays in limbo until the cache lines tagged with it
>> are evicted, and a limbo worker reads its occupancy counter in the
>> meantime. When resctrl is mounted again, the worker reads the RMIDs of
>> the groups removed by the unmount.
>>
>> On MPAM the counter is selected by the PARTID of the CLOSID that owned
>> the RMID. Without CDP, CLOSID c is PARTID c; with CDP it is PARTIDs 2c
>> and 2c+1. resctrl_unmount() clears the CDP state before it removes the
>> groups, so their monitor configuration is also removed with a different
>> CLOSID to PARTID mapping. With 5 PARTIDs:
>>
>>   mount                  CDP off, CLOSID c -> PARTID c
>>   mkdir g1..g4           CLOSIDs 1..4
>>   umount
>>     rdt_disable_ctx()    clear CDP
>>     remove g1..g4        RMIDs enter limbo with CLOSIDs 1..4
>>   mount -o cdp           CDP on, CLOSID c -> PARTIDs 2c, 2c+1
>>   limbo worker           reads CLOSID 2 as PARTID 5 > PARTID_MAX 4
>>                          MSC error interrupt, mpam_disable()
> 
> As I understand this issue goes both ways. If resctrl is first mounted

Right! it also have the same problem.

> with CDP the issue may even be worse since there is no error interrupt that
> draws attention to the problem that the wrong (albeit in range) RMID entries

Yes, there is no error irq.

> are tested. In such opposite scenario CDP is disabled on unmount causing
> the limbo handler that keeps running to immediately use the wrong index
> before resctrl is mounted again.

I agree.

> 
>>
>> The groups are removed after rdt_disable_ctx() clears CDP, so their
>> monitor configuration is cleared with a CLOSID to PARTID mapping that
>> differs from the one used to set it up.
> 
> hmmm .. this paragraph seems out of place after the limbo handler introduction.
> This looks to be a (very) high level description of counter reconfiguration done
> on unmount where MPAM also needs to translate a CLOSID into PARTID. This brief snippet
> seems to be what is connected to the patch subject, not the limbo handler, and
> describes the portion of the patch that moves rdt_disable_ctx() to be after
> resctrl_fs_teardown()? 

Yes, it does.

> 
> As I see it it the move of rdt_disable_ctx() has nothing to do with the limbo handler.
> In the spirit of separating logical changes, could you please separate this fix into
> its own patch? 

Will put it in a seperate patch, thank you!

> 
>>
>> Clear the CDP state after the groups are removed. While resctrl is
>> unmounted, don't read the RMIDs in limbo. When resctrl is mounted,
>> return the RMIDs freed under the other CDP state to the free list
>> without reading their counters, then restart the limbo worker unless it
>> is already queued. The RMIDs stay in limbo across the unmount so that a
>> quick remount does not give a new group an RMID that cache lines from
>> the removed group still carry.
>>
>> Only MPAM with CDP enabled through the cdp or cdpl2 mount option is
> 
> It is unexpected that a mount with cdpl2 impacts the limbo handler that
> only taked the L3 resource into account. (more later)
> 
>> affected; CDP depends on CONFIG_EXPERT. x86 is not affected as its RMID
>> does not depend on the CLOSID.
>>
>> 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.
>>
> 
> ...> ---
>>  fs/resctrl/internal.h |  1 +
>>  fs/resctrl/monitor.c  | 71 ++++++++++++++++++++++++++++++++++++++-----
>>  fs/resctrl/rdtgroup.c |  7 +++--
>>  3 files changed, 70 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
>> index 1c85e06f09bf..c95fa6ce62e6 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_resume(void);
>>  
>>  int alloc_rmid(u32 closid);
>>  
>> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
>> index ae9ddd380515..717c3888c2b5 100644
>> --- a/fs/resctrl/monitor.c
>> +++ b/fs/resctrl/monitor.c
>> @@ -70,6 +70,11 @@ static u32 *closid_num_dirty_rmid;
>>   */
>>  static unsigned int rmid_limbo_count;
>>  
>> +/*
>> + * @limbo_cdp - The CDP state under which the RMIDs in limbo were freed.
>> + */
>> +static bool limbo_cdp;
> 
> The variable is described as "The CDP state ..." but its type is a boolean.
> What does "true" mean? Could this be renamed limbo_cdp_enabled?
> 
> Looks like this is only set if CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is set,
> please mention that in the comments.
> 
>> +
>>  /*
>>   * @rmid_entry - The entry in the limbo and free lists.
>>   */
>> @@ -852,6 +857,10 @@ void cqm_handle_limbo(struct work_struct *work)
>>  	 */
>>  	mutex_lock(&rdtgroup_mutex);
>>  
>> +	/* Don't read the RMIDs in limbo until a mount sets up the CDP state. */
>> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) && !resctrl_mounted)
>> +		goto out_unlock;
>> +
> 
> Is this IS_ENABLED() check needed? There are several flows managing the limbo handler
> yet just cqm_handle_limbo() and resctrl_limbo_resume() use this IS_ENABLED() check to manage
> the lifecycle in a unique way that I am concerned will trip people up in the future when
> trying to untangle the limbo worker lifecycle.
> 
> I understand that the IS_ENABLED() check is added to support MPAM specifically here but
> I think we should aim for as much as possible similar behavior across all architectures.

Yes, it is for mpam.

> To that end I do not see why limbo handler should not run on MPAM when resctrl fs is unmounted
> while on x86 it should. 

Is it because the CDP implementation of x86 is different from MPAM, and
MPAM is implemented through simulation?

> 
> Having the limbo handler only depend on resctrl_mounted here will make the behavior consistent
> across architectures while also matching the overflow handler. Keeping these behaviors similar
> just makes the code easier to understand and reason about. Having resctrl filesystem provide
> consistent flows across all architectures ensures the same flows are always tested.
> 
> Yes, x86 would lose the ability to drain RMIDs across an unmount, but nothing needs these
> RMIDs while resctrl is unmounted. This work ensures that the RMIDs are freed shortly
> after a mount when they will be needed.

I believe this behavior is also universal for x86.

> 
> Could you please separate out a patch that _only_ changes the limbo handler lifetime
> independent of architecture? This patch can introduce resctrl_limbo_resume() but not release

Will put it in a seperate patch, thank you!

> any RMIDs from it. Splitting the limbo worker lifecycle change that impacts both
> architectures from the MPAM specific change that releases RMIDs on mount would make
> the impact easier to describe to reviewers.
> 
>>  	/*
>>  	 * Ensure the worker is dedicated to a CPU as intended and not
>>  	 * relocated by workqueue subsystem as part of CPU going offline.
>> @@ -868,10 +877,12 @@ void cqm_handle_limbo(struct work_struct *work)
>>  	__check_limbo(d);
>>  
>>  	if (has_busy_rmid(d)) {
>> -		d->cqm_work_cpu = cpumask_any_housekeeping(&d->hdr.cpu_mask,
>> -							   RESCTRL_PICK_ANY_CPU);
>> -		schedule_delayed_work_on(d->cqm_work_cpu, &d->cqm_limbo,
>> -					 delay);
>> +		int cpu;
>> +
>> +		cpu = cpumask_any_housekeeping(&d->hdr.cpu_mask,
>> +					       RESCTRL_PICK_ANY_CPU);
>> +		if (schedule_delayed_work_on(cpu, &d->cqm_limbo, delay))
>> +			d->cqm_work_cpu = cpu;
>>  	}
>>  
>>  out_unlock:
>> @@ -893,10 +904,13 @@ void cqm_setup_limbo_handler(struct rdt_l3_mon_domain *dom, unsigned long delay_
>>  	int cpu;
>>  
>>  	cpu = cpumask_any_housekeeping(&dom->hdr.cpu_mask, exclude_cpu);
>> -	dom->cqm_work_cpu = cpu;
>> +	if (cpu >= nr_cpu_ids) {
>> +		dom->cqm_work_cpu = cpu;
>> +		return;
>> +	}
>>  
>> -	if (cpu < nr_cpu_ids)
>> -		schedule_delayed_work_on(cpu, &dom->cqm_limbo, delay);
>> +	if (schedule_delayed_work_on(cpu, &dom->cqm_limbo, delay))
>> +		dom->cqm_work_cpu = cpu;
>>  }
> 
> hmmm ... this cqm_work_cpu wrangling is a concern and surely looks like something that
> we will trip over later. Making it worse is that there is no justification or explanation
> for why this is needed. I *assume* that it is done with anticipation that
> schedule_delayed_work_on() may fail since limbo handler is already pending, but please

Yes, that it the reason.

> also keep in mind that the worker my already be running and blocked on rdtgroup_mutex.
> We just had to fix a similar issues in this area recently so I would like to avoid any new
> issues here.
> 
> Looks like sashiko also uncovered a few issues in this area:
> 	https://sashiko.dev/#/patchset/20261105070934.2258613-1-ruanjinjie%40huawei.com
> 
> Could this implementation not instead just ensure that the worker is not pending and then
> the above two hunks can be dropped? (more later)
> 
>>  
>>  void mbm_handle_overflow(struct work_struct *work)
>> @@ -1033,6 +1047,49 @@ int setup_rmid_lru_list(void)
>>  	return 0;
>>  }
>>  
>> +/**
>> + * resctrl_limbo_resume() - Resume RMID limbo handling after a mount.
>> + *
>> + * Release the RMIDs that were freed under another CDP state without
>> + * reading them, and restart the limbo handler.
>> + */
>> +void resctrl_limbo_resume(void)
>> +{
>> +	struct rdt_l3_mon_domain *d;
>> +	struct rdt_resource *r;
>> +	bool cdp;
>> +
>> +	if (!IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) ||
> 
> Please drop this check from the core limbo handler resume code that
> should be done for all architectures. Instead just use it to gate whether
> release_limbo_rmids() is called or not.

OK.

> 
>> +	    !resctrl_arch_mon_capable())
>> +		return;
>> +
>> +	lockdep_assert_held(&rdtgroup_mutex);
> 
> Please move to beginning of function. How about lockdep_assert_cpus_held()?

OK.

> 
>> +
>> +	r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
>> +
>> +	cdp = resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2) ||
>> +	      resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3);
> 
> This is unexpected and looks to be leaking an MPAM quirk into
> the filesystem? I see there is a global "cdp_enabled" that guides
> MPAM internals but on the filesystem side it does not look right for
> the L2 CDP state to impact the limbo handler that only operates on
> L3 resource.

I will recheck this logic, Thank you!

> 
>> +
>> +	if (cdp != limbo_cdp) {
>> +		list_for_each_entry_rcu(d, &r->mon_domains, hdr.list,
>> +					lockdep_is_cpus_held()) {
>> +			if (d->rmid_busy_llc && has_busy_rmid(d))
> 
> How about calling cancel_delayed_work(&d->cqm_limbo) here? Could doing
> so eliminate the need for all that cqm_work_cpu wrangling?

Make sense, so we need not check whether it is running later.

> 
>> +				release_limbo_rmids(d);
>> +		}
>> +	}
>> +	limbo_cdp = cdp;
>> +
>> +	if (!resctrl_is_mon_event_enabled(QOS_L3_OCCUP_EVENT_ID))
>> +		return;
> 
> Could this be moved to the top of the function? QOS_L3_OCCUP_EVENT_ID
> state cannot change between mounts and this is the event that determines
> whether limbo handler runs at all. Moving this check would also eliminate the 
> need for the "d->rmid_busy_llc" check above.

But limbo_cdp must be updated for next time check.

> 
>> +
>> +	list_for_each_entry_rcu(d, &r->mon_domains, hdr.list,
>> +				lockdep_is_cpus_held()) {
>> +		/* A queued worker will run, don't move its cqm_work_cpu. */
>> +		if (has_busy_rmid(d) && !delayed_work_pending(&d->cqm_limbo))
>> +			cqm_setup_limbo_handler(d, 0, RESCTRL_PICK_ANY_CPU);
>> +	}
>> +}
>> +
>>  void free_rmid_lru_list(void)
>>  {
>>  	if (!resctrl_arch_mon_capable())
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index 5d0351453c2f..e6b64a5c9302 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -3150,13 +3150,14 @@ static void resctrl_unmount(void)
>>  	cpus_read_lock();
>>  	mutex_lock(&rdtgroup_mutex);
>>  
>> -	rdt_disable_ctx();
>> -
>>  	/* Put everything back to default values. */
>>  	for_each_alloc_capable_rdt_resource(r)
>>  		resctrl_arch_reset_all_ctrls(r);
>>  
>>  	resctrl_fs_teardown();
>> +
>> +	/* Remove the groups under the CDP state they were set up with. */
> 
> This comment looks out of place as it documents what happens _before_
> rdt_disable_ctx(). It also only refers to removal of group while I believe

It should be moved before resctrl_fs_teardown().

> (when looking an MPAM's __config_cntr()) there is an issue with counter reset 
> also?
> 
> How about something like:
> 
> 	Clear the CDP state after the cleanup. The CDP state decides
> 	which hardware configuration a CLOSID refers to, so the teardown
> 	must run with the state the configuration was created under.

It looks a bit more complete, will update the comment as it.

> 
>> +	rdt_disable_ctx();
>>  	if (resctrl_arch_alloc_capable())
>>  		resctrl_arch_disable_alloc();
>>  	if (resctrl_arch_mon_capable())
>> @@ -3261,6 +3262,8 @@ static int rdt_get_tree(struct fs_context *fc)
>>  						   RESCTRL_PICK_ANY_CPU);
>>  	}
>>  
>> +	resctrl_limbo_resume();
>> +
>>  	/*
>>  	 * Ensure root remains accessible after mutex is unlocked so that
>>  	 * kernfs_kill_sb() can run safely if called by kernfs_get_tree()'s
> 
> Reinette

-- 
Best regards,
Jinjie