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

[Jinjie Ruan <ruanjinjie@huawei.com>]

在 2026/11/10 6:21, Reinette Chatre 写道:
> Hi Jinjie,

Hi Reinette,

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

I got it, only QOS_L3_OCCUP_EVENT_ID enabled we can call
add_rmid_to_limbo(), and rmid_busy_llc will not be NULL.

free_rmid()
{
    ...
    if (resctrl_is_mon_event_enabled(QOS_L3_OCCUP_EVENT_ID))
       add_rmid_to_limbo(entry);
    ...
}

domain_setup_l3_mon_state()
{
     ...
     if (resctrl_is_mon_event_enabled(QOS_L3_OCCUP_EVENT_ID))
         d->rmid_busy_llc = bitmap_zalloc(idx_limit, GFP_KERNEL);
     ...
}


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

Yes, you are right!

> 
> Reinette

-- 
Best regards,
Jinjie