[...]
>> + /*
>> + * We have to make sure that while we clear PageAnonExclusive, that
>> + * the page is not pinned and that concurrent GUP-fast won't succeed in
>> + * concurrently pinning the page.
>> + *
>> + * Conceptually, GUP-fast pinning code of anon pages consists of:
>> + * (1) Read the PTE
>> + * (2) Pin the mapped page
>> + * (3) Check if the PTE changed by re-reading it; back off if so.
>> + * (4) Check if PageAnonExclusive is not set; back off if so.
>> + *
>> + * Conceptually, PageAnonExclusive clearing code consists of:
>> + * (1) Clear PTE
>> + * (2) Check if the page is pinned; back off if so.
>> + * (3) Clear PageAnonExclusive
>> + * (4) Restore PTE (optional)
>> + *
>> + * In GUP-fast, we have to make sure that (2),(3) and (4) happen in
>> + * the right order. Memory order between (2) and (3) is handled by
>> + * GUP-fast, independent of PageAnonExclusive.
>> + *
>> + * When clearing PageAnonExclusive(), we have to make sure that (1),
>> + * (2), (3) and (4) happen in the right order.
>> + *
>> + * Note that (4) has to happen after (3) in both cases to handle the
>> + * corner case whereby the PTE is restored to the original value after
>> + * clearing PageAnonExclusive and while GUP-fast might not detect the
>> + * PTE change, it will detect the PageAnonExclusive change.
>> + *
>> + * We assume that there might not be a memory barrier after
>> + * clearing/invalidating the PTE (1) and before restoring the PTE (4),
>> + * so we use explicit ones here.
>> + *
>> + * These memory barriers are paired with memory barriers in GUP-fast
>> + * code, including gup_must_unshare().
>> + */
>> +
>> + /* Clear/invalidate the PTE before checking for PINs. */
>> + if (IS_ENABLED(CONFIG_HAVE_FAST_GUP))
>> + smp_mb();
>
> Wondering whether this could be smp_mb__before_atomic().
We'll read via atomic_read().
That's a non-RMW operation. smp_mb__before_atomic() only applies to
RMW (Read Modify Write) operations.
I have an updated patch with improved description/comments, that includes the
following explanation/example and showcases how the two barrier pairs
interact:
Thread 0 (KSM) Thread 1 (GUP-fast)
(B1) Read the PTE
# (B2) skipped without FOLL_WRITE
(A1) Clear PTE
smb_mb()
(A2) Check pinned
(B3) Pin the mapped page
smb_mb()
(A3) Clear PageAnonExclusive
smb_wmb()
(A4) Restore PTE
(B4) Check if the PTE changed
smb_rmb()
(B5) Check PageAnonExclusive
>
>> +
>> + if (unlikely(page_maybe_dma_pinned(page)))
>> + return -EBUSY;
>> ClearPageAnonExclusive(page);
>> +
>> + /* Clear PageAnonExclusive() before eventually restoring the PTE. */
>> + if (IS_ENABLED(CONFIG_HAVE_FAST_GUP))
>> + smp_mb__after_atomic();
>> return 0;
>> }
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index e9414ee57c5b..2aef8d76fcf2 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -2140,6 +2140,8 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>> *
>> * In case we cannot clear PageAnonExclusive(), split the PMD
>> * only and let try_to_migrate_one() fail later.
>> + *
>> + * See page_try_share_anon_rmap(): invalidate PMD first.
>> */
>> anon_exclusive = PageAnon(page) && PageAnonExclusive(page);
>> if (freeze && anon_exclusive && page_try_share_anon_rmap(page))
>> @@ -3177,6 +3179,7 @@ int set_pmd_migration_entry(struct page_vma_mapped_walk *pvmw,
>> flush_cache_range(vma, address, address + HPAGE_PMD_SIZE);
>> pmdval = pmdp_invalidate(vma, address, pvmw->pmd);
>>
>> + /* See page_try_share_anon_rmap(): invalidate PMD first. */
>> anon_exclusive = PageAnon(page) && PageAnonExclusive(page);
>> if (anon_exclusive && page_try_share_anon_rmap(page)) {
>> set_pmd_at(mm, address, pvmw->pmd, pmdval);
>> diff --git a/mm/ksm.c b/mm/ksm.c
>> index d7526c705081..971cf923c0eb 100644
>> --- a/mm/ksm.c
>> +++ b/mm/ksm.c
>> @@ -1091,6 +1091,7 @@ static int write_protect_page(struct vm_area_struct *vma, struct page *page,
>> goto out_unlock;
>> }
>>
>> + /* See page_try_share_anon_rmap(): clear PTE first. */
>> if (anon_exclusive && page_try_share_anon_rmap(page)) {
>> set_pte_at(mm, pvmw.address, pvmw.pte, entry);
>> goto out_unlock;
>> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
>> index 27fb37d65476..47e955212f15 100644
>> --- a/mm/migrate_device.c
>> +++ b/mm/migrate_device.c
>> @@ -193,20 +193,16 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>> bool anon_exclusive;
>> pte_t swp_pte;
>>
>
> flush_cache_page() missing here?
Hmm, wouldn't that already be missing on the !anon path right now?
>
> Better copy Alistair too when post formally since this will have a slight
> conflict with the other thread.
Yes, I'll give him a heads-up right away: full patch in
https://lkml.kernel.org/r/68b38ac4-c680-b694-21a9-1971396d63b9@xxxxxxxxxx
Thanks for having a look Peter1
--
Thanks,
David / dhildenb
