[v2 PATCH] mm: thp: check total_mapcount instead of page_mapcount

[v2 PATCH] mm: thp: check total_mapcount instead of page_mapcount

[Yang Shi]

When debugging the bug reported by Wang Yugui [1], try_to_unmap() may
return false positive for PTE-mapped THP since page_mapcount() is used
to check if the THP is unmapped, but it just checks compound mapount and
head page's mapcount.  If the THP is PTE-mapped and head page is not
mapped, it may return false positive.

Use total_mapcount() instead of page_mapcount() for try_to_unmap() and
do so for the VM_BUG_ON_PAGE in split_huge_page_to_list as well.

This changed the semantic of try_to_unmap(), but I don't see there is
any usecase that expects try_to_unmap() just unmap one subpage of a huge
page.  So using page_mapcount() seems like a bug.

[1] https://lore.kernel.org/linux-mm/20210412180659.B9E3.409509F4@e16-tech.com/

Signed-off-by: Yang Shi <shy828301@gmail.com>
---
v2: Removed dead code and updated the comment of try_to_unmap() per Zi
    Yan.

 mm/huge_memory.c | 11 +----------
 mm/rmap.c        | 10 ++++++----
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 63ed6b25deaa..3b08b9ba1578 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2348,7 +2348,6 @@ static void unmap_page(struct page *page)
 		ttu_flags |= TTU_SPLIT_FREEZE;
 
 	unmap_success = try_to_unmap(page, ttu_flags);
-	VM_BUG_ON_PAGE(!unmap_success, page);
 }
 
 static void remap_page(struct page *page, unsigned int nr)
@@ -2718,7 +2717,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
 	}
 
 	unmap_page(head);
-	VM_BUG_ON_PAGE(compound_mapcount(head), head);
+	VM_BUG_ON_PAGE(total_mapcount(head), head);
 
 	/* block interrupt reentry in xa_lock and spinlock */
 	local_irq_disable();
@@ -2758,14 +2757,6 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
 		__split_huge_page(page, list, end);
 		ret = 0;
 	} else {
-		if (IS_ENABLED(CONFIG_DEBUG_VM) && mapcount) {
-			pr_alert("total_mapcount: %u, page_count(): %u\n",
-					mapcount, count);
-			if (PageTail(page))
-				dump_page(head, NULL);
-			dump_page(page, "total_mapcount(head) > 0");
-			BUG();
-		}
 		spin_unlock(&ds_queue->split_queue_lock);
 fail:		if (mapping)
 			xa_unlock(&mapping->i_pages);
diff --git a/mm/rmap.c b/mm/rmap.c
index 693a610e181d..f52825b1330d 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1742,12 +1742,14 @@ static int page_not_mapped(struct page *page)
 }
 
 /**
- * try_to_unmap - try to remove all page table mappings to a page
- * @page: the page to get unmapped
+ * try_to_unmap - try to remove all page table mappings to a page and the
+ *                compound page it belongs to
+ * @page: the page or the subpages of compound page to get unmapped
  * @flags: action and flags
  *
  * Tries to remove all the page table entries which are mapping this
- * page, used in the pageout path.  Caller must hold the page lock.
+ * page and the compound page it belongs to, used in the pageout path.
+ * Caller must hold the page lock.
  *
  * If unmap is successful, return true. Otherwise, false.
  */
@@ -1777,7 +1779,7 @@ bool try_to_unmap(struct page *page, enum ttu_flags flags)
 	else
 		rmap_walk(page, &rwc);
 
