Re: [PATCH 2/2] fs/resctrl: Skip monitor context allocation when force-freeing RMIDs

[Jinjie Ruan <ruanjinjie@huawei.com>]

在 2026/10/30 2:15, Reinette Chatre 写道:
> Hi Jinjie,
> 
> There is no "force-freeing" of RMIDs. How about
> 	fs/resctrl: Do not allocate a monitor context when releasing limbo RMIDs
> 
> On 10/29/26 1:15 AM, Jinjie Ruan wrote:
>> __check_limbo() allocates the architecture monitor context before the
>> scan loop, even when the domain is going offline and the RMIDs will
>> be freed without reading their occupancy. So if the allocation fails,
>> the offline path returns early and leaves the busy RMIDs in limbo
>> which will be leaked instead of being released.
> 
> (please separate the context, problem, and solution descriptions).

OK.

> 
> "will be leaked" is a bit vague to gain confidence that there is an issue here.
> Compare with, for example, "The caller goes on to destroy the domain's
> monitoring state, freeing the bitmap that records which RMIDs are busy,
> so nothing can clear those bits afterwards and the busy counts never reach zero."
> 
> I think that there is an argument for this change even in the successful
> allocation path. If I understand correctly MPAM's resctrl_arch_mon_ctx_alloc()
> may need to block as it waits for a monitor? This would cause the CPU-offline
> path to block on a counter that will never be read.

I agree.

> 
>>
>> Split out release_limbo_rmids() for the offline path, which doesn't need
>> the monitor context.
> 
> Considering the shape of this fix and the "Fixes:" tag I think it may be
> worth a mention that this is not a revert of the indicated commit. This change
> maintains the intent of the original fix that avoids reading a counter in the
> offline path.

I agree.

> 
>>
>> Cc: James Morse <james.morse@arm.com>
>> Cc: Reinette Chatre <reinette.chatre@intel.com>
>> Fixes: fc16126cc11d ("x86,fs/resctrl: Prevent out-of-bounds access while offlining CPU when SNC enabled")
> 
> The changelog is missing a clear description of the impact of this issue.
> Nowhere does it mention that this only impacts MPAM and how likely this issue is. 

Will add the description.

> 
> This is important because this "Fixes:" commit has a "Cc: <stable@kernel.org>" so
> we need to help maintainers determine the impact of this issue to 
> guide where, if at all, it needs to be backported.

Thank you for the reminder.

> 
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>  fs/resctrl/internal.h |  3 +-
>>  fs/resctrl/monitor.c  | 69 ++++++++++++++++++++++++++++---------------
>>  fs/resctrl/rdtgroup.c |  2 +-
>>  3 files changed, 48 insertions(+), 26 deletions(-)
>>
>> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
>> index e62a277dee85..a1837679b27a 100644
>> --- a/fs/resctrl/internal.h
>> +++ b/fs/resctrl/internal.h
>> @@ -405,7 +405,8 @@ void cqm_handle_limbo(struct work_struct *work);
>>  
>>  bool has_busy_rmid(struct rdt_l3_mon_domain *d);
>>  
>> -void __check_limbo(struct rdt_l3_mon_domain *d, bool force_free);
>> +void __check_limbo(struct rdt_l3_mon_domain *d);
> 
> Looks like __check_limbo() can be static now?
> 
>> +void release_limbo_rmids(struct rdt_l3_mon_domain *d);
>>  
>>  void resctrl_file_fflags_init(const char *config, unsigned long fflags);
>>  
>> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
>> index b8f4e1ff4083..020140b794a7 100644
>> --- a/fs/resctrl/monitor.c
>> +++ b/fs/resctrl/monitor.c
>> @@ -124,13 +124,36 @@ static void limbo_release_entry(struct rmid_entry *entry)
>>  		closid_num_dirty_rmid[entry->closid]--;
>>  }
>>  
>> +/*
>> + * Release the busy RMIDs of a domain that is going offline. Their occupancy
>> + * can't be read any more, so they are freed without being checked.
> 
> Please document the function self, not the caller. Considering the comments in
> resctrl_offline_mon_domain() the caller already does a good job at this.

OK.

> 
> The "Their occupancy can't be read any more" also does not look right. I think
> this claim is based on x86's cpumask_empty() guard in resctrl_arch_rmid_read(), for
> x86 that does not actually mean that the occupancy cannot be read anymore, just that
> the CPU may already been removed from the domain at that point. This is really
> a platform specific detail that resctrl fs cannot assume.
> 
> How about something like:
> 
> 	/*
> 	 * Release the RMIDs that are marked as busy for this domain without
> 	 * checking their LLC occupancy. Cache lines may still be tagged with
> 	 * these RMIDs and will be accounted to the next user of each RMID.
> 	 */
> 
> I think such comment pairs well with the comment in the caller - it documents
> the consequence that the caller documents it is willing to take.
> 
>> + */
>> +void release_limbo_rmids(struct rdt_l3_mon_domain *d)
>> +{
>> +	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
>> +	struct rmid_entry *entry;
>> +	u32 idx, cur_idx = 1;
>> +
>> +	for (;;) {
>> +		idx = find_next_bit(d->rmid_busy_llc, idx_limit, cur_idx);
>> +		if (idx >= idx_limit)
>> +			break;
>> +
>> +		entry = __rmid_entry(idx);
>> +		clear_bit(idx, d->rmid_busy_llc);
>> +		if (!--entry->busy)
>> +			limbo_release_entry(entry);
>> +		cur_idx = idx + 1;
>> +	}
>> +}
>> +
>>  /*
>>   * Check the RMIDs that are marked as busy for this domain. If the
>>   * reported LLC occupancy is below the threshold clear the busy bit and
>>   * decrement the count. If the busy count gets to zero on an RMID, we
>>   * free the RMID
>>   */
>> -void __check_limbo(struct rdt_l3_mon_domain *d, bool force_free)
>> +void __check_limbo(struct rdt_l3_mon_domain *d)
> 
> Now __check_limbO() can be static?

