Re: [PATCH v7 26/31] fs/resctrl: Fix life-cycle of closid_num_dirty_rmid

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

Hi Tony,

"Fix" is a loaded word to use in a patch subject and its use cannot be
justified for a non-functional change such as this.

Do not make false claims of code being broken as patch motivation.

On 7/11/25 4:53 PM, Tony Luck wrote:
> closid_num_dirty_rmid is specific to the L3 resource, but it
> is allocated/freed in the more generic dom_data_{init,exit}().

Quite bold make this argument when "the more generic
dom_data_{init,exit}()" is only called with L3 resource as argument.

This is a very straight forward change but the description so far
totally obfuscates this.

This patch does two things:
a) Rename resctrl_mon_resource_init()/resctrl_mon_resource_exit() to
   resctrl_mon_l3_resource_init()/resctrl_mon_l3_resource_exit()
   respectively. As mentioned earlier, this can be done as part of
   earlier patch that does the renaming.
b) Separate closid_num_dirty_rmid and rmid_ptrs[] allocation done in
   dom_data_init() in preparation for rmid_ptrs[] to be allocated on
   resctrl mount in support of the new telemetry events.

> 
> Add helpers to allocate/free closid_num_dirty_rmid.
> 
> Rename resctrl_mon_resource_init() to resctrl_mon_l3_resource_init()
> and call the closid_num_dirty_rmid_init() here, instead of
> allocating in dom_data_init().
> 
> Making matching changes to the exit path by renaming
> resctrl_mon_resource_exit() to resctrl_mon_l3_resource_exit()
> and free closid_num_dirty_rmid here instead of in dom_data_exit().
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  fs/resctrl/internal.h |  6 ++--
>  fs/resctrl/monitor.c  | 69 ++++++++++++++++++++++++-------------------
>  fs/resctrl/rdtgroup.c | 12 ++++----
>  3 files changed, 48 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index 56fdccb39375..28d505efdb7c 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -358,7 +358,9 @@ int alloc_rmid(u32 closid);
>  
>  void free_rmid(u32 closid, u32 rmid);
>  
> -void resctrl_mon_resource_exit(void);
> +int resctrl_mon_l3_resource_init(void);
> +
> +void resctrl_mon_l3_resource_exit(void);
>  
>  void mon_event_count(void *info);
>  
> @@ -368,8 +370,6 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
>  		    struct rdt_domain_hdr *hdr, struct rdtgroup *rdtgrp,
>  		    cpumask_t *cpumask, struct mon_evt *evt, int first);
>  
> -int resctrl_mon_resource_init(void);
> -
>  void mbm_setup_overflow_handler(struct rdt_l3_mon_domain *dom,
>  				unsigned long delay_ms,
>  				int exclude_cpu);
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 92798e1fb5b0..e3eceba70713 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -86,6 +86,37 @@ unsigned int resctrl_rmid_realloc_threshold;
>   */
>  unsigned int resctrl_rmid_realloc_limit;
>  
> +static int closid_num_dirty_rmid_init(struct rdt_resource *r)

It is not clear to me that these new helpers are needed. To me it seems
easier to follow if they are just open coded.

> +{
> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) &&
> +	    !closid_num_dirty_rmid) {
> +		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;
> +	}
> +
> +	return 0;
> +}
> +
> +static void closid_num_dirty_rmid_exit(void)
> +{
> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> +		kfree(closid_num_dirty_rmid);
> +		closid_num_dirty_rmid = NULL;
> +	}
> +}
> +
>  /*
>   * x86 and arm64 differ in their handling of monitoring.
>   * x86's RMID are independent numbers, there is only one source of traffic

...

> @@ -938,7 +942,7 @@ bool resctrl_is_mon_event_enabled(enum resctrl_event_id eventid)
>  }
>  
>  /**
> - * resctrl_mon_resource_init() - Initialise global monitoring structures.
> + * resctrl_mon_l3_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.
> @@ -949,7 +953,7 @@ bool resctrl_is_mon_event_enabled(enum resctrl_event_id eventid)
>   *
>   * Returns 0 for success, or -ENOMEM.
>   */
> -int resctrl_mon_resource_init(void)
> +int resctrl_mon_l3_resource_init(void)
>  {
>  	struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
>  	int ret;
> @@ -957,6 +961,10 @@ int resctrl_mon_resource_init(void)
>  	if (!r->mon_capable)
>  		return 0;
>  
> +	ret = closid_num_dirty_rmid_init(r);
> +	if (ret)
> +		return ret;
> +
>  	ret = dom_data_init(r);
>  	if (ret)

Leaking closid_num_dirty_rmid here?

>  		return ret;

Reinette