-	return !page_mapcount(page) ? true : false;
+	return !total_mapcount(page) ? true : false;
 }
 
 /**
-- 
2.26.2

Re: [v2 PATCH] mm: thp: check total_mapcount instead of page_mapcount

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]

On 13 May 2021, at 17:23, Yang Shi wrote:

> When debugging the bug reported by Wang Yugui [1], try_to_unmap() may
> return false positive for PTE-mapped THP since page_mapcount() is used
> to check if the THP is unmapped, but it just checks compound mapount and
> head page's mapcount.  If the THP is PTE-mapped and head page is not
> mapped, it may return false positive.
>
> Use total_mapcount() instead of page_mapcount() for try_to_unmap() and
> do so for the VM_BUG_ON_PAGE in split_huge_page_to_list as well.
>
> This changed the semantic of try_to_unmap(), but I don't see there is
> any usecase that expects try_to_unmap() just unmap one subpage of a huge
> page.  So using page_mapcount() seems like a bug.
>
> [1] https://lore.kernel.org/linux-mm/20210412180659.B9E3.409509F4@e16-tech.com/
>
> Signed-off-by: Yang Shi <shy828301@gmail.com>
> ---
> v2: Removed dead code and updated the comment of try_to_unmap() per Zi
>     Yan.
>
>  mm/huge_memory.c | 11 +----------
>  mm/rmap.c        | 10 ++++++----
>  2 files changed, 7 insertions(+), 14 deletions(-)

LGTM. Thanks. Reviewed-by: Zi Yan <ziy@nvidia.com>

—
Best Regards,
Yan Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [v2 PATCH] mm: thp: check total_mapcount instead of page_mapcount

[Hugh Dickins]

On Thu, 13 May 2021, Yang Shi wrote:

> When debugging the bug reported by Wang Yugui [1], try_to_unmap() may
> return false positive for PTE-mapped THP since page_mapcount() is used
> to check if the THP is unmapped, but it just checks compound mapount and
> head page's mapcount.  If the THP is PTE-mapped and head page is not
> mapped, it may return false positive.
> 
> Use total_mapcount() instead of page_mapcount() for try_to_unmap() and
> do so for the VM_BUG_ON_PAGE in split_huge_page_to_list as well.
> 
> This changed the semantic of try_to_unmap(), but I don't see there is
> any usecase that expects try_to_unmap() just unmap one subpage of a huge
> page.  So using page_mapcount() seems like a bug.
> 
> [1] https://lore.kernel.org/linux-mm/20210412180659.B9E3.409509F4@e16-tech.com/
> 
> Signed-off-by: Yang Shi <shy828301@gmail.com>

I don't object to this patch, I've no reason to NAK it; but I'll
point out a few deficiencies which might make you want to revisit it.

> ---
> v2: Removed dead code and updated the comment of try_to_unmap() per Zi
>     Yan.
> 
>  mm/huge_memory.c | 11 +----------
>  mm/rmap.c        | 10 ++++++----
>  2 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 63ed6b25deaa..3b08b9ba1578 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2348,7 +2348,6 @@ static void unmap_page(struct page *page)
>  		ttu_flags |= TTU_SPLIT_FREEZE;
>  
>  	unmap_success = try_to_unmap(page, ttu_flags);
> -	VM_BUG_ON_PAGE(!unmap_success, page);

The unused variable unmap_success has already been reported and
dealt with.  But I couldn't tell what you intended: why change
try_to_unmap()'s output, if you then ignore it?

>  }
>  
>  static void remap_page(struct page *page, unsigned int nr)
> @@ -2718,7 +2717,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>  	}
>  
>  	unmap_page(head);
> -	VM_BUG_ON_PAGE(compound_mapcount(head), head);
> +	VM_BUG_ON_PAGE(total_mapcount(head), head);

And having forced try_to_unmap() to do the expensive-on-a-THP
total_mapcount() calculation, you now repeat it here.  Better
to stick with the previous VM_BUG_ON_PAGE(!unmap_success).

Or better a VM_WARN_ONCE(), accompanied by dump_page()s as before,
to get some perhaps useful info out, which this patch has deleted.
Probably better inside unmap_page() than cluttering up here.

VM_WARN_ONCE() because nothing in this patch fixes whatever Wang
Yugui is suffering from; and (aside from the BUG()) it's harmless,
because there are other ways in which the page_ref_freeze() can fail,
and that is allowed for.  We would like to know when this problem
occurs: there is something wrong, but no reason to crash.

>  
>  	/* block interrupt reentry in xa_lock and spinlock */
>  	local_irq_disable();
> @@ -2758,14 +2757,6 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>  		__split_huge_page(page, list, end);
>  		ret = 0;
>  	} else {
> -		if (IS_ENABLED(CONFIG_DEBUG_VM) && mapcount) {
> -			pr_alert("total_mapcount: %u, page_count(): %u\n",
> -					mapcount, count);
> -			if (PageTail(page))
> -				dump_page(head, NULL);
> -			dump_page(page, "total_mapcount(head) > 0");
> -			BUG();
> -		}

This has always looked ugly (as if Kirill had hit an unsolved case),
so it is nice to remove it; but you're losing the dump_page() info,
and not really gaining anything more than a cosmetic cleanup.

>  		spin_unlock(&ds_queue->split_queue_lock);
>  fail:		if (mapping)
>  			xa_unlock(&mapping->i_pages);
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 693a610e181d..f52825b1330d 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1742,12 +1742,14 @@ static int page_not_mapped(struct page *page)
>  }
>  
>  /**
> - * try_to_unmap - try to remove all page table mappings to a page
> - * @page: the page to get unmapped
> + * try_to_unmap - try to remove all page table mappings to a page and the
> + *                compound page it belongs to
> + * @page: the page or the subpages of compound page to get unmapped
>   * @flags: action and flags
>   *
>   * Tries to remove all the page table entries which are mapping this
> - * page, used in the pageout path.  Caller must hold the page lock.
> + * page and the compound page it belongs to, used in the pageout path.
> + * Caller must hold the page lock.
>   *
>   * If unmap is successful, return true. Otherwise, false.
>   */
> @@ -1777,7 +1779,7 @@ bool try_to_unmap(struct page *page, enum ttu_flags flags)
>  	else
>  		rmap_walk(page, &rwc);
>  
> -	return !page_mapcount(page) ? true : false;
> +	return !total_mapcount(page) ? true : false;

That always made me wince: "return !total_mapcount(page);" surely.

Or slightly better, "return !page_mapped(page);", since at least that
one breaks out as soon as it sees a mapcount.  Though I guess I'm
being silly there, since that case should never occur, so both
total_mapcount() and page_mapped() scan through all pages.

Or better, change try_to_unmap() to void: most callers ignore its
return value anyway, and make their own decisions; the remaining
few could be changed to do the same.  Though again, I may be
being silly, since the expensive THP case is not the common case.

