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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.10.2024 12:23, Bernhard Kaindl wrote:
> From: Bernhard Kaindl <bernhard.kaindl@xxxxxxxxx>
> 
> Changes in v2:
> - Remove update of numainfo call, only calculate RAM for each node.
> - Calculate RAM based on page boundaries, coding style fixes

Nit: This shouldn't be part of the description (eventual commit message);
it belongs ...

> Some admin tools like 'xl info -n' like to display the total memory
> for each NUMA node. The Xen backend[1] of hwloc comes to mind too.
> 
> The total amount of RAM on a NUMA node is not needed by Xen internally:
> Xen only uses NODE_DATA->node_spanned_pages, but that can be confusing
> for users as it includes memory holes (can be as large as 2GB on x86).
> 
> Calculate the RAM per NUMA node by iterating over arch_get_ram_range()
> which returns the e820 RAM entries on x86 and update it on memory_add().
> 
> Use NODE_DATA->node_present_pages (like in the Linux kernel) to hold
> this info and in a later commit, find a way for tools to read it.
> 
> [1] hwloc with Xen backend: https://github.com/xenserver-next/hwloc/
> 
> Signed-off-by: Bernhard Kaindl <bernhard.kaindl@xxxxxxxxx>
> ---

... below this first (or any later) such separator.

> --- a/xen/common/numa.c
> +++ b/xen/common/numa.c
> @@ -499,15 +499,41 @@ 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 given nodeid and start/end 
> addrs.
> + *
> + * This function sets up the boot memory for a given NUMA node by calculating
> + * the node's start and end page frame numbers (PFNs) and determining
> + * the number of present RAM pages within the node's memory range.
> + *
> + * @param nodeid The identifier of the node to initialize.
> + * @param start The starting physical address of the node's memory range.
> + * @param end The 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);
> +    unsigned long ram_start_pfn, ram_end_pfn;

These two variables would better live inside the loop, as it's only there
that they're used.

> +    unsigned int idx = 0;
> +    int err;
> +    paddr_t ram_start, ram_end;
>  
>      NODE_DATA(nodeid)->node_start_pfn = start_pfn;
>      NODE_DATA(nodeid)->node_spanned_pages = end_pfn - start_pfn;
>  
> +    /* Calculate the numer of present RAM pages within the node: */

Nit: number

> +    NODE_DATA(nodeid)->node_present_pages = 0;
> +    while ( (err = arch_get_ram_range(idx++, &ram_start, &ram_end)) != 
> -ENOENT )
> +    {
> +        if ( err || ram_start >= end || ram_end <= start )
> +            continue;  /* Not RAM (err != 0) or range is outside the node */
> +
> +        /* Add RAM that in the node's memory range (within start and end): */

Nit: I think you either want "that's" or s/that in/from/ (or something
substantially similar).

> +        ram_end_pfn = min(ram_end, end) >> PAGE_SHIFT;
> +        ram_start_pfn = (PAGE_ALIGN(max(ram_start, start))) >> PAGE_SHIFT;

You effectively do the comparisons twice - once in the if() and then a
second time in the min() / max(). Can't you simply check ram_start_pfn
against ram_end_pfn, totaling to one less comparison?

Also please don't open-code PFN_DOWN() / PFN_UP().

Jan