Re: [PATCH v11 27/31] x86/resctrl: Enable RDT_RESOURCE_PERF_PKG

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

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@intel.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