Re: [PATCH v9 27/31] fs/resctrl: Move RMID initialization to first mount

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 8/29/25 12:33 PM, Tony Luck wrote:
> L3 monitor features are enumerated during resctrl initialization
> and rmid_ptrs[] that tracks all RMIDs and depends on the
> number of supported RMIDs is allocated during this time.
> 
> Telemetry monitor features are enumerated during first resctrl mount and
> may support a different number of RMIDs compared to L3 monitor features.
> 
> Delay allocation and initialization of rmid_ptrs[] until first mount.
> Since the number of RMIDs cannot change on later mounts, keep the same
> set of rmid_ptrs[] until resctrl_exit().

Another strong reason is that the limbo handler keeps running after resctrl is
unmounted and may likely need to access rmid_ptrs[] as it keeps tracking
busy RMIDs after unmount.


> 
> Rename routines to match what they now do:
> dom_data_init() -> setup_rmid_lru_list()
> dom_data_exit() -> free_rmid_lru_list()
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  fs/resctrl/internal.h |  4 ++++
>  fs/resctrl/monitor.c  | 50 ++++++++++++++-----------------------------
>  fs/resctrl/rdtgroup.c |  7 ++++++
>  3 files changed, 27 insertions(+), 34 deletions(-)
> 
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index 7c37142df1cc..6dde28d9cfac 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -357,6 +357,10 @@ int closids_supported(void);
>  
>  void closid_free(int closid);
>  
> +int setup_rmid_lru_list(void);
> +
> +void free_rmid_lru_list(void);
> +
>  int alloc_rmid(u32 closid);
>  
>  void free_rmid(u32 closid, u32 rmid);
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index d5b96aca5d03..636342fad5a4 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -802,20 +802,24 @@ void mbm_setup_overflow_handler(struct rdt_l3_mon_domain *dom, unsigned long del
>  		schedule_delayed_work_on(cpu, &dom->mbm_over, delay);
>  }
>  
> -static int dom_data_init(struct rdt_resource *r)
> +int setup_rmid_lru_list(void)
>  {
>  	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
>  	struct rmid_entry *entry = NULL;
> -	int err = 0, i;
>  	u32 idx;
> +	int i;
>  
> -	mutex_lock(&rdtgroup_mutex);
> +	/*
> +	 * 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().
> +	 */
> +	if (rmid_ptrs)
> +		return 0;
>  
>  	rmid_ptrs = kcalloc(idx_limit, sizeof(struct rmid_entry), GFP_KERNEL);
> -	if (!rmid_ptrs) {
> -		err = -ENOMEM;
> -		goto out_unlock;
> -	}
> +	if (!rmid_ptrs)
> +		return -ENOMEM;
>  
>  	for (i = 0; i < idx_limit; i++) {
>  		entry = &rmid_ptrs[i];
> @@ -828,30 +832,21 @@ static int dom_data_init(struct rdt_resource *r)
>  	/*
>  	 * RESCTRL_RESERVED_CLOSID and RESCTRL_RESERVED_RMID are special and
>  	 * are always allocated. These are used for the rdtgroup_default
> -	 * control group, which will be setup later in resctrl_init().
> +	 * 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);
>  
> -out_unlock:
> -	mutex_unlock(&rdtgroup_mutex);
> -
> -	return err;
> +	return 0;
>  }
>  
> -static void dom_data_exit(struct rdt_resource *r)
> +void free_rmid_lru_list(void)
>  {
>  	mutex_lock(&rdtgroup_mutex);
> -
> -	if (!r->mon_capable)
> -		goto out_unlock;
> -

Can use resctrl_arch_mon_capable() instead to keep this check while maintaining
symmetry with setup_rmid_lru_list() that will make this easier to follow? 
Having the resctrl_arch_mon_capable() check within setup_rmid_lru_list() will 
also help.


>  	kfree(rmid_ptrs);
>  	rmid_ptrs = NULL;
> -
> -out_unlock:
>  	mutex_unlock(&rdtgroup_mutex);
>  }
>  
> @@ -914,7 +909,8 @@ bool resctrl_is_mon_event_enabled(enum resctrl_event_id eventid)
>   * resctrl_l3_mon_resource_init() - Initialise global monitoring structures.
>   *
>   * Allocate and initialise global monitor resources that do not belong to a
> - * specific domain. i.e. the rmid_ptrs[] used for the limbo and free lists.
> + * specific domain. i.e. the closid_num_dirty_rmid[] used to find the CLOSID
> + * with the cleanest set of RMIDs.
>   * Called once during boot after the struct rdt_resource's have been configured
>   * but before the filesystem is mounted.
>   * Resctrl's cpuhp callbacks may be called before this point to bring a domain
> @@ -925,7 +921,6 @@ bool resctrl_is_mon_event_enabled(enum resctrl_event_id eventid)
>  int resctrl_l3_mon_resource_init(void)
>  {
>  	struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
> -	int ret;
>  
>  	if (!r->mon_capable)
>  		return 0;
> @@ -947,15 +942,6 @@ int resctrl_l3_mon_resource_init(void)
>  		closid_num_dirty_rmid = tmp;
>  	}
>  
> -	ret = dom_data_init(r);
> -	if (ret) {
> -		if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> -			kfree(closid_num_dirty_rmid);
> -			closid_num_dirty_rmid = NULL;
> -		}
> -		return ret;
> -	}
> -
>  	if (resctrl_arch_is_evt_configurable(QOS_L3_MBM_TOTAL_EVENT_ID)) {
>  		mon_event_all[QOS_L3_MBM_TOTAL_EVENT_ID].configurable = true;
>  		resctrl_file_fflags_init("mbm_total_bytes_config",
> @@ -977,12 +963,8 @@ int resctrl_l3_mon_resource_init(void)
>  
>  void resctrl_l3_mon_resource_exit(void)
>  {
> -	struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
> -

As mentioned earlier, please try to maintain symmetry with resctrl_l3_mon_resource_init().

>  	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
>  		kfree(closid_num_dirty_rmid);
>  		closid_num_dirty_rmid = NULL;
>  	}
> -
> -	dom_data_exit(r);
>  }
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 3201e9cad8da..886de0e01e8a 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -2599,6 +2599,12 @@ static int rdt_get_tree(struct fs_context *fc)
>  		goto out;
>  	}
>  
> +	if (resctrl_arch_mon_capable()) {
> +		ret = setup_rmid_lru_list();
> +		if (ret)
> +			goto out;
> +	}

As mentioned earlier, please try to make code symmetrical to make this
easier to understand. As this is written the reader sees that 
setup_rmid_lru_list() needs to be protected by resctrl_arch_mon_capable()
while free_rmid_lru_list() can always be called whether system is
monitoring capable or not. This creates confusion that symmetrical code
can help with.

> +
>  	ret = rdtgroup_setup_root(ctx);
>  	if (ret)
>  		goto out;
> @@ -4408,4 +4414,5 @@ void resctrl_exit(void)
>  	 */
>  
>  	resctrl_l3_mon_resource_exit();
> +	free_rmid_lru_list();
>  }

Reinette