Re: [PATCH v5 04/10] x86/resctrl: Attach ACPI ERDT information to L3 mon domain on CPU online
From: Chen, Yu C
Date: Tue Jul 14 2026 - 04:51:37 EST
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