Received-SPF: pass (google.com: domain of linux-kernel+bounces-160555-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 26 Apr 2024 20:14:48 +0200
From: Robert Richter <rrichter@amd.com>
To: Alison Schofield <alison.schofield@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-cxl@vger.kernel.org, Len Brown <lenb@kernel.org>
Subject: Re: [PATCH v4 6/7] ACPI/NUMA: Add log messages for memory ranges
 found in CEDT
Message-ID: <ZivvGJcXKPN_L5fn@rric.localdomain>
References: <20240424154846.2152750-1-rrichter@amd.com>
 <20240424154846.2152750-7-rrichter@amd.com>
 <6629474eab671_b6e0294e8@dwillia2-mobl3.amr.corp.intel.com.notmuch>
 <ZioGhwNWQyFjRtZ-@rric.localdomain>
 <ZiqnbD0CB9WUL1zu@aschofie-mobl2>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZiqnbD0CB9WUL1zu@aschofie-mobl2>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?EErnr3DKZ55sGKhr43I6sC7a9/59+xgHcQcG+FB0QRlp5D6ZmAraEoSZpGYU?=
 =?us-ascii?Q?XBEho68ejuwYircG/AYGkuHrTZFTX9oMTciLzBFMlFe4guDECpL2k2eXPrm6?=
 =?us-ascii?Q?xwTnlj61MAtk2KERcW0T0pvCcKLZyLsQvjPP0mMm87oYS4D4b2OngrHo1MiM?=
 =?us-ascii?Q?ASPx0TZjpzb/lrDQlTCjZcWJjfwffDxeipMo586ZYpm/y7E0mjn+l9KZ3CW4?=
 =?us-ascii?Q?U4yY2uATcR5v2ae5RF5i0pXbj7Jahi6xjVJYBiDi6gehLdeVDRepVIbcnwBC?=
 =?us-ascii?Q?Cz4zqXCi6Vw2RTV6P666gz0NlysCGjfSFH7t2099YrdKuvO+0E/Uhk5un1XE?=
 =?us-ascii?Q?cGFmB7ztj2m6Iclu/Deml+kj6OCyI50tXGkfeQrLByxymnucrn64P2H1Hosg?=
 =?us-ascii?Q?MuSo8UsCvxDTat3pXlcrVfB2UCH1A92/OVo5ZrswEesn7lzUPW0dEq7PJFs+?=
 =?us-ascii?Q?8CEqlu0T0ogr70eid3q954Cqnr8KKm5lr0aFO6gFtpS6x6eQPF/F3w1Smy6g?=
 =?us-ascii?Q?UwUgHnVOKFSPjtz5V9SI5h7akEhATbJTiS9i6TiojbPrGBZXg1adG2R+N07d?=
 =?us-ascii?Q?1wspCArvyhdTU82CILfG7wrvSfgghf7Ne6s+xQXqXuugcBGHH9sFdVMkLzaC?=
 =?us-ascii?Q?osA+Pni1YbQGwiOilTsKQm6F8oWolfaEKveMLifwOQ4UKfgETy8343itjDRg?=
 =?us-ascii?Q?z4dWnxcGK3swZnfrFGfYZWGwOUiaKiCcjjqrnk9ZUqc71raAZ4Zo1taqnF4k?=
 =?us-ascii?Q?7cKxJUyhspIzMpkFE1tcH1gNappWYAjdfR1AzRZf+MYcvPTjLMFfKHaafNg3?=
 =?us-ascii?Q?+xJ9iXTHwQem5sr+Y9MfvrYUP7qOPRdNJ8P3F/+VOfJY4CSxgWifJVpPlIJp?=
 =?us-ascii?Q?eNNAN3Y9ykXWwbzrxjXImC4aV9MzDFkwaeqSa/yv5MKNrvVv4PuvO6F8P4Cj?=
 =?us-ascii?Q?Tpkt1wWLnNnBOZDoN1co2ZzqtwQTBY7n3JM7vZ6wxknkUEvEEcwwbp3kjtOv?=
 =?us-ascii?Q?QEixVGzYG9gCU8Yxq4F2lvlJSeo/Oj2d5t2k6Ku1kglk6EYU3RA71Bjr/IAS?=
 =?us-ascii?Q?C4tatGQx0tXIngCPlP2qpqy3oihwsn2VFZO38dgJ/DCTOzH1ERuUHO8deom8?=
 =?us-ascii?Q?R5/tJ0zdXbItiL+IEkdMLJQCD/57HsA5EhKVQ3caebvo07Dx9WQKv0mV4dcn?=
 =?us-ascii?Q?vuhfX/K0tmETIvTKiaU5tQJlwvJLvAIsakbbMWle4QTGRxZbar9rOHZDIRNW?=
 =?us-ascii?Q?+dvryPI6O6y5kiD6Yg78vw08KDCC9ctKix8W112NfNGfNqep+kFXQCSnK5mj?=
 =?us-ascii?Q?6uxE7dFyeYMvJc3H1StL6ROksYiJFTwJsMyEYuCukxanLzl864jRXTNAYaFk?=
 =?us-ascii?Q?TsPKxVpqjDWdPqa3LavfthGhlxYeC61NdA1sy/0M8B98gXp9A89FTqqlePG1?=
 =?us-ascii?Q?05gIZQjlNJM9f/TBim04LdcgQp7Tyz/urR7Xj8qYD/yGVAY73ruIPgaff9Gp?=
 =?us-ascii?Q?Goi/YsaDnDfJuiQ4jd8Q/D2v/I0ShABRKiJzYZqWnPgR58GUObh0aVKYkk7O?=
 =?us-ascii?Q?6/GUd4hGDXWj2xYetUW89aK7qw0K/bReJIAZuAPK?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e0a7c89e-da41-45f2-31c9-08dc661cc5c0
X-MS-Exchange-CrossTenant-AuthSource: CYYPR12MB8750.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Apr 2024 18:14:54.3931
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Jeg4t6dSqIPuwfFM1kYMZYSlvFr33tHvePmVMl1Rbu8Tg/GI/O8qsSgp5KIS/zXzeGdDlv/04tHYHD6qUxcIDg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV2PR12MB5823

On 25.04.24 11:56:44, Alison Schofield wrote:
> On Thu, Apr 25, 2024 at 09:30:15AM +0200, Robert Richter wrote:
> > On 24.04.24 10:54:22, Dan Williams wrote:
> > > Robert Richter wrote:
> > > > Adding a pr_info() when successfully adding a CFMWS memory range.
> > > > 
> > > > Suggested-by: Alison Schofield <alison.schofield@intel.com>
> > > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > > ---
> > > >  drivers/acpi/numa/srat.c | 10 +++++++++-
> > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
> > > > index e3f26e71637a..c62e4636e472 100644
> > > > --- a/drivers/acpi/numa/srat.c
> > > > +++ b/drivers/acpi/numa/srat.c
> > > > @@ -338,8 +338,11 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> > > >  	 * found for any portion of the window to cover the entire
> > > >  	 * window.
> > > >  	 */
> > > > -	if (!numa_fill_memblks(start, end))
> > > > +	if (!numa_fill_memblks(start, end)) {
> > > > +		pr_info("CEDT: memblk extended [mem %#010Lx-%#010Lx]\n",
> > > > +			(unsigned long long) start, (unsigned long long) end - 1);
> > > 
> > > This looks like pr_debug() material to me.
> > 
> > This should have the same log level as the message below and/or its
> > corresponding SRAT message. CEDT mem blocks wouldn't be reported
> > otherwise only because a smaller (overlapping) entry was registered
> > before. That is why I used pr_info here.
> 
> It does feel like this message belongs but maybe it should also 
> mimic the SRAT define message and emit what node maps this range
> if memblocks were extended.
> 
> But...seems this will emit a message for every CFMWS range, even those
> where no memblk needed to be extended - ie the range was fully described
> in the SRAT.
> 
> Sadly, numa_fill_memblks() return of 'success' has double meaning.
> It can mean memblks were extended, or that (start, end) was found fully
> described. I don't see an good place to insert the message in
> numa_fill_memblks(). 
> 
> Sorry, just stirring the pot here, with no clear suggestion on how
> to emit info.

Ok, I have changed numa_fill_memblks() to also return if memblks have
been modified. That information is used to print the message.

> 
> > 
> > > 
> > > >  		return 0;
> > > > +	}
> > > >  
> > > >  	/* No SRAT description. Create a new node. */
> > > >  	node = acpi_map_pxm_to_node(*fake_pxm);
> > > > @@ -354,8 +357,13 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> > > >  		pr_warn("ACPI NUMA: Failed to add memblk for CFMWS node %d [mem %#llx-%#llx]\n",
> > > >  			node, start, end);
> > > >  	}
> > > > +
> > > >  	node_set(node, numa_nodes_parsed);
> > > >  
> > > > +	pr_info("CEDT: Node %u PXM %u [mem %#010Lx-%#010Lx]\n",
> > > > +		node, *fake_pxm,
> > > > +		(unsigned long long) start, (unsigned long long) end - 1);
> > > > +
> > > 
> > > This makes sense to mirror the SRAT pr_info().
> > 
> > I evaluated this.
> > 
> 
> I read Dan's comment as simple acceptance. Like, yeah this one is good
> because it mimics the SRAT pr_info.

Ah, I misread this, thanks for clarification.

-Robert