RE: [PATCH v8 5/6] xen/x86: move NUMA process nodes nodes code from x86 to common

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> > 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.
> 
> The use of "prefix" here must have been stale for a while, perhaps
> resulting from an incomplete re-write of what was here earlier on?
> 

Yes, I forgot to update this description when we're changing the
method from extending parameter to use numa_fw_nid_name. I will
correct it.

> > -static int __init nodes_cover_memory(void)
> > -{
> > -   unsigned int i;
> > -
> > -   for (i = 0; ; i++) {
> > -                           }
> > -           } 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";
> 
> I guess this can't go without a comment, now that we have ...
> 

Ok, how about following comment:
      /*
       * In an extremely unlikely case, srat_parse_regions might not
       * be called. So set "PXM" before the first caller to use it to
       * make it more safe.
       */
And ...

> > +   if (!numa_update_node_memblks(node, pxm, ma->base_address, ma-
> >length,
> > +                                 ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE))
> > +           numa_fw_bad();
> >  }
> >
> >  void __init acpi_numa_arch_fixup(void) {}
> > @@ -534,6 +295,7 @@ void __init srat_parse_regions(paddr_t addr)
> >         acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat))
> >             return;
> >
> > +   numa_fw_nid_name = "PXM";
> 
> ... this as well. Otherwise someone may spot the redundancy and either
> propose to drop one, or it'll take them quite a bit of time to figure
> why both are there. I thought this would go without saying, so I'm
> sorry for not making this explicit earlier on.

...
      /* Set "PXM" as earlier as we can for those functions will use it. */
> > +   numa_fw_nid_name = "PXM";

Cheers,
Wei Chen

> 
> Jan