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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 16/12/2022 11:59 am, Jan Beulich wrote:
> 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)?

Yeah, the tweak sounded entirely reasonable.

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

They're still read-write as far as C is concerned, and some of these
uses are before modifications finish.

>  Are you sure the compiler will ignore
> a "pure" attribute if it finds it (formally) violated?

Yes, very sure.  It got discussed at length on one of the speculation lists.

When the compiler can prove that the programmer doesn't know the rules
concerning pure/const, the attributes will be discarded.

To abuse the rules, you really do need the operation hidden in a place
that GCC can't see, so either a separate translation unit, or in inline
assembly.

~Andrew