Re: [PATCH v5 5/6] xen/x86: move NUMA scan nodes codes from x86 to common

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.10.2022 09:03, Wei Chen wrote:
> On 2022/10/9 15:25, Wei Chen wrote:
>>>>> Even more so an answer to my question would be nice: You'll then have
>>>>> CONFIG_HAS_NUMA_NODE_FWID=y even on Arm (using PXM as mandated by ACPI
>>>>> when in ACPI mode). But then what's the FWID for DT? I know it was me
>>>>> to suggest this build time distinction, but I'm afraid I wasn't doing
>>>>> much good with that (and I'm sorry).
>>>>
>>>> How about introducing a flag for selected NUMA implementation to
>>>> set it in runtime?
>>>> For example:
>>>> bool numa_has_fw_nodeid;
>>>>
>>>> ACPI NUMA will set this flag to 1, but 0 for DT NUMA.
>>>
>>> That's an option alongside going back to what you had in an earlier
>>> version. Another would be (name subject to improvement)
>>>
>>> const char *__ro_after_init numa_fw_nid_name;
>>>
> 
> When I was dealing with this comment, I found that I was still a little 
> unclear:
> 
> When we were introducing "CONFIG_HAS_NUMA_NODE_FWID", we wanted to 
> eliminate the redundant code of:
> if ( fwid_name not equal to "node" )
>      printk(KERN_INFO "NUMA: Node %u %s %u [%"PRIpaddr"%"PRIpaddr"]%s\n",
>             node, fwid_name , arch_nid, start, end - 1,
>             hotplug ? " (hotplug)" : "");
> else
>      printk(KERN_INFO "NUMA: Node %u [%"PRIpaddr", %"PRIpaddr"]%s\n",
>             node, start, end - 1, hotplug ? " (hotplug)" : "");
> 
> But when I am working with numa_fw_nid_name, I find it's still not
> easy to reduce above redundant code. For example:

As said - this attempt to limit redundancy was a mistake when it comes
to the existence of two models in parallel for an arch (with either
one picked at runtime), unless ...

> "NUMA: Node %u %s %u
> When numa_fw_nid_name = NULL, we can print "" for %s, but can't reduce
> the second %u.
> 
> So can we split this message into 3 lines like:
>      printk(KERN_INFO "NUMA: Node %u"...);
>      if (numa_fw_nid_name)
>          printk(KERN_INFO " %s %u"...);
>      printk(KERN_INFO "[%"PRIpaddr"%"PRIpaddr"]%s\n"...);
> 
> Or another option, we can force each NUMA implementation to assign a
> string for numa_fw_nid_name. For example, in DT NUMA, we can assign
> numa_fw_nid_name="SOCKET".

... we can assume numa_fw_nid_name to always be non-NULL (is
there's any way to reach this piece of code). I have no insight whether
"socket" is a correct term to use in the DT case; that would need to be
confirmed by an Arm person.

Jan