On 20.10.2022 08:14, Wei Chen wrote:
> x86 has implemented a set of codes to process NUMA nodes. These
> codes will parse NUMA memory and processor information from
> ACPI SRAT table. But except some ACPI specific codes, most
> of the process code like memory blocks validation, node memory
> range updates and some sanity check can be reused by other
> NUMA implementation.
>
> So in this patch, we move some variables and related functions
> for NUMA memory and processor to common as library. At the
> same time, numa_set_processor_nodes_parsed has been introduced
> for ACPI specific code to update processor parsing results.
> With this helper, we can reuse most of NUMA memory affinity init
> code from ACPI. As bad_srat and node_to_pxm functions have been
> used in common code to do architectural fallback and node to
> architectural node info translation. But it doesn't make sense
> to reuse the functions names in common code, we have rename them
> to neutral names as well.
>
> PXM is an ACPI specific item, we can't use it in common code
> directly. As an alternative, we extend the parameters of
> numa_update_node_memblks. The caller can pass the PXM as print
> messages' prefix or as architectural node id. And we introduced
> an numa_fw_nid_name for each NUMA implementation to set their
> specific firmware NUMA node name. In this case, we do not need
> to retain a lot of per-arch code but still can print architectural
> log messages for different NUMA implementations. A default value
> "???" will be set to indicate an unset numa_fw_nid_name.
>
> mem_hotplug is accessed by common code if memory hotplug is
> activated. Even if this is only supported by x86, export the
> variable so that other architectures could support it in the future.
>
> As asm/acpi.h has been removed from common/numa.c, we have to
> move NR_NODE_MEMBLKS from asm/acpi.h to xen/numa.h in this patch
> as well.
>
> Signed-off-by: Wei Chen <wei.c...@arm.com>
There's just one remaining concern I have: I continue to consider ...
> @@ -341,159 +247,14 @@ acpi_numa_memory_affinity_init(const struct
> acpi_srat_mem_affinity *ma)
> pxm &= 0xff;
> node = setup_node(pxm);
> if (node == NUMA_NO_NODE) {
> - bad_srat();
> + numa_fw_bad();
> 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*/
> - switch (conflicting_memblks(node, start, end, nd_start, nd_end, &i)) {
> - 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 - 1, node_memblk_range[i].start,
> - node_memblk_range[i].end - 1);
> - if (mismatch) {
> - bad_srat();
> - return;
> - }
> - break;
> - }
> -
> - printk(KERN_ERR
> - "SRAT: PXM %u [%"PRIpaddr", %"PRIpaddr"] overlaps with
> PXM %u [%"PRIpaddr", %"PRIpaddr"]\n",
> - pxm, start, end - 1, node_to_pxm(memblk_nodeid[i]),
> - node_memblk_range[i].start,
> - node_memblk_range[i].end - 1);
> - bad_srat();
> - return;
> -
> - case INTERLEAVE:
> - printk(KERN_ERR
> - "SRAT： PXM %u: [%"PRIpaddr", %"PRIpaddr"] interleaves
> with PXM %u memblk [%"PRIpaddr", %"PRIpaddr"]\n",
> - pxm, nd_start, nd_end - 1, node_to_pxm(memblk_nodeid[i]),
> - node_memblk_range[i].start, node_memblk_range[i].end -
> 1);
> - bad_srat();
> - return;
> -
> - case NO_CONFLICT:
> - 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 - 1,
> - ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE ? " (hotplug)" : "");
> -
> - /* Keep node_memblk_range[] sorted by address. */
> - for (i = 0; i < num_node_memblks; ++i)
> - if (node_memblk_range[i].start > start ||
> - (node_memblk_range[i].start == start &&
> - node_memblk_range[i].end > end))
> - break;
> -
> - memmove(&node_memblk_range[i + 1], &node_memblk_range[i],
> - (num_node_memblks - i) * sizeof(*node_memblk_range));
> - node_memblk_range[i].start = start;
> - node_memblk_range[i].end = end;
> -
> - memmove(&memblk_nodeid[i + 1], &memblk_nodeid[i],
> - (num_node_memblks - i) * sizeof(*memblk_nodeid));
> - memblk_nodeid[i] = node;
> -
> - if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
> - next = true;
> - if (end > mem_hotplug)
> - mem_hotplug = end;
> - }
> - for (; i <= num_node_memblks; ++i) {
> - bool prev = next;
> -
> - next = test_bit(i, memblk_hotplug);
> - if (prev)
> - __set_bit(i, memblk_hotplug);
> - else
> - __clear_bit(i, memblk_hotplug);
> - }
> -
> - num_node_memblks++;
> -}
> -
> -/* Sanity check to catch more bad SRATs (they are amazingly common).
> - Make sure the PXMs cover all memory. */
> -static int __init nodes_cover_memory(void)
> -{
> - unsigned int i;
> -
> - for (i = 0; ; i++) {
> - int err;
> - unsigned int j;
> - bool found;
> - paddr_t start, end;
> -
> - /* Try to loop memory map from index 0 to end to get RAM
> ranges. */
> - err = arch_get_ram_range(i, &start, &end);
> -
> - /* Reached the end of the memory map? */
> - if (err == -ENOENT)
> - break;
> -
> - /* Skip non-RAM entries. */
> - if (err)
> - continue;
> -
> - do {
> - found = false;
> - for_each_node_mask(j, memory_nodes_parsed)
> - if (start < nodes[j].end
> - && end > nodes[j].start) {
> - if (start >= nodes[j].start) {
> - start = nodes[j].end;
> - found = true;
> - }
> - if (end <= nodes[j].end) {
> - end = nodes[j].start;
> - found = true;
> - }
> - }
> - } while (found && start < end);
> -
> - if (start < end) {
> - printk(KERN_ERR "NUMA: No NODE for RAM range: "
> - "[%"PRIpaddr", %"PRIpaddr"]\n", start, end - 1);
> - return 0;
> - }
> - }
> - return 1;
> + numa_fw_nid_name = "PXM";
... this to be happening too late. Not because I can see a way for current
code to use the variable earlier, but because of the risk of future code
potentially doing so. Afaics srat_parse_regions() is called quite a bit
earlier, so perhaps the field should (also?) be set there, presumably
after acpi_table_parse() has succeeded. I've included "(also?)" because I
think to be on the safe side the setting here may want keeping, albeit
perhaps moving up in the function.
Jan
