[PATCH 0/4] Remove/consolidate calls to find_get_entry

[PATCH 0/4] Remove/consolidate calls to find_get_entry

[Matthew Wilcox (Oracle)]

As part of the THP work, I audited the callers of find_get_entry() and
found one bug and three opportunities for optimisation.  Patch 2 is the
bugfix as it depends on using the same code as mincore_page().

I don't really like having find_get_swap_page() in mincore.c, but I
can't find anywhere better to put it.

Matthew Wilcox (Oracle) (4):
  mm: Factor find_get_swap_page out of mincore_page
  mm: Use find_get_swap_page in memcontrol
  mm: Optimise madvise WILLNEED
  proc: Optimise smaps for shmem entries

 fs/proc/task_mmu.c      |  8 +-----
 include/linux/pagemap.h |  1 +
 mm/madvise.c            | 20 +++++++-------
 mm/memcontrol.c         | 25 ++---------------
 mm/mincore.c            | 59 +++++++++++++++++++++++------------------
 5 files changed, 47 insertions(+), 66 deletions(-)

-- 
2.27.0

[PATCH 1/4] mm: Factor find_get_swap_page out of mincore_page

[Matthew Wilcox (Oracle)]

This function can also be used elsewhere.  I've left it in mincore.c for
now because I can't find a better place to put it.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/pagemap.h |  1 +
 mm/mincore.c            | 59 +++++++++++++++++++++++------------------
 2 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index cf2468da68e9..b921581a7dcf 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -386,6 +386,7 @@ static inline struct page *find_subpage(struct page *head, pgoff_t index)
 
 struct page *find_get_entry(struct address_space *mapping, pgoff_t offset);
 struct page *find_lock_entry(struct address_space *mapping, pgoff_t offset);
+struct page *find_get_swap_page(struct address_space *mapping, pgoff_t index);
 unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
 			  unsigned int nr_entries, struct page **entries,
 			  pgoff_t *indices);
diff --git a/mm/mincore.c b/mm/mincore.c
index 453ff112470f..abc24ca6f0f7 100644
--- a/mm/mincore.c
+++ b/mm/mincore.c
@@ -42,13 +42,44 @@ static int mincore_hugetlb(pte_t *pte, unsigned long hmask, unsigned long addr,
 	return 0;
 }
 
+/**
+ * find_get_swap_page - Find and get a page from the page or swap caches.
+ * @mapping: The address_space to search.
+ * @index: The page cache index.
+ *
+ * This is like find_get_page(), but if we find a swap entry for
+ * shmem/tmpfs, we also look in the swap cache for the page.
+ *
+ * Return: The found page or %NULL.
+ */
+struct page *find_get_swap_page(struct address_space *mapping, pgoff_t index)
+{
+	swp_entry_t swp;
+	struct swap_info_struct *si;
+	struct page *page = find_get_entry(mapping, index);
+
+	if (!xa_is_value(page))
+		return page;
+	if (!IS_ENABLED(CONFIG_SWAP) || !shmem_mapping(mapping))
+		return NULL;
+
+	swp = radix_to_swp_entry(page);
+	/* Prevent swapoff from happening to us */
+	si = get_swap_device(swp);
+	if (!si)
+		return NULL;
+	page = find_get_page(swap_address_space(swp), swp_offset(swp));
+	put_swap_device(si);
+	return page;
+}
+
 /*
  * Later we can get more picky about what "in core" means precisely.
  * For now, simply check to see if the page is in the page cache,
  * and is up to date; i.e. that no page-in operation would be required
  * at this time if an application were to map and access this page.
  */
