Message ID | 20220511014639.197825-9-wei.chen@arm.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | Device tree based NUMA support for Arm - Part#1 | expand |
On 11.05.2022 03:46, Wei Chen wrote: > --- 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 = 0, No need for the "= 0". > + ERR_OVERLAP, While this at least can be an error (the self-overlap case is merely warned about), ... > + ERR_INTERLEAVE, ... I don't think this is, and hence I'd recommend to drop "ERR_". > @@ -119,20 +125,43 @@ 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, int *mblkid) Why "int"? Can the value passed back be negative? > { > 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; Style: Please maintain a blank line between declaration(s) and statement(s). > @@ -310,42 +342,67 @@ 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 < end) Did you mean nd->end here on the right side of < ? By intentionally not adding "default:" in the body, you then also allow the compiler to point out that addition of a new enumerator also needs handling here. > + 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); > + if (status == ERR_OVERLAP) { Please use switch(status) when checking enumerated values. > + if (memblk_nodeid[i] == node) { > + bool mismatch = !(ma->flags & > + ACPI_SRAT_MEM_HOT_PLUGGABLE) != > + !test_bit(i, memblk_hotplug); Style: The middle line wants indenting by two more characters. > + > + 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; > + } > + } 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 { > + } else if (status == ERR_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: Node %u: (%"PRIpaddr"-%"PRIpaddr") interleaves with node %u memblk (%"PRIpaddr"-%"PRIpaddr")\n", > + node, nd_start, nd_end, memblk_nodeid[i], Please log pxm (not node) here just like is done in the overlap case. The remote node ID will then require converting to PXM, of course. Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2022年5月18日 21:31 > 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 v3 8/9] xen/x86: add detection of memory interleaves > for different nodes > > On 11.05.2022 03:46, Wei Chen wrote: > > --- 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 = 0, > > No need for the "= 0". > Ack. > > + ERR_OVERLAP, > > While this at least can be an error (the self-overlap case is merely > warned about), ... > > > + ERR_INTERLEAVE, > > ... I don't think this is, and hence I'd recommend to drop "ERR_". > Oh, yes. I all drop it for all above enumerations. > > @@ -119,20 +125,43 @@ 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, int *mblkid) > > Why "int"? Can the value passed back be negative? > The caller "acpi_numa_memory_affinity_init" defines int for node memory block id, and as a return value at the same time. So I haven't changed it. As we don't use this "int" for return value any more, I agree, it will never be negative, I would fix it. > > { > > 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; > > Style: Please maintain a blank line between declaration(s) and > statement(s). > Ok. > > @@ -310,42 +342,67 @@ 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 < end) > > Did you mean nd->end here on the right side of < ? By intentionally Oh! thanks for pointing out this one! Yes, right side should be nd->end. > not adding "default:" in the body, you then also allow the compiler > to point out that addition of a new enumerator also needs handling > here. > Did you mean, we need to add if ... else ... in this block? If yes, is it ok to update this block like: if (nd->start != nd->end) { nd_start = min(nd_start, nd->start); nd_end = max(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); > > + if (status == ERR_OVERLAP) { > > Please use switch(status) when checking enumerated values. > Ok, I will do it. > > + if (memblk_nodeid[i] == node) { > > + bool mismatch = !(ma->flags & > > + ACPI_SRAT_MEM_HOT_PLUGGABLE) != > > + !test_bit(i, memblk_hotplug); > > Style: The middle line wants indenting by two more characters. > Yes, I will do it. > > + > > + 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; > > + } > > + } 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 { > > + } else if (status == ERR_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: Node %u: (%"PRIpaddr"-%"PRIpaddr") interleaves > with node %u memblk (%"PRIpaddr"-%"PRIpaddr")\n", > > + node, nd_start, nd_end, memblk_nodeid[i], > > Please log pxm (not node) here just like is done in the overlap case. > The remote node ID will then require converting to PXM, of course. > Ok, will use PXM here. But I have question for upcoming changes, if we move this part of code to common. As device tree NUMA doesn't have PXM concept (even I can use a fake node_to_pxm to do 1:1 mapping), so can we still use PXM here? > Jan
On 19.05.2022 05:37, Wei Chen wrote: >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 2022年5月18日 21:31 >> >> On 11.05.2022 03:46, Wei Chen wrote: >>> @@ -310,42 +342,67 @@ 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 < end) >> >> Did you mean nd->end here on the right side of < ? By intentionally > > Oh! thanks for pointing out this one! Yes, right side should be nd->end. > >> not adding "default:" in the body, you then also allow the compiler >> to point out that addition of a new enumerator also needs handling >> here. >> > > Did you mean, we need to add if ... else ... in this block? If yes, > is it ok to update this block like: > if (nd->start != nd->end) { > nd_start = min(nd_start, nd->start); > nd_end = max(nd_end, nd->end); > } > ? No. I attached this part about "default:" late in the process of writing the reply, and I did put it in the wrong spot. I'm sorry. It really was meant to go ... >>> + 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); >>> + if (status == ERR_OVERLAP) { >> >> Please use switch(status) when checking enumerated values. >> > > Ok, I will do it. ... here, explaining the request to use switch(). >>> + 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; >>> + } >>> + } 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 { >>> + } else if (status == ERR_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: Node %u: (%"PRIpaddr"-%"PRIpaddr") interleaves >> with node %u memblk (%"PRIpaddr"-%"PRIpaddr")\n", >>> + node, nd_start, nd_end, memblk_nodeid[i], >> >> Please log pxm (not node) here just like is done in the overlap case. >> The remote node ID will then require converting to PXM, of course. >> > > Ok, will use PXM here. But I have question for upcoming changes, if we > move this part of code to common. As device tree NUMA doesn't have > PXM concept (even I can use a fake node_to_pxm to do 1:1 mapping), so > can we still use PXM here? This will want properly abstracting once made common, yes. What the correct model on Arm/DT is I can't really tell. But my (earlier voiced) request remains: What is logged should by referring the firmware provided values, not Xen-internal ones. Otherwise someone reading the log cannot easily know / derive what's wrong where. Jan
diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c index 8ffe43bdfe..53835ae3eb 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 = 0, + ERR_OVERLAP, + ERR_INTERLEAVE, +}; + static inline bool node_found(unsigned idx, unsigned pxm) { return ((pxm2node[idx].pxm == pxm) && @@ -119,20 +125,43 @@ 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, int *mblkid) { 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 ERR_OVERLAP; if (nd->end == end && nd->start == start) - return i; + return ERR_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 ERR_INTERLEAVE; } - return -1; + + return NO_CONFLICT; } static __init void cutoff_node(int i, paddr_t start, paddr_t end) @@ -275,6 +304,9 @@ 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; @@ -310,42 +342,67 @@ 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 < 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); + if (status == ERR_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; + } + } 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 { + } else if (status == ERR_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: Node %u: (%"PRIpaddr"-%"PRIpaddr") interleaves with node %u memblk (%"PRIpaddr"-%"PRIpaddr")\n", + node, nd_start, nd_end, 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; - } + 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)" : "");