Re: [PATCH v9 05/12] x86/resctrl: Attach ACPI ERDT information to L3 mon domain on CPU online

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

Hi Chenyu,

On 10/13/26 7:58 PM, Chen Yu wrote:
> In preparation for MMIO-based LLC occupancy monitoring, link the per-domain
> ERDT information to the resctrl L3 monitoring domain, so the MMIO read path
> can locate a domain's MMIO registers.

Please review "Changelog" section of Documentation/process/maintainer-tip.rst
I understand other subsystems need a snippet that describes the change at the
top of changelog but x86 does not. Unless it is something really simple, which
this is not, there has to be some context before the change is described.

> 
> ERDT and CPUID enumerate CPU-to-L3-domain membership independently: CPUID leaf 4
> describes the L3 cache topology, while the firmware CACD sub-table lists the
> CPUs of each ERDT domain. Both views must agree on a CPU's L3 domain for that
> CPU to be monitored safely.
> 
> When a CPU comes online, validate that firmware and CPUID agree on its L3 domain
> and bind the CPU to its ERDT domain before adding it to any resctrl domain.

"bind the CPU to its ERDT domain" is this not what patch #3 did?

"adding it" - what does "it" refer to? the ERDT domain or the CPU?

> Fail the CPU online operation on a mismatch for now. Leaving the CPU
> online but outside all resctrl domains is doable, but it would require a
> change to cpus_ctrl_write() to reject CPUs that are online but not tracked by
> any resctrl group. The current approach is stricter, but simpler.

I do not believe failing the CPU online operation is actually possible.

resctrl_arch_late_init() is a late initcall and all boot CPUs are already online,
so the failure during this time will result in resctrl not being initialized at
all without impacting online CPUs. A CPU onlined after that will fail to
initialize, but that will make the CPU unusable for *everything*, not just 
unmonitored. This sounds extreme for an ERDT table issue that should never happen.

A CPU that is online is still able to run tasks and the earlier enumeration 
ensured that MSR_IA32_PQR_ASSOC is accessible and what the valid values for it
are. Even this "failure" path demonstrates this by calling clear_closid_rmid()
that writes to MSR_IA32_PQR_ASSOC *after* the supposed failure.

resctrl's current behavior when a CPU cannot be onlined, for example when
domain creation fails, is to *not* prevent CPU from onlining. If a domain
cannot be created then the CPU is still added to the default group's
CPU mask (resctrl_online_cpu()) to ensure that tasks can continue to
be scheduled on it by writing to MSR_IA32_PQR_ASSOC with values known to
be valid.

Such a CPU is *not* added to a domain's CPU mask so resctrl will not
attempt to make any configuration changes to it or read any counters from it.

> 
> The ID used to match a resctrl monitoring domain against an ERDT domain is
> always the L3 cache ID. ERDT is only enabled when Sub-NUMA Clustering is
> disabled, so resctrl keeps the L3 monitoring domains at L3 cache scope
> instead of node scope.

Please always write with imperative tone.

> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> Tested-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> ---

...

> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 0ea888807bd0..9a59643f366c 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -33,6 +33,8 @@
>   * The rdt_resource's domain list is updated when this happens. Readers of
>   * the domain list must either take cpus_read_lock(), or rely on an RCU
>   * read-side critical section, to avoid observing concurrent modification.
> + * This mutex also protects the ERDT domain_info_list, which is modified when a
> + * CPU comes online.

This places unrelated text in the middle of the description about how reads and writes to
existing domain list is managed. Please consider entire paragraph and ensure that the end
result is coherent.

