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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.12.2022 12:49, Andrew Cooper wrote:
> On 13/12/2022 11:38 am, 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.
> 
> Thanks.  This will help RISC-V too.
> 
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>,

Thanks. You realize though that the patch may change depending on the
verdict on patch 1 (and, if that one's to change, the two likely
flipped with the actual fix moving here in the form of more relaxed
assertions, one way or another)?

> albeit with one deletion.
> 
>> --- a/xen/include/xen/numa.h
>> +++ b/xen/include/xen/numa.h
>> @@ -1,6 +1,7 @@
>>  #ifndef _XEN_NUMA_H
>>  #define _XEN_NUMA_H
>>  
>> +#include <xen/mm-frame.h>
>>  #include <asm/numa.h>
>>  
>>  #define NUMA_NO_NODE     0xFF
>> @@ -68,12 +69,15 @@ struct node_data {
>>  
>>  extern struct node_data node_data[];
>>  
>> -static inline nodeid_t __attribute_pure__ phys_to_nid(paddr_t addr)
>> +static inline nodeid_t __attribute_pure__ mfn_to_nid(mfn_t mfn)
>>  {
>>      nodeid_t nid;
>> -    ASSERT((paddr_to_pdx(addr) >> memnode_shift) < memnodemapsize);
>> -    nid = memnodemap[paddr_to_pdx(addr) >> memnode_shift];
>> +    unsigned long pdx = mfn_to_pdx(mfn);
>> +
>> +    ASSERT((pdx >> memnode_shift) < memnodemapsize);
>> +    nid = memnodemap[pdx >> memnode_shift];
>>      ASSERT(nid < MAX_NUMNODES && node_data[nid].node_spanned_pages);
>> +
>>      return nid;
>>  }
>>  
>> @@ -102,6 +106,15 @@ extern bool numa_update_node_memblks(nod
>>                                       paddr_t start, paddr_t size, bool 
>> hotplug);
>>  extern void numa_set_processor_nodes_parsed(nodeid_t node);
>>  
>> +#else
>> +
>> +static inline nodeid_t __attribute_pure__ mfn_to_nid(mfn_t mfn)
>> +{
>> +    return 0;
>> +}
> 
> pure is useless on a stub like this, whereas its false on the non-stub
> form (uses several non-const variables) in a way that the compiler can
> prove (because it's static inline), and will discard.
> 
> As you're modifying both lines anyway, just drop the attribute.

Hmm, yes, I agree for the stub, so I've dropped it there. "Several non-
const variables", however, is only partly true. These are __ro_after_init
and not written anymore once set. Are you sure the compiler will ignore
a "pure" attribute if it finds it (formally) violated? That would be
somewhat odd, as it means differing behavior depending on whether the
same piece of code is in an inline or out-of-line function.

Jan