Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1325525 > unrolled thread

[PATCH 0/4] thp: simplify freeze_page() and unfreeze_page()

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] thp: simplify freeze_page() and unfreeze_page() "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2016-02-03 16:20 +0100
    [PATCH 1/4] rmap: introduce rmap_walk_locked() "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2016-02-03 16:20 +0100
      Re: [PATCH 1/4] rmap: introduce rmap_walk_locked() Andrew Morton <akpm@linux-foundation.org> - 2016-02-03 23:50 +0100
        Re: [PATCH 1/4] rmap: introduce rmap_walk_locked() "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2016-02-03 23:50 +0100
          Re: [PATCH 1/4] rmap: introduce rmap_walk_locked() Andrew Morton <akpm@linux-foundation.org> - 2016-02-04 00:00 +0100
            Re: [PATCH 1/4] rmap: introduce rmap_walk_locked() "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-02-04 15:40 +0100
    [PATCH 3/4] mm: make remove_migration_ptes() beyond mm/migration.c "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2016-02-03 16:20 +0100
    [PATCH 4/4] thp: rewrite freeze_page()/unfreeze_page() with generic rmap walkers "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2016-02-03 16:20 +0100
      Re: [PATCH 4/4] thp: rewrite freeze_page()/unfreeze_page() with  generic rmap walkers Dave Hansen <dave.hansen@intel.com> - 2016-02-03 16:50 +0100
        Re: [PATCH 4/4] thp: rewrite freeze_page()/unfreeze_page() with  generic rmap walkers Andrew Morton <akpm@linux-foundation.org> - 2016-02-03 23:50 +0100
          Re: [PATCH 4/4] thp: rewrite freeze_page()/unfreeze_page() with  generic rmap walkers "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2016-02-04 00:00 +0100
        Re: [PATCH 4/4] thp: rewrite freeze_page()/unfreeze_page() with  generic rmap walkers "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-02-04 15:40 +0100
        Re: [PATCH 4/4] thp: rewrite freeze_page()/unfreeze_page() with  generic rmap walkers "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-02-05 01:00 +0100

#1325525 — [PATCH 0/4] thp: simplify freeze_page() and unfreeze_page()

This patchset rewrites freeze_page() and unfreeze_page() using try_to_unmap()
and remove_migration_ptes(). Result is much simplier, but somewhat slower.
See the last patch for details.

I did quick sanity check. More testing is required.

Any comments?

Kirill A. Shutemov (4):
  rmap: introduce rmap_walk_locked()
  rmap: extend try_to_unmap() to be usable by split_huge_page()
  mm: make remove_migration_ptes() beyond mm/migration.c
  thp: rewrite freeze_page()/unfreeze_page() with generic rmap walkers

 include/linux/huge_mm.h |   7 ++
 include/linux/rmap.h    |   6 ++
 mm/huge_memory.c        | 219 ++++++------------------------------------------
 mm/migrate.c            |  13 +--
 mm/rmap.c               |  49 ++++++++---
 5 files changed, 83 insertions(+), 211 deletions(-)

-- 
2.7.0

[toc] | [next] | [standalone]

#1325527 — [PATCH 1/4] rmap: introduce rmap_walk_locked()

rmap_walk_locked() is the same as rmap_walk(), but caller takes care
about relevant rmap lock. It only supports anonymous pages for now.

It's preparation to switch THP splitting from custom rmap walk in
freeze_page()/unfreeze_page() to generic one.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 include/linux/rmap.h |  1 +
 mm/rmap.c            | 25 +++++++++++++++++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index bdf597c4f0be..23a03fbeef61 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -280,6 +280,7 @@ struct rmap_walk_control {
 };
 
 int rmap_walk(struct page *page, struct rmap_walk_control *rwc);
+int rmap_walk_locked(struct page *page, struct rmap_walk_control *rwc);
 
 #else	/* !CONFIG_MMU */
 
