RE: [PATCH v4 09/17] xen/arm: introduce a helper to parse device tree NUMA distance map

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Subject: Re: [PATCH v4 09/17] xen/arm: introduce a helper to parse device
> tree NUMA distance map
> 
> > +        unsigned int from, to, distance, opposite;
> 
> With these ...
> 
> > +        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#%"PRIu32"-
> >NODE#%"PRIu32":%"PRIu32"\n",
> 
> ... you don't mean PRIu32 here and ...
> 
> > +                   from, to, distance);
> > +            goto invalid_data;
> > +        }
> > +
> > +        printk(XENLOG_INFO "NUMA: distance: NODE#%"PRIu32"-
> >NODE#%"PRIu32":%"PRIu32"\n",
> 
> ... here and yet further down anymore. That'll at the same time shorten
> all these lines quite a bit.

I did a little bit archeology to check the discussion back to 2 years ago when
Wei originally sent the v1 to the mailing list. Changing %u to %"PRIu32" was
one of the comment at that time [1] when these variables were defined as
uint32_t. But now with these variables being unsigned int I think now %u is
a more proper way here. I will do the according changes in v5.

> 
> > +               from, to, distance);
> > +
> > +        /* Get opposite way distance */
> > +        opposite = __node_distance(to, from);
> > +        /* The default value in node_distance_map is NUMA_NO_DISTANCE
> */
> > +        if ( opposite == NUMA_NO_DISTANCE )
> 
> And the matrix you're reading from can't hold NUMA_NO_DISTANCE entries?
> I ask because you don't check this above; you only check against
> NUMA_LOCAL_DISTANCE.

My understanding for the purpose of this part of code is to check if the 
opposite
way distance has already been set, so we need to compare the opposite way
distance with the default value NUMA_NO_DISTANCE here.

Back to your question, I can see your point of the question. However I don't 
think
NUMA_NO_DISTANCE is a valid value to describe the node distance in the device
tree. This is because I hunted down the previous discussions and found [2] about
we should try to keep consistent between the value used in device tree and ACPI
tables. From the ACPI spec, 0xFF, i.e. NUMA_NO_DISTANCE means unreachable.
I think this is also the reason why NUMA_NO_DISTANCE can be used as the default
value of the distance map, otherwise we won't have any value to use.

> 
> > +        {
> > +            /* 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#%"PRIu32"-
> >NODE#%"PRIu32":%"PRIu32", NODE#%"PRIu32"-
> >NODE#%"PRIu32":%"PRIu32"\n",
> > +                       from, to, distance, to, from, opposite);
> > +            }
> > +
> > +            /* Opposite way distance has been set, just set this way */
> > +            numa_set_distance(from, to, distance);
> 
> It took me a while to understand what the comment is to tell me,
> because in this iteration the opposite entry wasn't set. May I
> suggest to make more explicit that you refer to an earlier iteration,
> e.g. by "... was set before, ..."?

Yes, thanks for pointing this out. I will make the comment more explicit
as you suggested.

> 
> > +        }
> > +    }
> > +
> > +    return 0;
> > +
> > +invalid_data:
> 
> Nit: Style (labels to be indented by [at least] one blank).

Sure. Will add a space before.

[1] https://lists.xenproject.org/archives/html/xen-devel/2021-09/msg02066.html
[2] https://lists.xenproject.org/archives/html/xen-devel/2023-01/msg00690.html

Kind regards,
Henry

> 
> Jan