[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Vijay,

On 20/07/17 09:55, Vijay Kilari wrote:
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().

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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.