diff --git a/mm/rmap.c b/mm/rmap.c
index 79f3bf047f38..a9cffb784502 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1719,14 +1719,21 @@ static struct anon_vma *rmap_walk_anon_lock(struct page *page,
  * vm_flags for that VMA.  That should be OK, because that vma shouldn't be
  * LOCKED.
  */
-static int rmap_walk_anon(struct page *page, struct rmap_walk_control *rwc)
+static int rmap_walk_anon(struct page *page, struct rmap_walk_control *rwc,
+		bool locked)
 {
 	struct anon_vma *anon_vma;
 	pgoff_t pgoff;
 	struct anon_vma_chain *avc;
 	int ret = SWAP_AGAIN;
 
-	anon_vma = rmap_walk_anon_lock(page, rwc);
+	if (locked) {
+		anon_vma = page_anon_vma(page);
+		/* anon_vma disappear under us? */
+		VM_BUG_ON_PAGE(!anon_vma, page);
+	} else {
+		anon_vma = rmap_walk_anon_lock(page, rwc);
+	}
 	if (!anon_vma)
 		return ret;
 
@@ -1746,7 +1753,9 @@ static int rmap_walk_anon(struct page *page, struct rmap_walk_control *rwc)
 		if (rwc->done && rwc->done(page))
 			break;
 	}
-	anon_vma_unlock_read(anon_vma);
+
+	if (!locked)
+		anon_vma_unlock_read(anon_vma);
 	return ret;
 }
 
@@ -1808,11 +1817,19 @@ int rmap_walk(struct page *page, struct rmap_walk_control *rwc)
 	if (unlikely(PageKsm(page)))
 		return rmap_walk_ksm(page, rwc);
 	else if (PageAnon(page))
-		return rmap_walk_anon(page, rwc);
+		return rmap_walk_anon(page, rwc, false);
 	else
 		return rmap_walk_file(page, rwc);
 }
 
+/* Like rmap_walk, but caller holds relevant rmap lock */
+int rmap_walk_locked(struct page *page, struct rmap_walk_control *rwc)
+{
+	/* only for anon pages for now */
+	VM_BUG_ON_PAGE(!PageAnon(page) || PageKsm(page), page);
+	return rmap_walk_anon(page, rwc, true);
+}
+
 #ifdef CONFIG_HUGETLB_PAGE
 /*
  * The following three functions are for anonymous (private mapped) hugepages.
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1326058 — Re: [PATCH 1/4] rmap: introduce rmap_walk_locked()

On Wed,  3 Feb 2016 18:14:16 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:

> rmap_walk_locked() is the same as rmap_walk(), but caller takes care
> about relevant rmap lock. It only supports anonymous pages for now.
> 
> It's preparation to switch THP splitting from custom rmap walk in
> freeze_page()/unfreeze_page() to generic one.
> 
> ...
>
> +/* Like rmap_walk, but caller holds relevant rmap lock */
> +int rmap_walk_locked(struct page *page, struct rmap_walk_control *rwc)
> +{
> +	/* only for anon pages for now */
> +	VM_BUG_ON_PAGE(!PageAnon(page) || PageKsm(page), page);
> +	return rmap_walk_anon(page, rwc, true);
> +}

Should be rmap_walk_anon_locked()?

[toc] | [prev] | [next] | [standalone]

#1326080 — Re: [PATCH 1/4] rmap: introduce rmap_walk_locked()

On Wed, Feb 03, 2016 at 02:40:19PM -0800, Andrew Morton wrote:
> On Wed,  3 Feb 2016 18:14:16 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> 
> > rmap_walk_locked() is the same as rmap_walk(), but caller takes care
> > about relevant rmap lock. It only supports anonymous pages for now.
> > 
> > It's preparation to switch THP splitting from custom rmap walk in
> > freeze_page()/unfreeze_page() to generic one.
> > 
> > ...
> >
> > +/* Like rmap_walk, but caller holds relevant rmap lock */
> > +int rmap_walk_locked(struct page *page, struct rmap_walk_control *rwc)
> > +{
> > +	/* only for anon pages for now */
> > +	VM_BUG_ON_PAGE(!PageAnon(page) || PageKsm(page), page);
> > +	return rmap_walk_anon(page, rwc, true);
> > +}
> 
> Should be rmap_walk_anon_locked()?

I leave interface open for further extension for file mappings, once it
will be needed. Interface is mirroring plain rmap_walk()

If you prefer to rename the function, I can do it too.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1326113 — Re: [PATCH 1/4] rmap: introduce rmap_walk_locked()

On Thu, 4 Feb 2016 01:45:07 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:

