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

[Alison Schofield <alison.schofield@intel.com>]

On Wed, Jan 03, 2024 at 12:18:17PM -0800, Dan Williams wrote:
> 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?
> 

You're 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.
> 

Thanks for calling my attention to the available memblock helper.  
I went ahead and used memblock_addrs_overlap() for the comparison
and verified that it fixes the case where an existing memblk is
extended rather than creating a new node and memblock. It actually
is not clear how that relates to what the user reported: numa init
failure due to nodes overlapping, so I'm holding off rev'ing this
patch until that user reported failure is understood.

> > 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.

Got it.