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

From: Wei Chen <Wei.Chen@arm.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: nd <nd@arm.com>, "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Julien Grall" <julien@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v5 5/6] xen/x86: move NUMA scan nodes codes from x86 to common
Date: Mon, 10 Oct 2022 15:03:53 +0800	[thread overview]
Message-ID: <e28f8faf-89fe-1e8f-4b8c-77097c2f2a90@arm.com> (raw)
In-Reply-To: <PAXPR08MB7420DE5F342F17E77F5483879E219@PAXPR08MB7420.eurprd08.prod.outlook.com>

Hi Jan,

On 2022/10/9 15:25, Wei Chen wrote:
> Hi Jan,
> >
>>>> 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:

"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".

Cheers,
Wei Chen

>> which for ACPI would be set to "PXM" (eliminating the need to pass
>> it to certain functions, albeit the fw_nid will continue to need to
>> be passed anyway). I guess I'm not really certain which of this and
>> your earlier approach I prefer; the boolean you suggest above looks
>> less desirable to me, though.
>>
> 
> Ok, I will follow your suggestion.
> 
> Cheers,
> Wei Chen
> 
>> Jan