RE: [PATCH v3 07/22] xen/device-tree: Read NUMA node distance from Device Tree 'distance-map'

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hello,

> >
> > +#ifdef CONFIG_NUMA
> > +    numa_distance_table_init();
> > +#endif /* CONFIG_NUMA */
> 
> Generally we prefer to avoid such #ifdef-ary in main source code by providing 
> stub
> (inline) functions in headers. Yet then I'm not an Arm maintainer ...

Okay, I will add stub inline functions.

> > +/*
> > + * Parse the '/distance-map' node from the flattened device tree
> > + * and extract the 3-tuple triplets <from, to, distance>.
> > + */
> > +static void __init dt_numa_parse_distance_map(void)
> > +{
> > +    const void *fdt = device_tree_flattened;
> > +    const struct fdt_property *prop;
> > +    const __be32 *matrix;
> > +    int entry_count;
> > +    int node;
> > +    int len;
> > +    int i;
> 
> Nit: Plain int only when values can actually go negative, or when not-yet-
> tidied-up code elsewhere (e.g. libfdt here) makes this necessary.

Okay, I will

> > +
> > +    matrix = (const __be32*)prop->data;
> 
> Nit: Blank before * please.

Ok.

> > +    entry_count = len / sizeof(__be32);
> 
> Nit: Better sizeof(<expression>).

Is the following line better?
entry_count = len / sizeof(*matrix);

> > +    if ( (entry_count <= 0) || (entry_count % 3) )
> > +        return;
> > +
> > +    for ( i = 0; i + 2 < entry_count; i += 3 )
> > +    {
> > +        uint32_t nodea, nodeb, distance;
> 
> Again, no apparent need for a fixed-width type here.

Okay, I will make them unsigned int.

> > +        nodea = dt_next_cell(1, &matrix);
> > +        nodeb = dt_next_cell(1, &matrix);
> > +        distance = dt_next_cell(1, &matrix);
> > +
> > +        if ( (nodea == nodeb && distance != LOCAL_DISTANCE) ||
> > +             (nodea != nodeb && distance <= LOCAL_DISTANCE) )
> > +        {
> > +            printk(XENLOG_WARNING "Invalid distance[node%d ->
> node%d] = %d\n",
> > +                   nodea, nodeb, distance);
> 
> Nit: %u please with unsigned quantities (applies, like all such comments,
> also elsewhere).

Okay.

> > +
> > +void __init dt_numa_distance_table_init(void)
> > +{
> > +    dt_numa_parse_distance_map();
> > +}
> 
> I assume there are going to be further additions to this function?

Yes. Currently, the parsing logic is specific to the "numa-distance-map-v1" 
compatible string. If "numa-distance-map-v2" is introduced in the future,
I will make this function to handle the branching logic for different versions.

> >  #include <xen/errno.h>
> >  #include <xen/init.h>
> >  #include <xen/nodemask.h>
> >  #include <xen/numa.h>
> > +#include <xen/acpi.h>
> > +
> >
> >  #define LOCAL_DISTANCE      10
> 
> Nit: No double blank lines please.

Okay.

> >  #define REMOTE_DISTANCE     20
> >
> > +uint8_t * __ro_after_init numa_distance;
> 
> Nit: Excess blank after *.

Okay.

> >  /*
> >   * Get the distance between node 'from' and node 'to'.
> >   */
> >  uint8_t numa_node_distance(unsigned int from, unsigned int to)
> >  {
> > -    if ( from != to )
> > -        return REMOTE_DISTANCE;
> > -    return LOCAL_DISTANCE;
> 
> Why did you introduce the function as a fallback when now you remove the
> fallback logic entirely? Can't you introduce the function right here,
> omitting the earlier patch?

I will remove the earlier patch.

> > +    const unsigned int nr_nodes = last_node(node_online_map) + 1U;
> > +
> > +    if ( from >= nr_nodes || to >= nr_nodes )
> > +        return from == to ? LOCAL_DISTANCE : REMOTE_DISTANCE;
> 
> What if either node is NUMA_NO_NODE?

This behavior comes from the Linux kernel. It seems it exists as a defensive
fallback to keep the system running even with invalid or unassigned nodes.

Do you think it is better to make it return 0xFF instead whenever any
out-of-bounds node or NUMA_NO_NODE is passed?

> > +    return numa_distance[from * nr_nodes + to];
> > +}
> > +
> > +void __init numa_set_distance(unsigned int from, unsigned int to,
> > +                                     unsigned int distance)
> 
> Nit: Indentation.

Okay.

> > +void __init numa_distance_table_init(void)
> > +{
> > +    const unsigned int nr_nodes = last_node(node_online_map) + 1U;
> > +    unsigned int i, j;
> > +
> > +    numa_distance = xzalloc_array(uint8_t, nr_nodes * nr_nodes);
> 
> xvzalloc*() family of functions in new code, please.
> 
> Further there's an at least abstract risk of the multiplication overflowing.
> See how xvmalloc_array() allows for multiple dimensions to be passed.

Thanks for the great advice.
I will use xvmalloc_array().

> > +    if ( !numa_distance )
> > +        panic("Failed to allocate memory for numa distance-map
> array\n");
> > +
> > +    /* fill with the default distances */
> 
> Nit: Comment style.

okay.

> > +    for ( i = 0U; i < nr_nodes; i++ )
> > +        for ( j = 0U; j < nr_nodes; j++ )
> 
> Why the U suffixes?

I added the U suffixes because variables i and j are unsigned types. 
If a plain 0 is preferred here, I will remove them.

> > +            numa_distance[i * nr_nodes + j] = i == j ?
> > +                LOCAL_DISTANCE : REMOTE_DISTANCE;
> 
> While binary operators really want to go at the end of wrapped lines, for
> the conditional operator we would generally prefer e.g.
> 
>             numa_distance[i * nr_nodes + j] = i == j
>                 ? LOCAL_DISTANCE : REMOTE_DISTANCE;
> 
> while specifically here it might be yet better as
> 
>             numa_distance[i * nr_nodes + j] =
>                 i == j ? LOCAL_DISTANCE : REMOTE_DISTANCE;

I will chose this style.

> You fill the entire array here. Why do you then use the zero-filling form
> of the allocation function?

You are right. I will use xvmalloc_array() instead.

Thank you,
Hirokazu Takahashi.