On Wed, Oct 24, 2018 at 03:57:24PM +0300, Kirill A. Shutemov wrote:
> On Wed, Oct 24, 2018 at 10:57:33PM +1100, Balbir Singh wrote:
> > On Wed, Oct 24, 2018 at 01:12:56PM +0300, Kirill A. Shutemov wrote:
> > > On Fri, Oct 12, 2018 at 06:31:58PM -0700, Joel Fernandes (Google) wrote:
> > > > diff --git a/mm/mremap.c b/mm/mremap.c
> > > > index 9e68a02a52b1..2fd163cff406 100644
> > > > --- a/mm/mremap.c
> > > > +++ b/mm/mremap.c
> > > > @@ -191,6 +191,54 @@ static void move_ptes(struct vm_area_struct *vma, pmd_t *old_pmd,
> > > > drop_rmap_locks(vma);
> > > > }
> > > >
> > > > +static bool move_normal_pmd(struct vm_area_struct *vma, unsigned long old_addr,
> > > > + unsigned long new_addr, unsigned long old_end,
> > > > + pmd_t *old_pmd, pmd_t *new_pmd, bool *need_flush)
> > > > +{
> > > > + spinlock_t *old_ptl, *new_ptl;
> > > > + struct mm_struct *mm = vma->vm_mm;
> > > > +
> > > > + if ((old_addr & ~PMD_MASK) || (new_addr & ~PMD_MASK)
> > > > + || old_end - old_addr < PMD_SIZE)
> > > > + return false;
> > > > +
> > > > + /*
> > > > + * The destination pmd shouldn't be established, free_pgtables()
> > > > + * should have release it.
> > > > + */
> > > > + if (WARN_ON(!pmd_none(*new_pmd)))
> > > > + return false;
> > > > +
> > > > + /*
> > > > + * We don't have to worry about the ordering of src and dst
> > > > + * ptlocks because exclusive mmap_sem prevents deadlock.
> > > > + */
> > > > + old_ptl = pmd_lock(vma->vm_mm, old_pmd);
> > > > + if (old_ptl) {
> > >
> > > How can it ever be false?
Kirill,
It cannot, you are right. I'll remove the test.
By the way, there are new changes upstream by Linus which flush the TLB
before releasing the ptlock instead of after. I'm guessing that patch came
about because of reviews of this patch and someone spotted an issue in the
existing code :)
Anyway the patch in concern is:
eb66ae030829 ("mremap: properly flush TLB before releasing the page")
I need to rebase on top of that with appropriate modifications, but I worry
that this patch will slow down performance since we have to flush at every
PMD/PTE move before releasing the ptlock. Where as with my patch, the
intention is to flush only at once in the end of move_page_tables. When I
tried to flush TLB on every PMD move, it was quite slow on my arm64 device [2].
Further observation [1] is, it seems like the move_huge_pmds and move_ptes code
is a bit sub optimal in the sense, we are acquiring and releasing the same
ptlock for a bunch of PMDs if the said PMDs are on the same page-table page
right? Instead we can do better by acquiring and release the ptlock less
often.
I think this observation [1] and the frequent TLB flush issue [2] can be solved
by acquiring the ptlock once for a bunch of PMDs, move them all, then flush
the tlb and then release the ptlock, and then proceed to doing the same thing
for the PMDs in the next page-table page. What do you think?
- Joel
