Re: [PATCH v5 7/9] mm/mremap: Move TLB flush outside page table lock

["Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxx>]

On 5/20/21 8:56 PM, Aneesh Kumar K.V wrote:
> On 4/22/21 11:13 AM, Aneesh Kumar K.V wrote:
> > Move TLB flush outside page table lock so that kernel does
> > less with page table lock held. Releasing the ptl with old
> > TLB contents still valid will behave such that such access
> > happened before the level3 or level2 entry update.
>
>
> Ok this break the page lifetime rule
>
> commit: eb66ae030829 ("mremap: properly flush TLB before releasing the 
> page")
>
> I will respin dropping this change and add a comment around explaining 
> why we need to do tlb flush before dropping ptl.

Wondering whether this is correct considering we are holding mmap_sem in 
write mode in mremap. Can a parallel free/zap happen?

> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxx>
> > ---
> >   mm/mremap.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/mremap.c b/mm/mremap.c
> > index 109560977944..9effca76bf17 100644
> > --- a/mm/mremap.c
> > +++ b/mm/mremap.c
> > @@ -258,7 +258,7 @@ static bool move_normal_pmd(struct vm_area_struct 
> > *vma, unsigned long old_addr,
> >        * We don't have to worry about the ordering of src and dst
> >        * ptlocks because exclusive mmap_lock prevents deadlock.
> >        */
> > -    old_ptl = pmd_lock(vma->vm_mm, old_pmd);
> > +    old_ptl = pmd_lock(mm, old_pmd);
> >       new_ptl = pmd_lockptr(mm, new_pmd);
> >       if (new_ptl != old_ptl)
> >           spin_lock_nested(new_ptl, SINGLE_DEPTH_NESTING);
> > @@ -270,11 +270,11 @@ static bool move_normal_pmd(struct 
> > vm_area_struct *vma, unsigned long old_addr,
> >       VM_BUG_ON(!pmd_none(*new_pmd));
> >       pmd_populate(mm, new_pmd, (pgtable_t)pmd_page_vaddr(pmd));
> > -    flush_pte_tlb_pwc_range(vma, old_addr, old_addr + PMD_SIZE);
> >       if (new_ptl != old_ptl)
> >           spin_unlock(new_ptl);
> >       spin_unlock(old_ptl);
> > +    flush_pte_tlb_pwc_range(vma, old_addr, old_addr + PMD_SIZE);
> >       return true;
> >   }
> >   #else
> > @@ -305,7 +305,7 @@ static bool move_normal_pud(struct vm_area_struct 
> > *vma, unsigned long old_addr,
> >        * We don't have to worry about the ordering of src and dst
> >        * ptlocks because exclusive mmap_lock prevents deadlock.
> >        */
> > -    old_ptl = pud_lock(vma->vm_mm, old_pud);
> > +    old_ptl = pud_lock(mm, old_pud);
> >       new_ptl = pud_lockptr(mm, new_pud);
> >       if (new_ptl != old_ptl)
> >           spin_lock_nested(new_ptl, SINGLE_DEPTH_NESTING);
> > @@ -317,11 +317,11 @@ static bool move_normal_pud(struct 
> > vm_area_struct *vma, unsigned long old_addr,
> >       VM_BUG_ON(!pud_none(*new_pud));
> >       pud_populate(mm, new_pud, (pmd_t *)pud_page_vaddr(pud));
> > -    flush_pte_tlb_pwc_range(vma, old_addr, old_addr + PUD_SIZE);
> >       if (new_ptl != old_ptl)
> >           spin_unlock(new_ptl);
> >       spin_unlock(old_ptl);
> > +    flush_pte_tlb_pwc_range(vma, old_addr, old_addr + PUD_SIZE);
> >       return true;
> >   }
> >   #else