> On Wed, Feb 03, 2016 at 02:40:19PM -0800, Andrew Morton wrote:
> > On Wed,  3 Feb 2016 18:14:16 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> > 
> > > rmap_walk_locked() is the same as rmap_walk(), but caller takes care
> > > about relevant rmap lock. It only supports anonymous pages for now.
> > > 
> > > It's preparation to switch THP splitting from custom rmap walk in
> > > freeze_page()/unfreeze_page() to generic one.
> > > 
> > > ...
> > >
> > > +/* Like rmap_walk, but caller holds relevant rmap lock */
> > > +int rmap_walk_locked(struct page *page, struct rmap_walk_control *rwc)
> > > +{
> > > +	/* only for anon pages for now */
> > > +	VM_BUG_ON_PAGE(!PageAnon(page) || PageKsm(page), page);
> > > +	return rmap_walk_anon(page, rwc, true);
> > > +}
> > 
> > Should be rmap_walk_anon_locked()?
> 
> I leave interface open for further extension for file mappings, once it
> will be needed. Interface is mirroring plain rmap_walk()

hm, yes, I see.

> If you prefer to rename the function, I can do it too.

Well, what does "unlocked" mean in the context of rmap_walk_ksm() and
rmap_walk_file()?  That the caller holds totally different locks.  I
expect that sitting down and writing out the interface definition for
such an rmap_walk_locked() would reveal that we shouldn't have created
it.

I mean, if the caller is to call such an rmap_walk_locked(), he first
needs to work out if it's a ksm page or an anon page or a file page,
then take the appropriate lock and then call rmap_walk_locked(). 
That's silly - at this point he should directly call
rmap_walk_ksm_locked()?

[toc] | [prev] | [next] | [standalone]

#1326871 — Re: [PATCH 1/4] rmap: introduce rmap_walk_locked()

On Wed, Feb 03, 2016 at 02:56:07PM -0800, Andrew Morton wrote:
> On Thu, 4 Feb 2016 01:45:07 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> 
> > On Wed, Feb 03, 2016 at 02:40:19PM -0800, Andrew Morton wrote:
> > > On Wed,  3 Feb 2016 18:14:16 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> > > 
> > > > rmap_walk_locked() is the same as rmap_walk(), but caller takes care
> > > > about relevant rmap lock. It only supports anonymous pages for now.
> > > > 
> > > > It's preparation to switch THP splitting from custom rmap walk in
> > > > freeze_page()/unfreeze_page() to generic one.
> > > > 
> > > > ...
> > > >
> > > > +/* Like rmap_walk, but caller holds relevant rmap lock */
> > > > +int rmap_walk_locked(struct page *page, struct rmap_walk_control *rwc)
> > > > +{
> > > > +	/* only for anon pages for now */
> > > > +	VM_BUG_ON_PAGE(!PageAnon(page) || PageKsm(page), page);
> > > > +	return rmap_walk_anon(page, rwc, true);
> > > > +}
> > > 
> > > Should be rmap_walk_anon_locked()?
> > 
> > I leave interface open for further extension for file mappings, once it
> > will be needed. Interface is mirroring plain rmap_walk()
> 
> hm, yes, I see.
> 
> > If you prefer to rename the function, I can do it too.
> 
> Well, what does "unlocked" mean in the context of rmap_walk_ksm() and
> rmap_walk_file()?

For rmap_walk_file(), caller should take i_mmap_lock for page->mapping at
least for read.

Not sure about KSM..

> That the caller holds totally different locks.  I expect that sitting
> down and writing out the interface definition for such an
> rmap_walk_locked() would reveal that we shouldn't have created it.
> 
> I mean, if the caller is to call such an rmap_walk_locked(), he first
> needs to work out if it's a ksm page or an anon page or a file page,
> then take the appropriate lock and then call rmap_walk_locked(). 
> That's silly - at this point he should directly call
> rmap_walk_ksm_locked()?

It makes sense if you have multiple pages to process and it's known that
they share reverse mapping.

Or if you want to keep the reverse mapping locked to keep continuity with
other operations.

In THP case, we have 512 subpages to unmap and we want to keep anon_vma
locked until the THP is split.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1325529 — [PATCH 3/4] mm: make remove_migration_ptes() beyond mm/migration.c

The patch makes remove_migration_ptes() available to be used in
split_huge_page().

New parameter 'locked' added: as with try_to_umap() we need a way to
indicate that caller holds rmap lock.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 include/linux/rmap.h |  2 ++
 mm/migrate.c         | 13 ++++++++-----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index 1fdde1ee7042..675b070f489a 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -257,6 +257,8 @@ int page_mkclean(struct page *);
  */
 int try_to_munlock(struct page *);
 
+void remove_migration_ptes(struct page *old, struct page *new, bool locked);
+
 /*
  * Called by memory-failure.c to kill processes.
  */
