Re: [PATCH v4] NUMA: Introduce NODE_DATA->node_present_pages(RAM pages)

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.10.2024 13:05, Alejandro Vallejo wrote:
> On Sun Oct 27, 2024 at 2:43 PM GMT, Bernhard Kaindl wrote:
>> @@ -499,15 +500,44 @@ int __init compute_hash_shift(const struct node *nodes,
>>      return shift;
>>  }
>>  
>> -/* Initialize NODE_DATA given nodeid and start/end */
>> +/**
>> + * @brief Initialize a NUMA node's node_data structure at boot.
>> + *
>> + * It is given the NUMA node's index in the node_data array as well
>> + * as the start and exclusive end address of the node's memory span
>> + * as arguments and initializes the node_data entry with this information.
>> + *
>> + * It then initializes the total number of usable memory pages within
>> + * the NUMA node's memory span using the arch_get_ram_range() function.
>> + *
>> + * @param nodeid The index into the node_data array for the node.
>> + * @param start The starting physical address of the node's memory range.
>> + * @param end The exclusive ending physical address of the node's memory 
>> range.
>> + */
>>  void __init setup_node_bootmem(nodeid_t nodeid, paddr_t start, paddr_t end)
>>  {
>>      unsigned long start_pfn = paddr_to_pfn(start);
>>      unsigned long end_pfn = paddr_to_pfn(end);
>> +    struct node_data *numa_node = NODE_DATA(nodeid);
>> +    paddr_t start_ram, end_ram;
> 
> With the loop in place and arch_get_ram_range() being called inside, these two
> can further reduce scope by being moved inside as well.
> 
>> +    unsigned int idx = 0;
>> +    unsigned long *pages = &numa_node->node_present_pages;
>>  
>> -    NODE_DATA(nodeid)->node_start_pfn = start_pfn;
>> -    NODE_DATA(nodeid)->node_spanned_pages = end_pfn - start_pfn;
>> +    numa_node->node_start_pfn = start_pfn;
>> +    numa_node->node_spanned_pages = end_pfn - start_pfn;
>> +
>> +    /* Calculate the number of present RAM pages within the node: */
> 
> nit: that last ":" feels a bit out of place
> 
>> +    *pages = 0;
>> +    do {
>> +        int err = arch_get_ram_range(idx++, &start_ram, &end_ram);
>> +
>> +        if (err == -ENOENT)
> 
> Missing spaces between condition and the parenthesis of the conditional. 
> But...
> 
>> +            break;
>> +        if ( err || start_ram >= end || end_ram <= start )
>> +            continue;  /* range is outside of the node, or not usable RAM */
>>  
>> +        *pages += PFN_DOWN(min(end_ram, end)) - PFN_UP(max(start_ram, 
>> start));
>> +    } while (1);
> 
> ... testing for validity rather than invalidity would allow the loop to be
> checked for termination on the termination condition rather than the ad-hoc
> check inside. That is...
> 
>     (untested)
> 
>     do {
>         paddr_t start_ram, end_ram;
>         int err = arch_get_ram_range(idx++, &start_ram, &end_ram);
> 
>         if ( !err && start_ram < end && end_ram > start )
>             *pages += PFN_DOWN(min(end_ram, end)) -
>                       PFN_UP(max(start_ram, start));
>     } while (err != ENOENT);

     } while ( err != -ENOENT );

> That said, take all of this with a pinch of salt. I'm not a maintainer here,
> after all, and you might want to wait for Andrew, Jan or Roger to chip in.

Apart from the small remark above I agree with the comments made, fwiw.

Jan