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

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

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;
> -	}
>  
>  	rmid_ptrs = kcalloc(idx_limit, sizeof(struct rmid_entry), GFP_KERNEL);
>  	if (!rmid_ptrs) {
> -		if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> -			kfree(closid_num_dirty_rmid);
> -			closid_num_dirty_rmid = NULL;
> -		}
>  		err = -ENOMEM;
>  		goto out_unlock;
>  	}
> @@ -870,11 +848,6 @@ static void dom_data_exit(struct rdt_resource *r)
>  	if (!r->mon_capable)
>  		goto out_unlock;
>  
> -	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> -		kfree(closid_num_dirty_rmid);
> -		closid_num_dirty_rmid = NULL;
> -	}
> -

mutex is held in original free code  ...

>  	kfree(rmid_ptrs);
>  	rmid_ptrs = NULL;
>  
> @@ -957,9 +930,31 @@ int resctrl_l3_mon_resource_init(void)
>  	if (!r->mon_capable)
>  		return 0;
>  
> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> +		u32 num_closid = resctrl_arch_get_num_closid(r);
> +		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)
> +			return -ENOMEM;
> +
> +		closid_num_dirty_rmid = tmp;
> +	}

mutex no longer held during allocation ... no mention in changelog why this is ok?

> +
>  	ret = dom_data_init(r);
> -	if (ret)
> +	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;
> @@ -984,5 +979,10 @@ void resctrl_l3_mon_resource_exit(void)
>  {
>  	struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
>  

If you also use the "if (!r->mon_capable)" check as is done in original code
then resctrl_l3_mon_resource_exit() will be symmetrical with
resctrl_l3_mon_resource_init(). Since resctrl_l3_mon_resource_exit() is
_always_ called, whether monitoring is enabled or not, adding this check
will also make the code easier to follow and not require that reader knows
how closid_num_dirty_rmid[] is connected.
Another benefit of keeping this check is that it forces the function to
remain L3 resource specific (not just by name) and not seemingly become generic
later in series ("fs/resctrl: Move RMID initialization to first mount")

> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> +		kfree(closid_num_dirty_rmid);
> +		closid_num_dirty_rmid = NULL;
> +	}

... now freed without mutex without mention in changelog?

> +
>  	dom_data_exit(r);
>  }

Reinette