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

["Luck, Tony" <tony.luck@intel.com>]

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.

> 
> This ensures that any entries left over from a previous mount are properly
> detached before the list is reinitialized.
> 
> 
> > +
> >   	/*
> >   	 * RESCTRL_RESERVED_CLOSID and RESCTRL_RESERVED_RMID are special and
> >   	 * are always allocated. These are used for the rdtgroup_default
> >   	 * control group, which was setup earlier in rdtgroup_setup_default().
> >   	 */
> > -	idx = resctrl_arch_rmid_idx_encode(RESCTRL_RESERVED_CLOSID,
> > -					   RESCTRL_RESERVED_RMID);
> > -	entry = __rmid_entry(idx);
> > -	list_del(&entry->list);
> > +	rsvd_idx = resctrl_arch_rmid_idx_encode(RESCTRL_RESERVED_CLOSID,
> > +						RESCTRL_RESERVED_RMID);
> > +
> > +	for (i = 0; i < cur_idx_limit; i++) {
> > +		entry = &rmid_ptrs[i];
> > +		/* Don't add reserved or busy entries to free list */
> > +		if (i == rsvd_idx || entry->busy)
> > +			continue;
> > +		list_add_tail(&entry->list, &rmid_free_lru);
> 
> [2]. Here nodes are linked to head.  Basically, Head <->A <->B <->HEAD

As mentioned above, the ->next and ->prev fields of this entry are not
used when adding it to the rmid_free_lru list.
> 
> 
> Thanks
> 
> Babu

-Tony