Re: [PATCH v4 17/31] x86/resctrl: Add second part of telemetry event enumeration

[Reinette Chatre]

Hi Tony,

On 4/28/25 5:33 PM, Tony Luck wrote:
> There may be multiple telemetry aggregators per package, each enumerated
> by a telemetry region structure in the feature group.
> 
> Scan the array of telemetry region structures and count how many are
> in each package in preparation to allocate structures to save the MMIO
> addresses for each in a convenient format for use when reading event
> counters.

Note that reader does not know at this point that the subsequent processing
will be done by further expanding configure_events() or via a new function
called by get_pmt_feature() after configure_events() completes. Without knowing this
this patch looks buggy since it seems to forget to save a pointer to
initialized data.

> 
> Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index dda44baf75ae..a0365c3ce982 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -52,6 +52,27 @@ static struct event_group *known_event_groups[] = {
>  
>  static bool configure_events(struct event_group *e, struct pmt_feature_group *p)
>  {
> +	int *pkgcounts __free(kfree) = NULL;
> +	struct telemetry_region *tr;
> +	int num_pkgs;
> +
> +	num_pkgs = topology_max_packages();
> +	pkgcounts = kcalloc(num_pkgs, sizeof(*pkgcounts), GFP_KERNEL);
> +	if (!pkgcounts)
> +		return false;

-ENOMEM

> +
> +	/* Get per-package counts of telemetry_region for this guid */

How should "telemetry_region" be interpreted? If this is intended to refer
to the individual structs then it should be "counts of telemetry_region structs",
if it is intended to refer to what the structs represent, it can be "telemetry
regions"

Also, it is not obvious what is mean with "this guid" ... there are two guids in
snippet below.

> +	for (int i = 0; i < p->count; i++) {
> +		tr = &p->regions[i];
> +		if (tr->guid != e->guid)
> +			continue;
> +		if (tr->plat_info.package_id >= num_pkgs) {
> +			pr_warn_once("Bad package %d\n", tr->plat_info.package_id);
> +			continue;
> +		}
> +		pkgcounts[tr->plat_info.package_id]++;
> +	}
> +
>  	return false;

configure_events() returns false on success and failure? Perhaps this is temporary
until all parsing has been implemented but that is another thing that reader needs
to guess now or look at later patches to understand.

>  }
>  

Reinette