Re: [PATCH v3 16/26] x86/resctrl: Third phase of telemetry event enumeration

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

Hi Tony,

On 4/7/25 4:40 PM, Tony Luck wrote:
> Allocate per-package arrays for the known telemetry_regions and
> initialize with pointers to the structures acquired from the
> intel_pmt_get_regions_by_feature() calls.

Why? 

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 38 +++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index 9d414dd40f8b..fb03f2e76306 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -108,6 +108,30 @@ static bool count_events(struct pkg_info *pkg, int max_pkgs, struct pmt_feature_
>  	return found;
>  }
>  
> +/*
> + * Copy the pointers to telemetry regions associated with a given package
> + * and with known guids over to the pkg_info structure for that package.
> + */
> +static int setup(struct pkg_info *pkg, int pkgnum, struct pmt_feature_group *p, int slot)

setup() is very generic. I do not have a suggestion since it is not yet clear
what this does (more below).

> +{
> +	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];

(please fix spacing)

hmmm .. this looks like one structure copied to another, not the pointer
copy the comment mentions.


> +		}
> +	}
> +
> +	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))
> @@ -128,6 +152,7 @@ bool intel_aet_get_events(void)
>  	struct pkg_info *pkg __free(free_pkg_info) = NULL;
>  	int num_pkgs = topology_max_packages();
>  	bool use_p1, use_p2;
> +	int slot;
>  
>  	pkg = kcalloc(num_pkgs, sizeof(*pkg_info), GFP_KERNEL);
>  	if (!pkg)
> @@ -146,6 +171,19 @@ bool intel_aet_get_events(void)
>  		return false;
>  	}
>  
> +	for (int i = 0; i < num_pkgs; i++) {
> +		if (!pkg[i].count)
> +			continue;
> +		pkg[i].regions = kmalloc_array(pkg[i].count, sizeof(*pkg[i].regions), GFP_KERNEL);

As I understand it sizeof(*pkg[i].regions) would be the size of a
struct telemetry_region that the code in setup() initializes by copying 
the data from the struct pmt_feature_group.

The changelog and comments creates impression that resctrl's initialization
consists of adding pointers to the data in struct pmt_feature_group but as
I read it the data is copied instead. Am I reading it wrong?

> +		if (!pkg[i].regions)
> +			return false;
> +		slot = 0;
> +		if (use_p1)
> +			slot = setup(pkg, i, p1, slot);
> +		if (use_p2)
> +			slot = setup(pkg, i, p2, slot);
> +	}
> +
>  	if (use_p1)
>  		feat_energy = no_free_ptr(p1);
>  	if (use_p2)

Reinette