* Re: [PATCH] mm: gup: fix the fast GUP race against THP collapse [not found] ` <CAHbLzkohKvOFyfsVr=ry8Goi6kgxh9ig84FX0+pY4qzL4i0xWg@mail.gmail.com> @ 2022-09-07 4:50 ` Aneesh Kumar K V 2022-09-07 17:08 ` Yang Shi 0 siblings, 1 reply; 2+ messages in thread From: Aneesh Kumar K V @ 2022-09-07 4:50 UTC (permalink / raw) To: Yang Shi Cc: david, jhubbard, hughd, linux-kernel, Peter Xu, linux-mm, jgg, akpm, linuxppc-dev, kirill.shutemov On 9/7/22 12:37 AM, Yang Shi wrote: > On Mon, Sep 5, 2022 at 1:56 AM Aneesh Kumar K.V > <aneesh.kumar@linux.ibm.com> wrote: >> >> Yang Shi <shy828301@gmail.com> writes: >> >>> >>> On Fri, Sep 2, 2022 at 9:00 AM Peter Xu <peterx@redhat.com> wrote: >>>> >>>> On Thu, Sep 01, 2022 at 04:50:45PM -0700, Yang Shi wrote: >>>>> On Thu, Sep 1, 2022 at 4:26 PM Peter Xu <peterx@redhat.com> wrote: >>>>>> >>>>>> Hi, Yang, >>>>>> >>>>>> On Thu, Sep 01, 2022 at 03:27:07PM -0700, Yang Shi wrote: >>>>>>> Since general RCU GUP fast was introduced in commit 2667f50e8b81 ("mm: >>>>>>> introduce a general RCU get_user_pages_fast()"), a TLB flush is no longer >>>>>>> sufficient to handle concurrent GUP-fast in all cases, it only handles >>>>>>> traditional IPI-based GUP-fast correctly. >>>>>> >>>>>> If TLB flush (or, IPI broadcasts) used to work to protect against gup-fast, >>>>>> I'm kind of confused why it's not sufficient even if with RCU gup? Isn't >>>>>> that'll keep working as long as interrupt disabled (which current fast-gup >>>>>> will still do)? >>>>> >>>>> Actually the wording was copied from David's commit log for his >>>>> PageAnonExclusive fix. My understanding is the IPI broadcast still >>>>> works, but it may not be supported by all architectures and not >>>>> preferred anymore. So we should avoid depending on IPI broadcast IIUC. >>>>> >>>>>> >>>>>> IIUC the issue is you suspect not all archs correctly implemented >>>>>> pmdp_collapse_flush(), or am I wrong? >>>>> >>>>> This is a possible fix, please see below for details. >>>>> >>>>>> >>>>>>> On architectures that send >>>>>>> an IPI broadcast on TLB flush, it works as expected. But on the >>>>>>> architectures that do not use IPI to broadcast TLB flush, it may have >>>>>>> the below race: >>>>>>> >>>>>>> CPU A CPU B >>>>>>> THP collapse fast GUP >>>>>>> gup_pmd_range() <-- see valid pmd >>>>>>> gup_pte_range() <-- work on pte >>>>>>> pmdp_collapse_flush() <-- clear pmd and flush >>>>>>> __collapse_huge_page_isolate() >>>>>>> check page pinned <-- before GUP bump refcount >>>>>>> pin the page >>>>>>> check PTE <-- no change >>>>>>> __collapse_huge_page_copy() >>>>>>> copy data to huge page >>>>>>> ptep_clear() >>>>>>> install huge pmd for the huge page >>>>>>> return the stale page >>>>>>> discard the stale page >>>>>>> >>>>>>> The race could be fixed by checking whether PMD is changed or not after >>>>>>> taking the page pin in fast GUP, just like what it does for PTE. If the >>>>>>> PMD is changed it means there may be parallel THP collapse, so GUP >>>>>>> should back off. >>>>>> >>>>>> Could the race also be fixed by impl pmdp_collapse_flush() correctly for >>>>>> the archs that are missing? Do you know which arch(s) is broken with it? >>>>> >>>>> Yes, and this was suggested by me in the first place, but per the >>>>> suggestion from John and David, this is not the preferred way. I think >>>>> it is because: >>>>> >>>>> Firstly, using IPI to serialize against fast GUP is not recommended >>>>> anymore since fast GUP does check PTE then back off so we should avoid >>>>> it. >>>>> Secondly, if checking PMD then backing off could solve the problem, >>>>> why do we still need broadcast IPI? It doesn't sound performant. >>>>> >>>>>> >>>>>> It's just not clear to me whether this patch is an optimization or a fix, >>>>>> if it's a fix whether the IPI broadcast in ppc pmdp_collapse_flush() would >>>>>> still be needed. >>>>> >>>>> It is a fix and the fix will make IPI broadcast not useful anymore. >>>> >>>> How about another patch to remove the ppc impl too? Then it can be a two >>>> patches series. >>> >>> BTW, I don't think we could remove the ppc implementation since it is >>> different from the generic pmdp_collapse_flush(), particularly for the >>> hash part IIUC. >>> >>> The generic version calls flush_tlb_range() -> hash__flush_tlb_range() >>> for hash, but the hash call is actually no-op. The ppc version calls >>> hash__pmdp_collapse_flush() -> flush_tlb_pmd_range(), which does >>> something useful. >>> >> >> We should actually rename flush_tlb_pmd_range(). It actually flush the >> hash page table entries. >> >> I will do the below patch for ppc64 to clarify this better > > Thanks, Aneesh. It looks more readable. A follow-up question, I think > we could remove serialize_against_pte_lookup(), which just issues IPI > broadcast to run a dummy function. This IPI should not be needed > anymore with my patch. Of course, we need to keep the memory barrier. > For radix translation yes. For hash we still need that. W.r.t memory barrier, radix do use radix__flush_tlb_collapsed_pmd() which does a tlb invalidate. IIUC that will enfocre the required memory barrier there. So you should be able to just remove modified arch/powerpc/mm/book3s64/radix_pgtable.c @@ -937,15 +937,6 @@ pmd_t radix__pmdp_collapse_flush(struct vm_area_struct *vma, unsigned long addre pmd = *pmdp; pmd_clear(pmdp); - /* - * pmdp collapse_flush need to ensure that there are no parallel gup - * walk after this call. This is needed so that we can have stable - * page ref count when collapsing a page. We don't allow a collapse page - * if we have gup taken on the page. We can ensure that by sending IPI - * because gup walk happens with IRQ disabled. - */ - serialize_against_pte_lookup(vma->vm_mm); - radix__flush_tlb_collapsed_pmd(vma->vm_mm, address); return pmd; in your patch. This will also consolidate all the related changes together. >> >> diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h b/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h >> index 8b762f282190..fd30fa20c392 100644 >> --- a/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h >> +++ b/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h >> @@ -112,13 +112,12 @@ static inline void hash__flush_tlb_kernel_range(unsigned long start, >> >> struct mmu_gather; >> extern void hash__tlb_flush(struct mmu_gather *tlb); >> -void flush_tlb_pmd_range(struct mm_struct *mm, pmd_t *pmd, unsigned long addr); >> >> #ifdef CONFIG_PPC_64S_HASH_MMU >> /* Private function for use by PCI IO mapping code */ >> extern void __flush_hash_table_range(unsigned long start, unsigned long end); >> -extern void flush_tlb_pmd_range(struct mm_struct *mm, pmd_t *pmd, >> - unsigned long addr); >> +extern void flush_hash_table_pmd_range(struct mm_struct *mm, pmd_t *pmd, >> + unsigned long addr); >> #else >> static inline void __flush_hash_table_range(unsigned long start, unsigned long end) { } >> #endif >> diff --git a/arch/powerpc/mm/book3s64/hash_pgtable.c b/arch/powerpc/mm/book3s64/hash_pgtable.c >> index ae008b9df0e6..f30131933a01 100644 >> --- a/arch/powerpc/mm/book3s64/hash_pgtable.c >> +++ b/arch/powerpc/mm/book3s64/hash_pgtable.c >> @@ -256,7 +256,7 @@ pmd_t hash__pmdp_collapse_flush(struct vm_area_struct *vma, unsigned long addres >> * the __collapse_huge_page_copy can result in copying >> * the old content. >> */ >> - flush_tlb_pmd_range(vma->vm_mm, &pmd, address); >> + flush_hash_table_pmd_range(vma->vm_mm, &pmd, address); >> return pmd; >> } >> >> diff --git a/arch/powerpc/mm/book3s64/hash_tlb.c b/arch/powerpc/mm/book3s64/hash_tlb.c >> index eb0bccaf221e..a64ea0a7ef96 100644 >> --- a/arch/powerpc/mm/book3s64/hash_tlb.c >> +++ b/arch/powerpc/mm/book3s64/hash_tlb.c >> @@ -221,7 +221,7 @@ void __flush_hash_table_range(unsigned long start, unsigned long end) >> local_irq_restore(flags); >> } >> >> -void flush_tlb_pmd_range(struct mm_struct *mm, pmd_t *pmd, unsigned long addr) >> +void flush_hash_table_pmd_range(struct mm_struct *mm, pmd_t *pmd, unsigned long addr) >> { >> pte_t *pte; >> pte_t *start_pte; >> ^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] mm: gup: fix the fast GUP race against THP collapse 2022-09-07 4:50 ` [PATCH] mm: gup: fix the fast GUP race against THP collapse Aneesh Kumar K V @ 2022-09-07 17:08 ` Yang Shi 0 siblings, 0 replies; 2+ messages in thread From: Yang Shi @ 2022-09-07 17:08 UTC (permalink / raw) To: Aneesh Kumar K V Cc: david, jhubbard, hughd, linux-kernel, Peter Xu, linux-mm, jgg, akpm, linuxppc-dev, kirill.shutemov On Tue, Sep 6, 2022 at 9:51 PM Aneesh Kumar K V <aneesh.kumar@linux.ibm.com> wrote: > > On 9/7/22 12:37 AM, Yang Shi wrote: > > On Mon, Sep 5, 2022 at 1:56 AM Aneesh Kumar K.V > > <aneesh.kumar@linux.ibm.com> wrote: > >> > >> Yang Shi <shy828301@gmail.com> writes: > >> > >>> > >>> On Fri, Sep 2, 2022 at 9:00 AM Peter Xu <peterx@redhat.com> wrote: > >>>> > >>>> On Thu, Sep 01, 2022 at 04:50:45PM -0700, Yang Shi wrote: > >>>>> On Thu, Sep 1, 2022 at 4:26 PM Peter Xu <peterx@redhat.com> wrote: > >>>>>> > >>>>>> Hi, Yang, > >>>>>> > >>>>>> On Thu, Sep 01, 2022 at 03:27:07PM -0700, Yang Shi wrote: > >>>>>>> Since general RCU GUP fast was introduced in commit 2667f50e8b81 ("mm: > >>>>>>> introduce a general RCU get_user_pages_fast()"), a TLB flush is no longer > >>>>>>> sufficient to handle concurrent GUP-fast in all cases, it only handles > >>>>>>> traditional IPI-based GUP-fast correctly. > >>>>>> > >>>>>> If TLB flush (or, IPI broadcasts) used to work to protect against gup-fast, > >>>>>> I'm kind of confused why it's not sufficient even if with RCU gup? Isn't > >>>>>> that'll keep working as long as interrupt disabled (which current fast-gup > >>>>>> will still do)? > >>>>> > >>>>> Actually the wording was copied from David's commit log for his > >>>>> PageAnonExclusive fix. My understanding is the IPI broadcast still > >>>>> works, but it may not be supported by all architectures and not > >>>>> preferred anymore. So we should avoid depending on IPI broadcast IIUC. > >>>>> > >>>>>> > >>>>>> IIUC the issue is you suspect not all archs correctly implemented > >>>>>> pmdp_collapse_flush(), or am I wrong? > >>>>> > >>>>> This is a possible fix, please see below for details. > >>>>> > >>>>>> > >>>>>>> On architectures that send > >>>>>>> an IPI broadcast on TLB flush, it works as expected. But on the > >>>>>>> architectures that do not use IPI to broadcast TLB flush, it may have > >>>>>>> the below race: > >>>>>>> > >>>>>>> CPU A CPU B > >>>>>>> THP collapse fast GUP > >>>>>>> gup_pmd_range() <-- see valid pmd > >>>>>>> gup_pte_range() <-- work on pte > >>>>>>> pmdp_collapse_flush() <-- clear pmd and flush > >>>>>>> __collapse_huge_page_isolate() > >>>>>>> check page pinned <-- before GUP bump refcount > >>>>>>> pin the page > >>>>>>> check PTE <-- no change > >>>>>>> __collapse_huge_page_copy() > >>>>>>> copy data to huge page > >>>>>>> ptep_clear() > >>>>>>> install huge pmd for the huge page > >>>>>>> return the stale page > >>>>>>> discard the stale page > >>>>>>> > >>>>>>> The race could be fixed by checking whether PMD is changed or not after > >>>>>>> taking the page pin in fast GUP, just like what it does for PTE. If the > >>>>>>> PMD is changed it means there may be parallel THP collapse, so GUP > >>>>>>> should back off. > >>>>>> > >>>>>> Could the race also be fixed by impl pmdp_collapse_flush() correctly for > >>>>>> the archs that are missing? Do you know which arch(s) is broken with it? > >>>>> > >>>>> Yes, and this was suggested by me in the first place, but per the > >>>>> suggestion from John and David, this is not the preferred way. I think > >>>>> it is because: > >>>>> > >>>>> Firstly, using IPI to serialize against fast GUP is not recommended > >>>>> anymore since fast GUP does check PTE then back off so we should avoid > >>>>> it. > >>>>> Secondly, if checking PMD then backing off could solve the problem, > >>>>> why do we still need broadcast IPI? It doesn't sound performant. > >>>>> > >>>>>> > >>>>>> It's just not clear to me whether this patch is an optimization or a fix, > >>>>>> if it's a fix whether the IPI broadcast in ppc pmdp_collapse_flush() would > >>>>>> still be needed. > >>>>> > >>>>> It is a fix and the fix will make IPI broadcast not useful anymore. > >>>> > >>>> How about another patch to remove the ppc impl too? Then it can be a two > >>>> patches series. > >>> > >>> BTW, I don't think we could remove the ppc implementation since it is > >>> different from the generic pmdp_collapse_flush(), particularly for the > >>> hash part IIUC. > >>> > >>> The generic version calls flush_tlb_range() -> hash__flush_tlb_range() > >>> for hash, but the hash call is actually no-op. The ppc version calls > >>> hash__pmdp_collapse_flush() -> flush_tlb_pmd_range(), which does > >>> something useful. > >>> > >> > >> We should actually rename flush_tlb_pmd_range(). It actually flush the > >> hash page table entries. > >> > >> I will do the below patch for ppc64 to clarify this better > > > > Thanks, Aneesh. It looks more readable. A follow-up question, I think > > we could remove serialize_against_pte_lookup(), which just issues IPI > > broadcast to run a dummy function. This IPI should not be needed > > anymore with my patch. Of course, we need to keep the memory barrier. > > > > > For radix translation yes. For hash we still need that. W.r.t memory barrier, > radix do use radix__flush_tlb_collapsed_pmd() which does a tlb invalidate. > IIUC that will enfocre the required memory barrier there. So you should be able > to just remove > > modified arch/powerpc/mm/book3s64/radix_pgtable.c > @@ -937,15 +937,6 @@ pmd_t radix__pmdp_collapse_flush(struct vm_area_struct *vma, unsigned long addre > pmd = *pmdp; > pmd_clear(pmdp); > > - /* > - * pmdp collapse_flush need to ensure that there are no parallel gup > - * walk after this call. This is needed so that we can have stable > - * page ref count when collapsing a page. We don't allow a collapse page > - * if we have gup taken on the page. We can ensure that by sending IPI > - * because gup walk happens with IRQ disabled. > - */ > - serialize_against_pte_lookup(vma->vm_mm); > - > radix__flush_tlb_collapsed_pmd(vma->vm_mm, address); > > return pmd; > > in your patch. This will also consolidate all the related changes together. Thanks, Aneesh. It may be better to have the ppc cleanup in a separate patch. > > >> > >> diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h b/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h > >> index 8b762f282190..fd30fa20c392 100644 > >> --- a/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h > >> +++ b/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h > >> @@ -112,13 +112,12 @@ static inline void hash__flush_tlb_kernel_range(unsigned long start, > >> > >> struct mmu_gather; > >> extern void hash__tlb_flush(struct mmu_gather *tlb); > >> -void flush_tlb_pmd_range(struct mm_struct *mm, pmd_t *pmd, unsigned long addr); > >> > >> #ifdef CONFIG_PPC_64S_HASH_MMU > >> /* Private function for use by PCI IO mapping code */ > >> extern void __flush_hash_table_range(unsigned long start, unsigned long end); > >> -extern void flush_tlb_pmd_range(struct mm_struct *mm, pmd_t *pmd, > >> - unsigned long addr); > >> +extern void flush_hash_table_pmd_range(struct mm_struct *mm, pmd_t *pmd, > >> + unsigned long addr); > >> #else > >> static inline void __flush_hash_table_range(unsigned long start, unsigned long end) { } > >> #endif > >> diff --git a/arch/powerpc/mm/book3s64/hash_pgtable.c b/arch/powerpc/mm/book3s64/hash_pgtable.c > >> index ae008b9df0e6..f30131933a01 100644 > >> --- a/arch/powerpc/mm/book3s64/hash_pgtable.c > >> +++ b/arch/powerpc/mm/book3s64/hash_pgtable.c > >> @@ -256,7 +256,7 @@ pmd_t hash__pmdp_collapse_flush(struct vm_area_struct *vma, unsigned long addres > >> * the __collapse_huge_page_copy can result in copying > >> * the old content. > >> */ > >> - flush_tlb_pmd_range(vma->vm_mm, &pmd, address); > >> + flush_hash_table_pmd_range(vma->vm_mm, &pmd, address); > >> return pmd; > >> } > >> > >> diff --git a/arch/powerpc/mm/book3s64/hash_tlb.c b/arch/powerpc/mm/book3s64/hash_tlb.c > >> index eb0bccaf221e..a64ea0a7ef96 100644 > >> --- a/arch/powerpc/mm/book3s64/hash_tlb.c > >> +++ b/arch/powerpc/mm/book3s64/hash_tlb.c > >> @@ -221,7 +221,7 @@ void __flush_hash_table_range(unsigned long start, unsigned long end) > >> local_irq_restore(flags); > >> } > >> > >> -void flush_tlb_pmd_range(struct mm_struct *mm, pmd_t *pmd, unsigned long addr) > >> +void flush_hash_table_pmd_range(struct mm_struct *mm, pmd_t *pmd, unsigned long addr) > >> { > >> pte_t *pte; > >> pte_t *start_pte; > >> > ^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-09-07 17:09 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220901222707.477402-1-shy828301@gmail.com>
[not found] ` <YxE/vuQlWJCJMuG2@xz-m1.local>
[not found] ` <CAHbLzkqjZ_UhUbJ_f9Br7WCAgQvjrm5bMPRsKYvaFc2bzSuzrw@mail.gmail.com>
[not found] ` <YxIofPiI8jvGzcjC@xz-m1.local>
[not found] ` <CAHbLzkqGdnwY4P8jKQR0ojm6QV6b3dBi5pwrC1UJ4dqi3EqS4w@mail.gmail.com>
[not found] ` <87ilm2jj4t.fsf@linux.ibm.com>
[not found] ` <CAHbLzkohKvOFyfsVr=ry8Goi6kgxh9ig84FX0+pY4qzL4i0xWg@mail.gmail.com>
2022-09-07 4:50 ` [PATCH] mm: gup: fix the fast GUP race against THP collapse Aneesh Kumar K V
2022-09-07 17:08 ` Yang Shi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox