RE: [PATCH v4 03/17] xen/arm: implement node distance helpers for Arm

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> 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