[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <49a197e5-6d4b-4e20-a135-676c5bf14c66@intel.com>
Date: Fri, 3 Oct 2025 17:23:19 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>, 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>, Chen Yu <yu.c.chen@...el.com>
CC: <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
<patches@...ts.linux.dev>
Subject: Re: [PATCH v11 27/31] x86/resctrl: Enable RDT_RESOURCE_PERF_PKG
Hi Tony,
On 9/25/25 1:03 PM, Tony Luck wrote:
> Mark the RDT_RESOURCE_PERF_PKG resource as mon_capable and set the global
> rdt_mon_capable flag.
Above is clear from patch.
>
> Call domain_add_cpu_mon() for each online CPU to allocate all domains
> for the RDT_RESOURCE_PERF_PKG since they were not created during resctrl
> initialization because of the enumeration delay until first mount.
Attempt at alternative:
Since telemetry events are enumerated on resctrl mount the RDT_RESOURCE_PERF_PKG
resource is not considered "monitoring capable" during early resctrl initialization.
This means that the domain list for RDT_RESOURCE_PERF_PKG is not built when the CPU
hot plug notifiers are registered and run for the first time right after resctrl
initialization.
Mark the RDT_RESOURCE_PERF_PKG as "monitoring capable" upon successful telemetry event
enumeration to ensure future CPU hotplug events include this resource and initialize its
domain list for CPUs that are already online.
>
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
> arch/x86/kernel/cpu/resctrl/core.c | 17 ++++++++++++++++-
> arch/x86/kernel/cpu/resctrl/intel_aet.c | 5 +++++
> 2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 1d43087c5975..48ed6242d136 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -755,14 +755,29 @@ static int resctrl_arch_offline_cpu(unsigned int cpu)
>
> void resctrl_arch_pre_mount(void)
> {
> + struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_PERF_PKG].r_resctrl;
> static atomic_t only_once = ATOMIC_INIT(0);
> - int old = 0;
> + int cpu, old = 0;
>
> if (!atomic_try_cmpxchg(&only_once, &old, 1))
> return;
>
> if (!intel_aet_get_events())
> return;
> +
> + if (!r->mon_capable)
> + return;
Is this necessary? Can r->mon_capable be false if intel_aet_get_events() fails?
> +
> + /*
> + * Late discovery of telemetry events means the domains for the
> + * resource were not built. Do that now.
> + */
> + cpus_read_lock();
hmmm ... until this point CPUs can come and go. This means that from the moment
r->mon_capable is set resctrl_arch_online_cpu() may run and thus domain_add_cpu_mon()
could be called twice for PERF_PKG? If all the second run does is set (again) a bit
in the cpumask then that *may* be ok (but should be documented) but the flow does not
seem safe to end up like that (more below)
> + mutex_lock(&domain_list_lock);
> + for_each_online_cpu(cpu)
> + domain_add_cpu_mon(cpu, r);
> + mutex_unlock(&domain_list_lock);
> + cpus_read_unlock();
> }
>
> enum {
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index 039e63d8c2e7..f6afe862b9de 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -214,6 +214,9 @@ static bool enable_events(struct event_group *e, struct pmt_feature_group *p)
> if (!usable_events)
> return false;
>
> + r->mon_capable = true;
> + rdt_mon_capable = true;
> +
> if (r->mon.num_rmid)
> r->mon.num_rmid = min(r->mon.num_rmid, e->num_rmids);
> else
> @@ -223,6 +226,8 @@ static bool enable_events(struct event_group *e, struct pmt_feature_group *p)
> resctrl_enable_mon_event(e->evts[j].id, true,
> e->evts[j].bin_bits, &e->evts[j]);
I notice that the mon_capable flags are set *before* the events are enabled. If the first
CPU of a package comes online between setting the flag and enabling the events then the initial
domain creation will not be correct?
What if the mon_capable flags are set in resctrl_arch_pre_mount() after a successful
intel_aet_get_events()? Perhaps with CPU hotplug lock held? From what I can tell doing so will
impact the debugfs flow since that depends on the resource being mon_capable. Would there be a
problem with delaying the debugfs setup until after domain list is built?
>
> + pr_info("%s %s monitoring detected\n", r->name, e->name);
> +
> return true;
> }
>
Reinette
Powered by blists - more mailing lists