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

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

Hi Reinette,

On 8/20/2026 7:04 AM, Reinette Chatre wrote:
> 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".
> 

Got it, will do.

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

OK, will do.

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

Yes, moving it to resctrl_arch_online_cpu() is more reasonable, will 
adjust the code.

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

OK, will add lockdep_assert_held(&domain_list_lock) in erdt_cpu_valid().

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

OK, l3_mon_domain_setup() can leverage erdt_l3_mon_domain_setup() to attach
the ERDT domain to the corresponding newly-created hw domain. And later when
other CPUs of the same domain are onlined, they share the same hw_dom, 
so there
is no need to re-attach the ERDT domain again. I'll adjust the code 
accordingly.


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

Right, will fix this and also !erdt_enabled case, and add a warning here.

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

OK, will remove this.

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

Let me switch to to compare the erdt_domain.id and the llc_id directly.

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

Yes, this should be a firmware bug and let me add a warning here.

thanks,
Chenyu