>  }
>  
>  /**
> -- 
> 2.26.2

Re: [v2 PATCH] mm: thp: check total_mapcount instead of page_mapcount

[Yang Shi]

On Thu, May 20, 2021 at 10:06 PM Hugh Dickins <hughd@google.com> wrote:
>
> On Thu, 13 May 2021, Yang Shi wrote:
>
> > When debugging the bug reported by Wang Yugui [1], try_to_unmap() may
> > return false positive for PTE-mapped THP since page_mapcount() is used
> > to check if the THP is unmapped, but it just checks compound mapount and
> > head page's mapcount.  If the THP is PTE-mapped and head page is not
> > mapped, it may return false positive.
> >
> > Use total_mapcount() instead of page_mapcount() for try_to_unmap() and
> > do so for the VM_BUG_ON_PAGE in split_huge_page_to_list as well.
> >
> > This changed the semantic of try_to_unmap(), but I don't see there is
> > any usecase that expects try_to_unmap() just unmap one subpage of a huge
> > page.  So using page_mapcount() seems like a bug.
> >
> > [1] https://lore.kernel.org/linux-mm/20210412180659.B9E3.409509F4@e16-tech.com/
> >
> > Signed-off-by: Yang Shi <shy828301@gmail.com>
>
> I don't object to this patch, I've no reason to NAK it; but I'll
> point out a few deficiencies which might make you want to revisit it.
>
> > ---
> > v2: Removed dead code and updated the comment of try_to_unmap() per Zi
> >     Yan.
> >
> >  mm/huge_memory.c | 11 +----------
> >  mm/rmap.c        | 10 ++++++----
> >  2 files changed, 7 insertions(+), 14 deletions(-)
> >
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 63ed6b25deaa..3b08b9ba1578 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -2348,7 +2348,6 @@ static void unmap_page(struct page *page)
> >               ttu_flags |= TTU_SPLIT_FREEZE;
> >
> >       unmap_success = try_to_unmap(page, ttu_flags);
> > -     VM_BUG_ON_PAGE(!unmap_success, page);
>
> The unused variable unmap_success has already been reported and
> dealt with.  But I couldn't tell what you intended: why change
> try_to_unmap()'s output, if you then ignore it?

Because some other callers of try_to_unmap() check the output.

>
> >  }
> >
> >  static void remap_page(struct page *page, unsigned int nr)
> > @@ -2718,7 +2717,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
> >       }
> >
> >       unmap_page(head);
> > -     VM_BUG_ON_PAGE(compound_mapcount(head), head);
> > +     VM_BUG_ON_PAGE(total_mapcount(head), head);
>
> And having forced try_to_unmap() to do the expensive-on-a-THP
> total_mapcount() calculation, you now repeat it here.  Better
> to stick with the previous VM_BUG_ON_PAGE(!unmap_success).
>
> Or better a VM_WARN_ONCE(), accompanied by dump_page()s as before,
> to get some perhaps useful info out, which this patch has deleted.
> Probably better inside unmap_page() than cluttering up here.

Moving the BUG or WARN into unmap_page() looks fine to me. IIUC,
VM_BUG_ON_PAGE or VM_WARN_ON_PAGE does call dump_page(), so dumping
something useful is not deleted.

>
> VM_WARN_ONCE() because nothing in this patch fixes whatever Wang
> Yugui is suffering from; and (aside from the BUG()) it's harmless,
> because there are other ways in which the page_ref_freeze() can fail,
> and that is allowed for.  We would like to know when this problem
> occurs: there is something wrong, but no reason to crash.

Yes, it fixes nothing. I didn't figure out why try_to_unmap() failed.
I agree BUG_ON could be relaxed.

>
> >
> >       /* block interrupt reentry in xa_lock and spinlock */
> >       local_irq_disable();
> > @@ -2758,14 +2757,6 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
> >               __split_huge_page(page, list, end);
> >               ret = 0;
> >       } else {
> > -             if (IS_ENABLED(CONFIG_DEBUG_VM) && mapcount) {
> > -                     pr_alert("total_mapcount: %u, page_count(): %u\n",
> > -                                     mapcount, count);
> > -                     if (PageTail(page))
> > -                             dump_page(head, NULL);
> > -                     dump_page(page, "total_mapcount(head) > 0");
> > -                     BUG();
> > -             }
>
> This has always looked ugly (as if Kirill had hit an unsolved case),
> so it is nice to remove it; but you're losing the dump_page() info,
> and not really gaining anything more than a cosmetic cleanup.

As I mentioned above, IIUC VM_BUG_ON_PAGE and VM_WARN_ON_PAGE do call
dump_page().

>
> >               spin_unlock(&ds_queue->split_queue_lock);
> >  fail:                if (mapping)
> >                       xa_unlock(&mapping->i_pages);
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index 693a610e181d..f52825b1330d 100644
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -1742,12 +1742,14 @@ static int page_not_mapped(struct page *page)
> >  }
> >
> >  /**
> > - * try_to_unmap - try to remove all page table mappings to a page
> > - * @page: the page to get unmapped
> > + * try_to_unmap - try to remove all page table mappings to a page and the
> > + *                compound page it belongs to
> > + * @page: the page or the subpages of compound page to get unmapped
> >   * @flags: action and flags
> >   *
> >   * Tries to remove all the page table entries which are mapping this
> > - * page, used in the pageout path.  Caller must hold the page lock.
> > + * page and the compound page it belongs to, used in the pageout path.
> > + * Caller must hold the page lock.
> >   *
> >   * If unmap is successful, return true. Otherwise, false.
> >   */
> > @@ -1777,7 +1779,7 @@ bool try_to_unmap(struct page *page, enum ttu_flags flags)
> >       else
> >               rmap_walk(page, &rwc);
> >
> > -     return !page_mapcount(page) ? true : false;
> > +     return !total_mapcount(page) ? true : false;
>
> That always made me wince: "return !total_mapcount(page);" surely.

But page_mapcount() seems not correct, it may return false positive,
right? Or it is harmless?

And I actually spotted a few other places which should use
total_mapcount() but using page_mapcount() instead, for example, some
madvise code check if the page is shared by using page_mapcount(),
however it may return false negative (double mapped THP, but head page
is not PTE-mapped, just like what Wang Yugui reported). It is not
fatal, but not expected behavior. I understand total_mapcount() is
expensive, so is it a trade-off between cost and correctness or just
overlooked the false negative case in the first place? I can't tell.

>
> Or slightly better, "return !page_mapped(page);", since at least that
> one breaks out as soon as it sees a mapcount.  Though I guess I'm
> being silly there, since that case should never occur, so both
> total_mapcount() and page_mapped() scan through all pages.
>
> Or better, change try_to_unmap() to void: most callers ignore its
> return value anyway, and make their own decisions; the remaining
> few could be changed to do the same.  Though again, I may be
> being silly, since the expensive THP case is not the common case.

I'd say half callers ignore its return value. But I think it should be
worth doing. At least we could remove half unnecessary
total_mapcount() or page_mapped() call.

Thanks a lot for all the suggestions, will incorporate them in the new version.

