Re: [PATCH v9 25/31] fs/resctrl: Move allocation/free of closid_num_dirty_rmid

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

On Wed, Sep 10, 2025 at 10:55:23AM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> In subject:
> closid_num_dirty_rmid -> closid_num_dirty_rmid[]
> 
> On 8/29/25 12:33 PM, Tony Luck wrote:
> > closid_num_dirty_rmid[] is allocated in dom_data_init() during resctrl
> > initialization and freed by dom_data_exit() during resctrl exit giving
> > it the same life cycle as rmid_ptrs[].
> > 
> > Move closid_num_dirty_rmid[] allocaction/free out to
> > resctrl_l3_mon_resource_init() and resctrl_l3_mon_resource_exit() in
> > preparation for rmid_ptrs[] to be allocated on resctrl mount in support
> > of the new telemetry events.
> > 
> > Signed-off-by: Tony Luck <tony.luck@intel.com>
> > ---
> >  fs/resctrl/monitor.c | 56 ++++++++++++++++++++++----------------------
> >  1 file changed, 28 insertions(+), 28 deletions(-)
> > 
> > diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> > index 33432a7f56da..d5b96aca5d03 100644
> > --- a/fs/resctrl/monitor.c
> > +++ b/fs/resctrl/monitor.c
> > @@ -805,36 +805,14 @@ void mbm_setup_overflow_handler(struct rdt_l3_mon_domain *dom, unsigned long del
> >  static int dom_data_init(struct rdt_resource *r)
> >  {
> >  	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
> > -	u32 num_closid = resctrl_arch_get_num_closid(r);
> >  	struct rmid_entry *entry = NULL;
> >  	int err = 0, i;
> >  	u32 idx;
> >  
> >  	mutex_lock(&rdtgroup_mutex);
> 
> mutex is held during original allocation code below ...
> 
> > -	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> > -		u32 *tmp;
> > -
> > -		/*
> > -		 * If the architecture hasn't provided a sanitised value here,
> > -		 * this may result in larger arrays than necessary. Resctrl will
> > -		 * use a smaller system wide value based on the resources in
> > -		 * use.
> > -		 */
> > -		tmp = kcalloc(num_closid, sizeof(*tmp), GFP_KERNEL);
> > -		if (!tmp) {
> > -			err = -ENOMEM;
> > -			goto out_unlock;
> > -		}
> > -
> > -		closid_num_dirty_rmid = tmp;
> > -	}

James,

While refactoring code, I missed moving the mutex_lock(&rdtgroup_mutex);
that was protecting the allocation of "closid_num_dirty_rmid[]" ... and
Reinette caught this change.

But looking at the code, I'm not at all sure what protection is needed
for the allocation/free of this array. The current calling sequence for
allocation is:

Linux late_init
    resctrl_arch_late_init()
	resctrl_init()
	    resctrl_l3_mon_resource_init
		 dom_data_init()

which doesn't appear to provide any scope for other CPUs to come in and
start using closid_num_dirty_rmid[]

The free path also seems safe too, as all resctrl functions should
be shutdown before calling:

resctrl_arch_exit()
    resctrl_exit()
	resctrl_l3_mon_resource_exit()
	    dom_data_exit()

and if they were not, holding the rdtgroup_mutex around:

	kfree(closid_num_dirty_rmid);
	closid_num_dirty_rmid = NULL;

would do nothing to prevent some still active resctrl function
from tripping over a NULL pointer.


So, unless I'm missing something, I'm planning to address Reinette's
find by documenting inmy commit message that holding rdtgroup_mutex
has always been unnecessary, so it is dropped as part of this refactor.

-Tony