diff --git a/mm/migrate.c b/mm/migrate.c
index b1034f9c77e7..244a267fdb61 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -186,14 +186,17 @@ out:
  * Get rid of all migration entries and replace them by
  * references to the indicated page.
  */
-static void remove_migration_ptes(struct page *old, struct page *new)
+void remove_migration_ptes(struct page *old, struct page *new, bool locked)
 {
 	struct rmap_walk_control rwc = {
 		.rmap_one = remove_migration_pte,
 		.arg = old,
 	};
 
-	rmap_walk(new, &rwc);
+	if (locked)
+		rmap_walk_locked(new, &rwc);
+	else
+		rmap_walk(new, &rwc);
 }
 
 /*
@@ -698,7 +701,7 @@ static int writeout(struct address_space *mapping, struct page *page)
 	 * At this point we know that the migration attempt cannot
 	 * be successful.
 	 */
-	remove_migration_ptes(page, page);
+	remove_migration_ptes(page, page, false);
 
 	rc = mapping->a_ops->writepage(page, &wbc);
 
@@ -897,7 +900,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
 
 	if (page_was_mapped)
 		remove_migration_ptes(page,
-			rc == MIGRATEPAGE_SUCCESS ? newpage : page);
+			rc == MIGRATEPAGE_SUCCESS ? newpage : page, false);
 
 out_unlock_both:
 	unlock_page(newpage);
@@ -1065,7 +1068,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
 
 	if (page_was_mapped)
 		remove_migration_ptes(hpage,
-			rc == MIGRATEPAGE_SUCCESS ? new_hpage : hpage);
+			rc == MIGRATEPAGE_SUCCESS ? new_hpage : hpage, false);
 
 	unlock_page(new_hpage);
 
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1325533 — [PATCH 4/4] thp: rewrite freeze_page()/unfreeze_page() with generic rmap walkers

freeze_page() and unfreeze_page() helpers evolved in rather complex
beasts. It would be nice to cut complexity of this code.

This patch rewrites freeze_page() using standard try_to_unmap().
unfreeze_page() is rewritten with remove_migration_ptes().

The result is much simpler.

But the new variant is somewhat slower. Current helpers iterates over
VMAs the compound page is mapped to, and then over ptes within this VMA.
New helpers iterates over small page, then over VMA the small page
mapped to, and only then find relevant pte.

Also we've lost optimization which allows to split PMD directly into
migration entries.

I don't think the slowdown is critical, considering how much simpler
result is and that split_huge_page() is quite rare nowadays. It only
happens due memory pressure or migration.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 mm/huge_memory.c | 214 +++++++------------------------------------------------
 1 file changed, 24 insertions(+), 190 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 4213c76c8434..3bab26781f8d 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2825,7 +2825,7 @@ static void __split_huge_zero_page_pmd(struct vm_area_struct *vma,
 }
 
 static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
-		unsigned long haddr, bool freeze)
+		unsigned long haddr)
 {
 	struct mm_struct *mm = vma->vm_mm;
 	struct page *page;
@@ -2867,18 +2867,12 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 		 * transferred to avoid any possibility of altering
 		 * permissions across VMAs.
 		 */
-		if (freeze) {
-			swp_entry_t swp_entry;
-			swp_entry = make_migration_entry(page + i, write);
-			entry = swp_entry_to_pte(swp_entry);
-		} else {
-			entry = mk_pte(page + i, vma->vm_page_prot);
-			entry = maybe_mkwrite(entry, vma);
-			if (!write)
-				entry = pte_wrprotect(entry);
-			if (!young)
-				entry = pte_mkold(entry);
-		}
+		entry = mk_pte(page + i, vma->vm_page_prot);
+		entry = maybe_mkwrite(entry, vma);
+		if (!write)
+			entry = pte_wrprotect(entry);
+		if (!young)
+			entry = pte_mkold(entry);
 		if (dirty)
 			SetPageDirty(page + i);
 		pte = pte_offset_map(&_pmd, haddr);
@@ -2931,13 +2925,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 	 */
 	pmdp_invalidate(vma, haddr, pmd);
 	pmd_populate(mm, pmd, pgtable);
-
-	if (freeze) {
-		for (i = 0; i < HPAGE_PMD_NR; i++, haddr += PAGE_SIZE) {
-			page_remove_rmap(page + i, false);
-			put_page(page + i);
-		}
-	}
 }
 
 void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