>
> >  }
> >
> >  /**
> > --
> > 2.26.2

Re: [v2 PATCH] mm: thp: check total_mapcount instead of page_mapcount

[Yang Shi]

On Fri, May 21, 2021 at 10:16 AM Yang Shi <shy828301@gmail.com> wrote:
>
> On Thu, May 20, 2021 at 10:06 PM Hugh Dickins <hughd@google.com> wrote:
> >
> > On Thu, 13 May 2021, Yang Shi wrote:
> >
> > > When debugging the bug reported by Wang Yugui [1], try_to_unmap() may
> > > return false positive for PTE-mapped THP since page_mapcount() is used
> > > to check if the THP is unmapped, but it just checks compound mapount and
> > > head page's mapcount.  If the THP is PTE-mapped and head page is not
> > > mapped, it may return false positive.
> > >
> > > Use total_mapcount() instead of page_mapcount() for try_to_unmap() and
> > > do so for the VM_BUG_ON_PAGE in split_huge_page_to_list as well.
> > >
> > > This changed the semantic of try_to_unmap(), but I don't see there is
> > > any usecase that expects try_to_unmap() just unmap one subpage of a huge
> > > page.  So using page_mapcount() seems like a bug.
> > >
> > > [1] https://lore.kernel.org/linux-mm/20210412180659.B9E3.409509F4@e16-tech.com/
> > >
> > > Signed-off-by: Yang Shi <shy828301@gmail.com>
> >
> > I don't object to this patch, I've no reason to NAK it; but I'll
> > point out a few deficiencies which might make you want to revisit it.
> >
> > > ---
> > > v2: Removed dead code and updated the comment of try_to_unmap() per Zi
> > >     Yan.
> > >
> > >  mm/huge_memory.c | 11 +----------
> > >  mm/rmap.c        | 10 ++++++----
> > >  2 files changed, 7 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > > index 63ed6b25deaa..3b08b9ba1578 100644
> > > --- a/mm/huge_memory.c
> > > +++ b/mm/huge_memory.c
> > > @@ -2348,7 +2348,6 @@ static void unmap_page(struct page *page)
> > >               ttu_flags |= TTU_SPLIT_FREEZE;
> > >
> > >       unmap_success = try_to_unmap(page, ttu_flags);
> > > -     VM_BUG_ON_PAGE(!unmap_success, page);
> >
> > The unused variable unmap_success has already been reported and
> > dealt with.  But I couldn't tell what you intended: why change
> > try_to_unmap()'s output, if you then ignore it?
>
> Because some other callers of try_to_unmap() check the output.
>
> >
> > >  }
> > >
> > >  static void remap_page(struct page *page, unsigned int nr)
> > > @@ -2718,7 +2717,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
> > >       }
> > >
> > >       unmap_page(head);
> > > -     VM_BUG_ON_PAGE(compound_mapcount(head), head);
> > > +     VM_BUG_ON_PAGE(total_mapcount(head), head);
> >
> > And having forced try_to_unmap() to do the expensive-on-a-THP
> > total_mapcount() calculation, you now repeat it here.  Better
> > to stick with the previous VM_BUG_ON_PAGE(!unmap_success).
> >
> > Or better a VM_WARN_ONCE(), accompanied by dump_page()s as before,
> > to get some perhaps useful info out, which this patch has deleted.
> > Probably better inside unmap_page() than cluttering up here.
>
> Moving the BUG or WARN into unmap_page() looks fine to me. IIUC,
> VM_BUG_ON_PAGE or VM_WARN_ON_PAGE does call dump_page(), so dumping
> something useful is not deleted.

I misspelled the function name. There is *NOT* VM_WARN_ON_PAGE(), the
name is VM_WARN_ON_ONCE_PAGE(). We may need to add VM_WARN_ON_PAGE()
since I'd like this warning to be printed every time when it is met.

>
> >
> > VM_WARN_ONCE() because nothing in this patch fixes whatever Wang
> > Yugui is suffering from; and (aside from the BUG()) it's harmless,
> > because there are other ways in which the page_ref_freeze() can fail,
> > and that is allowed for.  We would like to know when this problem
> > occurs: there is something wrong, but no reason to crash.
>
> Yes, it fixes nothing. I didn't figure out why try_to_unmap() failed.
> I agree BUG_ON could be relaxed.
>
> >
> > >
> > >       /* block interrupt reentry in xa_lock and spinlock */
> > >       local_irq_disable();
> > > @@ -2758,14 +2757,6 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
> > >               __split_huge_page(page, list, end);
> > >               ret = 0;
> > >       } else {
> > > -             if (IS_ENABLED(CONFIG_DEBUG_VM) && mapcount) {
> > > -                     pr_alert("total_mapcount: %u, page_count(): %u\n",
> > > -                                     mapcount, count);
> > > -                     if (PageTail(page))
> > > -                             dump_page(head, NULL);
> > > -                     dump_page(page, "total_mapcount(head) > 0");
> > > -                     BUG();
> > > -             }
> >
> > This has always looked ugly (as if Kirill had hit an unsolved case),
> > so it is nice to remove it; but you're losing the dump_page() info,
> > and not really gaining anything more than a cosmetic cleanup.
>
> As I mentioned above, IIUC VM_BUG_ON_PAGE and VM_WARN_ON_PAGE do call
> dump_page().
>
> >
> > >               spin_unlock(&ds_queue->split_queue_lock);
> > >  fail:                if (mapping)
> > >                       xa_unlock(&mapping->i_pages);
> > > diff --git a/mm/rmap.c b/mm/rmap.c
> > > index 693a610e181d..f52825b1330d 100644
> > > --- a/mm/rmap.c
> > > +++ b/mm/rmap.c
> > > @@ -1742,12 +1742,14 @@ static int page_not_mapped(struct page *page)
> > >  }
> > >
> > >  /**
> > > - * try_to_unmap - try to remove all page table mappings to a page
> > > - * @page: the page to get unmapped
> > > + * try_to_unmap - try to remove all page table mappings to a page and the
> > > + *                compound page it belongs to
> > > + * @page: the page or the subpages of compound page to get unmapped
> > >   * @flags: action and flags
> > >   *
> > >   * Tries to remove all the page table entries which are mapping this
> > > - * page, used in the pageout path.  Caller must hold the page lock.
> > > + * page and the compound page it belongs to, used in the pageout path.
> > > + * Caller must hold the page lock.
> > >   *
> > >   * If unmap is successful, return true. Otherwise, false.
> > >   */
> > > @@ -1777,7 +1779,7 @@ bool try_to_unmap(struct page *page, enum ttu_flags flags)
> > >       else
> > >               rmap_walk(page, &rwc);
> > >
> > > -     return !page_mapcount(page) ? true : false;
> > > +     return !total_mapcount(page) ? true : false;
> >
> > That always made me wince: "return !total_mapcount(page);" surely.
>
> But page_mapcount() seems not correct, it may return false positive,
> right? Or it is harmless?
>
> And I actually spotted a few other places which should use
> total_mapcount() but using page_mapcount() instead, for example, some
> madvise code check if the page is shared by using page_mapcount(),
> however it may return false negative (double mapped THP, but head page
> is not PTE-mapped, just like what Wang Yugui reported). It is not
> fatal, but not expected behavior. I understand total_mapcount() is
> expensive, so is it a trade-off between cost and correctness or just
> overlooked the false negative case in the first place? I can't tell.
>
> >
> > Or slightly better, "return !page_mapped(page);", since at least that
> > one breaks out as soon as it sees a mapcount.  Though I guess I'm
> > being silly there, since that case should never occur, so both
> > total_mapcount() and page_mapped() scan through all pages.
> >
> > Or better, change try_to_unmap() to void: most callers ignore its
> > return value anyway, and make their own decisions; the remaining
> > few could be changed to do the same.  Though again, I may be
> > being silly, since the expensive THP case is not the common case.
>
> I'd say half callers ignore its return value. But I think it should be
> worth doing. At least we could remove half unnecessary
> total_mapcount() or page_mapped() call.
>
> Thanks a lot for all the suggestions, will incorporate them in the new version.
>
> >
> > >  }
> > >
> > >  /**
> > > --
> > > 2.26.2

Re: [v2 PATCH] mm: thp: check total_mapcount instead of page_mapcount

[Hugh Dickins]

On Fri, 21 May 2021, Yang Shi wrote:
> On Fri, May 21, 2021 at 10:16 AM Yang Shi <shy828301@gmail.com> wrote:
> > On Thu, May 20, 2021 at 10:06 PM Hugh Dickins <hughd@google.com> wrote:
> > > On Thu, 13 May 2021, Yang Shi wrote:
> > >
> > > > When debugging the bug reported by Wang Yugui [1], try_to_unmap() may
> > > > return false positive for PTE-mapped THP since page_mapcount() is used
> > > > to check if the THP is unmapped, but it just checks compound mapount and
> > > > head page's mapcount.  If the THP is PTE-mapped and head page is not
> > > > mapped, it may return false positive.
> > > >
> > > > Use total_mapcount() instead of page_mapcount() for try_to_unmap() and
> > > > do so for the VM_BUG_ON_PAGE in split_huge_page_to_list as well.
> > > >
> > > > This changed the semantic of try_to_unmap(), but I don't see there is
> > > > any usecase that expects try_to_unmap() just unmap one subpage of a huge
> > > > page.  So using page_mapcount() seems like a bug.
> > > >
> > > > [1] https://lore.kernel.org/linux-mm/20210412180659.B9E3.409509F4@e16-tech.com/
> > > >
> > > > Signed-off-by: Yang Shi <shy828301@gmail.com>
> > >
> > > I don't object to this patch, I've no reason to NAK it; but I'll
> > > point out a few deficiencies which might make you want to revisit it.
> > >
> > > > ---
> > > > v2: Removed dead code and updated the comment of try_to_unmap() per Zi
> > > >     Yan.
> > > >
> > > >  mm/huge_memory.c | 11 +----------
> > > >  mm/rmap.c        | 10 ++++++----
> > > >  2 files changed, 7 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > > > index 63ed6b25deaa..3b08b9ba1578 100644
> > > > --- a/mm/huge_memory.c
> > > > +++ b/mm/huge_memory.c
> > > > @@ -2348,7 +2348,6 @@ static void unmap_page(struct page *page)
> > > >               ttu_flags |= TTU_SPLIT_FREEZE;
> > > >
> > > >       unmap_success = try_to_unmap(page, ttu_flags);
> > > > -     VM_BUG_ON_PAGE(!unmap_success, page);
> > >
> > > The unused variable unmap_success has already been reported and
> > > dealt with.  But I couldn't tell what you intended: why change
> > > try_to_unmap()'s output, if you then ignore it?
> >
> > Because some other callers of try_to_unmap() check the output.

memory-failure.c and one in vmscan.c: most callers are not interested,
so replacing a quick atomic_read by a scan of 512 struct pages is not
necessarily a good idea. But I am exaggerating, it's not usually that
bad: probably most of those callers will rarely encounter a THP.

> >
> > >
> > > >  }
> > > >
> > > >  static void remap_page(struct page *page, unsigned int nr)
> > > > @@ -2718,7 +2717,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
> > > >       }
> > > >
> > > >       unmap_page(head);
> > > > -     VM_BUG_ON_PAGE(compound_mapcount(head), head);
> > > > +     VM_BUG_ON_PAGE(total_mapcount(head), head);
> > >
> > > And having forced try_to_unmap() to do the expensive-on-a-THP
> > > total_mapcount() calculation, you now repeat it here.  Better
> > > to stick with the previous VM_BUG_ON_PAGE(!unmap_success).
> > >
> > > Or better a VM_WARN_ONCE(), accompanied by dump_page()s as before,
> > > to get some perhaps useful info out, which this patch has deleted.
> > > Probably better inside unmap_page() than cluttering up here.
> >
> > Moving the BUG or WARN into unmap_page() looks fine to me. IIUC,
> > VM_BUG_ON_PAGE or VM_WARN_ON_PAGE does call dump_page(), so dumping
> > something useful is not deleted.

Yes, you're right, I was forgetting that. The original DEBUG_VM block
does do a separate dump_page() for head and tail (if different), but
I don't think you need to replicate that now: (a) it was more a mark
of frustration at not getting enough info to solve the problem (and
if we really want more info, it's a dump of 512 struct pages needed);
and (b) I think Matthew and others have enhanced dump_page() meanwhile,
to issue some head info when dumping a tail; and (c) most splitters
pass the head anyway, IIRC there's only one or two who pass a tail.

> 
> I misspelled the function name. There is *NOT* VM_WARN_ON_PAGE(), the
> name is VM_WARN_ON_ONCE_PAGE(). We may need to add VM_WARN_ON_PAGE()
> since I'd like this warning to be printed every time when it is met.

I get very confused by all those variants too; and then indeed very
often the one variant you think you want turns out not to exist yet.

But I'm not sure that printing it every time will be good: isn't it
quite likely that the situation is long-lasting, and that the page
is on the anon deferred or shmem unused list, and will keep coming
back to report the same unhelpful info, just cluttering up the logs?
I'd be more comfortable with _ONCE myself.

But I'd also suggest dropping the VM_ part of it: we have no idea
how often this happens on non-DEBUG_VM machines in production,
and a pattern might be revealed there which would help to solve it.

> 
> >
> > >
> > > VM_WARN_ONCE() because nothing in this patch fixes whatever Wang
> > > Yugui is suffering from; and (aside from the BUG()) it's harmless,
> > > because there are other ways in which the page_ref_freeze() can fail,
> > > and that is allowed for.  We would like to know when this problem
> > > occurs: there is something wrong, but no reason to crash.
> >
> > Yes, it fixes nothing. I didn't figure out why try_to_unmap() failed.
> > I agree BUG_ON could be relaxed.

I have to confess, I have known of several reasons why one or other
of those BUG()s can be hit. Sigh. I'll rearrange priorities, research
back through what work I've done and not had time to upstream, filter
out the patches relevant to this "mysterious failure to unmap a THP".

I did not speak up earlier because I did not notice any obvious
connection between Wang Yugui's occurrence, and anything we have
fixed; and (as usual) there's other work I should be doing.  But
it's getting to feel dishonest, having this conversation without
showing our fixes: I'd better sort those out and post (but that
will take me more than a week I expect).

Not saying that you should stop with this patch at all:
no, I can easily rebase on top of what you end up with here.

> >
> > >
> > > >
> > > >       /* block interrupt reentry in xa_lock and spinlock */
> > > >       local_irq_disable();
> > > > @@ -2758,14 +2757,6 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
> > > >               __split_huge_page(page, list, end);
> > > >               ret = 0;
> > > >       } else {
> > > > -             if (IS_ENABLED(CONFIG_DEBUG_VM) && mapcount) {
> > > > -                     pr_alert("total_mapcount: %u, page_count(): %u\n",
> > > > -                                     mapcount, count);
> > > > -                     if (PageTail(page))
> > > > -                             dump_page(head, NULL);
> > > > -                     dump_page(page, "total_mapcount(head) > 0");
> > > > -                     BUG();
> > > > -             }
> > >
> > > This has always looked ugly (as if Kirill had hit an unsolved case),
> > > so it is nice to remove it; but you're losing the dump_page() info,
> > > and not really gaining anything more than a cosmetic cleanup.
> >
> > As I mentioned above, IIUC VM_BUG_ON_PAGE and VM_WARN_ON_PAGE do call
> > dump_page().
> >
> > >
> > > >               spin_unlock(&ds_queue->split_queue_lock);
> > > >  fail:                if (mapping)
> > > >                       xa_unlock(&mapping->i_pages);
> > > > diff --git a/mm/rmap.c b/mm/rmap.c
> > > > index 693a610e181d..f52825b1330d 100644
> > > > --- a/mm/rmap.c
> > > > +++ b/mm/rmap.c
> > > > @@ -1742,12 +1742,14 @@ static int page_not_mapped(struct page *page)
> > > >  }
> > > >
> > > >  /**
> > > > - * try_to_unmap - try to remove all page table mappings to a page
> > > > - * @page: the page to get unmapped
> > > > + * try_to_unmap - try to remove all page table mappings to a page and the
> > > > + *                compound page it belongs to
> > > > + * @page: the page or the subpages of compound page to get unmapped
> > > >   * @flags: action and flags
> > > >   *
> > > >   * Tries to remove all the page table entries which are mapping this
> > > > - * page, used in the pageout path.  Caller must hold the page lock.
> > > > + * page and the compound page it belongs to, used in the pageout path.
> > > > + * Caller must hold the page lock.
> > > >   *
> > > >   * If unmap is successful, return true. Otherwise, false.
> > > >   */
> > > > @@ -1777,7 +1779,7 @@ bool try_to_unmap(struct page *page, enum ttu_flags flags)
> > > >       else
> > > >               rmap_walk(page, &rwc);
> > > >
> > > > -     return !page_mapcount(page) ? true : false;
> > > > +     return !total_mapcount(page) ? true : false;
> > >
> > > That always made me wince: "return !total_mapcount(page);" surely.
> >
> > But page_mapcount() seems not correct, it may return false positive,
> > right? Or it is harmless?

