Re: [PATCH] ACPI: NUMA: Only parse CFMWS at boot when CXL_ACPI is on

[<dan.j.williams@intel.com>]

Huang, Kai wrote:
[..]
> > I think it makes sense that if you disable CXL hotplug by setting
> > CONFIG_CXL_ACPI=n then no need to reserve numa ids. However, just do
> > something like this rather than add ifdefs to the code:
> > 
> > diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
> > index aa87ee1583a4..62d4a8df0b8c 100644
> > --- a/drivers/acpi/numa/srat.c
> > +++ b/drivers/acpi/numa/srat.c
> > @@ -654,8 +654,11 @@ int __init acpi_numa_init(void)
> >  	}
> >  	last_real_pxm = fake_pxm;
> >  	fake_pxm++;
> > -	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS, acpi_parse_cfmws,
> > -			      &fake_pxm);
> > +
> > +	/* No need to expand numa nodes if CXL is disabled */
> > +	if (IS_ENABLED(CONFIG_CXL_ACPI))
> > +		acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS, acpi_parse_cfmws,
> > +				      &fake_pxm);
> >  
> >  	if (cnt < 0)
> >  		return cnt;
> > 
> > The call to acpi_table_parse_cedt() will get skipped and the code for
> > acpi_parse_cfmws() will get automatically compiled out of the file.
> 
> Yeah agreed this is simpler.  Thanks for the suggestion.
> 
> I would like to honor Gregory's suggestion about the comment since it
> provides more info if you agree?
> 
> (I also added "impacts certain NUMA userspace ABIs")
> 
> /*
>  * Some platforms report CEDT CFMWS for hotplug device support. These
>  * windows are unusable without CXL drivers, so don't reserve fake nodes
>  * if they're compiled out - it wastes memory on per-node structures and
>  * impacts certain NUMA userspace ABIs.
>  */

I feel like this is just too much scary detail for a tiny corner case
that maybe only effects a handful of kernel developers at hardware
companies with advanced server platforms.

    /* No need to expand numa nodes if CXL is disabled */

...is more than sufficient.

> > At the same time I doubt this patch provides end users much value in
> > practice as most distro kernels have CONFIG_CXL_ACPI, and the few end
> > users that have CXL are not going blink at the overhead to support the
> > full feature set.
> > 
> > Can you not just disable CXL support in the BIOS for your system and
> > avoid complicating this code path for a small win?
> 
> Sure I can (I need to figure out how, though).  But I think it's still
> useful for developers because some of them will tend to only enable Kconfig
> options that they need to save build time.
> 
> And sometimes it's not quite easy to turn off CXL in the BIOS since the
> machine would be mostly remote.
> 
> So I think this patch is still worthy to do?

A small 2 line patch to save a few megabytes of memory, sure. A
paragraph of context that almost nobody will ever care about on a go
forward basis? No, thank you.