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

["Chen, Yu C" <yu.c.chen@intel.com>]

Hi Reinette,

On 7/11/2026 7:45 AM, Reinette Chatre wrote:
> 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.
> 

Okay, will update the changelog and all other patches accordingly.

>>
>> During attachment, a sanity check is triggered to verify whether the
> 
> Please write in imperative tone.
> 

OK.

>> @@ -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?
> 

erdt_l3_mon_domain_setup() was used to detect mismatches, while 
l3_mon_domain_setup()
would take corrective action upon a mismatch (CPU removal) - so they 
print slightly differently.
After switching to your proposed approach below, all mismatch logging 
can be centralized
within erdt.c

>> +		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?
> 

You are right; this cpumask check is unnecessary for this newly created
rdt_hw_l3_mon_domain case. erdt_l3_mon_domain_setup() was originally 
implemented
to handle both the cpumask validation and ERDT domain assignment, and it 
was intended
for use in both the !hdr and hdr paths.
As a result, the !hdr path (for newly allocated rdt_hw_l3_mon_domain) 
carries unnecessary
logic for cpumask mismatch detection.

> 
>> +
>>   	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);
> 			
> 			...
> 		}
> 

Yes, this version is clearer. I will switch over to this approach.

>> +		}
>>   		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.
> 

OK, will do.

>> +	 * 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?
> 

My intention was to state that the cpumask of hw_dom is a subset of the 
cpumask
exposed by ERDT. Let me revise the expression.

>> +	 * 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.)
> 

OK, will do.

>> +	 * 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?
> 

I think it should be CPU1 and CPU3 that will be removed from resctrl -
because the first CPU of the hw_dom->cpumask will not find a mismatch per
your description previously.

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

OK, will do.

>> +
>> +		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?
> 

Yes, let me replace this "set then clear on error" by "checking cpu 
sanity before setting it
to hdr->cpu_mask"

>> --- 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?
> 

Ok, will change it to
const struct erdt_domain_info		*d_info;
as after hw_dom->d_info = d, the content of the pointed-to 
erdt_domain_info is
never modified through d_info.

thanks,
Chenyu