| Message ID | 20220523062525.2504290-8-wei.chen@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Device tree based NUMA support for Arm - Part#1 | expand |
Hi, It seems that this patch is the only one that is not properly acked and reviewed in this series. So this is a gentle ping asking if everyone is happy with this patch or are there any more actions that should be taken from the author. Thanks! Kind regards, Henry > -----Original Message----- > Subject: [PATCH v4 7/8] xen/x86: add detection of memory interleaves for > different nodes > > One NUMA node may contain several memory blocks. In current Xen > code, Xen will maintain a node memory range for each node to cover > all its memory blocks. But here comes the problem, in the gap of > one node's two memory blocks, if there are some memory blocks don't > belong to this node (remote memory blocks). This node's memory range > will be expanded to cover these remote memory blocks. > > One node's memory range contains other nodes' memory, this is > obviously not very reasonable. This means current NUMA code only > can support node has no interleaved memory blocks. However, on a > physical machine, the addresses of multiple nodes can be interleaved. > > So in this patch, we add code to detect memory interleaves of > different nodes. NUMA initialization will be failed and error > messages will be printed when Xen detect such hardware configuration. > > Change-Id: Ia7ff9a9128ecbe3eb4dddd1307ae8fbe65575ccf > Issue-Id: SCM-2240 > Signed-off-by: Wei Chen <wei.chen@arm.com> > Tested-by: Jiamei Xie <jiamei.xie@arm.com> > --- > v3 -> v4: > 1. Drop "ERR" prefix for enumeration, and remove init value. > 2. Use "switch case" for enumeration, and add "default:" > 3. Use "PXM" in log messages. > 4. Use unsigned int for node memory block id. > 5. Fix some code-style comments. > 6. Use "nd->end" in node range expansion check. > v2 -> v3: > 1. Merge the check code from a separate function to > conflicting_memblks. This will reduce the loop > times of node memory blocks. > 2. Use an enumeration to indicate conflict check status. > 3. Use a pointer to get conflict memory block id. > v1 -> v2: > 1. Update the description to say we're after is no memory > interleaves of different nodes. > 2. Only update node range when it passes the interleave check. > 3. Don't use full upper-case for "node". > --- > xen/arch/x86/srat.c | 132 +++++++++++++++++++++++++++++++++----------- > 1 file changed, 101 insertions(+), 31 deletions(-) > > diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c > index 8ffe43bdfe..a831df7648 100644 > --- a/xen/arch/x86/srat.c > +++ b/xen/arch/x86/srat.c > @@ -42,6 +42,12 @@ static struct node > node_memblk_range[NR_NODE_MEMBLKS]; > static nodeid_t memblk_nodeid[NR_NODE_MEMBLKS]; > static __initdata DECLARE_BITMAP(memblk_hotplug, > NR_NODE_MEMBLKS); > > +enum conflicts { > + NO_CONFLICT, > + OVERLAP, > + INTERLEAVE, > +}; > + > static inline bool node_found(unsigned idx, unsigned pxm) > { > return ((pxm2node[idx].pxm == pxm) && > @@ -119,20 +125,45 @@ int valid_numa_range(paddr_t start, paddr_t end, > nodeid_t node) > return 0; > } > > -static __init int conflicting_memblks(paddr_t start, paddr_t end) > +static > +enum conflicts __init conflicting_memblks(nodeid_t nid, paddr_t start, > + paddr_t end, paddr_t nd_start, > + paddr_t nd_end, unsigned int > *mblkid) > { > - int i; > + unsigned int i; > > + /* > + * Scan all recorded nodes' memory blocks to check conflicts: > + * Overlap or interleave. > + */ > for (i = 0; i < num_node_memblks; i++) { > struct node *nd = &node_memblk_range[i]; > + > + *mblkid = i; > + > + /* Skip 0 bytes node memory block. */ > if (nd->start == nd->end) > continue; > + /* > + * Use memblk range to check memblk overlaps, include the > + * self-overlap case. > + */ > if (nd->end > start && nd->start < end) > - return i; > + return OVERLAP; > if (nd->end == end && nd->start == start) > - return i; > + return OVERLAP; > + /* > + * Use node memory range to check whether new range > contains > + * memory from other nodes - interleave check. We just > need > + * to check full contains situation. Because overlaps have > + * been checked above. > + */ > + if (nid != memblk_nodeid[i] && > + (nd_start < nd->start && nd->end < nd_end)) > + return INTERLEAVE; > } > - return -1; > + > + return NO_CONFLICT; > } > > static __init void cutoff_node(int i, paddr_t start, paddr_t end) > @@ -275,10 +306,13 @@ acpi_numa_processor_affinity_init(const struct > acpi_srat_cpu_affinity *pa) > void __init > acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma) > { > + enum conflicts status; > + struct node *nd; > + paddr_t nd_start, nd_end; > paddr_t start, end; > unsigned pxm; > nodeid_t node; > - int i; > + unsigned int i; > > if (srat_disabled()) > return; > @@ -310,42 +344,78 @@ acpi_numa_memory_affinity_init(const struct > acpi_srat_mem_affinity *ma) > bad_srat(); > return; > } > + > + /* > + * For the node that already has some memory blocks, we will > + * expand the node memory range temporarily to check memory > + * interleaves with other nodes. We will not use this node > + * temp memory range to check overlaps, because it will mask > + * the overlaps in same node. > + * > + * Node with 0 bytes memory doesn't need this expandsion. > + */ > + nd_start = start; > + nd_end = end; > + nd = &nodes[node]; > + if (nd->start != nd->end) { > + if (nd_start > nd->start) > + nd_start = nd->start; > + > + if (nd_end < nd->end) > + nd_end = nd->end; > + } > + > /* It is fine to add this area to the nodes data it will be used later*/ > - i = conflicting_memblks(start, end); > - if (i < 0) > - /* everything fine */; > - else if (memblk_nodeid[i] == node) { > - bool mismatch = !(ma->flags & > ACPI_SRAT_MEM_HOT_PLUGGABLE) != > - !test_bit(i, memblk_hotplug); > - > - printk("%sSRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") > overlaps with itself (%"PRIpaddr"-%"PRIpaddr")\n", > - mismatch ? KERN_ERR : KERN_WARNING, pxm, start, > end, > - node_memblk_range[i].start, > node_memblk_range[i].end); > - if (mismatch) { > + status = conflicting_memblks(node, start, end, nd_start, nd_end, > &i); > + switch(status) > + { > + case OVERLAP: > + { > + if (memblk_nodeid[i] == node) { > + bool mismatch = !(ma->flags & > + > ACPI_SRAT_MEM_HOT_PLUGGABLE) != > + !test_bit(i, memblk_hotplug); > + > + printk("%sSRAT: PXM %u (%"PRIpaddr"- > %"PRIpaddr") overlaps with itself (%"PRIpaddr"-%"PRIpaddr")\n", > + mismatch ? KERN_ERR : KERN_WARNING, pxm, > start, > + end, node_memblk_range[i].start, > + node_memblk_range[i].end); > + if (mismatch) { > + bad_srat(); > + return; > + } > + break; > + } else { > + printk(KERN_ERR > + "SRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") > overlaps with PXM %u (%"PRIpaddr"-%"PRIpaddr")\n", > + pxm, start, end, > node_to_pxm(memblk_nodeid[i]), > + node_memblk_range[i].start, > + node_memblk_range[i].end); > bad_srat(); > return; > } > - } else { > + } > + > + case INTERLEAVE: > + { > printk(KERN_ERR > - "SRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps > with PXM %u (%"PRIpaddr"-%"PRIpaddr")\n", > - pxm, start, end, node_to_pxm(memblk_nodeid[i]), > + "SRAT： PXM %u: (%"PRIpaddr"-%"PRIpaddr") > interleaves with PXM %u memblk (%"PRIpaddr"-%"PRIpaddr")\n", > + node, nd_start, nd_end, > node_to_pxm(memblk_nodeid[i]), > node_memblk_range[i].start, > node_memblk_range[i].end); > bad_srat(); > return; > } > - if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) { > - struct node *nd = &nodes[node]; > > - if (!node_test_and_set(node, memory_nodes_parsed)) { > - nd->start = start; > - nd->end = end; > - } else { > - if (start < nd->start) > - nd->start = start; > - if (nd->end < end) > - nd->end = end; > - } > + default: > + break; > + } > + > + if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) { > + node_set(node, memory_nodes_parsed); > + nd->start = nd_start; > + nd->end = nd_end; > } > + > printk(KERN_INFO "SRAT: Node %u PXM %u %"PRIpaddr"- > %"PRIpaddr"%s\n", > node, pxm, start, end, > ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE ? " (hotplug)" : > ""); > -- > 2.25.1 >
On 23.05.2022 08:25, Wei Chen wrote: > @@ -119,20 +125,45 @@ int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node) > return 0; > } > > -static __init int conflicting_memblks(paddr_t start, paddr_t end) > +static > +enum conflicts __init conflicting_memblks(nodeid_t nid, paddr_t start, > + paddr_t end, paddr_t nd_start, > + paddr_t nd_end, unsigned int *mblkid) > { > - int i; > + unsigned int i; > > + /* > + * Scan all recorded nodes' memory blocks to check conflicts: > + * Overlap or interleave. > + */ > for (i = 0; i < num_node_memblks; i++) { > struct node *nd = &node_memblk_range[i]; > + > + *mblkid = i; > + > + /* Skip 0 bytes node memory block. */ > if (nd->start == nd->end) > continue; > + /* > + * Use memblk range to check memblk overlaps, include the > + * self-overlap case. > + */ > if (nd->end > start && nd->start < end) > - return i; > + return OVERLAP; > if (nd->end == end && nd->start == start) > - return i; > + return OVERLAP; Knowing that nd's range is non-empty, is this 2nd condition actually needed here? (Such an adjustment, if you decided to make it and if not split out to a separate patch, would need calling out in the description.) > + /* > + * Use node memory range to check whether new range contains > + * memory from other nodes - interleave check. We just need > + * to check full contains situation. Because overlaps have > + * been checked above. > + */ > + if (nid != memblk_nodeid[i] && > + (nd_start < nd->start && nd->end < nd_end)) > + return INTERLEAVE; Doesn't this need to be <= in both cases (albeit I think one of the two expressions would want switching around, to better line up with the earlier one, visible in context further up). > @@ -275,10 +306,13 @@ acpi_numa_processor_affinity_init(const struct acpi_srat_cpu_affinity *pa) > void __init > acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma) > { > + enum conflicts status; I don't think you need this local variable. > @@ -310,42 +344,78 @@ acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma) > bad_srat(); > return; > } > + > + /* > + * For the node that already has some memory blocks, we will > + * expand the node memory range temporarily to check memory > + * interleaves with other nodes. We will not use this node > + * temp memory range to check overlaps, because it will mask > + * the overlaps in same node. > + * > + * Node with 0 bytes memory doesn't need this expandsion. > + */ > + nd_start = start; > + nd_end = end; > + nd = &nodes[node]; > + if (nd->start != nd->end) { > + if (nd_start > nd->start) > + nd_start = nd->start; > + > + if (nd_end < nd->end) > + nd_end = nd->end; > + } > + > /* It is fine to add this area to the nodes data it will be used later*/ > - i = conflicting_memblks(start, end); > - if (i < 0) > - /* everything fine */; > - else if (memblk_nodeid[i] == node) { > - bool mismatch = !(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) != > - !test_bit(i, memblk_hotplug); > - > - printk("%sSRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with itself (%"PRIpaddr"-%"PRIpaddr")\n", > - mismatch ? KERN_ERR : KERN_WARNING, pxm, start, end, > - node_memblk_range[i].start, node_memblk_range[i].end); > - if (mismatch) { > + status = conflicting_memblks(node, start, end, nd_start, nd_end, &i); > + switch(status) > + { Style: Missing blank before ( and the brace goes on the same line here (Linux style). > + case OVERLAP: > + { Please omit braces at case labels unless you need a new scope to declare variables. > + if (memblk_nodeid[i] == node) { > + bool mismatch = !(ma->flags & > + ACPI_SRAT_MEM_HOT_PLUGGABLE) != > + !test_bit(i, memblk_hotplug); > + > + printk("%sSRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with itself (%"PRIpaddr"-%"PRIpaddr")\n", > + mismatch ? KERN_ERR : KERN_WARNING, pxm, start, > + end, node_memblk_range[i].start, > + node_memblk_range[i].end); > + if (mismatch) { > + bad_srat(); > + return; > + } > + break; > + } else { > + printk(KERN_ERR > + "SRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with PXM %u (%"PRIpaddr"-%"PRIpaddr")\n", > + pxm, start, end, node_to_pxm(memblk_nodeid[i]), > + node_memblk_range[i].start, > + node_memblk_range[i].end); > bad_srat(); > return; > } To limit indentation depth, on of the two sides of the conditional can be moved out, by omitting the unnecessary "else". To reduce the diff it may be worthwhile to invert the if() condition, allowing the (then implicit) "else" case to remain (almost) unchanged from the original. > - } else { > + } > + > + case INTERLEAVE: > + { > printk(KERN_ERR > - "SRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with PXM %u (%"PRIpaddr"-%"PRIpaddr")\n", > - pxm, start, end, node_to_pxm(memblk_nodeid[i]), > + "SRAT： PXM %u: (%"PRIpaddr"-%"PRIpaddr") interleaves with PXM %u memblk (%"PRIpaddr"-%"PRIpaddr")\n", > + node, nd_start, nd_end, node_to_pxm(memblk_nodeid[i]), Hmm, you have PXM in the log message text, but you still pass "node" as first argument. Since you're touching all these messages, could I ask you to convert all ranges to proper mathematical interval representation? I.e. [start,end) here aiui as the end addresses look to be non-inclusive. > node_memblk_range[i].start, node_memblk_range[i].end); > bad_srat(); > return; > } > - if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) { > - struct node *nd = &nodes[node]; > > - if (!node_test_and_set(node, memory_nodes_parsed)) { > - nd->start = start; > - nd->end = end; > - } else { > - if (start < nd->start) > - nd->start = start; > - if (nd->end < end) > - nd->end = end; > - } > + default: > + break; This wants to be "case NO_CONFLICT:", such that the compiler would warn if a new enumerator appears without adding code here. (An alternative - which personally I don't like - would be to put ASSERT_UNREACHABLE() in the default: case. The downside is that then the issue would only be noticeable at runtime.) Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2022年5月31日 21:21 > To: Wei Chen <Wei.Chen@arm.com> > Cc: nd <nd@arm.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau > Monné <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Jiamei Xie > <Jiamei.Xie@arm.com>; xen-devel@lists.xenproject.org > Subject: Re: [PATCH v4 7/8] xen/x86: add detection of memory interleaves > for different nodes > > On 23.05.2022 08:25, Wei Chen wrote: > > @@ -119,20 +125,45 @@ int valid_numa_range(paddr_t start, paddr_t end, > nodeid_t node) > > return 0; > > } > > > > -static __init int conflicting_memblks(paddr_t start, paddr_t end) > > +static > > +enum conflicts __init conflicting_memblks(nodeid_t nid, paddr_t start, > > + paddr_t end, paddr_t nd_start, > > + paddr_t nd_end, unsigned int *mblkid) > > { > > - int i; > > + unsigned int i; > > > > + /* > > + * Scan all recorded nodes' memory blocks to check conflicts: > > + * Overlap or interleave. > > + */ > > for (i = 0; i < num_node_memblks; i++) { > > struct node *nd = &node_memblk_range[i]; > > + > > + *mblkid = i; > > + > > + /* Skip 0 bytes node memory block. */ > > if (nd->start == nd->end) > > continue; > > + /* > > + * Use memblk range to check memblk overlaps, include the > > + * self-overlap case. > > + */ > > if (nd->end > start && nd->start < end) > > - return i; > > + return OVERLAP; > > if (nd->end == end && nd->start == start) > > - return i; > > + return OVERLAP; > > Knowing that nd's range is non-empty, is this 2nd condition actually > needed here? (Such an adjustment, if you decided to make it and if not > split out to a separate patch, would need calling out in the > description.) The 2nd condition here, you meant is "(nd->end == end && nd->start == start)" or just "nd->start == start" after "&&"? My understanding is the first case, "(nd->end == end && nd->start == start)" will be covered by "(nd->end > start && nd->start < end)". If so, I'll remove it in the next version and add some descriptions in the commit log and code comment. > > > + /* > > + * Use node memory range to check whether new range contains > > + * memory from other nodes - interleave check. We just need > > + * to check full contains situation. Because overlaps have > > + * been checked above. > > + */ > > + if (nid != memblk_nodeid[i] && > > + (nd_start < nd->start && nd->end < nd_end)) > > + return INTERLEAVE; > > Doesn't this need to be <= in both cases (albeit I think one of the two > expressions would want switching around, to better line up with the > earlier one, visible in context further up). > Yes, I will add "="in both cases. But for switching around, I also wanted to make a better line up. But if nid == memblk_nodeid[i], the check of (nd_start < nd->start && nd->end < nd_end) is meaningless. I'll adjust their order in the next version if you think this is acceptable. > > @@ -275,10 +306,13 @@ acpi_numa_processor_affinity_init(const struct > acpi_srat_cpu_affinity *pa) > > void __init > > acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma) > > { > > + enum conflicts status; > > I don't think you need this local variable. > Why I don't need this one? Did you mean I can use switch (conflicting_memblks(...)) directly? > > @@ -310,42 +344,78 @@ acpi_numa_memory_affinity_init(const struct > acpi_srat_mem_affinity *ma) > > bad_srat(); > > return; > > } > > + > > + /* > > + * For the node that already has some memory blocks, we will > > + * expand the node memory range temporarily to check memory > > + * interleaves with other nodes. We will not use this node > > + * temp memory range to check overlaps, because it will mask > > + * the overlaps in same node. > > + * > > + * Node with 0 bytes memory doesn't need this expandsion. > > + */ > > + nd_start = start; > > + nd_end = end; > > + nd = &nodes[node]; > > + if (nd->start != nd->end) { > > + if (nd_start > nd->start) > > + nd_start = nd->start; > > + > > + if (nd_end < nd->end) > > + nd_end = nd->end; > > + } > > + > > /* It is fine to add this area to the nodes data it will be used > later*/ > > - i = conflicting_memblks(start, end); > > - if (i < 0) > > - /* everything fine */; > > - else if (memblk_nodeid[i] == node) { > > - bool mismatch = !(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) != > > - !test_bit(i, memblk_hotplug); > > - > > - printk("%sSRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with > itself (%"PRIpaddr"-%"PRIpaddr")\n", > > - mismatch ? KERN_ERR : KERN_WARNING, pxm, start, end, > > - node_memblk_range[i].start, node_memblk_range[i].end); > > - if (mismatch) { > > + status = conflicting_memblks(node, start, end, nd_start, nd_end, &i); > > + switch(status) > > + { > > Style: Missing blank before ( and the brace goes on the same line here > (Linux style). > Ok. > > + case OVERLAP: > > + { > > Please omit braces at case labels unless you need a new scope to declare > variables. > OK. > > + if (memblk_nodeid[i] == node) { > > + bool mismatch = !(ma->flags & > > + ACPI_SRAT_MEM_HOT_PLUGGABLE) != > > + !test_bit(i, memblk_hotplug); > > + > > + printk("%sSRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") > overlaps with itself (%"PRIpaddr"-%"PRIpaddr")\n", > > + mismatch ? KERN_ERR : KERN_WARNING, pxm, start, > > + end, node_memblk_range[i].start, > > + node_memblk_range[i].end); > > + if (mismatch) { > > + bad_srat(); > > + return; > > + } > > + break; > > + } else { > > + printk(KERN_ERR > > + "SRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps > with PXM %u (%"PRIpaddr"-%"PRIpaddr")\n", > > + pxm, start, end, node_to_pxm(memblk_nodeid[i]), > > + node_memblk_range[i].start, > > + node_memblk_range[i].end); > > bad_srat(); > > return; > > } > > To limit indentation depth, on of the two sides of the conditional can > be moved out, by omitting the unnecessary "else". To reduce the diff > it may be worthwhile to invert the if() condition, allowing the (then > implicit) "else" case to remain (almost) unchanged from the original. > I will adjust them in next version. > > - } else { > > + } > > + > > + case INTERLEAVE: > > + { > > printk(KERN_ERR > > - "SRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with > PXM %u (%"PRIpaddr"-%"PRIpaddr")\n", > > - pxm, start, end, node_to_pxm(memblk_nodeid[i]), > > + "SRAT： PXM %u: (%"PRIpaddr"-%"PRIpaddr") interleaves > with PXM %u memblk (%"PRIpaddr"-%"PRIpaddr")\n", > > + node, nd_start, nd_end, node_to_pxm(memblk_nodeid[i]), > > Hmm, you have PXM in the log message text, but you still pass "node" as > first argument. > I will fix it. > Since you're touching all these messages, could I ask you to convert > all ranges to proper mathematical interval representation? I.e. > [start,end) here aiui as the end addresses look to be non-inclusive. > Sure, I will do it. > > node_memblk_range[i].start, node_memblk_range[i].end); > > bad_srat(); > > return; > > } > > - if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) { > > - struct node *nd = &nodes[node]; > > > > - if (!node_test_and_set(node, memory_nodes_parsed)) { > > - nd->start = start; > > - nd->end = end; > > - } else { > > - if (start < nd->start) > > - nd->start = start; > > - if (nd->end < end) > > - nd->end = end; > > - } > > + default: > > + break; > > This wants to be "case NO_CONFLICT:", such that the compiler would > warn if a new enumerator appears without adding code here. (An > alternative - which personally I don't like - would be to put > ASSERT_UNREACHABLE() in the default: case. The downside is that > then the issue would only be noticeable at runtime.) > Thanks, I will adjust it to: case NO_CONFLICT: break; default: ASSERT_UNREACHABLE(); in next version. > Jan
On 01.06.2022 04:53, Wei Chen wrote: >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 2022年5月31日 21:21 >> >> On 23.05.2022 08:25, Wei Chen wrote: >>> @@ -119,20 +125,45 @@ int valid_numa_range(paddr_t start, paddr_t end, >> nodeid_t node) >>> return 0; >>> } >>> >>> -static __init int conflicting_memblks(paddr_t start, paddr_t end) >>> +static >>> +enum conflicts __init conflicting_memblks(nodeid_t nid, paddr_t start, >>> + paddr_t end, paddr_t nd_start, >>> + paddr_t nd_end, unsigned int *mblkid) >>> { >>> - int i; >>> + unsigned int i; >>> >>> + /* >>> + * Scan all recorded nodes' memory blocks to check conflicts: >>> + * Overlap or interleave. >>> + */ >>> for (i = 0; i < num_node_memblks; i++) { >>> struct node *nd = &node_memblk_range[i]; >>> + >>> + *mblkid = i; >>> + >>> + /* Skip 0 bytes node memory block. */ >>> if (nd->start == nd->end) >>> continue; >>> + /* >>> + * Use memblk range to check memblk overlaps, include the >>> + * self-overlap case. >>> + */ >>> if (nd->end > start && nd->start < end) >>> - return i; >>> + return OVERLAP; >>> if (nd->end == end && nd->start == start) >>> - return i; >>> + return OVERLAP; >> >> Knowing that nd's range is non-empty, is this 2nd condition actually >> needed here? (Such an adjustment, if you decided to make it and if not >> split out to a separate patch, would need calling out in the >> description.) > > The 2nd condition here, you meant is "(nd->end == end && nd->start == start)" > or just "nd->start == start" after "&&"? No, I mean the entire 2nd if(). >>> + /* >>> + * Use node memory range to check whether new range contains >>> + * memory from other nodes - interleave check. We just need >>> + * to check full contains situation. Because overlaps have >>> + * been checked above. >>> + */ >>> + if (nid != memblk_nodeid[i] && >>> + (nd_start < nd->start && nd->end < nd_end)) >>> + return INTERLEAVE; >> >> Doesn't this need to be <= in both cases (albeit I think one of the two >> expressions would want switching around, to better line up with the >> earlier one, visible in context further up). >> > > Yes, I will add "="in both cases. But for switching around, I also > wanted to make a better line up. But if nid == memblk_nodeid[i], > the check of (nd_start < nd->start && nd->end < nd_end) is meaningless. > I'll adjust their order in the next version if you think this is > acceptable. I wasn't referring to the "nid != memblk_nodeid[i]" part at all. What I'm after is for the two range checks to come as close as possible to what the other range check does. (Which, as I notice only now, would include the dropping of the unnecessary inner pair of parentheses.) E.g. (there are other variations of it) if (nid != memblk_nodeid[i] && nd->start >= nd_start && nd->end <= nd_end) return INTERLEAVE; >>> @@ -275,10 +306,13 @@ acpi_numa_processor_affinity_init(const struct >> acpi_srat_cpu_affinity *pa) >>> void __init >>> acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma) >>> { >>> + enum conflicts status; >> >> I don't think you need this local variable. >> > > Why I don't need this one? Did you mean I can use > switch (conflicting_memblks(...)) directly? Yes. Why could this not be possible? >>> node_memblk_range[i].start, node_memblk_range[i].end); >>> bad_srat(); >>> return; >>> } >>> - if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) { >>> - struct node *nd = &nodes[node]; >>> >>> - if (!node_test_and_set(node, memory_nodes_parsed)) { >>> - nd->start = start; >>> - nd->end = end; >>> - } else { >>> - if (start < nd->start) >>> - nd->start = start; >>> - if (nd->end < end) >>> - nd->end = end; >>> - } >>> + default: >>> + break; >> >> This wants to be "case NO_CONFLICT:", such that the compiler would >> warn if a new enumerator appears without adding code here. (An >> alternative - which personally I don't like - would be to put >> ASSERT_UNREACHABLE() in the default: case. The downside is that >> then the issue would only be noticeable at runtime.) >> > > Thanks, I will adjust it to: > case NO_CONFLICT: > break; > default: > ASSERT_UNREACHABLE(); > in next version. As said - I consider this form less desirable, as it'll defer noticing of an issue from build-time to runtime. If you think that form is better, may I ask why? Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2022年6月1日 14:32 > To: Wei Chen <Wei.Chen@arm.com> > Cc: nd <nd@arm.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau > Monné <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Jiamei Xie > <Jiamei.Xie@arm.com>; xen-devel@lists.xenproject.org > Subject: Re: [PATCH v4 7/8] xen/x86: add detection of memory interleaves > for different nodes > > On 01.06.2022 04:53, Wei Chen wrote: > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: 2022年5月31日 21:21 > >> > >> On 23.05.2022 08:25, Wei Chen wrote: > >>> @@ -119,20 +125,45 @@ int valid_numa_range(paddr_t start, paddr_t end, > >> nodeid_t node) > >>> return 0; > >>> } > >>> > >>> -static __init int conflicting_memblks(paddr_t start, paddr_t end) > >>> +static > >>> +enum conflicts __init conflicting_memblks(nodeid_t nid, paddr_t start, > >>> + paddr_t end, paddr_t nd_start, > >>> + paddr_t nd_end, unsigned int *mblkid) > >>> { > >>> - int i; > >>> + unsigned int i; > >>> > >>> + /* > >>> + * Scan all recorded nodes' memory blocks to check conflicts: > >>> + * Overlap or interleave. > >>> + */ > >>> for (i = 0; i < num_node_memblks; i++) { > >>> struct node *nd = &node_memblk_range[i]; > >>> + > >>> + *mblkid = i; > >>> + > >>> + /* Skip 0 bytes node memory block. */ > >>> if (nd->start == nd->end) > >>> continue; > >>> + /* > >>> + * Use memblk range to check memblk overlaps, include the > >>> + * self-overlap case. > >>> + */ > >>> if (nd->end > start && nd->start < end) > >>> - return i; > >>> + return OVERLAP; > >>> if (nd->end == end && nd->start == start) > >>> - return i; > >>> + return OVERLAP; > >> > >> Knowing that nd's range is non-empty, is this 2nd condition actually > >> needed here? (Such an adjustment, if you decided to make it and if not > >> split out to a separate patch, would need calling out in the > >> description.) > > > > The 2nd condition here, you meant is "(nd->end == end && nd->start == > start)" > > or just "nd->start == start" after "&&"? > > No, I mean the entire 2nd if(). > OK. > >>> + /* > >>> + * Use node memory range to check whether new range contains > >>> + * memory from other nodes - interleave check. We just need > >>> + * to check full contains situation. Because overlaps have > >>> + * been checked above. > >>> + */ > >>> + if (nid != memblk_nodeid[i] && > >>> + (nd_start < nd->start && nd->end < nd_end)) > >>> + return INTERLEAVE; > >> > >> Doesn't this need to be <= in both cases (albeit I think one of the two > >> expressions would want switching around, to better line up with the > >> earlier one, visible in context further up). > >> > > > > Yes, I will add "="in both cases. But for switching around, I also > > wanted to make a better line up. But if nid == memblk_nodeid[i], > > the check of (nd_start < nd->start && nd->end < nd_end) is meaningless. > > I'll adjust their order in the next version if you think this is > > acceptable. > > I wasn't referring to the "nid != memblk_nodeid[i]" part at all. What > I'm after is for the two range checks to come as close as possible to > what the other range check does. (Which, as I notice only now, would > include the dropping of the unnecessary inner pair of parentheses.) > E.g. (there are other variations of it) > > if (nid != memblk_nodeid[i] && > nd->start >= nd_start && nd->end <= nd_end) > return INTERLEAVE; > Oh, thanks. I had thought too much, I will drop them. > >>> @@ -275,10 +306,13 @@ acpi_numa_processor_affinity_init(const struct > >> acpi_srat_cpu_affinity *pa) > >>> void __init > >>> acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity > *ma) > >>> { > >>> + enum conflicts status; > >> > >> I don't think you need this local variable. > >> > > > > Why I don't need this one? Did you mean I can use > > switch (conflicting_memblks(...)) directly? > > Yes. Why could this not be possible? > Ok. > >>> node_memblk_range[i].start, node_memblk_range[i].end); > >>> bad_srat(); > >>> return; > >>> } > >>> - if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) { > >>> - struct node *nd = &nodes[node]; > >>> > >>> - if (!node_test_and_set(node, memory_nodes_parsed)) { > >>> - nd->start = start; > >>> - nd->end = end; > >>> - } else { > >>> - if (start < nd->start) > >>> - nd->start = start; > >>> - if (nd->end < end) > >>> - nd->end = end; > >>> - } > >>> + default: > >>> + break; > >> > >> This wants to be "case NO_CONFLICT:", such that the compiler would > >> warn if a new enumerator appears without adding code here. (An > >> alternative - which personally I don't like - would be to put > >> ASSERT_UNREACHABLE() in the default: case. The downside is that > >> then the issue would only be noticeable at runtime.) > >> > > > > Thanks, I will adjust it to: > > case NO_CONFLICT: > > break; > > default: > > ASSERT_UNREACHABLE(); > > in next version. > > As said - I consider this form less desirable, as it'll defer > noticing of an issue from build-time to runtime. If you think that > form is better, may I ask why? > Ok. I will drop the default. I had mis-understood your comment. > Jan
Hi Jan, On 2022/5/31 21:21, Jan Beulich wrote: > On 23.05.2022 08:25, Wei Chen wrote: >> @@ -119,20 +125,45 @@ int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node) >> return 0; > > To limit indentation depth, on of the two sides of the conditional can > be moved out, by omitting the unnecessary "else". To reduce the diff > it may be worthwhile to invert the if() condition, allowing the (then > implicit) "else" case to remain (almost) unchanged from the original. > >> - } else { >> + } >> + >> + case INTERLEAVE: >> + { >> printk(KERN_ERR >> - "SRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with PXM %u (%"PRIpaddr"-%"PRIpaddr")\n", >> - pxm, start, end, node_to_pxm(memblk_nodeid[i]), >> + "SRAT： PXM %u: (%"PRIpaddr"-%"PRIpaddr") interleaves with PXM %u memblk (%"PRIpaddr"-%"PRIpaddr")\n", >> + node, nd_start, nd_end, node_to_pxm(memblk_nodeid[i]), > > Hmm, you have PXM in the log message text, but you still pass "node" as > first argument. > > Since you're touching all these messages, could I ask you to convert > all ranges to proper mathematical interval representation? I.e. > [start,end) here aiui as the end addresses look to be non-inclusive. > Sorry, I want to confirm with you about this comment again. Now the messages look like: (XEN) NUMA： PXM 0: (0000000080000000-00000008d8000000) interleaves... So I want to know, is it [0000000080000000-00000008d8000000) or (0000000080000000-00000008d7ffffff) addressed your comment? Literally, I think it's case#1? Thanks, Wei Chen >
On 02.06.2022 06:10, Wei Chen wrote: > Hi Jan, > > On 2022/5/31 21:21, Jan Beulich wrote: >> On 23.05.2022 08:25, Wei Chen wrote: >>> @@ -119,20 +125,45 @@ int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node) >>> return 0; > >> >> To limit indentation depth, on of the two sides of the conditional can >> be moved out, by omitting the unnecessary "else". To reduce the diff >> it may be worthwhile to invert the if() condition, allowing the (then >> implicit) "else" case to remain (almost) unchanged from the original. >> >>> - } else { >>> + } >>> + >>> + case INTERLEAVE: >>> + { >>> printk(KERN_ERR >>> - "SRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with PXM %u (%"PRIpaddr"-%"PRIpaddr")\n", >>> - pxm, start, end, node_to_pxm(memblk_nodeid[i]), >>> + "SRAT： PXM %u: (%"PRIpaddr"-%"PRIpaddr") interleaves with PXM %u memblk (%"PRIpaddr"-%"PRIpaddr")\n", >>> + node, nd_start, nd_end, node_to_pxm(memblk_nodeid[i]), >> >> Hmm, you have PXM in the log message text, but you still pass "node" as >> first argument. >> >> Since you're touching all these messages, could I ask you to convert >> all ranges to proper mathematical interval representation? I.e. >> [start,end) here aiui as the end addresses look to be non-inclusive. >> > > Sorry, I want to confirm with you about this comment again. Now the > messages look like: > (XEN) NUMA： PXM 0: (0000000080000000-00000008d8000000) interleaves... > > So I want to know, is it [0000000080000000-00000008d8000000) or > (0000000080000000-00000008d7ffffff) addressed your comment? > Literally, I think it's case#1? The former or [0000000080000000-00000008d7ffffff]. Parentheses stand for exclusive boundaries, while square brackets stand for inclusive ones. Jan
diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c index 8ffe43bdfe..a831df7648 100644 --- a/xen/arch/x86/srat.c +++ b/xen/arch/x86/srat.c @@ -42,6 +42,12 @@ static struct node node_memblk_range[NR_NODE_MEMBLKS]; static nodeid_t memblk_nodeid[NR_NODE_MEMBLKS]; static __initdata DECLARE_BITMAP(memblk_hotplug, NR_NODE_MEMBLKS); +enum conflicts { + NO_CONFLICT, + OVERLAP, + INTERLEAVE, +}; + static inline bool node_found(unsigned idx, unsigned pxm) { return ((pxm2node[idx].pxm == pxm) && @@ -119,20 +125,45 @@ int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node) return 0; } -static __init int conflicting_memblks(paddr_t start, paddr_t end) +static +enum conflicts __init conflicting_memblks(nodeid_t nid, paddr_t start, + paddr_t end, paddr_t nd_start, + paddr_t nd_end, unsigned int *mblkid) { - int i; + unsigned int i; + /* + * Scan all recorded nodes' memory blocks to check conflicts: + * Overlap or interleave. + */ for (i = 0; i < num_node_memblks; i++) { struct node *nd = &node_memblk_range[i]; + + *mblkid = i; + + /* Skip 0 bytes node memory block. */ if (nd->start == nd->end) continue; + /* + * Use memblk range to check memblk overlaps, include the + * self-overlap case. + */ if (nd->end > start && nd->start < end) - return i; + return OVERLAP; if (nd->end == end && nd->start == start) - return i; + return OVERLAP; + /* + * Use node memory range to check whether new range contains + * memory from other nodes - interleave check. We just need + * to check full contains situation. Because overlaps have + * been checked above. + */ + if (nid != memblk_nodeid[i] && + (nd_start < nd->start && nd->end < nd_end)) + return INTERLEAVE; } - return -1; + + return NO_CONFLICT; } static __init void cutoff_node(int i, paddr_t start, paddr_t end) @@ -275,10 +306,13 @@ acpi_numa_processor_affinity_init(const struct acpi_srat_cpu_affinity *pa) void __init acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma) { + enum conflicts status; + struct node *nd; + paddr_t nd_start, nd_end; paddr_t start, end; unsigned pxm; nodeid_t node; - int i; + unsigned int i; if (srat_disabled()) return; @@ -310,42 +344,78 @@ acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma) bad_srat(); return; } + + /* + * For the node that already has some memory blocks, we will + * expand the node memory range temporarily to check memory + * interleaves with other nodes. We will not use this node + * temp memory range to check overlaps, because it will mask + * the overlaps in same node. + * + * Node with 0 bytes memory doesn't need this expandsion. + */ + nd_start = start; + nd_end = end; + nd = &nodes[node]; + if (nd->start != nd->end) { + if (nd_start > nd->start) + nd_start = nd->start; + + if (nd_end < nd->end) + nd_end = nd->end; + } + /* It is fine to add this area to the nodes data it will be used later*/ - i = conflicting_memblks(start, end); - if (i < 0) - /* everything fine */; - else if (memblk_nodeid[i] == node) { - bool mismatch = !(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) != - !test_bit(i, memblk_hotplug); - - printk("%sSRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with itself (%"PRIpaddr"-%"PRIpaddr")\n", - mismatch ? KERN_ERR : KERN_WARNING, pxm, start, end, - node_memblk_range[i].start, node_memblk_range[i].end); - if (mismatch) { + status = conflicting_memblks(node, start, end, nd_start, nd_end, &i); + switch(status) + { + case OVERLAP: + { + if (memblk_nodeid[i] == node) { + bool mismatch = !(ma->flags & + ACPI_SRAT_MEM_HOT_PLUGGABLE) != + !test_bit(i, memblk_hotplug); + + printk("%sSRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with itself (%"PRIpaddr"-%"PRIpaddr")\n", + mismatch ? KERN_ERR : KERN_WARNING, pxm, start, + end, node_memblk_range[i].start, + node_memblk_range[i].end); + if (mismatch) { + bad_srat(); + return; + } + break; + } else { + printk(KERN_ERR + "SRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with PXM %u (%"PRIpaddr"-%"PRIpaddr")\n", + pxm, start, end, node_to_pxm(memblk_nodeid[i]), + node_memblk_range[i].start, + node_memblk_range[i].end); bad_srat(); return; } - } else { + } + + case INTERLEAVE: + { printk(KERN_ERR - "SRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with PXM %u (%"PRIpaddr"-%"PRIpaddr")\n", - pxm, start, end, node_to_pxm(memblk_nodeid[i]), + "SRAT： PXM %u: (%"PRIpaddr"-%"PRIpaddr") interleaves with PXM %u memblk (%"PRIpaddr"-%"PRIpaddr")\n", + node, nd_start, nd_end, node_to_pxm(memblk_nodeid[i]), node_memblk_range[i].start, node_memblk_range[i].end); bad_srat(); return; } - if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) { - struct node *nd = &nodes[node]; - if (!node_test_and_set(node, memory_nodes_parsed)) { - nd->start = start; - nd->end = end; - } else { - if (start < nd->start) - nd->start = start; - if (nd->end < end) - nd->end = end; - } + default: + break; + } + + if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) { + node_set(node, memory_nodes_parsed); + nd->start = nd_start; + nd->end = nd_end; } + printk(KERN_INFO "SRAT: Node %u PXM %u %"PRIpaddr"-%"PRIpaddr"%s\n", node, pxm, start, end, ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE ? " (hotplug)" : "");