@@ -2958,7 +2945,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
 			page = NULL;
 	} else if (!pmd_devmap(*pmd))
 		goto out;
-	__split_huge_pmd_locked(vma, pmd, haddr, false);
+	__split_huge_pmd_locked(vma, pmd, haddr);
 out:
 	spin_unlock(ptl);
 	mmu_notifier_invalidate_range_end(mm, haddr, haddr + HPAGE_PMD_SIZE);
@@ -3035,180 +3022,27 @@ void vma_adjust_trans_huge(struct vm_area_struct *vma,
 	}
 }
 
-static void freeze_page_vma(struct vm_area_struct *vma, struct page *page,
-		unsigned long address)
+static void freeze_page(struct page *page)
 {
-	unsigned long haddr = address & HPAGE_PMD_MASK;
-	spinlock_t *ptl;
-	pgd_t *pgd;
-	pud_t *pud;
-	pmd_t *pmd;
-	pte_t *pte;
-	int i, nr = HPAGE_PMD_NR;
-
-	/* Skip pages which doesn't belong to the VMA */
-	if (address < vma->vm_start) {
-		int off = (vma->vm_start - address) >> PAGE_SHIFT;
-		page += off;
-		nr -= off;
-		address = vma->vm_start;
-	}
-
-	pgd = pgd_offset(vma->vm_mm, address);
-	if (!pgd_present(*pgd))
-		return;
-	pud = pud_offset(pgd, address);
-	if (!pud_present(*pud))
-		return;
-	pmd = pmd_offset(pud, address);
-	ptl = pmd_lock(vma->vm_mm, pmd);
-	if (!pmd_present(*pmd)) {
-		spin_unlock(ptl);
-		return;
-	}
-	if (pmd_trans_huge(*pmd)) {
-		if (page == pmd_page(*pmd))
-			__split_huge_pmd_locked(vma, pmd, haddr, true);
-		spin_unlock(ptl);
-		return;
-	}
-	spin_unlock(ptl);
-
-	pte = pte_offset_map_lock(vma->vm_mm, pmd, address, &ptl);
-	for (i = 0; i < nr; i++, address += PAGE_SIZE, page++, pte++) {
-		pte_t entry, swp_pte;
-		swp_entry_t swp_entry;
-
-		/*
-		 * We've just crossed page table boundary: need to map next one.
-		 * It can happen if THP was mremaped to non PMD-aligned address.
-		 */
-		if (unlikely(address == haddr + HPAGE_PMD_SIZE)) {
-			pte_unmap_unlock(pte - 1, ptl);
-			pmd = mm_find_pmd(vma->vm_mm, address);
-			if (!pmd)
-				return;
-			pte = pte_offset_map_lock(vma->vm_mm, pmd,
-					address, &ptl);
-		}
-
-		if (!pte_present(*pte))
-			continue;
-		if (page_to_pfn(page) != pte_pfn(*pte))
-			continue;
-		flush_cache_page(vma, address, page_to_pfn(page));
-		entry = ptep_clear_flush(vma, address, pte);
-		if (pte_dirty(entry))
-			SetPageDirty(page);
-		swp_entry = make_migration_entry(page, pte_write(entry));
-		swp_pte = swp_entry_to_pte(swp_entry);
-		if (pte_soft_dirty(entry))
-			swp_pte = pte_swp_mksoft_dirty(swp_pte);
-		set_pte_at(vma->vm_mm, address, pte, swp_pte);
-		page_remove_rmap(page, false);
-		put_page(page);
-	}
-	pte_unmap_unlock(pte - 1, ptl);
-}
-
-static void freeze_page(struct anon_vma *anon_vma, struct page *page)
-{
-	struct anon_vma_chain *avc;
-	pgoff_t pgoff = page_to_pgoff(page);
+	enum ttu_flags ttu_flags = TTU_MIGRATION | TTU_IGNORE_MLOCK |
+		TTU_IGNORE_ACCESS | TTU_RMAP_LOCKED;
+	int i, ret;
 
 	VM_BUG_ON_PAGE(!PageHead(page), page);
 
-	anon_vma_interval_tree_foreach(avc, &anon_vma->rb_root, pgoff,
-			pgoff + HPAGE_PMD_NR - 1) {
-		unsigned long address = __vma_address(page, avc->vma);
-
-		mmu_notifier_invalidate_range_start(avc->vma->vm_mm,
-				address, address + HPAGE_PMD_SIZE);
-		freeze_page_vma(avc->vma, page, address);
-		mmu_notifier_invalidate_range_end(avc->vma->vm_mm,
-				address, address + HPAGE_PMD_SIZE);
-	}
-}
-
-static void unfreeze_page_vma(struct vm_area_struct *vma, struct page *page,
-		unsigned long address)
-{
-	spinlock_t *ptl;
-	pmd_t *pmd;
-	pte_t *pte, entry;
-	swp_entry_t swp_entry;
-	unsigned long haddr = address & HPAGE_PMD_MASK;
-	int i, nr = HPAGE_PMD_NR;
-
-	/* Skip pages which doesn't belong to the VMA */
-	if (address < vma->vm_start) {
-		int off = (vma->vm_start - address) >> PAGE_SHIFT;
-		page += off;
-		nr -= off;
-		address = vma->vm_start;
-	}
-
-	pmd = mm_find_pmd(vma->vm_mm, address);
-	if (!pmd)
-		return;
-
-	pte = pte_offset_map_lock(vma->vm_mm, pmd, address, &ptl);
-	for (i = 0; i < nr; i++, address += PAGE_SIZE, page++, pte++) {
-		/*
-		 * We've just crossed page table boundary: need to map next one.
-		 * It can happen if THP was mremaped to non-PMD aligned address.
-		 */
-		if (unlikely(address == haddr + HPAGE_PMD_SIZE)) {
-			pte_unmap_unlock(pte - 1, ptl);
-			pmd = mm_find_pmd(vma->vm_mm, address);
-			if (!pmd)
-				return;
-			pte = pte_offset_map_lock(vma->vm_mm, pmd,
-					address, &ptl);
-		}
-
-		if (!is_swap_pte(*pte))
-			continue;
-
-		swp_entry = pte_to_swp_entry(*pte);
-		if (!is_migration_entry(swp_entry))
-			continue;
-		if (migration_entry_to_page(swp_entry) != page)
-			continue;
-
-		get_page(page);
-		page_add_anon_rmap(page, vma, address, false);
-
-		entry = pte_mkold(mk_pte(page, vma->vm_page_prot));
-		if (PageDirty(page))
-			entry = pte_mkdirty(entry);
-		if (is_write_migration_entry(swp_entry))
-			entry = maybe_mkwrite(entry, vma);
-
-		flush_dcache_page(page);
-		set_pte_at(vma->vm_mm, address, pte, entry);
-
-		/* No need to invalidate - it was non-present before */
-		update_mmu_cache(vma, address, pte);
-	}
-	pte_unmap_unlock(pte - 1, ptl);
+	/* We only need TTU_SPLIT_HUGE_PMD once */
+	ret = try_to_unmap(page, ttu_flags | TTU_SPLIT_HUGE_PMD);
+	for (i = 1; !ret && i < HPAGE_PMD_NR; i++)
+		ret = try_to_unmap(page + i, ttu_flags);
+	VM_BUG_ON(ret);
 }
 
