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.
