Re: [PATCH v3] mm: fix race between MADV_FREE reclaim and blkdev direct IO read

[John Hubbard <jhubbard@nvidia.com>]

On 2/3/22 21:56, Yu Zhao wrote:
...
>>> Got it. IIRC, get_user_pages() doesn't imply a write barrier. If so,
>>> there should be a smp_wmb() on the other side:
>>
>> If I understand it correctly, it actually implies a full memory
>> barrier, doesn't it?
>>
>> Because... gup_pte_range() (fast path) calls try_grab_compound_head(),
>> which eventually calls* atomic_add_unless(), an atomic conditional RMW
>> operation with return value, thus fully ordered on success (atomic_t.rst);
>> (on failure gup_pte_range() falls back to the slow path, below.)
>>
>> And follow_page_pte() (slow path) calls try_grab_page(), which also calls
>> into try_grab_compound_head(), as the above.

Well, doing so was a mistake, actually. I've recently reverted it, via:
commit c36c04c2e132 ("Revert "mm/gup: small refactoring: simplify
try_grab_page()""). Details are in the commit log.

Apologies for the confusion that this may have created.

thanks,
-- 
John Hubbard
NVIDIA

>>
>> (* on CONFIG_TINY_RCU, it calls just atomic_add(), which isn't ordered,
>> but that option is targeted for UP/!SMP, thus not a problem for this race.)
>>
>> Looking at the implementation of arch_atomic_fetch_add_unless() on
>> more relaxed/weakly ordered archs (arm, powerpc, if I got that right),
>> there are barriers like 'smp_mb()' and 'sync' instruction if 'old != unless',
>> so that seems to be OK.
>>
>> And the set_page_dirty() calls occur after get_user_pages() / that point.
>>
>> Does that make sense?
> 
> Yes, it does, thanks. I was thinking along the lines of whether there
> is an actual contract. The reason get_user_pages() currently works as
> a full barrier is not intentional but a side effect of this recent
> cleanup patch:
> commit 54d516b1d6 ("mm/gup: small refactoring: simplify try_grab_page()")
> But I agree your fix works as is.