-static void unfreeze_page(struct anon_vma *anon_vma, struct page *page)
+static void unfreeze_page(struct page *page)
 {
-	struct anon_vma_chain *avc;
-	pgoff_t pgoff = page_to_pgoff(page);
-
-	anon_vma_interval_tree_foreach(avc, &anon_vma->rb_root,
-			pgoff, pgoff + HPAGE_PMD_NR - 1) {
-		unsigned long address = __vma_address(page, avc->vma);
+	int i;
 
-		mmu_notifier_invalidate_range_start(avc->vma->vm_mm,
-				address, address + HPAGE_PMD_SIZE);
-		unfreeze_page_vma(avc->vma, page, address);
-		mmu_notifier_invalidate_range_end(avc->vma->vm_mm,
-				address, address + HPAGE_PMD_SIZE);
-	}
+	for (i = 0; i < HPAGE_PMD_NR; i++)
+		remove_migration_ptes(page + i, page + i, true);
 }
 
 static void __split_huge_page_tail(struct page *head, int tail,
@@ -3286,7 +3120,7 @@ static void __split_huge_page(struct page *page, struct list_head *list)
 	ClearPageCompound(head);
 	spin_unlock_irq(&zone->lru_lock);
 
-	unfreeze_page(page_anon_vma(head), head);
+	unfreeze_page(head);
 
 	for (i = 0; i < HPAGE_PMD_NR; i++) {
 		struct page *subpage = head + i;
@@ -3382,7 +3216,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
 	}
 
 	mlocked = PageMlocked(page);
-	freeze_page(anon_vma, head);
+	freeze_page(head);
 	VM_BUG_ON_PAGE(compound_mapcount(head), head);
 
 	/* Make sure the page is not on per-CPU pagevec as it takes pin */
@@ -3411,7 +3245,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
 		BUG();
 	} else {
 		spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
-		unfreeze_page(anon_vma, head);
+		unfreeze_page(head);
 		ret = -EBUSY;
 	}
 
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1325557 — Re: [PATCH 4/4] thp: rewrite freeze_page()/unfreeze_page() with generic rmap walkers

