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 <jbeul...@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.
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
