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

Re: [Xen-devel] [RFC PATCH v3 15/24] ARM: NUMA: DT: Add CPU NUMA support

On 25/07/17 07:47, Vijay Kilari wrote:
 void numa_failed(void)
     numa_off = true;
     node_distance_fn = NULL;
+    init_cpu_to_node();
+void __init numa_set_cpu_node(int cpu, unsigned int nid)
+    if ( !node_isset(nid, processor_nodes_parsed) || nid >= MAX_NUMNODES
+        nid = 0;

This looks wrong to me. If the node-id is invalid, why would you blindly set
to 0?

Generally this check will not pass. I will make this function return
error code in case
of wrong nid.

I don't really want to see error code and error handling everywhere in the initialization code. I would assume that if the NUMA bindings are wrong we should just crash Xen rather continuing with NUMA disabled.

Stefano do you have any opinion here?


 #include <asm/cpuerrata.h>
 #include <asm/gic.h>
 #include <asm/psci.h>
@@ -106,6 +107,7 @@ static void __init dt_smp_init_cpus(void)
         [0 ... NR_CPUS - 1] = MPIDR_INVALID
     bool_t bootcpu_valid = 0;
+    nodeid_t *cpu_to_nodemap;
     int rc;

     mpidr = boot_cpu_data.mpidr.bits & MPIDR_HWID_MASK;
@@ -117,11 +119,18 @@ static void __init dt_smp_init_cpus(void)

+    cpu_to_nodemap = xzalloc_array(nodeid_t, NR_CPUS);

Why do you need to allocate cpu_to_nodemap? Would not it be easier to put it
on the stack as we do for other variable?

This array holds nodemap indexed by cpuid once for all the cpus.
Later while setting the logical cpu id mapping, the node mapping is set
by calling numa_set_cpu_node().

This does not answer question... Please read it again and explain why you can't do:

nodeid_t cpu_to_nodemap[NR_CPUS];

diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
index d1dc83a..0d3146c 100644
--- a/xen/include/asm-arm/numa.h
+++ b/xen/include/asm-arm/numa.h
@@ -10,12 +10,19 @@ void init_dt_numa_distance(void);
 void numa_init(void);
 int dt_numa_init(void);
+void numa_set_cpu_node(int cpu, unsigned int nid);
 static inline void numa_init(void)

+static inline void numa_set_cpu_node(int cpu, unsigned int nid)
+    return;
 /* Fake one node for now. See also node_online_map. */
 #define cpu_to_node(cpu) 0
 #define node_to_cpumask(node)   (cpu_online_map)
diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
index ca0a2a6..fc4747f 100644
--- a/xen/include/asm-x86/numa.h
+++ b/xen/include/asm-x86/numa.h
@@ -15,7 +15,6 @@ extern nodeid_t acpi_setup_node(unsigned int pxm);
 extern void srat_detect_node(int cpu);

 extern nodeid_t apicid_to_node[];
-extern void init_cpu_to_node(void);

 void srat_parse_regions(paddr_t addr);
 unsigned int arch_get_dma_bitsize(void);
diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
index 10ef4c4..8a306e7 100644
--- a/xen/include/xen/numa.h
+++ b/xen/include/xen/numa.h
@@ -30,6 +30,7 @@ extern s8 acpi_numa;
 void numa_initmem_init(unsigned long start_pfn, unsigned long end_pfn);
 int srat_disabled(void);
 int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node);
+void init_cpu_to_node(void);

You never used this function in common code. So why did you move it in the
common headers?

Same was defined for x86 as well. So I have moved to common header file.

You should make common only functions that will be called from code common or are part of common code.

In this particular case, the name might be the same but the behavior is completely different and the way to use it too...


Julien Grall

Xen-devel mailing list



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