Yes

> 
>>  {
>>  	struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
>>  	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
>> @@ -161,30 +184,28 @@ void __check_limbo(struct rdt_l3_mon_domain *d, bool force_free)
>>  			break;
> 
> To be true to the fix I think the rmid_dirty initializer added by fc16126cc11d
> can also be removed?
> 	"bool rmid_dirty = true;" -> "bool rmid_dirty;"
Yes

> 
>>  
>>  		entry = __rmid_entry(idx);
>> -		if (!force_free) {
>> -			if (resctrl_arch_rmid_read(r, &d->hdr, entry->closid,
>> -						   entry->rmid, QOS_L3_OCCUP_EVENT_ID,
>> -						   arch_priv, &val, arch_mon_ctx)) {
>> -				rmid_dirty = true;
>> -			} else {
>> -				rmid_dirty = (val >= resctrl_rmid_realloc_threshold);
>> -
>> -				/*
>> -				 * x86's CLOSID and RMID are independent numbers,
>> -				 * so the entry's CLOSID is an empty CLOSID
>> -				 * (X86_RESCTRL_EMPTY_CLOSID). On Arm the RMID
>> -				 * (PMG) extends the CLOSID (PARTID) space with
>> -				 * bits that aren't used to select the configuration.
>> -				 * It is thus necessary to track both CLOSID and
>> -				 * RMID because there may be dependencies between
>> -				 * them on some architectures.
>> -				 */
>> -				trace_mon_llc_occupancy_limbo(entry->closid, entry->rmid,
>> -							      d->hdr.id, val);
>> -			}
>> +		if (resctrl_arch_rmid_read(r, &d->hdr, entry->closid,
>> +					   entry->rmid, QOS_L3_OCCUP_EVENT_ID,
>> +					   arch_priv, &val, arch_mon_ctx)) {
>> +			rmid_dirty = true;
>> +		} else {
>> +			rmid_dirty = (val >= resctrl_rmid_realloc_threshold);
>> +
>> +			/*
>> +			 * x86's CLOSID and RMID are independent numbers,
>> +			 * so the entry's CLOSID is an empty CLOSID
>> +			 * (X86_RESCTRL_EMPTY_CLOSID). On Arm the RMID
>> +			 * (PMG) extends the CLOSID (PARTID) space with
>> +			 * bits that aren't used to select the configuration.
>> +			 * It is thus necessary to track both CLOSID and
>> +			 * RMID because there may be dependencies between
>> +			 * them on some architectures.
>> +			 */
>> +			trace_mon_llc_occupancy_limbo(entry->closid, entry->rmid,
>> +						      d->hdr.id, val);
>>  		}
>>  
>> -		if (force_free || !rmid_dirty) {
>> +		if (!rmid_dirty) {
>>  			clear_bit(idx, d->rmid_busy_llc);
>>  			if (!--entry->busy)
>>  				limbo_release_entry(entry);
>> @@ -843,7 +864,7 @@ void cqm_handle_limbo(struct work_struct *work)
>>  	if (cpumask_empty(&d->hdr.cpu_mask))
>>  		goto out_unlock;
>>  
>> -	__check_limbo(d, false);
>> +	__check_limbo(d);
>>  
>>  	if (has_busy_rmid(d)) {
>>  		d->cqm_work_cpu = cpumask_any_housekeeping(&d->hdr.cpu_mask,
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index 57ab090072c2..5d0351453c2f 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -4590,7 +4590,7 @@ void resctrl_offline_mon_domain(struct rdt_resource *r, struct rdt_domain_hdr *h
>>  		 * the RMID as busy creates RMID leaks if the
>>  		 * package never comes back.
>>  		 */
>> -		__check_limbo(d, true);
>> +		release_limbo_rmids(d);
>>  	}
>>  
>>  	domain_destroy_l3_mon_state(d);
> 
> Thank you very much.
> 
> Reinette

-- 
Best regards,
Jinjie