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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.12.2022 13:06, Julien Grall wrote:
> 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@xxxxxxxx>
>> ---
>> 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.

Well, the purpose then still is the very same conversion, so the name
is quite appropriate. I don't view mfn_to_nid_bug_dont_look_very_closely()
(exaggerating) as very sensible a name.

>> --- 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?

Oh, indeed, I meant to say a word on this but then forgot. This
simply is because the adding of 1 to the start PFN (which by
itself is imo a little funny) makes it so that the printk()
inside the conditional would be certain to be called for an
empty (e.g. CPU-only) node.

Jan