Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeul...@suse.com> > Subject: Re: [PATCH v3 03/17] xen/arm: implement node distance helpers for > Arm > > As both x86 and Arm have implemented __node_distance, so we move > > its definition from asm/numa.h to xen/numa.h. > > Nit: You mean "declaration", not "definition".
Correct, I overlooked this miswording in commit message while going through your comments in v2. will correct in v4. > > > At same time, the > > outdated u8 return value of x86 has been changed to unsigned char. > > > > Signed-off-by: Wei Chen <wei.c...@arm.com> > > Signed-off-by: Henry Wang <henry.w...@arm.com> > > non-Arm parts; to more it's not applicable anyway: > Acked-by: Jan Beulich <jbeul...@suse.com> I will add # non-Arm parts in the end of the tag and take the tag. > > > --- a/xen/arch/arm/numa.c > > +++ b/xen/arch/arm/numa.c > > @@ -28,6 +28,11 @@ enum dt_numa_status { > > > > static enum dt_numa_status __ro_after_init device_tree_numa = > DT_NUMA_DEFAULT; > > > > +static unsigned char __ro_after_init > > +node_distance_map[MAX_NUMNODES][MAX_NUMNODES] = { > > + { 0 } > > +}; > > Nit: There's no (incomplete or complete) initializer needed here, if > all you're after is having all slots set to zero. Yeah, I agree with you, so I will drop the initializer in v4, however... > > However, looking at the code below, don't you mean to have the array > pre-set to all NUMA_NO_DISTANCE? ...I am a bit puzzled about why pre-setting the array to all NUMA_NO_DISTANCE matters here, as I think the node distance map will be populated when parsing the device tree anyway no matter what their initial values. > > > + /* NUMA defines 0xff as an unreachable node and 0-9 are undefined */ > > + if ( distance >= NUMA_NO_DISTANCE || > > + (distance >= NUMA_DISTANCE_UDF_MIN && > > Compilers may warn about comparison of "unsigned int" to be >= 0. I'm > not sure about the usefulness of the NUMA_DISTANCE_UDF_MIN define in > the first place, so maybe best drop it and its use here? Yeah, will do that in v4. > > > +unsigned char __node_distance(nodeid_t from, nodeid_t to) > > +{ > > + /* When NUMA is off, any distance will be treated as remote. */ > > + if ( numa_disabled() ) > > + return NUMA_REMOTE_DISTANCE; > > Wouldn't it make sense to have the "from == to" special case ahead of > this (rather than further down), thus yielding a sensible result for > from == to == 0? And else return NUMA_NO_DISTANCE, thus having a > sensible result also for any from/to != 0? Could you please elaborate a bit more about why 0 matters here? As from my understanding, (1) If from == to, then we set the distance to NUMA_LOCAL_DISTANCE which represents the diagonal of the matrix. (2) If from and to is in the matrix range, then we return node_distance_map[from][to]. (3) Other cases we return NUMA_NO_DISTANCE. So this diff is enough: diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c @@ -98,6 +98,9 @@ void numa_detect_cpu_node(unsigned int cpu) unsigned char __node_distance(nodeid_t from, nodeid_t to) { + if ( from == to ) + return NUMA_LOCAL_DISTANCE; + /* When NUMA is off, any distance will be treated as remote. */ if ( numa_disabled() ) return NUMA_REMOTE_DISTANCE; @@ -109,7 +112,7 @@ unsigned char __node_distance(nodeid_t from, nodeid_t to) */ if ( from >= ARRAY_SIZE(node_distance_map) || to >= ARRAY_SIZE(node_distance_map[0]) ) - return from == to ? NUMA_LOCAL_DISTANCE : NUMA_NO_DISTANCE; + return NUMA_NO_DISTANCE; return node_distance_map[from][to]; } Would you mind pointing out why my understanding is wrong? Thanks! > > > + /* > > + * Check whether the nodes are in the matrix range. > > + * When any node is out of range, except from and to nodes are the > > + * same, we treat them as unreachable (return 0xFF) > > + */ > > + if ( from >= ARRAY_SIZE(node_distance_map) || > > + to >= ARRAY_SIZE(node_distance_map[0]) ) > > + return from == to ? NUMA_LOCAL_DISTANCE : NUMA_NO_DISTANCE; > > + > > + return node_distance_map[from][to]; > > +} > > + > > +EXPORT_SYMBOL(__node_distance); > > What is this needed for? Will drop it. Kind regards, Henry > > Jan