Re: [PATCH v13 13/25] arm,x86,fs/resctrl: Allocate maximum needed rmid_ptrs[]

["Moger, Babu" <bmoger@amd.com>]

Hi Tony,

On 10/6/2026 5:07 PM, Luck, Tony wrote:
> Hi Babu,
> 
> On Tue, Oct 06, 2026 at 02:53:49PM -0500, Babu Moger wrote:
>> Hi Tony,
> 
> ... snip ...
> 
>>>    int setup_rmid_lru_list(void)
>>>    {
>>> -	struct rmid_entry *entry = NULL;
>>> -	u32 idx_limit;
>>> -	u32 idx;
>>> +	struct rmid_entry *entry;
>>> +	u32 cur_idx_limit;
>>> +	u32 rsvd_idx;
>>>    	int i;
>>>    	if (!resctrl_mon_capable())
>>>    		return 0;
>>>    	/*
>>> -	 * Called on every mount, but the number of RMIDs cannot change
>>> -	 * after the first mount, so keep using the same set of rmid_ptrs[]
>>> -	 * until resctrl_exit(). Note that the limbo handler continues to
>>> -	 * access rmid_ptrs[] after resctrl is unmounted.
>>> +	 * Allocate the largest number of RMIDs that this system will ever
>>> +	 * need. These cannot be freed until resctrl_exit() because the limbo
>>> +	 * handler continues to access rmid_ptrs[] after resctrl is unmounted.
>>>    	 */
>>> -	if (rmid_ptrs)
>>> -		return 0;
>>> +	if (!rmid_ptrs) {
>>> +		num_rmid_ptrs = resctrl_arch_system_max_rmid_idx();
>>> +		rmid_ptrs = kzalloc_objs(struct rmid_entry, num_rmid_ptrs);
>>> +		if (!rmid_ptrs) {
>>> +			num_rmid_ptrs = 0;
>>> +			return -ENOMEM;
>>> +		}
>>> -	idx_limit = resctrl_arch_system_num_rmid_idx();
>>> -	rmid_ptrs = kzalloc_objs(struct rmid_entry, idx_limit);
>>> -	if (!rmid_ptrs)
>>> -		return -ENOMEM;
>>> +		for (i = 0; i < num_rmid_ptrs; i++) {
>>> +			entry = &rmid_ptrs[i];
>>> +			INIT_LIST_HEAD(&entry->list);
>>
>> [1]. All the nodes are initialized. Basically, it is pointing to itself now.
> 
> This is setting the ->next and ->prev fields to point to the entry. So
> each of them is an empty list. Maybe I should just delete this as it is
> pointless. Later on these objects are added to the free list with:
> 
> 	list_add_tail(&entry->list, &rmid_free_lru);
> 
> but if you dig into that code you'll see that the ->next and ->prev
> fields are never read (not even in all the debug/sanity routines). Both
> fields are overwritten to add them to the free list.
> 
> Note that this seems to be a common pattern. Code does
> INIT_LIST_HEAD(&x->list) and then immediately uses list_add()
> or list_add_tail() to put "x" onto a list.
> 
>>
>>> -	for (i = 0; i < idx_limit; i++) {
>>> -		entry = &rmid_ptrs[i];
>>> -		INIT_LIST_HEAD(&entry->list);
>>> +			resctrl_arch_rmid_idx_decode(i, &entry->closid, &entry->rmid);
>>> +		}
>>> +	}
>>> -		resctrl_arch_rmid_idx_decode(i, &entry->closid, &entry->rmid);
>>> -		list_add_tail(&entry->list, &rmid_free_lru);
>>> +	/* Find how many RMIDs are available for this mount */
>>> +	cur_idx_limit = resctrl_arch_system_num_rmid_idx();
>>> +	if (cur_idx_limit > num_rmid_ptrs) {
>>> +		pr_warn_once("RMID count %u exceeds allocation. Limit to %u\n",
>>> +			     cur_idx_limit, num_rmid_ptrs);
>>> +		cur_idx_limit = num_rmid_ptrs;
>>>    	}
>>> +	INIT_LIST_HEAD(&rmid_free_lru);
>>
>> Initializing the list head without first cleaning up existing entries could
>> be problematic here.
> 
> I don't see a problem. Any entries on the list are left floating, but
> they are all in the rmid_ptrs[] so easy to access them again.
> rmid_free_list is now an empty list having discarded all entries that
> were on it. Sure, those floating entries have stale pointers sitting in
> their ->next and ->prev fields. But those pointers are not going to be
> used by any code.
> 
>> The first mount works correctly, and INIT_LIST_HEAD() is not needed in that
>> case because the list head is already statically initialized.
>>
>> However, on a subsequent mount, the list entries remain linked from the
>> previous mount through the setup performed in [2] below.
>>
>> INIT_LIST_HEAD() only reinitializes the list head itself. It does not
>> traverse the list and detach or reinitialize existing entries as was done in
>> [1].
>>
>> I think you need to replace
>>
>> INIT_LIST_HEAD(&rmid_free_lru);
>>
>> to
>>
>> while (!list_empty(&rmid_free_lru))
>>          list_del_init(rmid_free_lru.next);
> 
> That would set up all those floating entries each as an empty list. But
> doing that doesn't matter. No code is going to look at the ->next or
> ->prev fields.

That is correct. It may not be an issue in this case. The only concern 
is that the pointers are being overwritten without being cleaned up 
first. I agree that it may not cause a problem at this point, though I'm 
not sure whether list debugging would report any issues.

I'll leave it to you.

Thanks,
Babu