| Message ID | 20210923120236.3692135-32-wei.chen@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add device tree based NUMA support to Arm | expand |
On Thu, 23 Sep 2021, Wei Chen wrote: > A NUMA aware device tree will provide a "distance-map" node to > describe distance between any two nodes. This patch introduce a > new helper to parse this distance map. > > Signed-off-by: Wei Chen <wei.chen@arm.com> > --- > xen/arch/arm/numa_device_tree.c | 106 ++++++++++++++++++++++++++++++++ > 1 file changed, 106 insertions(+) > > diff --git a/xen/arch/arm/numa_device_tree.c b/xen/arch/arm/numa_device_tree.c > index 7918a397fa..e7fa84df4c 100644 > --- a/xen/arch/arm/numa_device_tree.c > +++ b/xen/arch/arm/numa_device_tree.c > @@ -136,3 +136,109 @@ static int __init fdt_parse_numa_memory_node(const void *fdt, int node, > > return 0; > } > + > + > +/* Parse NUMA distance map v1 */ > +static int __init fdt_parse_numa_distance_map_v1(const void *fdt, int node) > +{ > + const struct fdt_property *prop; > + const __be32 *matrix; > + uint32_t entry_count; > + int 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 > + "NUMA: No distance-matrix property in distance-map\n"); I haven't seen where this is called from yet but make sure to print an error here only if NUMA info is actually expected and required, not on regular non-NUMA boots on non-NUMA machines. > + return -EINVAL; > + } > + > + 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 ) > + { > + uint32_t from, to, distance, opposite; > + > + from = dt_next_cell(1, &matrix); > + to = dt_next_cell(1, &matrix); > + distance = dt_next_cell(1, &matrix); > + if ( (from == to && distance != NUMA_LOCAL_DISTANCE) || > + (from != to && distance <= NUMA_LOCAL_DISTANCE) ) > + { > + printk(XENLOG_WARNING > + "NUMA: Invalid distance: NODE#%u->NODE#%u:%u\n", > + from, to, distance); > + return -EINVAL; > + } > + > + printk(XENLOG_INFO "NUMA: distance: NODE#%u->NODE#%u:%u\n", > + from, to, distance); > + > + /* Get opposite way distance */ > + opposite = __node_distance(from, to); This is not checking for the opposite node distance but... > + if ( opposite == 0 ) > + { > + /* Bi-directions are not set, set both */ > + numa_set_distance(from, to, distance); > + numa_set_distance(to, from, distance); ...since you set both directions here at once then it is OK. You are checking if this direction has already been set which is correct I think. But the comment "Get opposite way distance" and the variable name "opposite" are wrong. > + } > + else > + { > + /* > + * Opposite way distance has been set to a different value. > + * It may be a firmware device tree bug? > + */ > + if ( opposite != distance ) > + { > + /* > + * In device tree NUMA distance-matrix binding: > + * https://www.kernel.org/doc/Documentation/devicetree/bindings/numa.txt > + * There is a notes mentions: > + * "Each entry represents distance from first node to > + * second node. The distances are equal in either > + * direction." > + * > + * That means device tree doesn't permit this case. > + * But in ACPI spec, it cares to specifically permit this > + * case: > + * "Except for the relative distance from a System Locality > + * to itself, each relative distance is stored twice in the > + * matrix. This provides the capability to describe the > + * scenario where the relative distances for the two > + * directions between System Localities is different." > + * > + * That means a real machine allows such NUMA configuration. > + * So, place a WARNING here to notice system administrators, > + * is it the specail case that they hijack the device tree > + * to support their rare machines? > + */ > + printk(XENLOG_WARNING > + "Un-matched bi-direction! NODE#%u->NODE#%u:%u, NODE#%u->NODE#%u:%u\n", > + from, to, distance, to, from, opposite); PRIu32 > + } > + > + /* Opposite way distance has been set, just set this way */ > + numa_set_distance(from, to, distance); > + } > + } > + > + return 0; > +} > -- > 2.25.1 >
Hi Stefano, > -----Original Message----- > From: Stefano Stabellini <sstabellini@kernel.org> > Sent: 2021年9月24日 11:05 > To: Wei Chen <Wei.Chen@arm.com> > Cc: xen-devel@lists.xenproject.org; sstabellini@kernel.org; julien@xen.org; > Bertrand Marquis <Bertrand.Marquis@arm.com> > Subject: Re: [PATCH 31/37] xen/arm: introduce a helper to parse device > tree NUMA distance map > > On Thu, 23 Sep 2021, Wei Chen wrote: > > A NUMA aware device tree will provide a "distance-map" node to > > describe distance between any two nodes. This patch introduce a > > new helper to parse this distance map. > > > > Signed-off-by: Wei Chen <wei.chen@arm.com> > > --- > > xen/arch/arm/numa_device_tree.c | 106 ++++++++++++++++++++++++++++++++ > > 1 file changed, 106 insertions(+) > > > > diff --git a/xen/arch/arm/numa_device_tree.c > b/xen/arch/arm/numa_device_tree.c > > index 7918a397fa..e7fa84df4c 100644 > > --- a/xen/arch/arm/numa_device_tree.c > > +++ b/xen/arch/arm/numa_device_tree.c > > @@ -136,3 +136,109 @@ static int __init fdt_parse_numa_memory_node(const > void *fdt, int node, > > > > return 0; > > } > > + > > + > > +/* Parse NUMA distance map v1 */ > > +static int __init fdt_parse_numa_distance_map_v1(const void *fdt, int > node) > > +{ > > + const struct fdt_property *prop; > > + const __be32 *matrix; > > + uint32_t entry_count; > > + int 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 > > + "NUMA: No distance-matrix property in distance-map\n"); > > I haven't seen where this is called from yet but make sure to print an > error here only if NUMA info is actually expected and required, not on > regular non-NUMA boots on non-NUMA machines. > Only users enable NUMA option and numa_off is false, then Xen can run into this function (this check is in numa_init). So non-NUMA machines will not reach here. > > > + return -EINVAL; > > + } > > + > > + 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 ) > > + { > > + uint32_t from, to, distance, opposite; > > + > > + from = dt_next_cell(1, &matrix); > > + to = dt_next_cell(1, &matrix); > > + distance = dt_next_cell(1, &matrix); > > + if ( (from == to && distance != NUMA_LOCAL_DISTANCE) || > > + (from != to && distance <= NUMA_LOCAL_DISTANCE) ) > > + { > > + printk(XENLOG_WARNING > > + "NUMA: Invalid distance: NODE#%u->NODE#%u:%u\n", > > + from, to, distance); > > + return -EINVAL; > > + } > > + > > + printk(XENLOG_INFO "NUMA: distance: NODE#%u->NODE#%u:%u\n", > > + from, to, distance); > > + > > + /* Get opposite way distance */ > > + opposite = __node_distance(from, to); > > This is not checking for the opposite node distance but... > Ah, yes, it's a mistake. It should be __node_distance(to, from); > > > + if ( opposite == 0 ) > > + { > > + /* Bi-directions are not set, set both */ > > + numa_set_distance(from, to, distance); > > + numa_set_distance(to, from, distance); > > ...since you set both directions here at once then it is OK. You are > checking if this direction has already been set which is correct I > think. But the comment "Get opposite way distance" and the variable name > "opposite" are wrong. > My above mistake make this mis-understanding: I want to check the opposite way distance is set or not. If opposite way distance is not set, I will set both way here. So I will change " opposite = __node_distance(from, to);" to " opposite = __node_distance(to, from);". And keep the comment. How do you think about it? > > > + } > > + else > > + { > > + /* > > + * Opposite way distance has been set to a different value. > > + * It may be a firmware device tree bug? > > + */ > > + if ( opposite != distance ) > > + { > > + /* > > + * In device tree NUMA distance-matrix binding: > > + * > https://www.kernel.org/doc/Documentation/devicetree/bindings/numa.txt > > + * There is a notes mentions: > > + * "Each entry represents distance from first node to > > + * second node. The distances are equal in either > > + * direction." > > + * > > + * That means device tree doesn't permit this case. > > + * But in ACPI spec, it cares to specifically permit > this > > + * case: > > + * "Except for the relative distance from a System > Locality > > + * to itself, each relative distance is stored twice > in the > > + * matrix. This provides the capability to describe > the > > + * scenario where the relative distances for the two > > + * directions between System Localities is different." > > + * > > + * That means a real machine allows such NUMA > configuration. > > + * So, place a WARNING here to notice system > administrators, > > + * is it the specail case that they hijack the device > tree > > + * to support their rare machines? > > + */ > > + printk(XENLOG_WARNING > > + "Un-matched bi-direction! NODE#%u->NODE#%u:%u, > NODE#%u->NODE#%u:%u\n", > > + from, to, distance, to, from, opposite); > > PRIu32 Yes. > > > > + } > > + > > + /* Opposite way distance has been set, just set this way */ > > + numa_set_distance(from, to, distance); > > + } > > + } > > + > > + return 0; > > +} > > -- > > 2.25.1 > >
diff --git a/xen/arch/arm/numa_device_tree.c b/xen/arch/arm/numa_device_tree.c index 7918a397fa..e7fa84df4c 100644 --- a/xen/arch/arm/numa_device_tree.c +++ b/xen/arch/arm/numa_device_tree.c @@ -136,3 +136,109 @@ static int __init fdt_parse_numa_memory_node(const void *fdt, int node, return 0; } + + +/* Parse NUMA distance map v1 */ +static int __init fdt_parse_numa_distance_map_v1(const void *fdt, int node) +{ + const struct fdt_property *prop; + const __be32 *matrix; + uint32_t entry_count; + int 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 + "NUMA: No distance-matrix property in distance-map\n"); + + return -EINVAL; + } + + 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 ) + { + uint32_t from, to, distance, opposite; + + from = dt_next_cell(1, &matrix); + to = dt_next_cell(1, &matrix); + distance = dt_next_cell(1, &matrix); + if ( (from == to && distance != NUMA_LOCAL_DISTANCE) || + (from != to && distance <= NUMA_LOCAL_DISTANCE) ) + { + printk(XENLOG_WARNING + "NUMA: Invalid distance: NODE#%u->NODE#%u:%u\n", + from, to, distance); + return -EINVAL; + } + + printk(XENLOG_INFO "NUMA: distance: NODE#%u->NODE#%u:%u\n", + from, to, distance); + + /* Get opposite way distance */ + opposite = __node_distance(from, to); + if ( opposite == 0 ) + { + /* Bi-directions are not set, set both */ + numa_set_distance(from, to, distance); + numa_set_distance(to, from, distance); + } + else + { + /* + * Opposite way distance has been set to a different value. + * It may be a firmware device tree bug? + */ + if ( opposite != distance ) + { + /* + * In device tree NUMA distance-matrix binding: + * https://www.kernel.org/doc/Documentation/devicetree/bindings/numa.txt + * There is a notes mentions: + * "Each entry represents distance from first node to + * second node. The distances are equal in either + * direction." + * + * That means device tree doesn't permit this case. + * But in ACPI spec, it cares to specifically permit this + * case: + * "Except for the relative distance from a System Locality + * to itself, each relative distance is stored twice in the + * matrix. This provides the capability to describe the + * scenario where the relative distances for the two + * directions between System Localities is different." + * + * That means a real machine allows such NUMA configuration. + * So, place a WARNING here to notice system administrators, + * is it the specail case that they hijack the device tree + * to support their rare machines? + */ + printk(XENLOG_WARNING + "Un-matched bi-direction! NODE#%u->NODE#%u:%u, NODE#%u->NODE#%u:%u\n", + from, to, distance, to, from, opposite); + } + + /* Opposite way distance has been set, just set this way */ + numa_set_distance(from, to, distance); + } + } + + return 0; +}
A NUMA aware device tree will provide a "distance-map" node to describe distance between any two nodes. This patch introduce a new helper to parse this distance map. Signed-off-by: Wei Chen <wei.chen@arm.com> --- xen/arch/arm/numa_device_tree.c | 106 ++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+)