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/27/26 3:36 AM, Chen Yu wrote:
> On Fri, Oct 23, 2026 at 04:57:54PM -0700, Reinette Chatre wrote:
>> On 10/13/26 7:58 PM, Chen Yu wrote:
...

>>> 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?
>>
> 
> Yes, this is done in patch #3. Here it actually assigns the L3 ID to the
> ERDT domain.
> 
>> "adding it" - what does "it" refer to? the ERDT domain or the CPU?
>>
> 
> "It" here refers to the CPU, but it is a bit confusing, I'll revise it to:
> When a CPU comes online, check that its ERDT domain and its CPUID L3 cache
> agree, and bind the ERDT domain to the CPU's L3 cache ID.

Looks good, thank you.

> 
>>> 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.
>>
> 
> Thanks for the detailed explanation, got it. I'll change it to:
> On a mismatch the CPU stays online and is not added to any monitor domain,
> but still goes through clear_closid_rmid() and resctrl_online_cpu(),
> so it joins the default group. How about the following:
> 
>         mutex_lock(&domain_list_lock);
>         /* Keep a CPU out of all domains if its ERDT and CPUID L3 domains conflict. */
>         if (!erdt_try_bind_domain(cpu))
>                 goto unlock;
> 
>         for_each_capable_rdt_resource(r)
>                 domain_add_cpu(cpu, r);
> unlock:
>         mutex_unlock(&domain_list_lock);
> 
>         clear_closid_rmid(cpu);
>         resctrl_online_cpu(cpu);
> 
>         return 0;
> 

Looks good, thank you.

> And you also mentioned that even if CPU1 is not eligible to be added to
> the monitor domain's cpumask, since the event can be read from any CPU,
> CPU1 can still read the domain's monitor data via (mmio_base, RMID) without
> involving any CPU, although the data read from this address is questionable
> because the ERDT is already broken.
> 

ack.

>>> 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.
>>
> 
> How about adding a separate paragraph after the domain list description. It also
> states more precisely what is protected: the binding:
> cpu_dom->dom_id = dom_id, not the list itself, because domain_info_list is only
> modified during init.
> 
>  * All writers take this mutex.
>  *
>  * This mutex also serializes binding an ERDT domain to an L3 cache ID
>  * (erdt_domain_info::dom_id) when a CPU comes online.
> 

Looks good, thank you.

> 
> On second thought, would this race condition really happen? erdt_try_bind_domain()
> is the function that changes erdt_domain_info::dom_id, and it is invoked by
> resctrl_arch_online_cpu(). CPU onlining is performed one at a time, not in parallel,
> so it is unlikely that two CPUs invoke erdt_try_bind_domain() at the same time. Although
> in the future, it is possible that onlining could be changed to run in parallel.

This assumes the current usage. If any future change needs to access erdt_domain_info::dom_id
then this makes it clear that such access requires domain_list_lock to not race with the
online handler that modifies it.

...

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

...

>>> + * 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?
>>
> 
> OK, how about changing it to:
> 
>  * Return: true if @cpu can be added to resctrl domains, false if @cpu must
>  * not be in any resctrl domain.

This refers to how caller acts on result of this function as opposed to what
this function actually returns. Compare to, for example:

 * Return: true if ERDT is unused, or if the ERDT domain containing @cpu is
 * bound to @cpu's L3 cache ID, whether that binding already existed or was
 * created by this call. false if ERDT and CPUID cannot be reconciled into a
 * one-to-one binding, in which case no binding is modified.

> 
> The other "covered by resctrl" wording in the comment is updated too.
> 
>>> + */
>>> +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?
>>
> 
> It should be always valid if ERDT is enabled. The CPU that creates the L3
> monitoring domain has passed erdt_try_bind_domain(), which bound an ERDT
> domain to that CPU's L3 cache ID. The L3 monitoring domain ID is the L3
> cache ID, so the lookup should always find a match.

Could you please add a this information to the function comment?

> 
> The check in erdt_mon_read() was only for robustness(I did not find
> scenario that it could be NULL), just in case I added "if (WARN_ON_ONCE(!d))"
> in the next version.

Sounds good, thank you.
>>> + */
>>> +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;
>>> +		}
>>> +	}
>>> +}
>>> +
Reinette