* [PATCH 0/2] x86/numa: Fix NUMA node overlap & init failure
@ 2024-01-12 20:09 alison.schofield
2024-01-12 20:09 ` [PATCH 1/2] x86/numa: Fix the address overlap check in numa_fill_memblks() alison.schofield
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: alison.schofield @ 2024-01-12 20:09 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Dan Williams,
Mike Rapoport
Cc: Alison Schofield, x86, linux-cxl
From: Alison Schofield <alison.schofield@intel.com>
A previously posted single patch [1] is obsoleted by this set. The
feedback from that review is applied and noted in Patch 1.
While trying to attribute a CXL user report to the bad selection of
overlapping memblks, as fixed in Patch 1, I found that two issues,
in sequence, lead to NUMA Node overlap and NUMA init failure.
An overlapping NUMA node occurs when a non-overlapping memblk is
selected to fill (Patch 1), and then a bad sort (Patch 2) puts the
memblk with the greater address ahead of the lesser address memblk
in the fill list.
It looked like this:
Existing memblks:
node 6 [mem 0xb90000000-0xc90000000]
node 7 [mem 0xc90000000-0xd90000000]
Call to numa_fill_memblks(b90000000,c90000000)
Error (Patch 1): collects 2 blks
blk[0] node 6 [0xb90000000-0xc90000000]
blk[1] node 7 [0xc90000000-0xd90000000]
Error (Patch 2): bad sort of the 2 blks
blk[0] node 7 [0xc90000000-0xd90000000]
blk[1] node 6 [0xb90000000-0xc90000000]
Seals the deal with a bad fill:
blk[0] node 7 [0xb90000000-0xd90000000]
Boom: numa_clean_meminfo() discovers the overlap in Nodes 6 & 7
and NUMA init fails.
Since the scenario above is not solely attributed to either patch,
the story is explicity shared here.
[1] https://lore.kernel.org/linux-cxl/20240102213206.1493733-1-alison.schofield@intel.com/
Alison Schofield (2):
x86/numa: Fix the address overlap check in numa_fill_memblks()
x86/numa: Fix the sort compare func used in numa_fill_memblks()
arch/x86/mm/numa.c | 21 ++++++++-------------
include/linux/memblock.h | 2 ++
mm/memblock.c | 5 +++--
3 files changed, 13 insertions(+), 15 deletions(-)
base-commit: bd009225e8cbb6e18ad3389328fa640e4887dd9e
--
2.37.3
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/2] x86/numa: Fix the address overlap check in numa_fill_memblks() 2024-01-12 20:09 [PATCH 0/2] x86/numa: Fix NUMA node overlap & init failure alison.schofield @ 2024-01-12 20:09 ` alison.schofield 2024-01-23 8:13 ` Mike Rapoport 2024-01-12 20:09 ` [PATCH 2/2] x86/numa: Fix the sort compare func used " alison.schofield 2024-01-12 22:27 ` [PATCH 0/2] x86/numa: Fix NUMA node overlap & init failure Dan Williams 2 siblings, 1 reply; 10+ messages in thread From: alison.schofield @ 2024-01-12 20:09 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Dan Williams, Mike Rapoport Cc: Alison Schofield, x86, linux-cxl From: Alison Schofield <alison.schofield@intel.com> numa_fill_memblks() fills in the gaps in numa_meminfo memblks over a physical address range. To do so, it first creates a list of existing memblks that overlap that address range. The issue is that it is off by one when comparing to the end of the address range, so memblks that do not overlap are selected. The impact of selecting a memblk that does not actually overlap is that an existing memblk may be filled when the expected action is to do nothing and return NUMA_NO_MEMBLK to the caller. The caller can then add a new NUMA node and memblk. Replace the broken open-coded search for address overlap with the memblock helper memblock_addrs_overlap(). Update the kernel doc and in code comments. Fixes: 8f012db27c95 ("x86/numa: Introduce numa_fill_memblks()") Suggested by: "Huang, Ying" <ying.huang@intel.com> Signed-off-by: Alison Schofield <alison.schofield@intel.com> --- Changes since single patch: - Use memblock_addrs_overlap() for address comparison (Dan) - Update kernel doc and in code comments - New commit message - Update commit log - Link to single patch: https://lore.kernel.org/linux-cxl/20240102213206.1493733-1-alison.schofield@intel.com/ arch/x86/mm/numa.c | 19 +++++++------------ include/linux/memblock.h | 2 ++ mm/memblock.c | 5 +++-- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c index adc497b93f03..8ada9bbfad58 100644 --- a/arch/x86/mm/numa.c +++ b/arch/x86/mm/numa.c @@ -944,14 +944,12 @@ static struct numa_memblk *numa_memblk_list[NR_NODE_MEMBLKS] __initdata; * @start: address to begin fill * @end: address to end fill * - * Find and extend numa_meminfo memblks to cover the @start-@end - * physical address range, such that the first memblk includes - * @start, the last memblk includes @end, and any gaps in between - * are filled. + * Find and extend numa_meminfo memblks to cover the physical + * address range @start-@end * * RETURNS: * 0 : Success - * NUMA_NO_MEMBLK : No memblk exists in @start-@end range + * NUMA_NO_MEMBLK : No memblks exist in address range @start-@end */ int __init numa_fill_memblks(u64 start, u64 end) @@ -963,17 +961,14 @@ int __init numa_fill_memblks(u64 start, u64 end) /* * Create a list of pointers to numa_meminfo memblks that - * overlap start, end. Exclude (start == bi->end) since - * end addresses in both a CFMWS range and a memblk range - * are exclusive. - * - * This list of pointers is used to make in-place changes - * that fill out the numa_meminfo memblks. + * overlap start, end. The list is used to make in-place + * changes that fill out the numa_meminfo memblks. */ for (int i = 0; i < mi->nr_blks; i++) { struct numa_memblk *bi = &mi->blk[i]; - if (start < bi->end && end >= bi->start) { + if (memblock_addrs_overlap(start, end - start, bi->start, + bi->end - bi->start)) { blk[count] = &mi->blk[i]; count++; } diff --git a/include/linux/memblock.h b/include/linux/memblock.h index b695f9e946da..e2082240586d 100644 --- a/include/linux/memblock.h +++ b/include/linux/memblock.h @@ -121,6 +121,8 @@ int memblock_reserve(phys_addr_t base, phys_addr_t size); int memblock_physmem_add(phys_addr_t base, phys_addr_t size); #endif void memblock_trim_memory(phys_addr_t align); +unsigned long memblock_addrs_overlap(phys_addr_t base1, phys_addr_t size1, + phys_addr_t base2, phys_addr_t size2); bool memblock_overlaps_region(struct memblock_type *type, phys_addr_t base, phys_addr_t size); bool memblock_validate_numa_coverage(unsigned long threshold_bytes); diff --git a/mm/memblock.c b/mm/memblock.c index 8c194d8afeec..d0cadeeecfe8 100644 --- a/mm/memblock.c +++ b/mm/memblock.c @@ -180,8 +180,9 @@ static inline phys_addr_t memblock_cap_size(phys_addr_t base, phys_addr_t *size) /* * Address comparison utilities */ -static unsigned long __init_memblock memblock_addrs_overlap(phys_addr_t base1, phys_addr_t size1, - phys_addr_t base2, phys_addr_t size2) +unsigned long __init_memblock +memblock_addrs_overlap(phys_addr_t base1, phys_addr_t size1, phys_addr_t base2, + phys_addr_t size2) { return ((base1 < (base2 + size2)) && (base2 < (base1 + size1))); } -- 2.37.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] x86/numa: Fix the address overlap check in numa_fill_memblks() 2024-01-12 20:09 ` [PATCH 1/2] x86/numa: Fix the address overlap check in numa_fill_memblks() alison.schofield @ 2024-01-23 8:13 ` Mike Rapoport 0 siblings, 0 replies; 10+ messages in thread From: Mike Rapoport @ 2024-01-23 8:13 UTC (permalink / raw) To: alison.schofield Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Dan Williams, x86, linux-cxl On Fri, Jan 12, 2024 at 12:09:50PM -0800, alison.schofield@intel.com wrote: > From: Alison Schofield <alison.schofield@intel.com> > > numa_fill_memblks() fills in the gaps in numa_meminfo memblks over a > physical address range. To do so, it first creates a list of existing > memblks that overlap that address range. The issue is that it is off > by one when comparing to the end of the address range, so memblks > that do not overlap are selected. > > The impact of selecting a memblk that does not actually overlap is > that an existing memblk may be filled when the expected action is to > do nothing and return NUMA_NO_MEMBLK to the caller. The caller can > then add a new NUMA node and memblk. > > Replace the broken open-coded search for address overlap with the > memblock helper memblock_addrs_overlap(). Update the kernel doc > and in code comments. > > Fixes: 8f012db27c95 ("x86/numa: Introduce numa_fill_memblks()") > Suggested by: "Huang, Ying" <ying.huang@intel.com> > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > --- > > Changes since single patch: > - Use memblock_addrs_overlap() for address comparison (Dan) > - Update kernel doc and in code comments > - New commit message > - Update commit log > - Link to single patch: > https://lore.kernel.org/linux-cxl/20240102213206.1493733-1-alison.schofield@intel.com/ > > arch/x86/mm/numa.c | 19 +++++++------------ > include/linux/memblock.h | 2 ++ > mm/memblock.c | 5 +++-- For memblock changes Acked-by: Mike Rapoport (IBM) <rppt@kernel.org> > 3 files changed, 12 insertions(+), 14 deletions(-) > > diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c > index adc497b93f03..8ada9bbfad58 100644 > --- a/arch/x86/mm/numa.c > +++ b/arch/x86/mm/numa.c > @@ -944,14 +944,12 @@ static struct numa_memblk *numa_memblk_list[NR_NODE_MEMBLKS] __initdata; > * @start: address to begin fill > * @end: address to end fill > * > - * Find and extend numa_meminfo memblks to cover the @start-@end > - * physical address range, such that the first memblk includes > - * @start, the last memblk includes @end, and any gaps in between > - * are filled. > + * Find and extend numa_meminfo memblks to cover the physical > + * address range @start-@end > * > * RETURNS: > * 0 : Success > - * NUMA_NO_MEMBLK : No memblk exists in @start-@end range > + * NUMA_NO_MEMBLK : No memblks exist in address range @start-@end > */ > > int __init numa_fill_memblks(u64 start, u64 end) > @@ -963,17 +961,14 @@ int __init numa_fill_memblks(u64 start, u64 end) > > /* > * Create a list of pointers to numa_meminfo memblks that > - * overlap start, end. Exclude (start == bi->end) since > - * end addresses in both a CFMWS range and a memblk range > - * are exclusive. > - * > - * This list of pointers is used to make in-place changes > - * that fill out the numa_meminfo memblks. > + * overlap start, end. The list is used to make in-place > + * changes that fill out the numa_meminfo memblks. > */ > for (int i = 0; i < mi->nr_blks; i++) { > struct numa_memblk *bi = &mi->blk[i]; > > - if (start < bi->end && end >= bi->start) { > + if (memblock_addrs_overlap(start, end - start, bi->start, > + bi->end - bi->start)) { > blk[count] = &mi->blk[i]; > count++; > } > diff --git a/include/linux/memblock.h b/include/linux/memblock.h > index b695f9e946da..e2082240586d 100644 > --- a/include/linux/memblock.h > +++ b/include/linux/memblock.h > @@ -121,6 +121,8 @@ int memblock_reserve(phys_addr_t base, phys_addr_t size); > int memblock_physmem_add(phys_addr_t base, phys_addr_t size); > #endif > void memblock_trim_memory(phys_addr_t align); > +unsigned long memblock_addrs_overlap(phys_addr_t base1, phys_addr_t size1, > + phys_addr_t base2, phys_addr_t size2); > bool memblock_overlaps_region(struct memblock_type *type, > phys_addr_t base, phys_addr_t size); > bool memblock_validate_numa_coverage(unsigned long threshold_bytes); > diff --git a/mm/memblock.c b/mm/memblock.c > index 8c194d8afeec..d0cadeeecfe8 100644 > --- a/mm/memblock.c > +++ b/mm/memblock.c > @@ -180,8 +180,9 @@ static inline phys_addr_t memblock_cap_size(phys_addr_t base, phys_addr_t *size) > /* > * Address comparison utilities > */ > -static unsigned long __init_memblock memblock_addrs_overlap(phys_addr_t base1, phys_addr_t size1, > - phys_addr_t base2, phys_addr_t size2) > +unsigned long __init_memblock > +memblock_addrs_overlap(phys_addr_t base1, phys_addr_t size1, phys_addr_t base2, > + phys_addr_t size2) > { > return ((base1 < (base2 + size2)) && (base2 < (base1 + size1))); > } > -- > 2.37.3 > -- Sincerely yours, Mike. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] x86/numa: Fix the sort compare func used in numa_fill_memblks() 2024-01-12 20:09 [PATCH 0/2] x86/numa: Fix NUMA node overlap & init failure alison.schofield 2024-01-12 20:09 ` [PATCH 1/2] x86/numa: Fix the address overlap check in numa_fill_memblks() alison.schofield @ 2024-01-12 20:09 ` alison.schofield 2024-01-12 22:20 ` Dan Williams 2024-01-12 22:27 ` [PATCH 0/2] x86/numa: Fix NUMA node overlap & init failure Dan Williams 2 siblings, 1 reply; 10+ messages in thread From: alison.schofield @ 2024-01-12 20:09 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Dan Williams, Mike Rapoport Cc: Alison Schofield, x86, linux-cxl From: Alison Schofield <alison.schofield@intel.com> The compare function used to sort memblks into starting address order fails when the result of its u64 address subtraction gets truncated to an int upon return. The impact of the bad sort is that memblks will be filled out incorrectly. Depending on the set of memblks, a user may see no errors at all but still have a bad fill, or see messages reporting a node overlap that leads to numa init failure: [] node 0 [mem: ] overlaps with node 1 [mem: ] [] No NUMA configuration found Replace with a comparison that can only result in: 1, 0, -1. Fixes: 8f012db27c95 ("x86/numa: Introduce numa_fill_memblks()") Signed-off-by: Alison Schofield <alison.schofield@intel.com> --- arch/x86/mm/numa.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c index 8ada9bbfad58..65e9a6e391c0 100644 --- a/arch/x86/mm/numa.c +++ b/arch/x86/mm/numa.c @@ -934,7 +934,7 @@ static int __init cmp_memblk(const void *a, const void *b) const struct numa_memblk *ma = *(const struct numa_memblk **)a; const struct numa_memblk *mb = *(const struct numa_memblk **)b; - return ma->start - mb->start; + return (ma->start > mb->start) - (ma->start < mb->start); } static struct numa_memblk *numa_memblk_list[NR_NODE_MEMBLKS] __initdata; -- 2.37.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH 2/2] x86/numa: Fix the sort compare func used in numa_fill_memblks() 2024-01-12 20:09 ` [PATCH 2/2] x86/numa: Fix the sort compare func used " alison.schofield @ 2024-01-12 22:20 ` Dan Williams 2024-01-13 0:35 ` Alison Schofield 0 siblings, 1 reply; 10+ messages in thread From: Dan Williams @ 2024-01-12 22:20 UTC (permalink / raw) To: alison.schofield, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Dan Williams, Mike Rapoport Cc: Alison Schofield, x86, linux-cxl alison.schofield@ wrote: > From: Alison Schofield <alison.schofield@intel.com> > > The compare function used to sort memblks into starting address > order fails when the result of its u64 address subtraction gets > truncated to an int upon return. > > The impact of the bad sort is that memblks will be filled out > incorrectly. Depending on the set of memblks, a user may see no > errors at all but still have a bad fill, or see messages reporting > a node overlap that leads to numa init failure: > > [] node 0 [mem: ] overlaps with node 1 [mem: ] > [] No NUMA configuration found > > Replace with a comparison that can only result in: 1, 0, -1. Good eye! > Fixes: 8f012db27c95 ("x86/numa: Introduce numa_fill_memblks()") > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > --- > arch/x86/mm/numa.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c > index 8ada9bbfad58..65e9a6e391c0 100644 > --- a/arch/x86/mm/numa.c > +++ b/arch/x86/mm/numa.c > @@ -934,7 +934,7 @@ static int __init cmp_memblk(const void *a, const void *b) > const struct numa_memblk *ma = *(const struct numa_memblk **)a; > const struct numa_memblk *mb = *(const struct numa_memblk **)b; > > - return ma->start - mb->start; > + return (ma->start > mb->start) - (ma->start < mb->start); Maybe just do the less clever but obviously correct thing like other unsigned compares like ktime_compare(): if (ma->start > mb->start) return 1; if (ma->start < mb->start) return -1; return 0; ...but otherwise this looks good to me. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] x86/numa: Fix the sort compare func used in numa_fill_memblks() 2024-01-12 22:20 ` Dan Williams @ 2024-01-13 0:35 ` Alison Schofield 2024-01-13 1:54 ` Dan Williams 0 siblings, 1 reply; 10+ messages in thread From: Alison Schofield @ 2024-01-13 0:35 UTC (permalink / raw) To: Dan Williams Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Mike Rapoport, x86, linux-cxl On Fri, Jan 12, 2024 at 02:20:04PM -0800, Dan Williams wrote: > alison.schofield@ wrote: > > From: Alison Schofield <alison.schofield@intel.com> > > > > The compare function used to sort memblks into starting address > > order fails when the result of its u64 address subtraction gets > > truncated to an int upon return. > > > > The impact of the bad sort is that memblks will be filled out > > incorrectly. Depending on the set of memblks, a user may see no > > errors at all but still have a bad fill, or see messages reporting > > a node overlap that leads to numa init failure: > > > > [] node 0 [mem: ] overlaps with node 1 [mem: ] > > [] No NUMA configuration found > > > > Replace with a comparison that can only result in: 1, 0, -1. > > Good eye! > > > Fixes: 8f012db27c95 ("x86/numa: Introduce numa_fill_memblks()") > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > > --- > > arch/x86/mm/numa.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c > > index 8ada9bbfad58..65e9a6e391c0 100644 > > --- a/arch/x86/mm/numa.c > > +++ b/arch/x86/mm/numa.c > > @@ -934,7 +934,7 @@ static int __init cmp_memblk(const void *a, const void *b) > > const struct numa_memblk *ma = *(const struct numa_memblk **)a; > > const struct numa_memblk *mb = *(const struct numa_memblk **)b; > > > > - return ma->start - mb->start; > > + return (ma->start > mb->start) - (ma->start < mb->start); > > Maybe just do the less clever but obviously correct thing like > other unsigned compares like ktime_compare(): > > if (ma->start > mb->start) > return 1; > if (ma->start < mb->start) > return -1; > return 0; > > ...but otherwise this looks good to me. During the upstream march of the original set, I oversimplified and broke. For this fix, I surveyed the sort compare funcs and lifted this simple method from commit: 4ac19ead0dfb ("kvm: x86/pmu: Fix the compare function used by the pmu event filter"). Let me see what else I get to trigger a v2. Thanks for the review! ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] x86/numa: Fix the sort compare func used in numa_fill_memblks() 2024-01-13 0:35 ` Alison Schofield @ 2024-01-13 1:54 ` Dan Williams 0 siblings, 0 replies; 10+ messages in thread From: Dan Williams @ 2024-01-13 1:54 UTC (permalink / raw) To: Alison Schofield, Dan Williams Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Mike Rapoport, x86, linux-cxl Alison Schofield wrote: > On Fri, Jan 12, 2024 at 02:20:04PM -0800, Dan Williams wrote: > > alison.schofield@ wrote: > > > From: Alison Schofield <alison.schofield@intel.com> > > > > > > The compare function used to sort memblks into starting address > > > order fails when the result of its u64 address subtraction gets > > > truncated to an int upon return. > > > > > > The impact of the bad sort is that memblks will be filled out > > > incorrectly. Depending on the set of memblks, a user may see no > > > errors at all but still have a bad fill, or see messages reporting > > > a node overlap that leads to numa init failure: > > > > > > [] node 0 [mem: ] overlaps with node 1 [mem: ] > > > [] No NUMA configuration found > > > > > > Replace with a comparison that can only result in: 1, 0, -1. > > > > Good eye! > > > > > Fixes: 8f012db27c95 ("x86/numa: Introduce numa_fill_memblks()") > > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > > > --- > > > arch/x86/mm/numa.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c > > > index 8ada9bbfad58..65e9a6e391c0 100644 > > > --- a/arch/x86/mm/numa.c > > > +++ b/arch/x86/mm/numa.c > > > @@ -934,7 +934,7 @@ static int __init cmp_memblk(const void *a, const void *b) > > > const struct numa_memblk *ma = *(const struct numa_memblk **)a; > > > const struct numa_memblk *mb = *(const struct numa_memblk **)b; > > > > > > - return ma->start - mb->start; > > > + return (ma->start > mb->start) - (ma->start < mb->start); > > > > Maybe just do the less clever but obviously correct thing like > > other unsigned compares like ktime_compare(): > > > > if (ma->start > mb->start) > > return 1; > > if (ma->start < mb->start) > > return -1; > > return 0; > > > > ...but otherwise this looks good to me. > > During the upstream march of the original set, I oversimplified and > broke. For this fix, I surveyed the sort compare funcs and lifted this > simple method from commit: 4ac19ead0dfb ("kvm: x86/pmu: Fix the compare > function used by the pmu event filter"). > > Let me see what else I get to trigger a v2. If nothing else comes in, then don't spin just for this. ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH 0/2] x86/numa: Fix NUMA node overlap & init failure 2024-01-12 20:09 [PATCH 0/2] x86/numa: Fix NUMA node overlap & init failure alison.schofield 2024-01-12 20:09 ` [PATCH 1/2] x86/numa: Fix the address overlap check in numa_fill_memblks() alison.schofield 2024-01-12 20:09 ` [PATCH 2/2] x86/numa: Fix the sort compare func used " alison.schofield @ 2024-01-12 22:27 ` Dan Williams 2024-01-25 21:49 ` Dan Williams 2 siblings, 1 reply; 10+ messages in thread From: Dan Williams @ 2024-01-12 22:27 UTC (permalink / raw) To: alison.schofield, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Dan Williams, Mike Rapoport Cc: Alison Schofield, x86, linux-cxl alison.schofield@ wrote: > From: Alison Schofield <alison.schofield@intel.com> > > A previously posted single patch [1] is obsoleted by this set. The > feedback from that review is applied and noted in Patch 1. > > While trying to attribute a CXL user report to the bad selection of > overlapping memblks, as fixed in Patch 1, I found that two issues, > in sequence, lead to NUMA Node overlap and NUMA init failure. > > An overlapping NUMA node occurs when a non-overlapping memblk is > selected to fill (Patch 1), and then a bad sort (Patch 2) puts the > memblk with the greater address ahead of the lesser address memblk > in the fill list. > > It looked like this: > > Existing memblks: > node 6 [mem 0xb90000000-0xc90000000] > node 7 [mem 0xc90000000-0xd90000000] > > Call to numa_fill_memblks(b90000000,c90000000) > > Error (Patch 1): collects 2 blks > blk[0] node 6 [0xb90000000-0xc90000000] > blk[1] node 7 [0xc90000000-0xd90000000] > > Error (Patch 2): bad sort of the 2 blks > blk[0] node 7 [0xc90000000-0xd90000000] > blk[1] node 6 [0xb90000000-0xc90000000] > > Seals the deal with a bad fill: > blk[0] node 7 [0xb90000000-0xd90000000] > > Boom: numa_clean_meminfo() discovers the overlap in Nodes 6 & 7 > and NUMA init fails. > > Since the scenario above is not solely attributed to either patch, > the story is explicity shared here. > > [1] https://lore.kernel.org/linux-cxl/20240102213206.1493733-1-alison.schofield@intel.com/ > > Alison Schofield (2): > x86/numa: Fix the address overlap check in numa_fill_memblks() > x86/numa: Fix the sort compare func used in numa_fill_memblks() > > arch/x86/mm/numa.c | 21 ++++++++------------- > include/linux/memblock.h | 2 ++ > mm/memblock.c | 5 +++-- > 3 files changed, 13 insertions(+), 15 deletions(-) For both fixes: Reviewed-by: Dan Williams <dan.j.williams@intel.com> ...if they get picked up into the x86 tree. Otherwise I'll circle back and take them through cxl.git with an x86 ack since this is all cxl-related fixups to numa_fill_memblks(). ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH 0/2] x86/numa: Fix NUMA node overlap & init failure 2024-01-12 22:27 ` [PATCH 0/2] x86/numa: Fix NUMA node overlap & init failure Dan Williams @ 2024-01-25 21:49 ` Dan Williams 2024-01-29 23:00 ` Dave Hansen 0 siblings, 1 reply; 10+ messages in thread From: Dan Williams @ 2024-01-25 21:49 UTC (permalink / raw) To: Dave Hansen, Peter Zijlstra Cc: Alison Schofield, x86, linux-cxl, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Andy Lutomirski, Mike Rapoport Dan Williams wrote: > alison.schofield@ wrote: > > From: Alison Schofield <alison.schofield@intel.com> > > > > A previously posted single patch [1] is obsoleted by this set. The > > feedback from that review is applied and noted in Patch 1. > > > > While trying to attribute a CXL user report to the bad selection of > > overlapping memblks, as fixed in Patch 1, I found that two issues, > > in sequence, lead to NUMA Node overlap and NUMA init failure. > > > > An overlapping NUMA node occurs when a non-overlapping memblk is > > selected to fill (Patch 1), and then a bad sort (Patch 2) puts the > > memblk with the greater address ahead of the lesser address memblk > > in the fill list. > > > > It looked like this: > > > > Existing memblks: > > node 6 [mem 0xb90000000-0xc90000000] > > node 7 [mem 0xc90000000-0xd90000000] > > > > Call to numa_fill_memblks(b90000000,c90000000) > > > > Error (Patch 1): collects 2 blks > > blk[0] node 6 [0xb90000000-0xc90000000] > > blk[1] node 7 [0xc90000000-0xd90000000] > > > > Error (Patch 2): bad sort of the 2 blks > > blk[0] node 7 [0xc90000000-0xd90000000] > > blk[1] node 6 [0xb90000000-0xc90000000] > > > > Seals the deal with a bad fill: > > blk[0] node 7 [0xb90000000-0xd90000000] > > > > Boom: numa_clean_meminfo() discovers the overlap in Nodes 6 & 7 > > and NUMA init fails. > > > > Since the scenario above is not solely attributed to either patch, > > the story is explicity shared here. > > > > [1] https://lore.kernel.org/linux-cxl/20240102213206.1493733-1-alison.schofield@intel.com/ > > > > Alison Schofield (2): > > x86/numa: Fix the address overlap check in numa_fill_memblks() > > x86/numa: Fix the sort compare func used in numa_fill_memblks() > > > > arch/x86/mm/numa.c | 21 ++++++++------------- > > include/linux/memblock.h | 2 ++ > > mm/memblock.c | 5 +++-- > > 3 files changed, 13 insertions(+), 15 deletions(-) > > For both fixes: > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > ...if they get picked up into the x86 tree. > > Otherwise I'll circle back and take them through cxl.git with an x86 ack > since this is all cxl-related fixups to numa_fill_memblks(). Circling back to check on these now that Mike has acked the memblock usage. Dave or Peter, please pull them into tip/x86/mm, or I can circle back and grab them next week. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] x86/numa: Fix NUMA node overlap & init failure 2024-01-25 21:49 ` Dan Williams @ 2024-01-29 23:00 ` Dave Hansen 0 siblings, 0 replies; 10+ messages in thread From: Dave Hansen @ 2024-01-29 23:00 UTC (permalink / raw) To: Dan Williams, Dave Hansen, Peter Zijlstra Cc: Alison Schofield, x86, linux-cxl, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Andy Lutomirski, Mike Rapoport On 1/25/24 13:49, Dan Williams wrote: > Circling back to check on these now that Mike has acked the memblock usage. > Dave or Peter, please pull them into tip/x86/mm, or I can circle back and grab > them next week. Hi Dan, Feel free to carry these: Acked-by: Dave Hansen <dave.hansen@linux.intel.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-01-29 23:00 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-12 20:09 [PATCH 0/2] x86/numa: Fix NUMA node overlap & init failure alison.schofield 2024-01-12 20:09 ` [PATCH 1/2] x86/numa: Fix the address overlap check in numa_fill_memblks() alison.schofield 2024-01-23 8:13 ` Mike Rapoport 2024-01-12 20:09 ` [PATCH 2/2] x86/numa: Fix the sort compare func used " alison.schofield 2024-01-12 22:20 ` Dan Williams 2024-01-13 0:35 ` Alison Schofield 2024-01-13 1:54 ` Dan Williams 2024-01-12 22:27 ` [PATCH 0/2] x86/numa: Fix NUMA node overlap & init failure Dan Williams 2024-01-25 21:49 ` Dan Williams 2024-01-29 23:00 ` Dave Hansen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox