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
