Re: [PATCH v5 04/10] x86/resctrl: Attach ACPI ERDT information to L3 mon domain on CPU online

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

Hi Chenyu,

On 7/1/26 6:45 AM, Chen Yu wrote:
> After the rdt_hw_l3_mon_domain has been created during CPU online,
> attach the pre-parsed ACPI ERDT table information to the
> rdt_hw_l3_mon_domain to facilitate monitor data read via the
> ERDT and its sub-tables information.

Please follow tip guidance for changelog by starting with context before
jumping to what the patch does.

> 
> During attachment, a sanity check is triggered to verify whether the

Please write in imperative tone.

> CPU mask reported by silicon (CPUID leaf 4) matches the information
> exposed by firmware (CACD table). If inconsistent, this CPU will
> be cleared from the legitimate domain mask.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Tested-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
> v4->v5:
>    new patch.
> ---
>  arch/x86/kernel/cpu/resctrl/core.c     | 17 +++++++++
>  arch/x86/kernel/cpu/resctrl/erdt.c     | 49 ++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/internal.h |  4 +++
>  3 files changed, 70 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 23925bcd71d7..2e95586ebe45 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -549,6 +549,19 @@ static void l3_mon_domain_setup(int cpu, int id, struct rdt_resource *r, struct
>  	d->ci_id = ci->id;
>  	cpumask_set_cpu(cpu, &d->hdr.cpu_mask);
>  
> +	/*
> +	 * Verify whether the CPU domain information matches the ACPI data.
> +	 * Skip adding the newly created domain to the list if there is a mismatch.
> +	 * ACPI information should be assigned to the domain prior to its insertion
> +	 * into the list, in case others might iterate the list in parallel.
> +	 */
> +	if (erdt_l3_mon_domain_setup(cpu, &d->hdr)) {
> +		pr_warn("CPU%d has inconsistent domain information, do not add this new domain\n", cpu);

erdt_l3_mon_domain_setup() already contains a pr_warn() when there is an error, is this
pr_warn() needed?

> +		cpumask_clear_cpu(cpu, &d->hdr.cpu_mask);
> +		l3_mon_domain_free(hw_dom);
> +		return;
> +	}

Since the rdt_hw_l3_mon_domain was just allocated and the current CPU is the *only*
CPU assigned to it, erdt_l3_mon_domain_setup() should never fail? I do not think
any CPU checking is necessary here - when the flow reaches l3_mon_domain_setup()
it can just assign the ERDT domain directly, no?


> +
>  	arch_mon_domain_online(r, d);
>  
>  	if (l3_mon_domain_mbm_alloc(r->mon.num_rmid, hw_dom)) {
> @@ -591,6 +604,10 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
>  			resctrl_arch_mbm_cntr_assign_set_one(r);
>  		if (!hdr)
>  			l3_mon_domain_setup(cpu, id, r, add_pos);
> +		else if (erdt_l3_mon_domain_setup(cpu, hdr)) {
> +			pr_warn("CPU%d has inconsistent domain information, remove it from the domain\n", cpu);
> +			cpumask_clear_cpu(cpu, &hdr->cpu_mask);

I do not think it is necessary to add the CPU to hdr->cpu_mask for the ERDT test to
be performed (more below). It thus looks to me as though this ERDT domain assignment
should be split - the error check is only needed if there is a header and if that finds
a problem it can just bail early (before adding CPU to hdr->cpu_mask). Something like:

		hdr = resctrl_find_domain(&r->mon_domains, id, &add_pos);
		if (hdr) {
			if (/* This CPU's ERDT domain a NOT subset of hdr->cpu_mask */)
				bail;
			cpumask_set_cpu(cpu, &hdr->cpu_mask);
		}

		switch (r->rid) {
		case RDT_RESOURCE_L3:
			...
			if (!hdr)
				/* l3_mon_domain_setup() initializes hw_dom->d_info without CPU checking */
				l3_mon_domain_setup(cpu, id, r, add_pos);
			
			...
		}

> +		}
>  		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 6405df9be817..6c1df7e43eab 100644
> --- a/arch/x86/kernel/cpu/resctrl/erdt.c
> +++ b/arch/x86/kernel/cpu/resctrl/erdt.c
> @@ -212,6 +212,55 @@ static __init bool parse_rmdd_entry(struct acpi_subtbl_hdr_16 *rmdd_hdr)
>  	return false;
>  }
>  
> +/*
> + * Associate ERDT table information with this domain.
> + */
> +int 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;
> +	struct list_head *pos;
> +
> +	if (!__erdt_enabled)
> +		return 0;
> +
> +	/*
> +	 * Find the erdt_domain_info that contains this CPU,

Please use entire line length available.

> +	 * compare erdt_domain_info's cpumask with the cpumask
> +	 * exposed by hw_dom (derived from CPUID leaf 4).
> +	 * If yes, assign the erdt_domain_info in the hw_dom,

What does "If yes" refer to?

> +	 * otherwise this CPU should be isolated from resctrl.
> +	 * For example, the hw_dom reports CPU{0,1} are in
> +	 * l3 domain0, CPU{2,3} belongs to domain1. Meanwhile

Please use consistent terms (l3 -> L3, cpumask -> cpu_mask, etc.)

> +	 * erdt_domain_info reports that CPU{0,2} are in domain0,
> +	 * CPU{1,3} are in domain1. So when it comes to CPU1,
> +	 * a mismatch is detected, we should remove CPU1 from
> +	 * resctrl.

CPU1 and CPU2?

> +	 */
> +	list_for_each(pos, &domain_info_list) {
> +		d = container_of(pos, struct erdt_domain_info, list);

list_for_each_entry()

> +
> +		if (cpumask_test_cpu(cpu, d->cpu_mask)) {
> +			if (!cpumask_subset(&hdr->cpu_mask, d->cpu_mask)) {

Consider the above two tests, the first, cpumask_test_cpu() already checks that CPU in
parameter belongs to d->cpu_mask, it is thus not required for the same CPU to be
a member of hdr->cpu_mask in the cpumask_subset() that follows? The callers currently
add the CPU to hdr->cpu_mask before calling erdt_l3_mon_domain_setup() and then
remove the CPU on failure ... that thus looks unnecessary? Caller can just add the
CPU to hdr->cpu_mask on success?

> +				pr_warn(FW_BUG "Mismatch detected, CPU%d in L3 domain(%*pbl) and CACD domain(%*pbl)\n",
> +					cpu, cpumask_pr_args(&hdr->cpu_mask), cpumask_pr_args(d->cpu_mask));
> +
> +				return -EIO;
> +			}
> +
> +			hw_dom = resctrl_to_arch_mon_dom(container_of(hdr, struct rdt_l3_mon_domain, hdr));
> +			/* No mismatch, assign the ERDT information to hw_dom */
> +			if (!hw_dom->d_info)
> +				hw_dom->d_info = d;
> +
> +			return 0;
> +		}
> +	}
> +
> +	pr_warn(FW_BUG "Cannot find CACD domain for CPU%d\n", cpu);
> +	return -ENOENT;
> +}
> +
>  void erdt_exit(void)
>  {
>  	struct erdt_domain_info *d;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 299d7222f693..7d9100b7648f 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -92,14 +92,18 @@ 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(read-only)

read-only in comments can be made explicit with a const below?

>   *
>   * 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];
> +	struct erdt_domain_info		*d_info;
>  };
>  
> +int 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