Re: [PATCH v4] mm: fix is_pinnable_page against on cma page

[John Hubbard <jhubbard@nvidia.com>]

On 5/17/22 07:00, Jason Gunthorpe wrote:
>>> It does change the generated code slightly. I don't know if this will
>>> affect performance here or not. But just for completeness, here you go:
>>>
>>> free_one_page() originally has this (just showing the changed parts):
>>>
>>>      mov    0x8(%rdx,%rax,8),%rbx
>>>      and    $0x3f,%ecx
>>>      shr    %cl,%rbx
>>>      and    $0x7,
>>>
>>>
>>> And after applying this diff:
>>>
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index 0e42038382c1..df1f8e9a294f 100644
>>> +++ b/mm/page_alloc.c
>>> @@ -482,7 +482,7 @@ unsigned long __get_pfnblock_flags_mask(const struct
>>> page *page,
>>>          word_bitidx = bitidx / BITS_PER_LONG;
>>>          bitidx &= (BITS_PER_LONG-1);
>>>
>>> -       word = bitmap[word_bitidx];
>>> +       word = READ_ONCE(bitmap[word_bitidx]);
>>>          return (word >> bitidx) & mask;
>>>   }
>>>
>>>
>>> ...it now does an extra memory dereference:
>>>
>>>      lea    0x8(%rdx,%rax,8),%rax
>>>      and    $0x3f,%ecx
>>>      mov    (%rax),%rbx
>>>      shr    %cl,%rbx
>>>      and    $0x7,%ebx
> 
> Where is the extra memory reference? 'lea' is not a memory reference,
> it is just some maths?

If you compare this to the snippet above, you'll see that there is
an extra mov statement, and that one dereferences a pointer from
%rax:

     mov    (%rax),%rbx

> 
>> Thanks for checking, John.
>>
>> I don't want to have the READ_ONCE in __get_pfnblock_flags_mask
>> atm even though it's an extra memory dereference for specific
>> architecutre and specific compiler unless other callsites *do*
>> need it.
> 
> If a callpath is called under locking or not under locking then I
> would expect to have two call chains clearly marked what their locking
> conditions are. ie __get_pfn_block_flags_mask_unlocked() - and

__get_pfn_block_flags_mask_unlocked() would definitely clarify things,
and allow some clear documentation, good idea.

I haven't checked to see if some code could keep using the normal
__get_pfn_block_flags_mask(), but if it could, that would help with the
problem of keeping the fast path fast.

> obviously clearly document and check what the locking requirements are
> of the locked path.
> 
> IMHO putting a READ_ONCE on something that is not a memory load from
> shared data is nonsense - if a simple == has a stability risk then so
> does the '(word >> bitidx) & mask'.
> 
> Jason

Doing something like this:

     int __x = y();
     int x = READ_ONCE(__x);

is just awful! I agree. Really, y() should handle any barriers, because
otherwise it really does look pointless, and people reading the code
need something that is clearer. My first reaction was that this was
pointless and wrong, and it turns out that that's only about 80% true:
as long as LTO-of-the-future doesn't arrive, and as long as no one
refactors y() to be inline.


thanks,
-- 
John Hubbard
NVIDIA