-static unsigned char mincore_page(struct address_space *mapping, pgoff_t pgoff)
+static unsigned char mincore_page(struct address_space *mapping, pgoff_t index)
 {
 	unsigned char present = 0;
 	struct page *page;
@@ -59,31 +90,7 @@ static unsigned char mincore_page(struct address_space *mapping, pgoff_t pgoff)
 	 * any other file mapping (ie. marked !present and faulted in with
 	 * tmpfs's .fault). So swapped out tmpfs mappings are tested here.
 	 */
-#ifdef CONFIG_SWAP
-	if (shmem_mapping(mapping)) {
-		page = find_get_entry(mapping, pgoff);
-		/*
-		 * shmem/tmpfs may return swap: account for swapcache
-		 * page too.
-		 */
-		if (xa_is_value(page)) {
-			swp_entry_t swp = radix_to_swp_entry(page);
-			struct swap_info_struct *si;
-
-			/* Prevent swap device to being swapoff under us */
-			si = get_swap_device(swp);
-			if (si) {
-				page = find_get_page(swap_address_space(swp),
-						     swp_offset(swp));
-				put_swap_device(si);
-			} else
-				page = NULL;
-		}
-	} else
-		page = find_get_page(mapping, pgoff);
-#else
-	page = find_get_page(mapping, pgoff);
-#endif
+	page = find_get_swap_page(mapping, index);
 	if (page) {
 		present = PageUptodate(page);
 		put_page(page);
-- 
2.27.0

[PATCH 2/4] mm: Use find_get_swap_page in memcontrol

[Matthew Wilcox (Oracle)]

The current code does not protect against swapoff of the underlying
swap device, so this is a bug fix as well as a worthwhile reduction in
code complexity.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/memcontrol.c | 25 ++-----------------------
 1 file changed, 2 insertions(+), 23 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 19622328e4b5..8b88a1d150c4 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5315,35 +5315,14 @@ static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
 static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
 			unsigned long addr, pte_t ptent, swp_entry_t *entry)
 {
-	struct page *page = NULL;
-	struct address_space *mapping;
-	pgoff_t pgoff;
-
 	if (!vma->vm_file) /* anonymous vma */
 		return NULL;
 	if (!(mc.flags & MOVE_FILE))
 		return NULL;
 
-	mapping = vma->vm_file->f_mapping;
-	pgoff = linear_page_index(vma, addr);
-
 	/* page is moved even if it's not RSS of this task(page-faulted). */
-#ifdef CONFIG_SWAP
-	/* shmem/tmpfs may report page out on swap: account for that too. */
-	if (shmem_mapping(mapping)) {
-		page = find_get_entry(mapping, pgoff);
-		if (xa_is_value(page)) {
-			swp_entry_t swp = radix_to_swp_entry(page);
-			*entry = swp;
-			page = find_get_page(swap_address_space(swp),
-					     swp_offset(swp));
-		}
-	} else
-		page = find_get_page(mapping, pgoff);
-#else
-	page = find_get_page(mapping, pgoff);
-#endif
-	return page;
+	return find_get_swap_page(vma->vm_file->f_mapping,
+			linear_page_index(vma, addr));
 }
 
 /**
-- 
2.27.0

[PATCH 3/4] mm: Optimise madvise WILLNEED

[Matthew Wilcox (Oracle)]

Instead of calling find_get_entry() for every page index, use an XArray
iterator to skip over NULL entries, and avoid calling get_page(),
because we only want the swap entries.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/madvise.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index dd1d43cf026d..9f46d2fd79d1 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -224,25 +224,25 @@ static void force_shm_swapin_readahead(struct vm_area_struct *vma,
 		unsigned long start, unsigned long end,
 		struct address_space *mapping)
 {
-	pgoff_t index;
+	XA_STATE(xas, &mapping->i_pages, linear_page_index(vma, start));
+	pgoff_t end_index = end / PAGE_SIZE;
 	struct page *page;
-	swp_entry_t swap;
 
-	for (; start < end; start += PAGE_SIZE) {
-		index = ((start - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
-
-		page = find_get_entry(mapping, index);
-		if (!xa_is_value(page)) {
-			if (page)
-				put_page(page);
+	rcu_read_lock();
+	xas_for_each(&xas, page, end_index) {
+		swp_entry_t swap;
+		if (!xa_is_value(page))
 			continue;
-		}
+
 		swap = radix_to_swp_entry(page);
 		page = read_swap_cache_async(swap, GFP_HIGHUSER_MOVABLE,
 							NULL, 0, false);
 		if (page)
 			put_page(page);
+		rcu_read_lock();
+		xas_reset(&xas);
 	}
+	rcu_read_unlock();
 
 	lru_add_drain();	/* Push any new pages onto the LRU now */
 }
-- 
2.27.0

[PATCH 4/4] proc: Optimise smaps for shmem entries

[Matthew Wilcox (Oracle)]

Avoid bumping the refcount on pages when we're only interested in the
swap entries.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/proc/task_mmu.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index dbda4499a859..7346f7cbbcc1 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -520,16 +520,10 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr,
 			page = device_private_entry_to_page(swpent);
 	} else if (unlikely(IS_ENABLED(CONFIG_SHMEM) && mss->check_shmem_swap
 							&& pte_none(*pte))) {
-		page = find_get_entry(vma->vm_file->f_mapping,
+		page = xa_load(&vma->vm_file->f_mapping->i_pages,
 						linear_page_index(vma, addr));
-		if (!page)
-			return;
-
 		if (xa_is_value(page))
 			mss->swap += PAGE_SIZE;
-		else
-			put_page(page);
-
 		return;
 	}
 
-- 
2.27.0

Re: [PATCH 0/4] Remove/consolidate calls to find_get_entry

[William Kucharski]

Nice cleanups and bug fix.

For the series:
Reviewed-by: William Kucharski <william.kucharski@oracle.com>

> On Jul 10, 2020, at 2:26 PM, Matthew Wilcox (Oracle) <willy@infradead.org> wrote:
> 
> As part of the THP work, I audited the callers of find_get_entry() and
> found one bug and three opportunities for optimisation.  Patch 2 is the
> bugfix as it depends on using the same code as mincore_page().
> 
> I don't really like having find_get_swap_page() in mincore.c, but I
> can't find anywhere better to put it.
> 
> Matthew Wilcox (Oracle) (4):
>  mm: Factor find_get_swap_page out of mincore_page
>  mm: Use find_get_swap_page in memcontrol
>  mm: Optimise madvise WILLNEED
>  proc: Optimise smaps for shmem entries
> 
> fs/proc/task_mmu.c      |  8 +-----
> include/linux/pagemap.h |  1 +
> mm/madvise.c            | 20 +++++++-------
> mm/memcontrol.c         | 25 ++---------------
> mm/mincore.c            | 59 +++++++++++++++++++++++------------------
> 5 files changed, 47 insertions(+), 66 deletions(-)
> 
> -- 
> 2.27.0
> 
> 

[PATCH 5/4] mm: Return head pages from find_get_entry

[Matthew Wilcox]

This was the original destination of the prior patch series.  I have a
few places in the THP patch set which add calls to find_get_entry() and it
annoyed me that I was carefully calling find_subpage() in find_get_entry()
only to immediately call thp_head() to get back to the original subpage.
I'm not sure it's worth applying this as part of the patch series,
which is why I left it out earlier.

There are going to be some other functions which return only head pages.
I currently have a find_get_heads_range_tag() in my tree, which is
probably going to become find_get_entries_range_tag().

--- 8< ---

mm: Return head pages from find_get_entry

All the callers of find_get_entry() call compound_head() to get back to
the head page.  They still do because compound_head() calls are hidden in
such functions as put_page() and lock_page(), but it lets us get rid of
a few calls.

diff --git a/mm/filemap.c b/mm/filemap.c
index f0ae9a6308cb..7e0a7d02e7aa 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1487,9 +1487,9 @@ EXPORT_SYMBOL(page_cache_prev_miss);
 /**
  * find_get_entry - find and get a page cache entry
  * @mapping: the address_space to search
- * @offset: the page cache index
+ * @index: the page cache index
  *
- * Looks up the page cache slot at @mapping & @offset.  If there is a
+ * Looks up the page cache slot at @mapping & @index.  If there is a
  * page cache page, it is returned with an increased refcount.
  *
  * If the slot holds a shadow entry of a previously evicted page, or a
@@ -1497,9 +1497,9 @@ EXPORT_SYMBOL(page_cache_prev_miss);
  *
  * Return: the found page or shadow entry, %NULL if nothing is found.
  */
-struct page *find_get_entry(struct address_space *mapping, pgoff_t offset)
+struct page *find_get_entry(struct address_space *mapping, pgoff_t index)
 {
-	XA_STATE(xas, &mapping->i_pages, offset);
+	XA_STATE(xas, &mapping->i_pages, index);
 	struct page *page;
 
 	rcu_read_lock();
@@ -1527,7 +1527,6 @@ struct page *find_get_entry(struct address_space *mapping, pgoff_t offset)
 		put_page(page);
 		goto repeat;
 	}
-	page = find_subpage(page, offset);
 out:
 	rcu_read_unlock();
 
@@ -1537,9 +1536,9 @@ struct page *find_get_entry(struct address_space *mapping, pgoff_t offset)
 /**
  * find_lock_entry - locate, pin and lock a page cache entry
  * @mapping: the address_space to search
- * @offset: the page cache index
+ * @index: the page cache index
  *
- * Looks up the page cache slot at @mapping & @offset.  If there is a
+ * Looks up the page cache slot at @mapping & @index.  If there is a
  * page cache page, it is returned locked and with an increased
  * refcount.
  *
@@ -1550,21 +1549,22 @@ struct page *find_get_entry(struct address_space *mapping, pgoff_t offset)
  *
  * Return: the found page or shadow entry, %NULL if nothing is found.
  */
-struct page *find_lock_entry(struct address_space *mapping, pgoff_t offset)
+struct page *find_lock_entry(struct address_space *mapping, pgoff_t index)
 {
 	struct page *page;
 
 repeat:
-	page = find_get_entry(mapping, offset);
+	page = find_get_entry(mapping, index);
 	if (page && !xa_is_value(page)) {
 		lock_page(page);
 		/* Has the page been truncated? */
-		if (unlikely(page_mapping(page) != mapping)) {
+		if (unlikely(page->mapping != mapping)) {
 			unlock_page(page);
 			put_page(page);
 			goto repeat;
 		}
-		VM_BUG_ON_PAGE(page_to_pgoff(page) != offset, page);
+		page = find_subpage(page, index);
+		VM_BUG_ON_PAGE(page_to_pgoff(page) != index, page);
 	}
 	return page;
 }
@@ -1620,12 +1620,13 @@ struct page *pagecache_get_page(struct address_space *mapping, pgoff_t index,
 		}
 
 		/* Has the page been truncated? */
-		if (unlikely(compound_head(page)->mapping != mapping)) {
+		if (unlikely(page->mapping != mapping)) {
 			unlock_page(page);
 			put_page(page);
 			goto repeat;
 		}
-		VM_BUG_ON_PAGE(page->index != index, page);
+		VM_BUG_ON_PAGE(page->index !=
+				(index & ~(thp_nr_pages(page) - 1)), page);
 	}
 
 	if (fgp_flags & FGP_ACCESSED)
@@ -1666,7 +1667,7 @@ struct page *pagecache_get_page(struct address_space *mapping, pgoff_t index,
 			unlock_page(page);
 	}
 
-	return page;
+	return find_subpage(page, index);
 }
 EXPORT_SYMBOL(pagecache_get_page);
 
diff --git a/mm/mincore.c b/mm/mincore.c
index abc24ca6f0f7..cda857110d44 100644
--- a/mm/mincore.c
+++ b/mm/mincore.c
@@ -58,8 +58,10 @@ struct page *find_get_swap_page(struct address_space *mapping, pgoff_t index)
 	struct swap_info_struct *si;
 	struct page *page = find_get_entry(mapping, index);
 
-	if (!xa_is_value(page))
+	if (!page)
 		return page;
+	if (!xa_is_value(page))
+		return find_subpage(page, index);
 	if (!IS_ENABLED(CONFIG_SWAP) || !shmem_mapping(mapping))
 		return NULL;