Re: [PATCH v2 10/16] x86/resctrl: Allocate per-package structures for known events

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

hi Tony,

On 3/21/25 4:16 PM, Tony Luck wrote:
> Use the per-package counts of known events to allocate arrays to
> make a copy of just the known events.
> 
> Add hook into resctrl_exit() to cleanup.

(above is not done in patch)

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h  |  2 +
>  arch/x86/kernel/cpu/resctrl/core.c      |  2 +
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 60 ++++++++++++++++++++++++-
>  3 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index ada402c7678b..2503a24e4177 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -170,8 +170,10 @@ int rdt_get_mon_l3_config(struct rdt_resource *r);
>  
>  #ifdef CONFIG_INTEL_AET_RESCTRL
>  int rdt_get_intel_aet_mon_config(void);
> +void rdt_intel_aet_exit(void);
>  #else
>  static inline int rdt_get_intel_aet_mon_config(void) { return 0; }
> +static inline void rdt_intel_aet_exit(void) { };
>  #endif
>  
>  bool rdt_cpu_has(int flag);
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 2adf40d8de32..d011c095aafa 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -1095,6 +1095,8 @@ static void __exit resctrl_arch_exit(void)
>  {
>  	cpuhp_remove_state(rdt_online);
>  
> +	rdt_intel_aet_exit();
> +
>  	resctrl_exit();
>  }
>  
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index 67862e81b9e0..e2d8eab997fc 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -188,6 +188,26 @@ static bool count_events(struct pkg_info *pkg, int max_pkgs, struct pmt_feature_
>  	return found;
>  }
>  
> +static int setup(struct pkg_info *pkg, int pkgnum, struct pmt_feature_group *p, int slot)

Could you please add a comment to this function to explain what it does?

> +{
> +	struct telem_entry **tentry;
> +
> +	for (int i = 0; i < p->count; i++) {
> +		for (tentry = telem_entry; *tentry; tentry++) {
> +			if (!(*tentry)->active)
> +				continue;
> +			if (pkgnum != p->regions[i].plat_info.package_id)
> +				continue;
> +			if (p->regions[i].guid != (*tentry)->guid)
> +				continue;
> +
> +			pkg[pkgnum].regions[slot++] =  p->regions[i];
> +		}
> +	}
> +
> +	return slot;
> +}
> +
>  DEFINE_FREE(intel_pmt_put_feature_group, struct pmt_feature_group *,	\
>  	if (!IS_ERR_OR_NULL(_T))					\
>  		intel_pmt_put_feature_group(_T))
> @@ -202,6 +222,8 @@ static bool get_events(void)
>  	struct pmt_feature_group *p2 __free(intel_pmt_put_feature_group) = NULL;
>  	int num_pkgs = topology_max_packages();
>  	struct pkg_info *pkg __free(kfree) = NULL;
> +	bool found_known_features = false;
> +	int i, slot;
>  
>  	pkg = kmalloc_array(num_pkgs, sizeof(*pkg_info), GFP_KERNEL | __GFP_ZERO);
>  	if (!pkg)
> @@ -220,13 +242,32 @@ static bool get_events(void)
>  		if (!count_events(pkg, num_pkgs, p2))
>  			intel_pmt_put_feature_group(no_free_ptr(p2));
>  
> +	for (i = 0; i < num_pkgs; i++) {
> +		if (!pkg[i].count)
> +			continue;
> +		found_known_features = true;
> +		pkg[i].regions = kmalloc_array(pkg[i].count, sizeof(*pkg[i].regions), GFP_KERNEL);
> +		if (!pkg[i].regions)
> +			goto fail;
> +
> +		slot = 0;
> +		if (!IS_ERR_VALUE(p1))
> +			slot = setup(pkg, i, p1, slot);
> +		if (!IS_ERR_VALUE(p2))
> +			slot = setup(pkg, i, p2, slot);
> +	}
> +
>  	if (!IS_ERR_OR_NULL(p1))
>  		feat_energy = no_free_ptr(p1);
>  	if (!IS_ERR_OR_NULL(p2))
>  		feat_perf = no_free_ptr(p2);
>  	pkg_info = no_free_ptr(pkg);
>  
> -	return true;
> +	return found_known_features;
> +fail:

include/linux/cleanup.h has this to say about mixing goto and free helpers:
"convert all resources that need a "goto" cleanup to scope-based cleanup, or convert
none of them"

> +	while (--i > 0)
> +		kfree(pkg[i].regions);
> +	return false;
>  }
>  
>  /*
> @@ -242,6 +283,23 @@ int rdt_get_intel_aet_mon_config(void)
>  	return 1;
>  }
>  
> +/* Clean up when resctrl shuts down completely */
> +void rdt_intel_aet_exit(void)
> +{
> +	int num_pkgs = topology_max_packages();
> +
> +	if (pkg_info) {
> +		for (int i = 0; i < num_pkgs; i++)
> +			kfree(pkg_info[i].regions);
> +		kfree(pkg_info);
> +	}
> +
> +	if (feat_energy)
> +		intel_pmt_put_feature_group(feat_energy);
> +	if (feat_perf)
> +		intel_pmt_put_feature_group(feat_perf);
> +}
> +
>  /*
>   * Late (first mount) initialization. Safe to ask OOBMSM which telemetry
>   * event groups are supported.

Reinette