[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [RFC PATCH v3 14/24] ARM: NUMA: DT: Parse NUMA distance information
- To: vijay.kilari@xxxxxxxxx, xen-devel@xxxxxxxxxxxxx
- From: Julien Grall <julien.grall@xxxxxxx>
- Date: Thu, 20 Jul 2017 14:02:27 +0100
- Cc: kevin.tian@xxxxxxxxx, sstabellini@xxxxxxxxxx, wei.liu2@xxxxxxxxxx, George.Dunlap@xxxxxxxxxxxxx, andrew.cooper3@xxxxxxxxxx, dario.faggioli@xxxxxxxxxx, ian.jackson@xxxxxxxxxxxxx, tim@xxxxxxx, jbeulich@xxxxxxxx, Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
- Delivery-date: Thu, 20 Jul 2017 13:02:40 +0000
- List-id: Xen developer discussion <xen-devel.lists.xen.org>
Hi Vijay,
On 18/07/17 12:41, vijay.kilari@xxxxxxxxx wrote:
From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
Parse distance-matrix and fetch node distance information.
Store distance information in node_distance[].
Register dt_node_distance() function pointer with
the ARM numa code. This approach can be later used for
ACPI.
After my comment on v1, I was expecting to see a link to the binding in
the commit message...
Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
---
v3: - Moved __node_distance() declaration to common
header file
- Use device_tree_node_compatible() instead of
device_tree_node_matches()
- Dropped xen/errno.h inclusion
---
xen/arch/arm/numa/dt_numa.c | 131 ++++++++++++++++++++++++++++++++++++++++++++
xen/arch/arm/numa/numa.c | 22 ++++++++
xen/include/asm-arm/numa.h | 2 +
xen/include/asm-x86/numa.h | 1 -
xen/include/xen/numa.h | 3 +
5 files changed, 158 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/numa/dt_numa.c b/xen/arch/arm/numa/dt_numa.c
index 84030e7..46c0346 100644
--- a/xen/arch/arm/numa/dt_numa.c
+++ b/xen/arch/arm/numa/dt_numa.c
@@ -23,6 +23,48 @@
#include <xen/numa.h>
#include <asm/setup.h>
+static uint8_t node_distance[MAX_NUMNODES][MAX_NUMNODES];
On v1, you said that you will look at allocating node_distance on the
fly. So why it is not done?
You can give a look at alloc_boot_pages(...).
+
+static uint8_t dt_node_distance(nodeid_t nodea, nodeid_t nodeb)
+{
+ if ( nodea >= MAX_NUMNODES || nodeb >= MAX_NUMNODES )
+ return nodea == nodeb ? LOCAL_DISTANCE : REMOTE_DISTANCE;
Do we really expect dt_node_distance to be called with wrong node?
Looking at the ACPI code, they don't check that... So likely this should
be an ASSERT(...).
+
+ return node_distance[nodea][nodeb];
+}
+
+static int dt_numa_set_distance(uint32_t nodea, uint32_t nodeb,
I think this should be __init.
+ uint32_t distance)
+{
+ /* node_distance is uint8_t. Ensure distance is less than 255 */
+ if ( nodea >= MAX_NUMNODES || nodeb >= MAX_NUMNODES || distance > 255 )
+ return -EINVAL;
+
+ node_distance[nodea][nodeb] = distance;
+
+ return 0;
+}
+
+void init_dt_numa_distance(void)
Ditto.
+{
+ int i, j;
+
+ for ( i = 0; i < MAX_NUMNODES; i++ )
+ {
+ for ( j = 0; j < MAX_NUMNODES; j++ )
+ {
+ /*
+ * Initialize distance 10 for local distance and
+ * 20 for remote distance.
+ */
+ if ( i == j )
+ node_distance[i][j] = LOCAL_DISTANCE;
+ else
+ node_distance[i][j] = REMOTE_DISTANCE;
+ }
+ }
+}
+
/*
* Even though we connect cpus to numa domains later in SMP
* init, we need to know the node ids now for all cpus.
@@ -58,6 +100,76 @@ static int __init dt_numa_process_cpu_node(const void *fdt)
return 0;
}
+static int __init dt_numa_parse_distance_map(const void *fdt, int node,
+ const char *name,
+ uint32_t address_cells,
+ uint32_t size_cells)
+{
+ const struct fdt_property *prop;
+ const __be32 *matrix;
+ int entry_count, len, i;
+
+ printk(XENLOG_INFO "NUMA: parsing numa-distance-map\n");
+
+ prop = fdt_get_property(fdt, node, "distance-matrix", &len);
+ if ( !prop )
+ {
+ printk(XENLOG_WARNING
s/XENLOG_WARNING/XENLOG_INFO/ because numa-distance-map is not mandatory.
+ "NUMA: No distance-matrix property in distance-map\n");
+
+ return -EINVAL;
If I am reading correctly the binding, the distance-matrix is not
mandatory. If it is not present, you should use a default matrix. But
here you will disable NUMA completely.
+ }
+
+ if ( len % sizeof(uint32_t) != 0 )
+ {
+ printk(XENLOG_WARNING
+ "distance-matrix in node is not a multiple of u32\n");
+
+ return -EINVAL;
+ }
+
+ entry_count = len / sizeof(uint32_t);
+ if ( entry_count <= 0 )
+ {
+ printk(XENLOG_WARNING "NUMA: Invalid distance-matrix\n");
+
+ return -EINVAL;
+ }
+
+ matrix = (const __be32 *)prop->data;
+ for ( i = 0; i + 2 < entry_count; i += 3 )
It would be easier to read if entry_count is the number of triplet. E.g
entry_count = (len / sizeof(uint32_t)) / 3;
for ( i = 0; i < entry_count; i++ )
+ {
+ uint32_t nodea, nodeb, distance;
+
+ nodea = dt_read_number(matrix, 1);
+ matrix++;
nodea = dt_next_cell(1, &matrix) will do the increment for you.
+ nodeb = dt_read_number(matrix, 1);
+ matrix++;
Ditto.
+ distance = dt_read_number(matrix, 1);
+ matrix++;
Ditto.
+
+ if ( dt_numa_set_distance(nodea, nodeb, distance) )
+ {
+ printk(XENLOG_WARNING
+ "NUMA: node-id out of range in distance matrix for [node%d ->
node%d]\n",
s/%d/%u/
+ nodea, nodeb);
+ return -EINVAL;
+
+ }
+ printk(XENLOG_INFO "NUMA: distance[node%d -> node%d] = %d\n",
+ nodea, nodeb, distance);
+
+ /*
+ * Set default distance of node B->A same as A->B.
+ * No need to check for return value of numa_set_distance.
+ */
+ if ( nodeb > nodea )
Mind explaining this if in the comment?
+ dt_numa_set_distance(nodeb, nodea, distance);
+ }
+
+ return 0;
+}
+
void __init dt_numa_process_memory_node(uint32_t nid, paddr_t start,
paddr_t size)
{
@@ -90,11 +202,30 @@ void __init dt_numa_process_memory_node(uint32_t nid,
paddr_t start,
return;
}
+static int __init dt_numa_scan_distance_node(const void *fdt, int node,
+ const char *name, int depth,
+ uint32_t address_cells,
+ uint32_t size_cells, void *data)
+{
+ if ( device_tree_node_compatible(fdt, node, "numa-distance-map-v1") )
+ return dt_numa_parse_distance_map(fdt, node, name, address_cells,
+ size_cells);
+
+ return 0;
+}
+
int __init dt_numa_init(void)
{
int ret;
ret = dt_numa_process_cpu_node((void *)device_tree_flattened);
+ if ( ret )
+ return ret;
+
+ ret = device_tree_for_each_node((void *)device_tree_flattened,
Why do you need the cast?
+ dt_numa_scan_distance_node, NULL);
+ if ( !ret )
+ register_node_distance(&dt_node_distance);
return ret;
}
diff --git a/xen/arch/arm/numa/numa.c b/xen/arch/arm/numa/numa.c
index 8227361..c00b92c 100644
--- a/xen/arch/arm/numa/numa.c
+++ b/xen/arch/arm/numa/numa.c
@@ -18,10 +18,30 @@
#include <xen/ctype.h>
#include <xen/nodemask.h>
#include <xen/numa.h>
+#include <asm/acpi.h>
I don't understand why you include asm/acpi.h with no code using ACPI at
the moment...
+
+static uint8_t (*node_distance_fn)(nodeid_t a, nodeid_t b);
void numa_failed(void)
{
numa_off = true;
+ init_dt_numa_distance();
Why do you need to initialize init_dt_numa_distance when it has failed?
The array will never be used in that case.
+ node_distance_fn = NULL;
+}
+
+uint8_t __node_distance(nodeid_t a, nodeid_t b)
+{
+ if ( node_distance_fn != NULL);
+ return node_distance_fn(a, b);
+
+ return a == b ? LOCAL_DISTANCE : REMOTE_DISTANCE;
+}
+
+EXPORT_SYMBOL(__node_distance);
Please drop EXPORT_SYMBOL, this is not used by Xen and only here when
the code is imported from Linux.
+
+void register_node_distance(uint8_t (fn)(nodeid_t a, nodeid_t b))
+{
+ node_distance_fn = fn;
}
void __init numa_init(void)
@@ -29,6 +49,8 @@ void __init numa_init(void)
int ret = 0;
nodes_clear(processor_nodes_parsed);
+ init_dt_numa_distance();
This should go in dt_numa_init and would avoid to export it.
+
if ( numa_off )
goto no_numa;
diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
index 36cd782..d1dc83a 100644
--- a/xen/include/asm-arm/numa.h
+++ b/xen/include/asm-arm/numa.h
@@ -4,6 +4,8 @@
typedef uint8_t nodeid_t;
void dt_numa_process_memory_node(uint32_t nid, paddr_t start, paddr_t size);
+void register_node_distance(uint8_t (fn)(nodeid_t a, nodeid_t b));
+void init_dt_numa_distance(void);
#ifdef CONFIG_NUMA
void numa_init(void);
diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
index d8a0a44..ca0a2a6 100644
--- a/xen/include/asm-x86/numa.h
+++ b/xen/include/asm-x86/numa.h
@@ -18,7 +18,6 @@ extern nodeid_t apicid_to_node[];
extern void init_cpu_to_node(void);
void srat_parse_regions(paddr_t addr);
-extern uint8_t __node_distance(nodeid_t a, nodeid_t b);
unsigned int arch_get_dma_bitsize(void);
#endif
diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
index 110d5dc..10ef4c4 100644
--- a/xen/include/xen/numa.h
+++ b/xen/include/xen/numa.h
@@ -6,6 +6,8 @@
#include <asm/numa.h>
#define NUMA_NO_NODE 0xFF
+#define LOCAL_DISTANCE 10
+#define REMOTE_DISTANCE 20
I would add DEFAULT in each name. Probably LOCAL_DEFAULT_DISTANCE and
REMOVE_LOCAL_DISTANCE.
#define NUMA_NO_DISTANCE 0xFF
#define MAX_NUMNODES NR_NODES
@@ -70,6 +72,7 @@ int numa_add_memblk(nodeid_t nodeid, paddr_t start, uint64_t
size);
int get_num_node_memblks(void);
bool arch_sanitize_nodes_memory(void);
void numa_failed(void);
+uint8_t __node_distance(nodeid_t a, nodeid_t b);
#else
static inline void numa_add_cpu(int cpu) { }
static inline void numa_set_node(int cpu, nodeid_t node) { }
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