On 02/03/2016 07:14 AM, Kirill A. Shutemov wrote:
> But the new variant is somewhat slower. Current helpers iterates over
> VMAs the compound page is mapped to, and then over ptes within this VMA.
> New helpers iterates over small page, then over VMA the small page
> mapped to, and only then find relevant pte.

The code simplification here is really attractive.  Can you quantify
what the slowdown is?  Is it noticeable, or would it be in the noise
during all the other stuff that happens under memory pressure?

[toc] | [prev] | [next] | [standalone]

#1326069 — Re: [PATCH 4/4] thp: rewrite freeze_page()/unfreeze_page() with generic rmap walkers

On Wed, 3 Feb 2016 07:42:01 -0800 Dave Hansen <dave.hansen@intel.com> wrote:

> On 02/03/2016 07:14 AM, Kirill A. Shutemov wrote:
> > But the new variant is somewhat slower. Current helpers iterates over
> > VMAs the compound page is mapped to, and then over ptes within this VMA.
> > New helpers iterates over small page, then over VMA the small page
> > mapped to, and only then find relevant pte.
> 
> The code simplification here is really attractive.  Can you quantify
> what the slowdown is?  Is it noticeable, or would it be in the noise
> during all the other stuff that happens under memory pressure?

yup.  And the "more testing is required" is a bit worrisome.  Is this
code really ready for getting pounded upon in -next?

[toc] | [prev] | [next] | [standalone]

#1326082 — Re: [PATCH 4/4] thp: rewrite freeze_page()/unfreeze_page() with generic rmap walkers

On Wed, Feb 03, 2016 at 02:43:16PM -0800, Andrew Morton wrote:
> On Wed, 3 Feb 2016 07:42:01 -0800 Dave Hansen <dave.hansen@intel.com> wrote:
> 
> > On 02/03/2016 07:14 AM, Kirill A. Shutemov wrote:
> > > But the new variant is somewhat slower. Current helpers iterates over
> > > VMAs the compound page is mapped to, and then over ptes within this VMA.
> > > New helpers iterates over small page, then over VMA the small page
> > > mapped to, and only then find relevant pte.
> > 
> > The code simplification here is really attractive.  Can you quantify
> > what the slowdown is?  Is it noticeable, or would it be in the noise
> > during all the other stuff that happens under memory pressure?
> 
> yup.

It is not really clear, how to quantify this properly. Let me think more
about it.

> And the "more testing is required" is a bit worrisome.  Is this
> code really ready for getting pounded upon in -next?

By now it survived 5+ hours of fuzzing in 16 VM in parallel. I'll continue
with other tests tomorrow.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1326873 — Re: [PATCH 4/4] thp: rewrite freeze_page()/unfreeze_page() with generic rmap walkers

On Wed, Feb 03, 2016 at 07:42:01AM -0800, Dave Hansen wrote:
> On 02/03/2016 07:14 AM, Kirill A. Shutemov wrote:
> > But the new variant is somewhat slower. Current helpers iterates over
> > VMAs the compound page is mapped to, and then over ptes within this VMA.
> > New helpers iterates over small page, then over VMA the small page
> > mapped to, and only then find relevant pte.
> 
> The code simplification here is really attractive.  Can you quantify
> what the slowdown is?  Is it noticeable, or would it be in the noise
> during all the other stuff that happens under memory pressure?

I don't know how to quantify it within whole memory pressure picture.
There're just too many variables to get some sense from split_huge_page()
contribution.

I've tried to measure split_huge_page() performance itself.

