diff mbox series

[RFC,v2,1/5] mm: Identify compound pages sooner in isolate_migratepages_block

Message ID 20200819042705.23414.84098.stgit@localhost.localdomain (mailing list archive)
State New, archived
Headers show
Series Minor cleanups and performance optimizations for LRU rework | expand

Commit Message

Alexander Duyck Aug. 19, 2020, 4:27 a.m. UTC 
From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Since we are holding a reference to the page much sooner in
isolate_migratepages_block we can move the PageCompound check out of the
LRU locked section and instead just place it after get_page_unless_zero. By
doing this we can allow any of the items that might trigger a failure to
trigger a failure for the compound page rather than the order 0 page and as
a result we should be able to process the pageblock faster.

In addition by testing for PageCompound sooner we can avoid having the LRU
flag cleared and then reset in the exception case. As a result this should
prevent possible races where another thread might be attempting to pull the
LRU pages from the list.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 mm/compaction.c |   33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

Comments

Alex Shi Aug. 19, 2020, 7:48 a.m. UTC | #1 
在 2020/8/19 下午12:27, Alexander Duyck 写道:
> In addition by testing for PageCompound sooner we can avoid having the LRU
> flag cleared and then reset in the exception case. As a result this should
> prevent possible races where another thread might be attempting to pull the
> LRU pages from the list.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Reviewed-by: Alex Shi <alex.shi@linux.alibaba.com>
Matthew Wilcox Aug. 19, 2020, 11:43 a.m. UTC | #2 
On Tue, Aug 18, 2020 at 09:27:05PM -0700, Alexander Duyck wrote:
> +		/*
> +		 * Page is compound. We know the order before we know if it is
> +		 * on the LRU so we cannot assume it is THP. However since the
> +		 * page will have the LRU validated shortly we can use the value
> +		 * to skip over this page for now or validate the LRU is set and
> +		 * then isolate the entire compound page if we are isolating to
> +		 * generate a CMA page.
> +		 */
> +		if (PageCompound(page)) {
> +			const unsigned int order = compound_order(page);
> +
> +			if (likely(order < MAX_ORDER))
> +				low_pfn += (1UL << order) - 1;

Hmm.  You're checking for PageCompound but then skipping 1UL << order.
That only works if PageHead.  If instead this is PageCompound because
it's PageTail, you need to do something like:

				low_pfn |= (1UL << order) - 1;

which will move you to the end of the page you're in the middle of.

If PageTail can't actually happen here, then it's better to check for
PageHead explicitly and WARN_ON if you get a PageTail (eg a page was
combined into a compound page after you processed the earlier head page).

Is it possible the page you've found is hugetlbfs?  Those can have orders
larger than MAX_ORDER.
Alexander Duyck Aug. 19, 2020, 2:48 p.m. UTC | #3 
On Wed, Aug 19, 2020 at 4:43 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Aug 18, 2020 at 09:27:05PM -0700, Alexander Duyck wrote:
> > +             /*
> > +              * Page is compound. We know the order before we know if it is
> > +              * on the LRU so we cannot assume it is THP. However since the
> > +              * page will have the LRU validated shortly we can use the value
> > +              * to skip over this page for now or validate the LRU is set and
> > +              * then isolate the entire compound page if we are isolating to
> > +              * generate a CMA page.
> > +              */
> > +             if (PageCompound(page)) {
> > +                     const unsigned int order = compound_order(page);
> > +
> > +                     if (likely(order < MAX_ORDER))
> > +                             low_pfn += (1UL << order) - 1;
>
> Hmm.  You're checking for PageCompound but then skipping 1UL << order.
> That only works if PageHead.  If instead this is PageCompound because
> it's PageTail, you need to do something like:
>
>                                 low_pfn |= (1UL << order) - 1;
>
> which will move you to the end of the page you're in the middle of.

Can you successfully call get_page_unless_zero in a tail page? I
thought their reference count was 0? There is a get_page_unless_zero
call before the PageCompound check, so I don't think we can get a tail
page.

> If PageTail can't actually happen here, then it's better to check for
> PageHead explicitly and WARN_ON if you get a PageTail (eg a page was
> combined into a compound page after you processed the earlier head page).
>
> Is it possible the page you've found is hugetlbfs?  Those can have orders
> larger than MAX_ORDER.

So in theory we only need to jump pageblock_order. However there are
some architectures where that is not a fixed constant and so it would
have some additional overhead if I am not mistaken. In addition we
should have been only provided a pageblock if i am not mistaken so the
check further down that prevents low_pfn from passing end_pfn should
reset to the correct value.
diff mbox series

Patch

diff --git a/mm/compaction.c b/mm/compaction.c
index d3f87f759773..88c7b950f676 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -984,6 +984,24 @@  static bool too_many_isolated(pg_data_t *pgdat)
 		if (unlikely(!get_page_unless_zero(page)))
 			goto isolate_fail;
 
+		/*
+		 * Page is compound. We know the order before we know if it is
+		 * on the LRU so we cannot assume it is THP. However since the
+		 * page will have the LRU validated shortly we can use the value
+		 * to skip over this page for now or validate the LRU is set and
+		 * then isolate the entire compound page if we are isolating to
+		 * generate a CMA page.
+		 */
+		if (PageCompound(page)) {
+			const unsigned int order = compound_order(page);
+
+			if (likely(order < MAX_ORDER))
+				low_pfn += (1UL << order) - 1;
+
+			if (!cc->alloc_contig)
+				goto isolate_fail_put;
+		}
+
 		if (__isolate_lru_page_prepare(page, isolate_mode) != 0)
 			goto isolate_fail_put;
 
@@ -1009,23 +1027,8 @@  static bool too_many_isolated(pg_data_t *pgdat)
 				if (test_and_set_skip(cc, page, low_pfn))
 					goto isolate_abort;
 			}
-
-			/*
-			 * Page become compound since the non-locked check,
-			 * and it's on LRU. It can only be a THP so the order
-			 * is safe to read and it's 0 for tail pages.
-			 */
-			if (unlikely(PageCompound(page) && !cc->alloc_contig)) {
-				low_pfn += compound_nr(page) - 1;
-				SetPageLRU(page);
-				goto isolate_fail_put;
-			}
 		}
 
-		/* The whole page is taken off the LRU; skip the tail pages. */
-		if (PageCompound(page))
-			low_pfn += compound_nr(page) - 1;
-
 		/* Successfully isolated */
 		del_page_from_lru_list(page, lruvec, page_lru(page));
 		mod_node_page_state(page_pgdat(page),