>   * All writers take this mutex:
>   */
>  static DEFINE_MUTEX(domain_list_lock);
> @@ -558,6 +560,8 @@ static void l3_mon_domain_setup(int cpu, int id, struct rdt_resource *r, struct
>  		return;
>  	}
>  
> +	erdt_l3_mon_domain_setup(&d->hdr);
> +
>  	err = resctrl_online_mon_domain(r, &d->hdr);
>  	if (err) {
>  		l3_mon_domain_free(hw_dom);
> @@ -742,6 +746,13 @@ static int resctrl_arch_online_cpu(unsigned int cpu)
>  	struct rdt_resource *r;
>  
>  	mutex_lock(&domain_list_lock);
> +	/* Refuse to online a CPU whose ERDT and CPUID L3 domain views disagree. */
> +	if (!erdt_try_bind_cpu(cpu)) {
> +		mutex_unlock(&domain_list_lock);
> +		clear_closid_rmid(cpu);
> +		return -EINVAL;

(comments added in response to changelog)

> +	}
> +
>  	for_each_capable_rdt_resource(r)
>  		domain_add_cpu(cpu, r);
>  	mutex_unlock(&domain_list_lock);
> diff --git a/arch/x86/kernel/cpu/resctrl/erdt.c b/arch/x86/kernel/cpu/resctrl/erdt.c
> index d94c54617fa1..d4b630358d6d 100644
> --- a/arch/x86/kernel/cpu/resctrl/erdt.c
> +++ b/arch/x86/kernel/cpu/resctrl/erdt.c
> @@ -199,6 +199,115 @@ static __init bool parse_rmdd_table(struct acpi_subtbl_hdr_16 *rmdd_hdr)
>  	return false;
>  }
>  
> +/*
> + * erdt_try_bind_cpu() - Validate @cpu's L3 domain against ERDT and bind the

Now that the changelog and descriptions come together I do not find my earlier
suggestion for function name to be appropriate. How about erdt_try_bind_domain()?

> + * ERDT domain containing @cpu to that L3 domain.
> + *
> + * Find the erdt_domain_info whose CACD lists @cpu, then bind that ERDT domain
> + * to @cpu's L3 id. A CPU whose L3 id does not match the binding of its ERDT
> + * domain cannot be covered by resctrl.
> + *
> + * For example, the CACD sub-tables report:
> + * domain0: CPU0, CPU2, domain1: CPU1, CPU3
> + * while CPUID/cacheinfo reports the L3 cache is shared by:
> + * id0: CPU0, CPU1, id1: CPU2, CPU3
> + * With the CPUs coming online in order:
> + * - CPU0 binds domain0 to L3 id0.
> + * - CPU1 is in the unbound domain1, but L3 id0 is already bound to domain0,
> + *   so CPU1 is not covered by resctrl.
> + * - CPU2 is in domain0, which is bound to L3 id0 rather than CPU2's L3 id1,
> + *   so CPU2 is not covered by resctrl.
> + * - CPU3 binds domain1 to L3 id1.
> + *
> + * Return: true if @cpu can be covered by resctrl, false otherwise.

Could "can be covered by resctrl" be more specific?

> + */
> +bool erdt_try_bind_cpu(int cpu)
> +{
> +	struct erdt_domain_info *d, *cpu_dom = NULL;
> +	int dom_id;
> +
> +	/* Without ERDT there is no firmware topology to disagree with. */
> +	if (!erdt_enabled)
> +		return true;
> +
> +	dom_id = get_cpu_cacheinfo_id(cpu, RESCTRL_L3_CACHE);
> +	if (dom_id < 0) {
> +		pr_warn_once(FW_BUG "Can't find L3 id for CPU:%d\n", cpu);
> +		return false;
> +	}
> +
> +	list_for_each_entry(d, &domain_info_list, entry) {
> +		if (cpumask_test_cpu(cpu, &d->cpu_mask)) {
> +			cpu_dom = d;
> +			break;
> +		}
> +	}
> +
> +	if (!cpu_dom) {
> +		pr_warn_once(FW_BUG "Cannot find the ERDT domain which has CPU%d\n", cpu);
> +		return false;
> +	}
> +
> +	/* This ERDT domain is already bound to this CPU's L3 domain. */
> +	if (cpu_dom->dom_id == dom_id)
> +		return true;
> +
> +	/*
> +	 * This ERDT domain is already bound to a different L3 domain. Rebinding it
> +	 * would leave two L3 domains reading the counters of one ERDT domain, so
> +	 * skip @cpu instead (CPU2 in the example above).
> +	 */
> +	if (cpu_dom->dom_id != ERDT_DOMAIN_ID_UNSET) {
> +		pr_warn_once(FW_BUG "CPU%d's id=%d not equal to CACD domain(%*pbl) id=%d, skip this CPU\n",
> +			     cpu, dom_id, cpumask_pr_args(&cpu_dom->cpu_mask), cpu_dom->dom_id);
> +
> +		return false;
> +	}
> +
> +	/*
> +	 * A possible new binding. Check if another ERDT domain shares the same
> +	 * L3 id. If yes, this is a conflict and @cpu should not be considered
> +	 * by resctrl (CPU1 in the example above).
> +	 */
> +	list_for_each_entry(d, &domain_info_list, entry) {
> +		if (d == cpu_dom)
> +			continue;
> +
> +		if (d->dom_id == dom_id) {
> +			pr_warn_once(FW_BUG "CPU%d's id=%d is already used by CACD domain(%*pbl), skip this CPU\n",
> +				     cpu, dom_id, cpumask_pr_args(&d->cpu_mask));
> +
> +			return false;
> +		}
> +	}
> +
> +	/* Eligible new binding, assign the L3 id. */
> +	cpu_dom->dom_id = dom_id;
> +
> +	return true;
> +}
> +
> +/*
> + * Associate ERDT table information with this domain.

Could this add a note (if it is actually true) that earlier checks ensures
that hw_dom->d_info will *always* point to a valid ERDT domain ... or does
resctrl need to check for hw_dom->d_info everywhere like erdt_mon_read()?
Or are those checks just for robustness?

> + */
> +void erdt_l3_mon_domain_setup(struct rdt_domain_hdr *hdr)
> +{
> +	struct rdt_hw_l3_mon_domain *hw_dom;
> +	struct erdt_domain_info *d;
> +
> +	if (!erdt_enabled)
> +		return;
> +
> +	hw_dom = resctrl_to_arch_mon_dom(container_of(hdr, struct rdt_l3_mon_domain, hdr));
> +
> +	list_for_each_entry(d, &domain_info_list, entry) {
> +		if (d->dom_id == hdr->id) {
> +			hw_dom->d_info = d;
> +			return;
> +		}
> +	}
> +}
> +
>  void erdt_exit(void)
>  {
>  	struct erdt_domain_info *d, *tmp;
Reinette