Message-ID: <44b25d54-5af3-4d70-a3e0-2322696ffc42@intel.com>
Date: Mon, 31 Mar 2025 17:07:48 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: "Luck, Tony" <tony.luck@...el.com>
CC: Fenghua Yu <fenghuay@...dia.com>, Maciej Wieczor-Retman
	<maciej.wieczor-retman@...el.com>, Peter Newman <peternewman@...gle.com>,
	James Morse <james.morse@....com>, Babu Moger <babu.moger@....com>, "Drew
 Fustini" <dfustini@...libre.com>, Dave Martin <Dave.Martin@....com>,
	<linux-kernel@...r.kernel.org>, <patches@...ts.linux.dev>
Subject: Re: [PATCH v2 07/16] x86/resctrl: Add initialization hook for Intel
 PMT events

Hi Tony,

On 3/31/25 2:53 PM, Luck, Tony wrote:
> On Mon, Mar 31, 2025 at 09:20:07AM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 3/21/25 4:15 PM, Tony Luck wrote:
>>> Call the OOBMSM discovery code to find out if there are any
>>> event groups that match unique identifiers understood by resctrl.
>>>
>>> Note that initiialzation must happen in two phases because the
>>
>> "initiialzation" -> "initialization"
> 
> Wil fix.
> 
>>> OOBMSM VSEC discovery process is not complete at resctrl
>>> "lateinit()" initialization time. So there is an initial hook
>>> that assumes that Intel PMT will exist, called early so that
>>> package scoped domain groups are initialized.
>>>
>>> At first mount the remainder of initialization is done. If there
>>> are no Intel PMT events, the package domain lists are removed.
>>>
>>> Move definition of struct mon_evt to <linux/resctrl_types.h>
>>
>> Why not include/resctrl.h?
> 
> I put in in resctrl_types.h because it is a type definition?
> I'm not sure of the criteria James used to decide what goes
> in resctrl_types.h and what goes in resctrl.h

Changelog of 
commit f16adbaf9272 ("x86/resctrl: Move resctrl types to a separate header")
explains it well.
Essentially I expect a new entry in resctrl_types.h to have a user in
arch/x86/include/asm/resctrl.h and since cover letter shows no changes to
this file the addition is unexpected to me.

  
>>>  static __init void __check_quirks_intel(void)
>>> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
>>> new file mode 100644
>>> index 000000000000..9a8ccb62b4ab
>>> --- /dev/null
>>> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
>>> @@ -0,0 +1,211 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Resource Director Technology(RDT)
>>> + * - Intel Application Energy Telemetry
>>> + *
>>> + * Copyright (C) 2025 Intel Corporation
>>> + *
>>> + * Author:
>>> + *    Tony Luck <tony.luck@...el.com>
>>> + */
>>> +
>>> +#define pr_fmt(fmt)   "resctrl: " fmt
>>> +
>>> +#include <linux/cpu.h>
>>> +#include <linux/cleanup.h>
>>> +#include <linux/slab.h>
>>> +#include "fake_intel_aet_features.h"
>>> +#include <linux/intel_vsec.h>
>>> +#include <asm/resctrl.h>
>>> +
>>> +#include "internal.h"
>>> +
>>> +static struct pmt_feature_group *feat_energy;
>>> +static struct pmt_feature_group *feat_perf;
>>> +
>>> +/* All telemetry events from Intel CPUs */
>>> +enum pmt_event_id {
>>> +	PMT_EVENT_ENERGY,
>>> +	PMT_EVENT_ACTIVITY,
>>> +	PMT_EVENT_STALLS_LLC_HIT,
>>> +	PMT_EVENT_C1_RES,
>>> +	PMT_EVENT_UNHALTED_CORE_CYCLES,
>>> +	PMT_EVENT_STALLS_LLC_MISS,
>>> +	PMT_EVENT_AUTO_C6_RES,
>>> +	PMT_EVENT_UNHALTED_REF_CYCLES,
>>> +	PMT_EVENT_UOPS_RETIRED,
>>> +
>>> +	PMT_NUM_EVENTS
>>> +};
>>
>> I expect the above to become part of resctrl fs. Actually, I think
>> all properties of the new events, the id, name and how to display it,
>> should be part of resctrl fs.
> 
> I'm not convinced about the amount of re-use that there will be
> for these events. I took a quick look at the current plan for a
> processor that follows Clearwater Forest and I see 22 events, only
> 3 of them match events in the above list.
>

This is not about re-use but instead these events becoming part of resctrl fs ABI.

 
>> We do not want other architectures to create their own display names for
>> the same events. I expect that this will require more plumbing between
>> arch and fs code to communicate which events are supported, similar to
>> what exists for the L3 events (for example, resctrl_arch_is_mbm_total_enabled()).
> 
> Supported monitor events are indicated by setting bits in "rdt_mon_features"
> currently "unsigned int" but could become "long" or a longer bitmap if
> we really want the FS layer to keep track of every possible event for
> every architecture.

That sounds fine. rdt_mon_features is x86 arch code and that can change as needed.
Previous discussions ended up with rdt_mon_features staying with x86 arch code
and accessed from resctrl fs with helpers. 

>>
>> This may result in struct rdt_core_mon_domain to no longer be empty but instead
>> for resctrl to use it to manage state of which events are enabled or not. Theoretically
>> all could be managed by the architecture but I think that could result in inconsistent
>> error codes to user space.
> 
> rdt_core_mon_domain seems like the wrong level (unless we expect to have
> different enabled events in different domains). rdt_resource seems
> plausible ... or just expand "rdt_mon_features".

Understood on the domain level. This work introduced quite a few new data structures that
I have not yet digested. I cannot tell at this time if new resctrl fs helpers will have
easier time with unique perf related data structure or if x86's internal rdt_mon_features
need changes in addition to all these data structures.


Reinette

Powered by blists - more mailing lists