I wasn't disagreeing with your use of total_mapcount() in that comment,
just with your propagating the "return boolean() ? true : false" style.

> >
> > And I actually spotted a few other places which should use
> > total_mapcount() but using page_mapcount() instead, for example, some
> > madvise code check if the page is shared by using page_mapcount(),
> > however it may return false negative (double mapped THP, but head page
> > is not PTE-mapped, just like what Wang Yugui reported). It is not
> > fatal, but not expected behavior. I understand total_mapcount() is
> > expensive, so is it a trade-off between cost and correctness or just
> > overlooked the false negative case in the first place? I can't tell.

I haven't looked through those other places. I do agree with you that
total_mapcount() or page_mapped() is likely to be more correct in most
instances, but it's often more costly than it's worth (on THPs).

And you've reminded me that some months back, Andrea pointed out how
total_mapcount() can race with something, and give the wrong answer:
he was arguing against relying on it (but has patches with additional
locking to make it more reliable - though that gets more complicated).

In Google prod actually, we have mostly avoided using total_mapcount(),
merely because I feared the cost and couldn't convince myself of its
safety - I'm not saying I ever saw it go wrong, I just didn't feel
comfortable relying on it. Maybe that's part of what I need to post
after sifting through.

Hugh

> >
> > >
> > > Or slightly better, "return !page_mapped(page);", since at least that
> > > one breaks out as soon as it sees a mapcount.  Though I guess I'm
> > > being silly there, since that case should never occur, so both
> > > total_mapcount() and page_mapped() scan through all pages.
> > >
> > > Or better, change try_to_unmap() to void: most callers ignore its
> > > return value anyway, and make their own decisions; the remaining
> > > few could be changed to do the same.  Though again, I may be
> > > being silly, since the expensive THP case is not the common case.
> >
> > I'd say half callers ignore its return value. But I think it should be
> > worth doing. At least we could remove half unnecessary
> > total_mapcount() or page_mapped() call.
> >
> > Thanks a lot for all the suggestions, will incorporate them in the new version.
> >
> > >
> > > >  }
> > > >
> > > >  /**
> > > > --
> > > > 2.26.2

Re: [v2 PATCH] mm: thp: check total_mapcount instead of page_mapcount

[Yang Shi]

On Fri, May 21, 2021 at 4:17 PM Hugh Dickins <hughd@google.com> wrote:
>
> On Fri, 21 May 2021, Yang Shi wrote:
> > On Fri, May 21, 2021 at 10:16 AM Yang Shi <shy828301@gmail.com> wrote:
> > > On Thu, May 20, 2021 at 10:06 PM Hugh Dickins <hughd@google.com> wrote:
> > > > On Thu, 13 May 2021, Yang Shi wrote:
> > > >
> > > > > When debugging the bug reported by Wang Yugui [1], try_to_unmap() may
> > > > > return false positive for PTE-mapped THP since page_mapcount() is used
> > > > > to check if the THP is unmapped, but it just checks compound mapount and
> > > > > head page's mapcount.  If the THP is PTE-mapped and head page is not
> > > > > mapped, it may return false positive.
> > > > >
> > > > > Use total_mapcount() instead of page_mapcount() for try_to_unmap() and
> > > > > do so for the VM_BUG_ON_PAGE in split_huge_page_to_list as well.
> > > > >
> > > > > This changed the semantic of try_to_unmap(), but I don't see there is
> > > > > any usecase that expects try_to_unmap() just unmap one subpage of a huge
> > > > > page.  So using page_mapcount() seems like a bug.
> > > > >
> > > > > [1] https://lore.kernel.org/linux-mm/20210412180659.B9E3.409509F4@e16-tech.com/
> > > > >
> > > > > Signed-off-by: Yang Shi <shy828301@gmail.com>
> > > >
> > > > I don't object to this patch, I've no reason to NAK it; but I'll
> > > > point out a few deficiencies which might make you want to revisit it.
> > > >
> > > > > ---
> > > > > v2: Removed dead code and updated the comment of try_to_unmap() per Zi
> > > > >     Yan.
> > > > >
> > > > >  mm/huge_memory.c | 11 +----------
> > > > >  mm/rmap.c        | 10 ++++++----
> > > > >  2 files changed, 7 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > > > > index 63ed6b25deaa..3b08b9ba1578 100644
> > > > > --- a/mm/huge_memory.c
> > > > > +++ b/mm/huge_memory.c
> > > > > @@ -2348,7 +2348,6 @@ static void unmap_page(struct page *page)
> > > > >               ttu_flags |= TTU_SPLIT_FREEZE;
> > > > >
> > > > >       unmap_success = try_to_unmap(page, ttu_flags);
> > > > > -     VM_BUG_ON_PAGE(!unmap_success, page);
> > > >
> > > > The unused variable unmap_success has already been reported and
> > > > dealt with.  But I couldn't tell what you intended: why change
> > > > try_to_unmap()'s output, if you then ignore it?
> > >
> > > Because some other callers of try_to_unmap() check the output.
>
> memory-failure.c and one in vmscan.c: most callers are not interested,
> so replacing a quick atomic_read by a scan of 512 struct pages is not
> necessarily a good idea. But I am exaggerating, it's not usually that
> bad: probably most of those callers will rarely encounter a THP.
>
> > >
> > > >
> > > > >  }
> > > > >
> > > > >  static void remap_page(struct page *page, unsigned int nr)
> > > > > @@ -2718,7 +2717,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
> > > > >       }
> > > > >
> > > > >       unmap_page(head);
> > > > > -     VM_BUG_ON_PAGE(compound_mapcount(head), head);
> > > > > +     VM_BUG_ON_PAGE(total_mapcount(head), head);
> > > >
> > > > And having forced try_to_unmap() to do the expensive-on-a-THP
> > > > total_mapcount() calculation, you now repeat it here.  Better
> > > > to stick with the previous VM_BUG_ON_PAGE(!unmap_success).
> > > >
> > > > Or better a VM_WARN_ONCE(), accompanied by dump_page()s as before,
> > > > to get some perhaps useful info out, which this patch has deleted.
> > > > Probably better inside unmap_page() than cluttering up here.
> > >
> > > Moving the BUG or WARN into unmap_page() looks fine to me. IIUC,
> > > VM_BUG_ON_PAGE or VM_WARN_ON_PAGE does call dump_page(), so dumping
> > > something useful is not deleted.
>
> Yes, you're right, I was forgetting that. The original DEBUG_VM block
> does do a separate dump_page() for head and tail (if different), but
> I don't think you need to replicate that now: (a) it was more a mark
> of frustration at not getting enough info to solve the problem (and
> if we really want more info, it's a dump of 512 struct pages needed);
> and (b) I think Matthew and others have enhanced dump_page() meanwhile,
> to issue some head info when dumping a tail; and (c) most splitters
> pass the head anyway, IIRC there's only one or two who pass a tail.

Yes, deferred split queue is one of them. We may just get rid of it by
passing the head page to reduce the exception case.

>
> >
> > I misspelled the function name. There is *NOT* VM_WARN_ON_PAGE(), the
> > name is VM_WARN_ON_ONCE_PAGE(). We may need to add VM_WARN_ON_PAGE()
> > since I'd like this warning to be printed every time when it is met.
>
> I get very confused by all those variants too; and then indeed very
> often the one variant you think you want turns out not to exist yet.
>
> But I'm not sure that printing it every time will be good: isn't it
> quite likely that the situation is long-lasting, and that the page
> is on the anon deferred or shmem unused list, and will keep coming
> back to report the same unhelpful info, just cluttering up the logs?
> I'd be more comfortable with _ONCE myself.
>
> But I'd also suggest dropping the VM_ part of it: we have no idea
> how often this happens on non-DEBUG_VM machines in production,
> and a pattern might be revealed there which would help to solve it.

If we use the non-VM_ version we have to call dump_page() explicitly
and duplicate what VM_WARN_ON_ONCE_PAGE() does since WARN_* don't dump
struct page at all.

>
> >
> > >
> > > >
> > > > VM_WARN_ONCE() because nothing in this patch fixes whatever Wang
> > > > Yugui is suffering from; and (aside from the BUG()) it's harmless,
> > > > because there are other ways in which the page_ref_freeze() can fail,
> > > > and that is allowed for.  We would like to know when this problem
> > > > occurs: there is something wrong, but no reason to crash.
> > >
> > > Yes, it fixes nothing. I didn't figure out why try_to_unmap() failed.
> > > I agree BUG_ON could be relaxed.
>
> I have to confess, I have known of several reasons why one or other
> of those BUG()s can be hit. Sigh. I'll rearrange priorities, research
> back through what work I've done and not had time to upstream, filter
> out the patches relevant to this "mysterious failure to unmap a THP".
>
> I did not speak up earlier because I did not notice any obvious
> connection between Wang Yugui's occurrence, and anything we have
> fixed; and (as usual) there's other work I should be doing.  But
> it's getting to feel dishonest, having this conversation without
> showing our fixes: I'd better sort those out and post (but that
> will take me more than a week I expect).

That would be great. I tried to figure out the problem, but the bug
report doesn't show a pattern, and I didn't get too much by staring at
the code.

>
> Not saying that you should stop with this patch at all:
> no, I can easily rebase on top of what you end up with here.

Thanks.

>
> > >
> > > >
> > > > >
> > > > >       /* block interrupt reentry in xa_lock and spinlock */
> > > > >       local_irq_disable();
> > > > > @@ -2758,14 +2757,6 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
> > > > >               __split_huge_page(page, list, end);
> > > > >               ret = 0;
> > > > >       } else {
> > > > > -             if (IS_ENABLED(CONFIG_DEBUG_VM) && mapcount) {
> > > > > -                     pr_alert("total_mapcount: %u, page_count(): %u\n",
> > > > > -                                     mapcount, count);
> > > > > -                     if (PageTail(page))
> > > > > -                             dump_page(head, NULL);
> > > > > -                     dump_page(page, "total_mapcount(head) > 0");
> > > > > -                     BUG();
> > > > > -             }
> > > >
> > > > This has always looked ugly (as if Kirill had hit an unsolved case),
> > > > so it is nice to remove it; but you're losing the dump_page() info,
> > > > and not really gaining anything more than a cosmetic cleanup.
> > >
> > > As I mentioned above, IIUC VM_BUG_ON_PAGE and VM_WARN_ON_PAGE do call
> > > dump_page().
> > >
> > > >
> > > > >               spin_unlock(&ds_queue->split_queue_lock);
> > > > >  fail:                if (mapping)
> > > > >                       xa_unlock(&mapping->i_pages);
> > > > > diff --git a/mm/rmap.c b/mm/rmap.c
> > > > > index 693a610e181d..f52825b1330d 100644
> > > > > --- a/mm/rmap.c
> > > > > +++ b/mm/rmap.c
> > > > > @@ -1742,12 +1742,14 @@ static int page_not_mapped(struct page *page)
> > > > >  }
> > > > >
> > > > >  /**
> > > > > - * try_to_unmap - try to remove all page table mappings to a page
> > > > > - * @page: the page to get unmapped
> > > > > + * try_to_unmap - try to remove all page table mappings to a page and the
> > > > > + *                compound page it belongs to
> > > > > + * @page: the page or the subpages of compound page to get unmapped
> > > > >   * @flags: action and flags
> > > > >   *
> > > > >   * Tries to remove all the page table entries which are mapping this
> > > > > - * page, used in the pageout path.  Caller must hold the page lock.
> > > > > + * page and the compound page it belongs to, used in the pageout path.
> > > > > + * Caller must hold the page lock.
> > > > >   *
> > > > >   * If unmap is successful, return true. Otherwise, false.
> > > > >   */
> > > > > @@ -1777,7 +1779,7 @@ bool try_to_unmap(struct page *page, enum ttu_flags flags)
> > > > >       else
> > > > >               rmap_walk(page, &rwc);
> > > > >
> > > > > -     return !page_mapcount(page) ? true : false;
> > > > > +     return !total_mapcount(page) ? true : false;
> > > >
> > > > That always made me wince: "return !total_mapcount(page);" surely.
> > >
> > > But page_mapcount() seems not correct, it may return false positive,
> > > right? Or it is harmless?
>
> I wasn't disagreeing with your use of total_mapcount() in that comment,
> just with your propagating the "return boolean() ? true : false" style.
>
> > >
> > > And I actually spotted a few other places which should use
> > > total_mapcount() but using page_mapcount() instead, for example, some
> > > madvise code check if the page is shared by using page_mapcount(),
> > > however it may return false negative (double mapped THP, but head page
> > > is not PTE-mapped, just like what Wang Yugui reported). It is not
> > > fatal, but not expected behavior. I understand total_mapcount() is
> > > expensive, so is it a trade-off between cost and correctness or just
> > > overlooked the false negative case in the first place? I can't tell.
>
> I haven't looked through those other places. I do agree with you that
> total_mapcount() or page_mapped() is likely to be more correct in most
> instances, but it's often more costly than it's worth (on THPs).

