Re: [PATCH 2/2] NUMA: replace phys_to_nid()

[Julien Grall <julien@xen.org>]

Hi Jan,

On 13/12/2022 11:38, Jan Beulich wrote:
> All callers convert frame numbers (perhaps in turn derived from struct
> page_info pointers) to an address, just for the function to convert it
> back to a frame number (as the first step of paddr_to_pdx()). Replace
> the function by mfn_to_nid() plus a page_to_nid() wrapper macro. Replace
> call sites by the respectively most suitable one.
> 
> While there also introduce a !NUMA stub, eliminating the need for Arm
> (and potentially other ports) to carry one individually.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> At the top of free_heap_pages() mfn_to_nid() could also be used, since
> the MFN is calculated immediately ahead. The choice of using
> page_to_nid() (for now at least) was with the earlier patch's RFC in
> mind, addressing of which may require to make mfn_to_nid() do weaker
> checking than page_to_nid().

I haven't looked in details at the previous patch. However, I don't like 
the idea of making mfn_to_nid() do weaker checking because this could 
easily confuse the reader/developper.

If you want to use weaker check, then it would be better if a separate 
helper is provided with a name reflecting its purpose.

> --- a/xen/common/numa.c
> +++ b/xen/common/numa.c
> @@ -671,15 +671,15 @@ static void cf_check dump_numa(unsigned
>   
>       for_each_online_node ( i )
>       {
> -        paddr_t pa = pfn_to_paddr(node_start_pfn(i) + 1);
> +        mfn_t mfn = _mfn(node_start_pfn(i) + 1);
>   
>           printk("NODE%u start->%lu size->%lu free->%lu\n",
>                  i, node_start_pfn(i), node_spanned_pages(i),
>                  avail_node_heap_pages(i));
> -        /* Sanity check phys_to_nid() */
> -        if ( phys_to_nid(pa) != i )
> -            printk("phys_to_nid(%"PRIpaddr") -> %d should be %u\n",
> -                   pa, phys_to_nid(pa), i);
> +        /* Sanity check mfn_to_nid() */
> +        if ( node_spanned_pages(i) && mfn_to_nid(mfn) != i )


 From the commit message, I would have expected that we would only 
replace phys_to_nid() with either mfn_to_nid() or page_to_nid(). 
However, here you added node_spanned_pages(). Can you explain why?

Cheers,

-- 
Julien Grall