Testcase:

	#define _GNU_SOURCE
	#include <stdio.h>
	#include <stdlib.h>
	#include <unistd.h>
	#include <sys/mman.h>

	#define MB (1024UL * 1024)
	#define SIZE (4 * 1024 * 2 * MB)
	#define BASE ((void *)0x400000000000)

	#define FORKS 0

	int main()
	{
		char *p;
		unsigned long i;

		p = mmap(BASE, SIZE, PROT_READ | PROT_WRITE,
				MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS | MAP_POPULATE,
				-1, 0);
		if (p == MAP_FAILED)
			perror("mmap"), exit(1);

		for (i = 0; i < SIZE; i += 2 * MB) {
			munmap(p + i, 4096);
		}

		for (i = 0; i < FORKS; i++) {
			if (!fork())
				pause();
		}

		system("grep thp /proc/vmstat");
		system("time /bin/echo 3 > /proc/sys/vm/drop_caches");
		system("grep thp /proc/vmstat");
		return 0;
	}

Basically, we allocate 4k THP, make them partially unmapped, optionally
fork() the process multiple times and then trigger shrinker, measuring how
long would it take.

Optional fork() will make THP shared, meaning we need to freeze/unfreeze
ptes in multiple VMAs.

Numbers doesn't look pretty:

		FORKS == 0		FORKS == 100
Baseline:	1.93s ± 0.017s		32.08s ± 0.246s
Patched:	5.636s ± 0.021s		405.943s ± 6.126s
Slowdown:	2.92x			12.65x

With FORKS == 100, it looks especially bad. But having that many mapping
of the page is uncommon.

Any comments?

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1327359 — Re: [PATCH 4/4] thp: rewrite freeze_page()/unfreeze_page() with generic rmap walkers

On Wed, Feb 03, 2016 at 07:42:01AM -0800, Dave Hansen wrote:
> On 02/03/2016 07:14 AM, Kirill A. Shutemov wrote:
> > But the new variant is somewhat slower. Current helpers iterates over
> > VMAs the compound page is mapped to, and then over ptes within this VMA.
> > New helpers iterates over small page, then over VMA the small page
> > mapped to, and only then find relevant pte.
> 
> The code simplification here is really attractive.  Can you quantify
> what the slowdown is?  Is it noticeable, or would it be in the noise
> during all the other stuff that happens under memory pressure?

Okay, here's more realistic scenario: migration 8GiB worth of THP.

Testcase:
	#define _GNU_SOURCE
	#include <stdio.h>
	#include <stdlib.h>
	#include <unistd.h>
	#include <sys/mman.h>
	#include <linux/mempolicy.h>
	#include <numaif.h>

	#define MB (1024UL * 1024)
	#define SIZE (4 * 1024 * 2 * MB)
	#define BASE ((void *)0x400000000000)

	#include <time.h>

	void timespec_diff(struct timespec *start, struct timespec *stop,
			struct timespec *result)
	{
		if ((stop->tv_nsec - start->tv_nsec) < 0) {
			result->tv_sec = stop->tv_sec - start->tv_sec - 1;
			result->tv_nsec = stop->tv_nsec - start->tv_nsec + 1000000000;
		} else {
			result->tv_sec = stop->tv_sec - start->tv_sec;
			result->tv_nsec = stop->tv_nsec - start->tv_nsec;
		}
	}

	int main()
	{
		char *p;
		unsigned long ret, node_mask;
		struct timespec start, stop, result;

		node_mask = 0b01;
		ret = set_mempolicy(MPOL_BIND, &node_mask, 64);
		if (ret == -1)
			perror("set_mempolicy"), exit(1);
		p = mmap(BASE, SIZE, PROT_READ | PROT_WRITE,
				MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS | MAP_POPULATE,
				-1, 0);
		if (p == MAP_FAILED)
			perror("mmap"), exit(1);

		system("grep thp /proc/vmstat");
		clock_gettime(CLOCK_MONOTONIC, &start);
		node_mask = 0b10;
		ret = mbind(p, SIZE, MPOL_BIND, &node_mask, 64, MPOL_MF_MOVE);
		if (ret == -1)
			perror("mbind"), exit(1);
		clock_gettime(CLOCK_MONOTONIC, &stop);
		system("grep thp /proc/vmstat");

		timespec_diff(&start, &stop, &result);
		printf("--------------------------\n");
		printf("%ld.%09lds\n", result.tv_sec, result.tv_nsec);

		return 0;
	}

Baseline: 25.146 ± 0.141
Patched:  28.684 ± 0.298
Slowdown: 1.14x

Can we tolerate this?

-- 
 Kirill A. Shutemov

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web