Re: [PATCH v2 2/2] fs/resctrl: Do not allocate a monitor context when releasing limbo RMIDs

[Jinjie Ruan]

在 2026/10/31 2:15, Reinette Chatre 写道:
> Hi Jinjie,
> 
> On 10/30/26 2:21 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. When a
>> domain goes offline, its busy RMIDs are released without checking
>> occupancy, since the domain is going away and the RMIDs must not be
>> left behind.
> 
> nit: "left behind" is vague. How about changing "RMIDs must not be
> left behind" to "RMIDs would otherwise never return to the free list."

That indeed sounds more clear.

> 
>>
>> On the monitor domain offline path, __check_limbo() allocates
>> the monitor context that an occupancy check needs before it scans
>> the busy RMIDs, but without using it, and returns early when that
>> allocation fails. Then the caller destroys the domain's monitoring state,
>> including the bitmap that records which RMIDs are busy, so nothing can
>> clear those bits afterwards and the busy counts never reach zero. Those
>> RMIDs never return to the free list and, where the RMID depends on
>> the CLOSID, the dirty count of their CLOSID never decreases. The allocation
>> can also wait for a monitor to become free, which stalls the CPU offline
>> path on a context that is not going to be used.
>>
>> Only MPAM systems are affected. An architecture that can always read an
>> RMID allocates nothing for the monitor context, so the allocation can
>> neither fail nor wait there.
>>
>> So move the release of busy RMIDs into a separate function that does
> 
> nit: Please drop "So" to stick with imperative. Just start with "Move the release ..."

Thank you! Will drop it.

> 
>> not take a monitor context and leave the occupancy check to
>> __check_limbo().
>>
>> This is not a revert of the fixed commit. It preserves the original fix's
>> intent of not reading a counter in the offline path.
> 
> More on this below but after reconsidering the fallout I think above paragraph
> needs to be dropped.
> 
> For entire changelog, could you please adjust the line wraps after edit to not
> jump around as much?
> 
>>
>> Fixes: fc16126cc11d ("x86,fs/resctrl: Prevent out-of-bounds access while offlining CPU when SNC enabled")
> 
> Taking a closer look, the issue as described is present before fc16126cc11d. 
> At fc16126cc11d^, __check_limbo() already returns early on allocation failure,

Indeed it is.

> and the force-release caller already falls through to domain_destroy_l3_mon_state(d),
> so the RMIDs leak there too. 

I agree.

> 
> The issue seems to originate from feature that added the allocation and early
> release, which is:
> e557999f80a5 ("x86/resctrl: Allow arch to allocate memory needed in resctrl_arch_rmid_read()")
> 
> Even so, as with patch #1, since the impact is only on MPAM this need not
> be backported as far. I thus think the tags should be:
> 	Fixes: e557999f80a5 ("x86/resctrl: Allow arch to allocate memory needed in resctrl_arch_rmid_read()")

Yes, I also find this patch add alloc code.

> and
> 	Cc: <stable@xxxxxxxxxxxxxxx> # 7.1.x
> 
> This fix does depend on fc16126cc11d to apply cleanly but fortunately it has been
> backported to 7.1.

Since commit 4aab135bda16 ("arm64: mpam: Select ARCH_HAS_CPU_RESCTR")
arm64 can use resctrl fs, which is v7.1-rc1.

> 
>> Signed-off-by: Jinjie Ruan <ruanjinjie@xxxxxxxxxx>
>> Cc: James Morse <james.morse@xxxxxxx>
>> Cc: Reinette Chatre <reinette.chatre@xxxxxxxxx>
>> Cc: <stable@xxxxxxxxxxxxxxx>
>> ---
> 
> ...
> 
>>  /*
>>   * 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)
>> +static void __check_limbo(struct rdt_l3_mon_domain *d)
>>  {
>>  	struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
>>  	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
>>  	struct rmid_entry *entry;
>> -	bool rmid_dirty = true;
>> +	bool rmid_dirty;
>>  	u32 idx, cur_idx = 1;
>>  	void *arch_mon_ctx;
>>  	void *arch_priv;
> 
> Another difference between MPAM and tip style is that tip prefers variable
> declarations to be in reverse fir order. For reference, see "Variable declarations" in 
> Documentation/process/maintainer-tip.rst.

I also prefer the reverse Christmas tree style for local variable
declarations, and it seems other modules have similar requirements.

> 
> Please re-order the declarations to maintain reverse fir.
> 
> Everything else looks good.
> 
> Thank you very much.
> 
> Reinette

-- 
Best regards,
Jinjie