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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Wed, Jul 19, 2017 at 11:11 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Vijay,
>
> On 18/07/17 12:41, vijay.kilari@xxxxxxxxx wrote:
>>
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>>
>> 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().

>
>> +#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.

>
> Cheers,
>
> --
> Julien Grall

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