From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mta1.migadu.com (out-178.mta1.migadu.com [95.215.58.178]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 53CCC23B638 for ; Mon, 24 Aug 2026 08:07:17 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.178 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787558840; cv=none; b=ApAedIg20dueDat3U7+IawXqSaZDvOIqTaJEzUW4Ao3cWzxgzkyb2oev8K+WuHHEM4yFPgUocfBJSlzILhIKhljqXyNdyuY5u9SNuZGzL9XkLPKTQaCTZXV8XVlS42yE5rtGVMVdc8KqhuNmiK9WVMhklihc7VtRszywYnnyAoA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787558840; c=relaxed/simple; bh=90qgDFNvaju89zdymfJp+uL9mZcLcBYb6iDBc/xkyQQ=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Glgl3eGo0Q7AH0fvybTuiZJQGOKlxUx9UKZtrJXljJ2aLMplvDii6Z5jxt4IggQwY83kB0S69MpadmL+yKtcbHvkor7Jhq3rOb+3ZX2vezUDSFXw9PYsF4pK7tS/fsTzC6ciXj8bWq8am0Y6YCbqEqR7jkYBtX1PPMauitaVF9w= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=cD6RS+zJ; arc=none smtp.client-ip=95.215.58.178 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="cD6RS+zJ" X-Envelope-To: linux-kernel@vger.kernel.org DKIM-Signature: a=rsa-sha256; bh=90qgDFNvaju89zdymfJp+uL9mZcLcBYb6iDBc/xkyQQ=; c=simple/simple; d=linux.dev; h=from:to:subject:date:message-id:mime-version:content-type; s=key1; t=1787558836; v=1; x=1788163636; b=cD6RS+zJnpj6NSKnhGwqqzo/bV+J+pPxlHYUa17HXZpupOSpAMKTPd1A8aY/NV96lFWxHrYQ jKIH5QQrzO3b8ugb0XnzbrfaYG2pa+BTA+45Q7yFpsqrrIkwrbRVq2SnPfmxJwHuwh6/4BjdBo3 bH7F+IOazjH6arS0p+9KHLbM= X-Envelope-To: linux-kernel@vger.kernel.org Received: from three-body (240e:398:90:76f0:e24e:f12a:c3d1:9cf4) by smtp.migadu.com with ESMTPS id ed0a27cf6e150315; Mon, 24 Aug 2026 08:07:06 +0000 X-Mizu-Trace-ID: ed0a27cf6e150315 X-Migadu-Flow: FLOW_OUT Date: Tue, 25 Aug 2026 16:06:41 +0800 From: Chen Yu To: Reinette Chatre Cc: Chen Yu , tony.luck@intel.com, tglx@kernel.org, bp@alien8.de, mingo@redhat.com, dave.hansen@linux.intel.com, hpa@zytor.com, fenghuay@nvidia.com, babu.moger@amd.com, x86@kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v6 3/9] x86/resctrl: Parse ACPI ERDT table and save CACD cpumask for RMDD domains Message-ID: References: <7eaafec96fb5494d3f4b927fa295d5b756f82c64.1784968626.git.yu.c.chen@intel.com> <9a86b245-916a-43ed-9bee-897aa3432a89@intel.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9a86b245-916a-43ed-9bee-897aa3432a89@intel.com> Hi Reinette, On Wed, Aug 19, 2026 at 04:01:33PM -0700, Reinette Chatre wrote: > > Hi Chenyu, > > On 7/25/26 2:22 AM, Chen Yu wrote: > > From: Anil S Keshavamurthy > > > > Parse the RMDD subtables within the ERDT ACPI table and their nested > > CACD entries to construct per-domain CPU masks. > > The changelog always needs to start with context. Please see "Changelog" > in Documentation/process/maintainer-tip.rst for complete expectations. > OK, I will first add some background information (including what ERDT and CACD are). > > > > There is one ERDT table per platform. Each RMDD describes one resource > > I do not think what "RMDD" stands for has been introduced at this point yet. > > > management domain (RMD), also known as an L3 domain, and carries MMIO > > base information for later monitoring support. > > > > For each RMDD, parse the associated CACD, map its x2APIC IDs to logical > > What is "CACD"? Please always expand acronym before its first use. > > > CPUs, and save the resulting CPU mask. This mask associates each ERDT > > domain with the CPUs that belong to it and is used later when attaching > > ERDT data to resctrl monitoring domains. > > Please let each patch description stand on its own without referring to later > patches in series. If a patch provides capability in preparation for > future changes then you can use language like, > "Associate every ERDT domain with the CPUs that belong to it to prepare > for attaching ERDT data to resctrl monitoring domains." > Thanks for the guidance, will adjust it. > > static __init bool get_rdt_resources(void) > > { > > + erdt_init(); > > rdt_alloc_capable = get_rdt_alloc_resources(); > > rdt_mon_capable = get_rdt_mon_resources(); > > > > Functions are not expected to leave dangling state when they return failure. get_rdt_resources() > returning false is considered a failure and now the caller is left to clean up the dangling > state which is not a familiar pattern to use and thus something that can/will trip people. > > On top of this this implementation pushes the cleanup very far from even the caller making > this unfamiliar pattern even harder to recognize. > > Please let get_rdt_resources() clean up after itself on failure to find any resources. > The original intent was to encapsulate the cleanup logic as a helper function (erdt_exit()), since it is needed in multiple places across the code. All other call sites invoke this function when necessary. If we place the cleanup call directly inside resctrl_arch_late_init(), it would only execute erdt_exit() once within that function. Alternatively, if we let get_rdt_resources() handle cleanup via erdt_exit(), then we would need to add explicit erdt_exit() calls in __resctrl_arch_late_init() for each failure path - for example, when cpuhp_setup_state() fails or when resctrl_init() fails. Anyway, if code readability is a priority, I'll change the logic as suggested. > > @@ -1114,7 +1115,7 @@ void resctrl_cpu_detect(struct cpuinfo_x86 *c) > > } > > } > > > > -static int __init resctrl_arch_late_init(void) > > +static int __init __resctrl_arch_late_init(void) > > { > > struct rdt_resource *r; > > int state, ret, i; > > @@ -1157,6 +1158,15 @@ static int __init resctrl_arch_late_init(void) > > return 0; > > } > > > > +static int __init resctrl_arch_late_init(void) > > +{ > > + int ret = __resctrl_arch_late_init(); > > + > > + if (ret) > > + erdt_exit(); > > + return ret; > > +} > > Related to earlier comment on cleanup I find this cleanup to be asymmentrical > and inconsistent with how resctrl usually does cleanup. Why not do cleanup in > (original) resctrl_arch_late_init() to be consistent with other cleanup when > failures are encountered during initialization, for example, cpuhp_remove_state()? > I find that having the cleanup handled where error is encountered is easier to understand. > Thomas previously suggested this approach to avoid using goto. https://lore.kernel.org/lkml/871pem5jnh.ffs@fw13/ However, I agree that we can adopt the individual cleanup strategy instead, as it will improve code readability. > > + > > late_initcall(resctrl_arch_late_init); > > > > static void __exit resctrl_arch_exit(void) > > @@ -1166,6 +1176,8 @@ static void __exit resctrl_arch_exit(void) > > cpuhp_remove_state(rdt_online); > > > > resctrl_exit(); > > + > > + erdt_exit(); > > > Here the cleanup is indeed done in the same place as cleanup of > other init work (cpuhp_remove_state()). > Understood. I will ensure consistency whenever cpuhp_remove_state() is called, erdt_exit() will also be invoked in the original resctrl_arch_late_init(). > > } > > +/* True when the ERDT ACPI table describes at least one domain with at least one CPU. */ > > +static bool erdt_enabled; > > + > > +#define ERDT_VALID_VERSION 1 > > +#define RMDD_FLAG_CPU_L3_DOMAIN BIT(0) > > + > > +/* Bitmask of valid sub-tables found in the first RMDD, used to ensure all RMDDs match. */ > > +static u32 valid_subtbl_mask; > > + > > +/* Domain ID of the first RMDD that established @valid_subtbl_mask, for diagnostics. */ > > +static u16 first_rmdd_domain_id; > > + > > +static int erdt_max_rmid; > > Could this ever be negative? Could this instead be of same type as the value it is > initialized with? Looks like this will improve type safety with the min_t() usage then > using the accurate and consistent type of both parameters? > It should not be negative, let me convert it into u32 and use min() directly. > I also think a comment describing erdt_max_rmid will be helpful, especially considering > that it has "max" in its name but then its value is determined using the *minimum* of > all domains' RMID? > OK, let me add the following comment when Tony also raised this question. resctrl_rmid_realloc_threshold is a single global value, and resctrl_arch_round_mon_val() takes no domain argument, so a single scale has to be derived from the per-domain cmrc->up_scale. max() was chosen because the rounding is a floor: a larger scale yields a slightly lower threshold, i.e. an RMID has to drop to a slightly lower occupancy before it is reused. https://lore.kernel.org/all/3053f498-1eb8-4c64-8d52-847829f48a1b@intel.com/ > > + > > +int erdt_get_max_rmid(void) > > +{ > > + return erdt_max_rmid; > > +} > > + > > +static void __iomem *erdt_ioremap(phys_addr_t base, u32 num_pages, const char *desc) > > +{ > > + void __iomem *addr; > > + size_t size; > > + > > + if (check_mul_overflow(num_pages, SZ_4K, &size)) > > + return NULL; > > + > > + addr = ioremap(base, size); > > The types seem to target a function with prototype ioremap(phys_addr_t base, size_t size) > but I find arch/x86/include/asm/io.h to declare: > void __iomem *ioremap(resource_size_t offset, unsigned long size); > > Since it is this code that determines the type it could just use matching accurate type > from the beginning? > OK, let me switch it to unsigned long to be consistent with x86 declaration. > > + if (!addr) > > + pr_warn(FW_BUG "ERDT: Failed to map %s at phys addr %pa (size: %u pages)\n", > > + desc, &base, num_pages); > > Please align to open parenthesis. > OK. > > +static __init int cacd_init(struct acpi_subtbl_hdr_16 *subtbl, > > + struct erdt_domain_info *domain_info) > > +{ > > + struct acpi_erdt_cacd *cacd = (struct acpi_erdt_cacd *)subtbl; > > + int num_ids, cpu; > > Can num_ids ever be negative? If not, please use unsigned type. > It will not be negative, let me switch it to unsigned int. > > +static __init bool parse_rmdd_table(struct acpi_subtbl_hdr_16 *rmdd_hdr) > > +{ > > + struct erdt_domain_info *domain_info; > > + struct acpi_subtbl_hdr_16 *subtbl; > > + struct acpi_erdt_rmdd *rmdd; > > + u32 subtbl_mask = 0; > > + > > + if (rmdd_hdr->length < sizeof(*rmdd)) { > > + pr_warn(FW_BUG "Invalid RMDD length %u bytes\n", rmdd_hdr->length); > > + return false; > > + } > > + > > + rmdd = (struct acpi_erdt_rmdd *)rmdd_hdr; > > Could this initialization be done at time of declaration to be consistent with the other > functions parsing tables? The length comparison could then use rmdd->header.length to > match how this check is done in all the other places. Being consistent makes the > code much easier to understand. > > OK, will modify the code. > > + > > + /* Quietly ignore non-CPU-based L3 domains */ > > + if (!(rmdd->flags & RMDD_FLAG_CPU_L3_DOMAIN)) > > + return true; > > + > > + domain_info = kzalloc_obj(*domain_info, GFP_KERNEL); > > + if (!domain_info) > > + return false; > > + > > + domain_info->dom_id = -1; > > + > > + domain_info->base[ERDT_MMIO_RMDD_CREG] = > > + erdt_ioremap(rmdd->creg_base, rmdd->creg_size, "RMDD ctrl base"); > > + if (!domain_info->base[ERDT_MMIO_RMDD_CREG]) > > + goto cleanup; > > + > > + for (subtbl = rmdd_subtbl(rmdd); > > + subtbl_valid((void *)rmdd + rmdd->header.length, subtbl); > > + subtbl = next_subtbl(subtbl)) { > > I find it curious how the implementation varies in how the tables are parsed. For example, here > it uses a for () loop to cycle through the tables while enumerate_erdt_table() uses a while() > loop for what appears to be the same flow. Are they actually different? Why are the two different > patterns needed? > Let me switch them both to "for" loop to keep consistence. > > +void erdt_exit(void) > > +{ > > + struct erdt_domain_info *d, *tmp; > > + > > + list_for_each_entry_safe(d, tmp, &domain_info_list, entry) { > > + list_del(&d->entry); > > + cleanup_one_domain(d); > > + } > > + erdt_enabled = false; > > + valid_subtbl_mask = 0; > > + first_rmdd_domain_id = 0; > > Should erdt_max_rmid be reset also? > Yes, let me fix this. > > + > > + if (erdt->header.length < sizeof(*erdt)) { > > + pr_warn(FW_BUG "ERDT: Invalid table length %u bytes\n", erdt->header.length); > > + return -EINVAL; > > + } > > + > > + subtbl = (void *)erdt + sizeof(struct acpi_table_erdt); > > Please use sizeof(*erdt) > OK, will do. > > +enum erdt_mmio_type { > > + ERDT_MMIO_RMDD_CREG, > > + ERDT_MMIO_LAST = ERDT_MMIO_RMDD_CREG > > +}; > > + > > +#define ERDT_MMIO_NUM_TYPES (ERDT_MMIO_LAST + 1) > > + > > +/** > > + * struct erdt_domain_info - Per-domain ERDT information > > + * @base: Array of ioremapped MMIO region base addresses, indexed by ERDT_MMIO_* type > > I think "type" can be dropped? The enum is already implicitly an "MMIO type"? > Got it, will remove this "type". thanks, Chenyu