Hi Jan,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Subject: Re: [PATCH v4 03/17] xen/arm: implement node distance helpers for
> Arm
>
> On 25.04.2023 09:56, Henry Wang wrote:
> > --- a/xen/arch/arm/numa.c
> > +++ b/xen/arch/arm/numa.c
> > @@ -28,6 +28,12 @@ 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 ... MAX_NUMNODES - 1] = { [0 ... MAX_NUMNODES - 1] =
> NUMA_NO_DISTANCE }
> > +};
> > +
> > +
>
> Nit: A stray 2nd blank line has appeared here.
Will fix this in v5.
>
> > + /* NUMA defines NUMA_NO_DISTANCE as unreachable and 0-9 are
> undefined */
> > + if ( distance >= NUMA_NO_DISTANCE || distance <=
> NUMA_DISTANCE_UDF_MAX ||
> > + (from == to && distance != NUMA_LOCAL_DISTANCE) )
> > + {
> > + printk(KERN_WARNING
> > + "NUMA: invalid distance: from=%"PRIu8" to=%"PRIu8"
> distance=%"PRIu32"\n",
> > + from, to, distance);
> > + return;
> > + }
>
> I appreciate the checking that node-local references are
> NUMA_LOCAL_DISTANCE,
> but if they're wrongly passed into here, shouldn't the resulting array still
> have NUMA_LOCAL_DISTANCE on its diagonal, at least as far as present nodes
> go?
Apologies in advance to ask more specific details from you as I am not sure
if I can correctly understand the "if they're wrongly passed into here" case.
Do you
mean we are always guaranteed that if from == to, the distance will always be
NUMA_LOCAL_DISTANCE so the (from == to && distance != NUMA_LOCAL_DISTANCE)
check is redundant and can be removed? Thanks.
>
> > + node_distance_map[from][to] = distance;
> > +}
[...]
> > + /*
> > + * 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.
>
> I think this "except ..." part is slightly confusing, as it doesn't comment
> the subsequent code, but instead refers to the first check in the function.
> If you want to keep it, may I suggest to add something like "(see above)"
> before the comma?
Sure, I will add "(see above)" in v5.
Kind regards,
Henry
>
> Jan
