Gregory Price wrote:
> On Tue, Nov 12, 2024 at 01:41:55PM -0800, Dan Williams wrote:
> > Gregory Price wrote:
> > > Capacity is stranded when CFMWS regions are not aligned to block size.
> > > On x86, block size increases with capacity (2G blocks @ 64G capacity).
> > >
> > > Use CFMWS base/size to report memory block size alignment advice.
> > >
> > > Suggested-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > > Signed-off-by: Gregory Price <gourry@xxxxxxxxxx>
> > > Acked-by: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>
> > > Acked-by: David Hildenbrand <david@xxxxxxxxxx>
> > > ---
> > > drivers/acpi/numa/srat.c | 12 +++++++++++-
> > > 1 file changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
> > > index 44f91f2c6c5d..34b6993e7d6c 100644
> > > --- a/drivers/acpi/numa/srat.c
> > > +++ b/drivers/acpi/numa/srat.c
> > > @@ -14,6 +14,7 @@
> > > #include <linux/errno.h>
> > > #include <linux/acpi.h>
> > > #include <linux/memblock.h>
> > > +#include <linux/memory.h>
> > > #include <linux/numa.h>
> > > #include <linux/nodemask.h>
> > > #include <linux/topology.h>
> > > @@ -338,13 +339,22 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> > > {
> > > struct acpi_cedt_cfmws *cfmws;
> > > int *fake_pxm = arg;
> > > - u64 start, end;
> > > + u64 start, end, align;
> > > int node;
> > >
> > > cfmws = (struct acpi_cedt_cfmws *)header;
> > > start = cfmws->base_hpa;
> > > end = cfmws->base_hpa + cfmws->window_size;
> > >
> > > + /* Align memblock size to CFMW regions if possible */
> > > + align = 1UL << __ffs(start | end);
> > > + if (align >= SZ_256M) {
> > > + if (memory_block_advise_max_size(align) < 0)
> > > + pr_warn("CFMWS: memblock size advise failed\n");
> >
> > Oh, this made me go back to look at what happens if CFMWS has multiple
> > alignment suggestions. Should not memory_block_advise_max_size() be
> > considering the max advice?
> >
> > if (memory_block_advised_size) {
> > ...
> > } else {
> > memory_block_advised_size = max(memory_block_advised_size, size);
> > }
> >
> > For example, if region0 is an x4 region and region1 is an x1 region then
> > the memory block size should be 1GB, not 256M. I.e. CFMWS alignment
> > follows CXL hardware decoder alignment of "256M * InterleaveWays".
>
> Max size to minimize capacity loss to due alignment truncation.
>
> If CFMW-0 is aligned at 1GB and CFMW-1 is aligned at 256MB, if you select 1GB
> then some portion of CFMW-1 will be unmappable.
>
> so you want min(memory_block_advised_size, size) to ensure the hotplug memblock
> size aligns to the *smallest* CFMW (or any other source) alignment.
>
> Unless I'm misunderstanding your feedback here.
No, whoops, you didn't misundertand, I just misread
memory_block_advise_max_size(). Makes sense and current code looks good,
you can add:
Acked-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> I'm not clear on why the interleave data is relevant here - that just tells us
> how decoders line up with the memory region described in the CFMW. The window
> still gets chopped up into N memblocks of memory_block_advised_size.
Yes the window still gets chopped, but the alignment is meant to follow
256M * InterleaveWays. The algorithm as you have it will pick that
up. So, no concerns from me from the CXL side.
