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

Reinette Chatre
2025-04-19T00:47:46Z

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