Kind of. But some of them are not a hot path, for example, madvise.

>
> And you've reminded me that some months back, Andrea pointed out how
> total_mapcount() can race with something, and give the wrong answer:
> he was arguing against relying on it (but has patches with additional
> locking to make it more reliable - though that gets more complicated).
>
> In Google prod actually, we have mostly avoided using total_mapcount(),
> merely because I feared the cost and couldn't convince myself of its
> safety - I'm not saying I ever saw it go wrong, I just didn't feel
> comfortable relying on it. Maybe that's part of what I need to post
> after sifting through.

I'm supposed it should be safe (but of course more costly) as long as
the page table lock is held. Very few places call page_mapcount() or
total_mapcount() *without* page table lock held, compaction is an
example.

>
> Hugh
>
> > >
> > > >
> > > > Or slightly better, "return !page_mapped(page);", since at least that
> > > > one breaks out as soon as it sees a mapcount.  Though I guess I'm
> > > > being silly there, since that case should never occur, so both
> > > > total_mapcount() and page_mapped() scan through all pages.
> > > >
> > > > Or better, change try_to_unmap() to void: most callers ignore its
> > > > return value anyway, and make their own decisions; the remaining
> > > > few could be changed to do the same.  Though again, I may be
> > > > being silly, since the expensive THP case is not the common case.
> > >
> > > I'd say half callers ignore its return value. But I think it should be
> > > worth doing. At least we could remove half unnecessary
> > > total_mapcount() or page_mapped() call.
> > >
> > > Thanks a lot for all the suggestions, will incorporate them in the new version.
> > >
> > > >
> > > > >  }
> > > > >
> > > > >  /**
> > > > > --
> > > > > 2.26.2