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

[Julien Grall <julien@xen.org>]

Hi Jan,

On 13/12/2022 12:46, Jan Beulich wrote:
> 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@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.

I understand they are both doing the same conversion. But the checks 
will be different. With your proposal, we are now going to say if the 
caller is "buggy" then use mfn_to_nid() if not then you can use any.

I think this is wrong to hide the "bug" just because the name is longer. 
In fact, it means that any non-buggy caller will still have relaxed 
check. The risk if we are going to introduce more "buggy" caller in the 
future.

So from my perspective there are only two acceptable solutions:
   1. Provide a different helper that will be used for just "buggy" 
caller. This will make super clear that the helper should only be used 
in very limited circumstances.
   2. Fix the "buggy" callers.

 From your previous e-mails, it wasn't clear whether 2) is possible. So 
that's leave us only with 1).

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

Ok. I think this wants to be a separate patch as this sounds like bug 
and we should avoid mixing code conversion with bug fix.

Cheers,

-- 
Julien Grall