Re: [RFC PATCH v3 2/2] ACPI: NUMA: debug invalid unused PXM value for CFMWs

[Mike Rapoport <rppt@kernel.org>]

Hi,

On Fri, Mar 21, 2025 at 11:51:16AM +0000, Jonathan Cameron wrote:
> On Fri, 21 Mar 2025 10:36:02 +0800
> Yuquan Wang <wangyuquan1236@phytium.com.cn> wrote:
> 
> > The absence of SRAT would cause the fake_pxm to be -1 and increment
> > to 0, then send to acpi_parse_cfmws(). If there exists CXL memory
> > ranges that are defined in the CFMWS and not already defined in the
> > SRAT, the new node (node0) for the CXL memory would be invalid, as
> > node0 is already in "used", and all CXL memory might be online on
> > node0.
> > 
> > This utilizes node_set(0, nodes_found_map) to set pxm&node map. With
> > this setting, acpi_map_pxm_to_node() could return the expected node
> > value even if no SRAT.
> > 
> > If SRAT is valid, the numa_memblks_init() would then utilize
> > numa_move_tail_memblk() to move the numa_memblk from numa_meminfo to
> > numa_reserved_meminfo in CFMWs fake node situation.
> 
> I would call out that numa_move_tail_memblk() is called in
> numa_cleanup_meminfo() which is indeed called by num_memblks_init()
> 
> > 
> > If SRAT is missing or bad, the numa_memblks_init() would fail since
> > init_func() would fail. And it causes that no numa_memblk in
> > numa_reserved_meminfo list and the following dax_cxl driver could
> > find the expected fake node.
> > 
> > Use numa_add_reserved_memblk() to replace numa_add_memblk(), since
> > the cxl numa_memblk added by numa_add_memblk() would finally be moved
> > to numa_reserved_meminfo, and numa_add_reserved_memblk() here could
> > add cxl numa_memblk into reserved list directly. Hence, no matter
> > SRAT is good or not, cxl numa_memblk could be allocated to reserved
> > list.
> > 
> > Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn>
> 
> This definitely wants input from Mike Rapoport.
> Looks fine to me, but there may be some subtle corners I'm missing.

I'm fine with exposing numa_add_reserved_memblk(), but I don't understand
CXL discovery enough to say if adding CXL ranges directly to
numa_reserved_meminfo.

If this is always the case that CFMW regions end up on
numa_reserved_meminfo, adding them there in the first place does make
sense.
 
> > ---
> >  drivers/acpi/numa/srat.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
> > index 00ac0d7bb8c9..50bfecfb9c16 100644
> > --- a/drivers/acpi/numa/srat.c
> > +++ b/drivers/acpi/numa/srat.c
> > @@ -458,11 +458,12 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> >  		return -EINVAL;
> >  	}
> >  
> > -	if (numa_add_memblk(node, start, end) < 0) {
> > +	if (numa_add_reserved_memblk(node, start, end) < 0) {
> >  		/* CXL driver must handle the NUMA_NO_NODE case */
> >  		pr_warn("ACPI NUMA: Failed to add memblk for CFMWS node %d [mem %#llx-%#llx]\n",
> >  			node, start, end);
> >  	}
> > +
> 
> Unrelated change.  Always give patches a final look through to spot
> things like this.  Trivial, but they all add noise to what we are focusing on.
> 
> >  	node_set(node, numa_nodes_parsed);
> >  
> >  	/* Set the next available fake_pxm value */
> > @@ -646,8 +647,12 @@ int __init acpi_numa_init(void)
> >  		if (node_to_pxm_map[i] > fake_pxm)
> >  			fake_pxm = node_to_pxm_map[i];
> >  	}
> > -	last_real_pxm = fake_pxm;
> > -	fake_pxm++;
> > +
> > +	/* Make sure CFMWs fake node >= 1 */
> > +	fake_pxm = max(fake_pxm, 0);
> > +	last_real_pxm = fake_pxm++;

I'd make it more explicit:

	/*
	 * Make sure CFMWs fake nodes follow last_real_pxm, even when SRAT
	 * is invalid
	 */
	last_real_pxm = max(fake_pxm, 0);
	fake_pxm = last_real_pxm + 1;	

> > +	node_set(0, nodes_found_map);
> > +
> >  	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS, acpi_parse_cfmws,
> >  			      &fake_pxm);
> >  
> 

-- 
Sincerely yours,
Mike.