Re: [RFC PATCH v3 08/24] NUMA: x86: Move numa code and make it generic

From: Julien Grall <julien.grall@arm.com>
To: Vijay Kilari <vijay.kilari@gmail.com>
Cc: Tim Deegan <tim@xen.org>,
	kevin.tian@intel.com, Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Dario Faggioli <dario.faggioli@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
	Jan Beulich <jbeulich@suse.com>,
	Vijaya Kumar K <Vijaya.Kumar@cavium.com>
Subject: Re: [RFC PATCH v3 08/24] NUMA: x86: Move numa code and make it generic
Date: Thu, 20 Jul 2017 12:14:53 +0100	[thread overview]
Message-ID: <e5b65719-883f-b61e-7937-fc2b89c73f23@arm.com> (raw)
In-Reply-To: <CALicx6vd_dVdFSGiyOp+v8_5=u=p+V1bAOdkoKy_Xds40Wkv4Q@mail.gmail.com>

Hi Vijay,

On 20/07/17 09:55, Vijay Kilari wrote:
> On Wed, Jul 19, 2017 at 11:11 PM, Julien Grall <julien.grall@arm.com> wrote:
>> Hi Vijay,
>>
>> On 18/07/17 12:41, vijay.kilari@gmail.com wrote:
>>>
>>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>>
>>> Move code from xen/arch/x86/numa.c to xen/common/numa.c
>>> so that it can be used by other archs.
>>>
>>> The following changes are done:
>>> - Few generic static functions in x86/numa.c is made
>>>   non-static common/numa.c
>>> - The generic contents of header file asm-x86/numa.h
>>>   are moved to xen/numa.h.
>>> - The header file includes are reordered and externs are
>>>   dropped.
>>> - Moved acpi_numa from asm-x86/acpi.h to xen/acpi.h
>>> - Coding style of code moved to commom/numa.c is changed
>>>   to Xen style.
>>> - numa_add_cpu() and numa_set_node() and moved to header
>>>   file and added inline function in case of CONFIG_NUMA
>>>   is not enabled because these functions are called from
>>>   generic code with out any config check.
>>>
>>> Also the node_online_map is defined in x86/numa.c for x86
>>> and arm/smpboot.c for ARM. For x86 it is moved to x86/smpboot.c
>>> If moved to common code the compilation fails because
>>> common/numa.c is compiled only when NUMA is enabled.
>>
>>
>> I would much prefer if this patch does one thing: Moving code. The rest
>> should be split out to help review and allowing us to easily verify you only
>> moved code...
>
> Yes, this patch is doing only code movement. Apart from adding inline function
> for numa_add_cpu() and numa_set_node().

The "apart" should then be in a separate patch. I don't want to spend 
hours trying to decipher a patch mixing code movement and add code at 
the same time.

>
>>
>>> +#define NODE_DATA(nid)          (&(node_data[nid]))
>>> +
>>> +#define node_start_pfn(nid)     NODE_DATA(nid)->node_start_pfn
>>> +#define node_spanned_pages(nid) NODE_DATA(nid)->node_spanned_pages
>>> +#define node_end_pfn(nid)       NODE_DATA(nid)->node_start_pfn + \
>>> +                                 NODE_DATA(nid)->node_spanned_pages
>>> +
>>> +void numa_add_cpu(int cpu);
>>> +void numa_set_node(int cpu, nodeid_t node);
>>> +#else
>>> +static inline void numa_add_cpu(int cpu) { }
>>> +static inline void numa_set_node(int cpu, nodeid_t node) { }
>>
>>
>> I am not sure why you need to define stub at least for numa_set_node... I
>> can't see use in non-NUMA code. I will comment about the numa_add_cpu later.
>
> x86 is using from setup.c. yes if we assume that numa is always enabled for x86,
> I can drop numa_set_node() inline function.

Looking at the code, I don't think there is any way to disable NUMA on 
x86 at the moment... So there is no point to keep it.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel