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

[Reinette Chatre]

Hi Jinjie,

On 11/9/26 3:48 AM, Jinjie Ruan wrote:
> 在 2026/11/7 6:52, Reinette Chatre 写道:
>> On 11/4/26 11:09 PM, Jinjie Ruan wrote:

...

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

In x86 the CLOSID and RMID are independent so on x86 the RMID (index) does not change between
CDP enabled and CDP disabled.

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

Right, without resctrl fs mounted there are no user requests for the monitoring data backed
by any architecture.

...

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

limbo_cdp (or limbo_cdp_enabled as per earlier comment) is only valid if
resctrl_is_mon_event_enabled(QOS_L3_OCCUP_EVENT_ID) *and*
IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID). Since QOS_L3_OCCUP_EVENT_ID
state cannot change between mounts there is no need to ever update limbo_cdp
if QOS_L3_OCCUP_EVENT_ID is disabled, no?

Reinette