Re: [PATCH v6 4/9] x86/resctrl: Attach ACPI ERDT information to L3 mon domain on CPU online

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

Hi Chenyu,

On 7/25/26 2:23 AM, Chen Yu wrote:
> Reading LLC occupancy counters via MMIO requires the per-domain ERDT
> information, parsed earlier from the ACPI ERDT table, to be reachable

Please avoid using terms about a patch's position in a series. You can
just drop "earlier".

> from the resctrl L3 monitoring domain. Nothing links the two yet, so
> the monitoring code cannot locate the MMIO registers of a domain.
> 
> 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 before adding it to a resctrl monitoring domain. Exclude the
> CPU from all monitoring domains on a mismatch because a topology
> inconsistency between ERDT and CPUID indicates a firmware defect that
> makes the CPU's domain placement unreliable for any resource. Otherwise
> attach the matching ERDT domain information to the L3 monitoring domain
> so that later code can read monitoring data via ERDT and its sub-tables.

(similar comment as above)
"so that later code can read monitoring data via ERDT and its sub-tables" ->
"so that monitoring data can be read via ERDT and its sub-tables"

> 
> 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 23925bcd71d7..c2568b29474e 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -580,6 +580,9 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
>  		return;
>  	}
>  
> +	if (!erdt_cpu_valid(cpu))
> +		return;
> +

Including this check in domain_add_cpu_mon() means that it is repeated for every monitoring
resource. I think this check only needs to be done once? How about moving it to resctrl_arch_online_cpu()
where this check can be done before cycling through *any* (monitoring or control) resource?

While domain_info_list is initialized early, this validity check makes concurrent changes to
it so locking is required. The current implementation already does this modification with
domain_list_lock held but it is not made explicit that this list is now under the protection
of this lock. Please add a snippet to the comment above the domain_list_lock to document that
it is now also used to protect domain_info_list.

>  	hdr = resctrl_find_domain(&r->mon_domains, id, &add_pos);
>  	if (hdr)
>  		cpumask_set_cpu(cpu, &hdr->cpu_mask);
> @@ -589,8 +592,14 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
>  		/* Update the mbm_assign_mode state for the CPU if supported */
>  		if (r->mon.mbm_cntr_assignable)
>  			resctrl_arch_mbm_cntr_assign_set_one(r);
> -		if (!hdr)
> +		if (!hdr) {
>  			l3_mon_domain_setup(cpu, id, r, add_pos);
> +			hdr = resctrl_find_domain(&r->mon_domains, id, NULL);
> +		}
> +
> +		if (hdr)
> +			erdt_l3_mon_domain_setup(cpu, hdr);

The additional search for "hdr" seems unnecessary. Could l3_mon_domain_setup()
just call erdt_l3_mon_domain_setup() directly?

> +
>  		break;
>  	case RDT_RESOURCE_PERF_PKG:
>  		if (!hdr)
> diff --git a/arch/x86/kernel/cpu/resctrl/erdt.c b/arch/x86/kernel/cpu/resctrl/erdt.c
> index 8998cae47090..6257869d0db2 100644
> --- a/arch/x86/kernel/cpu/resctrl/erdt.c
> +++ b/arch/x86/kernel/cpu/resctrl/erdt.c
> @@ -207,6 +207,72 @@ static __init bool parse_rmdd_table(struct acpi_subtbl_hdr_16 *rmdd_hdr)
>  	return false;
>  }
>  
> +bool erdt_cpu_valid(int cpu)
> +{
> +	struct erdt_domain_info *d;
> +	int dom_id;
> +
> +	if (!erdt_enabled)
> +		return true;
> +
> +	dom_id = get_cpu_cacheinfo_id(cpu, RESCTRL_L3_CACHE);
> +	if (dom_id < 0)
> +		return true;

Should this be "false"? Perhaps also with a warning similar to domain_add_cpu_mon()'s
warning when the domain ID cannot be determined?

> +
> +	/*
> +	 * Find the erdt_domain_info that contains this CPU,
> +	 * check if all CPUs in erdt_domain_info's cpumask
> +	 * have the same id(L3 id).
> +	 *
> +	 * For example, erdt_domain_info reports:
> +	 * domain0: CPU0, CPU2, domain1: CPU1, CPU3
> +	 * rdt_domain_hdr reports:
> +	 * domain0: CPU0, CPU1, domain1: CPU2, CPU3
> +	 * As a result, CPU1, CPU2 should not be covered by resctrl.
> +	 */
> +	list_for_each_entry(d, &domain_info_list, entry) {
> +

(unnecessary empty line)

> +		if (cpumask_test_cpu(cpu, &d->cpu_mask)) {
> +			if (d->dom_id == -1) {
> +				d->dom_id = dom_id;
> +			} else if (d->dom_id != dom_id) {
> +				pr_warn(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(&d->cpu_mask), d->dom_id);
> +
> +				return false;
> +			}
> +
> +			return true;
> +		}
> +	}
> +
> +	pr_warn(FW_BUG "Cannot find CACD domain for CPU%d\n", cpu);
> +	return false;
> +}
> +
> +/*
> + * Associate ERDT table information with this domain.
> + */
> +void erdt_l3_mon_domain_setup(int cpu, 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 (cpumask_test_cpu(cpu, &d->cpu_mask)) {

Any motivation for why the cpumask is used as a test instead of the domain ID?

> +			/* Assign the ERDT information to hw_dom */
> +			if (!hw_dom->d_info)
> +				hw_dom->d_info = d;

This should become obvious if this initialization is done from l3_mon_domain_setup()
where hw_mon would have been kzalloc'ed. This means that if hw_dom->d_info is
already initialized that there would be *two* ERDT domains that map to an existing
resctrl monitoring domain. That looks to be something to complain about?

> +			return;
> +		}
> +	}
> +}
> +
>  void erdt_exit(void)
>  {
>  	struct erdt_domain_info *d, *tmp;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index bdff3ea36e62..bd437c3e5bf0 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -99,14 +99,19 @@ struct rdt_hw_ctrl_domain {
>   * @arch_mbm_states:	Per-event pointer to the MBM event's saved state.
>   *			An MBM event's state is an array of struct arch_mbm_state
>   *			indexed by RMID on x86.
> + * @d_info:		ERDT table information of this domain
>   *
>   * Members of this structure are accessed via helpers that provide abstraction.
>   */
>  struct rdt_hw_l3_mon_domain {
>  	struct rdt_l3_mon_domain	d_resctrl;
>  	struct arch_mbm_state		*arch_mbm_states[QOS_NUM_L3_MBM_EVENTS];
> +	const struct erdt_domain_info	*d_info;
>  };
>  
> +bool erdt_cpu_valid(int cpu);
> +void erdt_l3_mon_domain_setup(int cpu, struct rdt_domain_hdr *hdr);
> +
>  static inline struct rdt_hw_ctrl_domain *resctrl_to_arch_ctrl_dom(struct rdt_ctrl_domain *r)
>  {
>  	return container_of(r, struct rdt_hw_ctrl_domain, d_resctrl);

Reinette