Re: [PATCH 1/2] x86/mm: avoid phys_to_nid() calls for invalid addresses

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.12.2022 04:28, Wei Chen wrote:
> Hi Jan,
> 
> On 2022/12/13 19:36, Jan Beulich wrote:
>> With phys_to_nid() now actively checking that a valid node ID is on
>> record, the two uses in paging_init() can actually trigger at least the
>> 2nd of the assertions there. They're used to calculate allocation flags,
>> but the calculated flags wouldn't be used when dealing with an invalid
>> (unpopulated) address range. Defer the calculations such that they can
>> be done with a validated MFN in hands. This also does away with the
>> artificial calculations of an address to pass to phys_to_nid().
>>
>> Note that while the variable is provably written before use, at least
>> some compiler versions can't actually verify that. Hence the variable
>> also needs to gain a (dead) initializer.
>>
>> Fixes: e9c72d524fbd ("xen/x86: Use ASSERT instead of VIRTUAL_BUG_ON for 
>> phys_to_nid")
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> RFC: With small enough a NUMA hash shift it would still be possible to
>>       hit an SRAT hole, despite mfn_valid() passing. Hence, like was the
>>       original plan, it may still be necessary to relax the checking in
>>       phys_to_nid() (or its designated replacements). At which point the
>>       value of this change here would shrink to merely reducing the
>>       chance of unintentionally doing NUMA_NO_NODE allocations.
>>
> 
> I think it's better to place the last sentence or the whole RFC to the
> commit log. Without the RFC content, after a while, when I check this 
> commit again, I will be confused about what problem this commit solved. 
> Because just looking at the changes, as your said in RFC, it doesn't 
> completely solve the problem.

Moving some/all of this to the commit message is one of the ways to
resolve this RFC, yes. But the other one, flipping the order of the
two patches and making mfn_to_nid() check less than page_to_nid() is
one where the commit message here would need re-writing anyway. IOW
the primary question here is what route to go.

Jan