Re: [PATCH] x86/numa: Make numa_fill_memblks() @end parameter exclusive

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

Alison Schofield wrote:
> On Tue, Jan 02, 2024 at 03:42:50PM -0800, Dan Williams wrote:
> > 
> > So I realize I asked for the minimal fix before a wider cleanup to
> > 'struct numa_memblk' to use 'struct range', but I want to see some of
> > that cleanup now. How about the below (only compile-tested) to at least
> > convert numa_fill_memblks(), since it is new, and then circle back
> > sometime later to move 'struct numa_memblk' to embed a 'struct range'
> > directly? I.e. save touching legacy code for later, but fix the bug in
> > the new code with some modernization. This also documents that
> > numa_add_memblk() expects an inclusive argument.
> > 
> > Note that this would be 3 patches, the minimal logic fix as you have it
> > without the comment changes since they become moot later, the
> > introduction of range_overlaps() with namespace conflict fixup with the
> > btrfs version, and using range_overlaps() to cleanup numa_fill_memblks()
> 
> Hi Dan,
> 
> Your idea to use struct range in struct numa_memblk comes as tremendous
> relief after staring at open coded usage of start/end in numa_memblk.
> 
> With that acknowledged, the minimal fix to the overlap issue could be
> either in x86 or acpi.

I don't see how it could be in acpi since there are currently
conflicting {in,ex}clusive usages of @end numa_fill_memblk(), right?

I.e.:
...
start < bi->end && end >= bi->start)
...
blk[count - 1]->end = max(blk[count - 1]->end, end);

> We're at x86 because we wanted to be 'like'
> numa_add_memblks(). A partial cleanup now makes numa_fill_memblks()
> diverge further.

Yes, but numa_add_memblks() parity is not necessarily a virtue, however
memblock (with an 'o') parity might be, see below:

>  And, to avoid the partial cleanup from being throw-away work, it
>  seems we'd want to define the full cleanup ahead of time.

Are you referring to throwing away this cleanup work for the eventual
memblock conversion, then yes, I agree, if this is just talking about
the numa_memblk with 'struct range' conversion, then I disagree.

> Also recall that we have a bigger hope of replacing this whole
> numa memblk usage with the "Memblock" (with an 'o') API.

2 wrinkles here...

1/ From a practical standpoint is anyone actively working on this? I
   would definitely stand down from the numa_memblk conversion in favor of
   memblock, but it has been years since this idea was floated, and I doubt
   it shows up anytime soon. However, "be more like memblock" is a good
   idea.

2/ memblock appears to be doing inclusive range math, which shoots holes
   in my position that numa_add_memblk() is an anti-pattern that merits
   conversion. I.e. "be more like memblock" contra-indicates a 'struct
   range' conversion.

> Maybe I need clarification on what you are suggesting wrt 3 patches?
> Are you saying you would not merge the minimal fix unless the partial
> clean up patches are included in the same set?

I think your memblock observation points to a third way. Lets uplevel
and steal memblock_addrs_overlap() and use it for this broken
comparison. That nods to an eventual full memblock conversion and keeps
the inclusive math status quo in memblock and numa_memblk related code.

> Also, is this something that has to be merged through x86 or might
> you be able to take it though cxl tree? Yes, that's me worrying 
> about the review and intake burden this puts on x86.

I can always merge things through cxl.git with an x86 Acked-by, or a
"holler if you don't want this to go through cxl.git" approach. Lets
keep maintainer busy-ness concerns separate from where the best long
term place for the changes to land.