linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] compaction: fix isolate_migratepages_block() for THP=n
@ 2015-04-27 11:26 Kirill A. Shutemov
  2015-04-27 11:50 ` Vlastimil Babka
  2015-04-28 22:14 ` Andrew Morton
  0 siblings, 2 replies; 8+ messages in thread
From: Kirill A. Shutemov @ 2015-04-27 11:26 UTC (permalink / raw)
  To: Andrew Morton, Vlastimil Babka; +Cc: linux-mm, linux-kernel, Kirill A. Shutemov

PageTrans* helpers are always-false if THP is disabled compile-time.
It means the fucntion will fail to detect hugetlb pages in this case.

Let's use PageCompound() instead. With small tweak to how we calculate
next low_pfn it will make function ready to see tail pages.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 mm/compaction.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 018f08da99a2..6ef2fdf1d6b6 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -732,18 +732,18 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 		 * splitting and collapsing (collapsing has already happened
 		 * if PageLRU is set) but the lock is not necessarily taken
 		 * here and it is wasteful to take it just to check transhuge.
-		 * Check TransHuge without lock and skip the whole pageblock if
-		 * it's either a transhuge or hugetlbfs page, as calling
+		 * Check PageCompound without lock and skip the whole pageblock
+		 * if it's either a transhuge or hugetlbfs page, as calling
 		 * compound_order() without preventing THP from splitting the
 		 * page underneath us may return surprising results.
 		 */
-		if (PageTransHuge(page)) {
-			if (!locked)
-				low_pfn = ALIGN(low_pfn + 1,
-						pageblock_nr_pages) - 1;
+		if (PageCompound(page)) {
+			int nr;
+			if (locked)
+				nr = 1 << compound_order(page);
 			else
-				low_pfn += (1 << compound_order(page)) - 1;
-
+				nr = pageblock_nr_pages;
+			low_pfn = ALIGN(low_pfn + 1, nr) - 1;
 			continue;
 		}
 
@@ -763,11 +763,12 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 			if (!locked)
 				break;
 
-			/* Recheck PageLRU and PageTransHuge under lock */
+			/* Recheck PageLRU and PageCompound under lock */
 			if (!PageLRU(page))
 				continue;
-			if (PageTransHuge(page)) {
-				low_pfn += (1 << compound_order(page)) - 1;
+			if (PageCompound(page)) {
+				int nr = 1 << compound_order(page);
+				low_pfn = ALIGN(low_pfn + 1, nr) - 1;
 				continue;
 			}
 		}
@@ -778,7 +779,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 		if (__isolate_lru_page(page, isolate_mode) != 0)
 			continue;
 
-		VM_BUG_ON_PAGE(PageTransCompound(page), page);
+		VM_BUG_ON_PAGE(PageCompound(page), page);
 
 		/* Successfully isolated */
 		del_page_from_lru_list(page, lruvec, page_lru(page));
-- 
2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] compaction: fix isolate_migratepages_block() for THP=n
  2015-04-27 11:26 [PATCH] compaction: fix isolate_migratepages_block() for THP=n Kirill A. Shutemov
@ 2015-04-27 11:50 ` Vlastimil Babka
  2015-04-28 22:14 ` Andrew Morton
  1 sibling, 0 replies; 8+ messages in thread
From: Vlastimil Babka @ 2015-04-27 11:50 UTC (permalink / raw)
  To: Kirill A. Shutemov, Andrew Morton; +Cc: linux-mm, linux-kernel

On 27.4.2015 13:26, Kirill A. Shutemov wrote:
> PageTrans* helpers are always-false if THP is disabled compile-time.
> It means the fucntion will fail to detect hugetlb pages in this case.
> 
> Let's use PageCompound() instead. With small tweak to how we calculate
> next low_pfn it will make function ready to see tail pages.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/compaction.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 018f08da99a2..6ef2fdf1d6b6 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -732,18 +732,18 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  		 * splitting and collapsing (collapsing has already happened
>  		 * if PageLRU is set) but the lock is not necessarily taken
>  		 * here and it is wasteful to take it just to check transhuge.
> -		 * Check TransHuge without lock and skip the whole pageblock if
> -		 * it's either a transhuge or hugetlbfs page, as calling
> +		 * Check PageCompound without lock and skip the whole pageblock
> +		 * if it's either a transhuge or hugetlbfs page, as calling
>  		 * compound_order() without preventing THP from splitting the
>  		 * page underneath us may return surprising results.
>  		 */
> -		if (PageTransHuge(page)) {
> -			if (!locked)
> -				low_pfn = ALIGN(low_pfn + 1,
> -						pageblock_nr_pages) - 1;
> +		if (PageCompound(page)) {
> +			int nr;
> +			if (locked)
> +				nr = 1 << compound_order(page);
>  			else
> -				low_pfn += (1 << compound_order(page)) - 1;
> -
> +				nr = pageblock_nr_pages;
> +			low_pfn = ALIGN(low_pfn + 1, nr) - 1;
>  			continue;
>  		}
>  
> @@ -763,11 +763,12 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  			if (!locked)
>  				break;
>  
> -			/* Recheck PageLRU and PageTransHuge under lock */
> +			/* Recheck PageLRU and PageCompound under lock */
>  			if (!PageLRU(page))
>  				continue;
> -			if (PageTransHuge(page)) {
> -				low_pfn += (1 << compound_order(page)) - 1;
> +			if (PageCompound(page)) {
> +				int nr = 1 << compound_order(page);
> +				low_pfn = ALIGN(low_pfn + 1, nr) - 1;
>  				continue;
>  			}
>  		}
> @@ -778,7 +779,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  		if (__isolate_lru_page(page, isolate_mode) != 0)
>  			continue;
>  
> -		VM_BUG_ON_PAGE(PageTransCompound(page), page);
> +		VM_BUG_ON_PAGE(PageCompound(page), page);
>  
>  		/* Successfully isolated */
>  		del_page_from_lru_list(page, lruvec, page_lru(page));
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] compaction: fix isolate_migratepages_block() for THP=n
  2015-04-27 11:26 [PATCH] compaction: fix isolate_migratepages_block() for THP=n Kirill A. Shutemov
  2015-04-27 11:50 ` Vlastimil Babka
@ 2015-04-28 22:14 ` Andrew Morton
  2015-04-28 22:28   ` Kirill A. Shutemov
  1 sibling, 1 reply; 8+ messages in thread
From: Andrew Morton @ 2015-04-28 22:14 UTC (permalink / raw)
  To: Kirill A. Shutemov; +Cc: Vlastimil Babka, linux-mm, linux-kernel

On Mon, 27 Apr 2015 14:26:46 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:

> PageTrans* helpers are always-false if THP is disabled compile-time.
> It means the fucntion will fail to detect hugetlb pages in this case.
> 
> Let's use PageCompound() instead. With small tweak to how we calculate
> next low_pfn it will make function ready to see tail pages.

<scratches head>

So this patch has no runtime effects at present?  It is preparation for
something else?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] compaction: fix isolate_migratepages_block() for THP=n
  2015-04-28 22:14 ` Andrew Morton
@ 2015-04-28 22:28   ` Kirill A. Shutemov
  2015-04-28 22:37     ` Andrew Morton
  0 siblings, 1 reply; 8+ messages in thread
From: Kirill A. Shutemov @ 2015-04-28 22:28 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kirill A. Shutemov, Vlastimil Babka, linux-mm, linux-kernel, Sasha Levin

On Tue, Apr 28, 2015 at 03:14:20PM -0700, Andrew Morton wrote:
> On Mon, 27 Apr 2015 14:26:46 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> 
> > PageTrans* helpers are always-false if THP is disabled compile-time.
> > It means the fucntion will fail to detect hugetlb pages in this case.
> > 
> > Let's use PageCompound() instead. With small tweak to how we calculate
> > next low_pfn it will make function ready to see tail pages.
> 
> <scratches head>
> 
> So this patch has no runtime effects at present?  It is preparation for
> something else?

I wrote this to fix bug I originally attributed to refcounting patchset,
but Sasha triggered the same bug on -next without the patchset applied:

http://lkml.kernel.org/g/553EB993.7030401@oracle.com

Now I think it's related to changing of PageLRU() behaviour on tail page
by my page flags patchset. PageLRU() on tail pages now reports true if
head page is on LRU. It means no we can go futher insede
isolate_migratepages_block() with tail page.

-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] compaction: fix isolate_migratepages_block() for THP=n
  2015-04-28 22:28   ` Kirill A. Shutemov
@ 2015-04-28 22:37     ` Andrew Morton
  2015-04-28 22:44       ` Kirill A. Shutemov
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Morton @ 2015-04-28 22:37 UTC (permalink / raw)
  To: Kirill A. Shutemov
  Cc: Kirill A. Shutemov, Vlastimil Babka, linux-mm, linux-kernel, Sasha Levin

On Wed, 29 Apr 2015 01:28:28 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> On Tue, Apr 28, 2015 at 03:14:20PM -0700, Andrew Morton wrote:
> > On Mon, 27 Apr 2015 14:26:46 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> > 
> > > PageTrans* helpers are always-false if THP is disabled compile-time.
> > > It means the fucntion will fail to detect hugetlb pages in this case.
> > > 
> > > Let's use PageCompound() instead. With small tweak to how we calculate
> > > next low_pfn it will make function ready to see tail pages.
> > 
> > <scratches head>
> > 
> > So this patch has no runtime effects at present?  It is preparation for
> > something else?
> 
> I wrote this to fix bug I originally attributed to refcounting patchset,
> but Sasha triggered the same bug on -next without the patchset applied:
>
> http://lkml.kernel.org/g/553EB993.7030401@oracle.com

Well why the heck didn't the changelog tell us this?!?!?

> Now I think it's related to changing of PageLRU() behaviour on tail page
> by my page flags patchset.

So this patch is a bugfix against one of

page-flags-trivial-cleanup-for-pagetrans-helpers.patch
page-flags-introduce-page-flags-policies-wrt-compound-pages.patch
page-flags-define-pg_locked-behavior-on-compound-pages.patch
page-flags-define-behavior-of-fs-io-related-flags-on-compound-pages.patch
page-flags-define-behavior-of-lru-related-flags-on-compound-pages.patch
page-flags-define-behavior-slb-related-flags-on-compound-pages.patch
page-flags-define-behavior-of-xen-related-flags-on-compound-pages.patch
page-flags-define-pg_reserved-behavior-on-compound-pages.patch
page-flags-define-pg_swapbacked-behavior-on-compound-pages.patch
page-flags-define-pg_swapcache-behavior-on-compound-pages.patch
page-flags-define-pg_mlocked-behavior-on-compound-pages.patch
page-flags-define-pg_uncached-behavior-on-compound-pages.patch
page-flags-define-pg_uptodate-behavior-on-compound-pages.patch
page-flags-look-on-head-page-if-the-flag-is-encoded-in-page-mapping.patch
mm-sanitize-page-mapping-for-tail-pages.patch
include-linux-page-flagsh-rename-macros-to-avoid-collisions.patch

Which one was the faulty patch?

> PageLRU() on tail pages now reports true if
> head page is on LRU. It means no we can go futher insede
> isolate_migratepages_block() with tail page.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] compaction: fix isolate_migratepages_block() for THP=n
  2015-04-28 22:37     ` Andrew Morton
@ 2015-04-28 22:44       ` Kirill A. Shutemov
  2015-06-03  9:14         ` Vlastimil Babka
  0 siblings, 1 reply; 8+ messages in thread
From: Kirill A. Shutemov @ 2015-04-28 22:44 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kirill A. Shutemov, Vlastimil Babka, linux-mm, linux-kernel, Sasha Levin

On Tue, Apr 28, 2015 at 03:37:24PM -0700, Andrew Morton wrote:
> On Wed, 29 Apr 2015 01:28:28 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> 
> > On Tue, Apr 28, 2015 at 03:14:20PM -0700, Andrew Morton wrote:
> > > On Mon, 27 Apr 2015 14:26:46 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> > > 
> > > > PageTrans* helpers are always-false if THP is disabled compile-time.
> > > > It means the fucntion will fail to detect hugetlb pages in this case.
> > > > 
> > > > Let's use PageCompound() instead. With small tweak to how we calculate
> > > > next low_pfn it will make function ready to see tail pages.
> > > 
> > > <scratches head>
> > > 
> > > So this patch has no runtime effects at present?  It is preparation for
> > > something else?
> > 
> > I wrote this to fix bug I originally attributed to refcounting patchset,
> > but Sasha triggered the same bug on -next without the patchset applied:
> >
> > http://lkml.kernel.org/g/553EB993.7030401@oracle.com
> 
> Well why the heck didn't the changelog tell us this?!?!?

Sasha reported bug in -next after I sent the patch.

> 
> > Now I think it's related to changing of PageLRU() behaviour on tail page
> > by my page flags patchset.
> 
> So this patch is a bugfix against one of
> 
> page-flags-trivial-cleanup-for-pagetrans-helpers.patch
> page-flags-introduce-page-flags-policies-wrt-compound-pages.patch
> page-flags-define-pg_locked-behavior-on-compound-pages.patch
> page-flags-define-behavior-of-fs-io-related-flags-on-compound-pages.patch
> page-flags-define-behavior-of-lru-related-flags-on-compound-pages.patch

^^^ this one is fault, I think.

> page-flags-define-behavior-slb-related-flags-on-compound-pages.patch
> page-flags-define-behavior-of-xen-related-flags-on-compound-pages.patch
> page-flags-define-pg_reserved-behavior-on-compound-pages.patch
> page-flags-define-pg_swapbacked-behavior-on-compound-pages.patch
> page-flags-define-pg_swapcache-behavior-on-compound-pages.patch
> page-flags-define-pg_mlocked-behavior-on-compound-pages.patch
> page-flags-define-pg_uncached-behavior-on-compound-pages.patch
> page-flags-define-pg_uptodate-behavior-on-compound-pages.patch
> page-flags-look-on-head-page-if-the-flag-is-encoded-in-page-mapping.patch
> mm-sanitize-page-mapping-for-tail-pages.patch
> include-linux-page-flagsh-rename-macros-to-avoid-collisions.patch
> 
> Which one was the faulty patch?
> 
> > PageLRU() on tail pages now reports true if
> > head page is on LRU. It means no we can go futher insede
> > isolate_migratepages_block() with tail page.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] compaction: fix isolate_migratepages_block() for THP=n
  2015-04-28 22:44       ` Kirill A. Shutemov
@ 2015-06-03  9:14         ` Vlastimil Babka
  2015-06-03 11:24           ` Kirill A. Shutemov
  0 siblings, 1 reply; 8+ messages in thread
From: Vlastimil Babka @ 2015-06-03  9:14 UTC (permalink / raw)
  To: Kirill A. Shutemov, Andrew Morton
  Cc: Kirill A. Shutemov, linux-mm, linux-kernel, Sasha Levin

On 04/29/2015 12:44 AM, Kirill A. Shutemov wrote:
>>>
>>> I wrote this to fix bug I originally attributed to refcounting patchset,
>>> but Sasha triggered the same bug on -next without the patchset applied:
>>>
>>> http://lkml.kernel.org/g/553EB993.7030401@oracle.com
>>
>> Well why the heck didn't the changelog tell us this?!?!?
> 
> Sasha reported bug in -next after I sent the patch.
> 
>>
>>> Now I think it's related to changing of PageLRU() behaviour on tail page
>>> by my page flags patchset.
>>
>> So this patch is a bugfix against one of
>>
>> page-flags-trivial-cleanup-for-pagetrans-helpers.patch
>> page-flags-introduce-page-flags-policies-wrt-compound-pages.patch
>> page-flags-define-pg_locked-behavior-on-compound-pages.patch
>> page-flags-define-behavior-of-fs-io-related-flags-on-compound-pages.patch
>> page-flags-define-behavior-of-lru-related-flags-on-compound-pages.patch
> 
> ^^^ this one is fault, I think.

So this patch is now:
http://www.ozlabs.org/~akpm/mmotm/broken-out/page-flags-define-behavior-of-lru-related-flags-on-compound-pages-fix.patch

I've found a non-fatal but misleading issues.

First, the "will fail to detect hugetlb pages in this case" part of the
changelog, and mention of hugetlbfs in the comment is AFAIK moot.
There's a PageLRU() check preceding the compound check, so hugetlbfs
pages (which are not PageLRU() AFAIK) are already skipped at that point.
I want to improve that in another series, but that's out of scope here.

Second, compound_order(page) returns 0 for a tail page, so the ALIGN()
trick that's supposed to properly advance pfn from a tail page is
useless. We could grab a head page, but stumbling on a THP tail page
should be very rare so it's not worth the trouble - just remove the ALIGN.

From: Vlastimil Babka <vbabka@suse.cz>
Date: Wed, 3 Jun 2015 11:03:37 +0200
Subject: [PATCH] page-flags-define-behavior-of-lru-related-flags-on-compound-pages-fix-fix

Mentioning hugetlbfs is misleading, because PageLRU() checks skip over hugetlb pages.
The ALIGN() parts are useless, because compound_order(page) returns 0 for tail pages.

---
 mm/compaction.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 6ef2fdf..16e1b57 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -733,9 +733,12 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 		 * if PageLRU is set) but the lock is not necessarily taken
 		 * here and it is wasteful to take it just to check transhuge.
 		 * Check PageCompound without lock and skip the whole pageblock
-		 * if it's either a transhuge or hugetlbfs page, as calling
-		 * compound_order() without preventing THP from splitting the
-		 * page underneath us may return surprising results.
+		 * if it's a transhuge page, as calling compound_order()
+		 * without preventing THP from splitting the page underneath us
+		 * may return surprising results.
+		 * If we happen to check a THP tail page, compound_order()
+		 * returns 0. It should be rare enough to not bother with
+		 * using compound_head() in that case.
 		 */
 		if (PageCompound(page)) {
 			int nr;
@@ -743,7 +746,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 				nr = 1 << compound_order(page);
 			else
 				nr = pageblock_nr_pages;
-			low_pfn = ALIGN(low_pfn + 1, nr) - 1;
+			low_pfn += nr - 1;
 			continue;
 		}
 
@@ -768,7 +771,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 				continue;
 			if (PageCompound(page)) {
 				int nr = 1 << compound_order(page);
-				low_pfn = ALIGN(low_pfn + 1, nr) - 1;
+				low_pfn += nr - 1;
 				continue;
 			}
 		}
-- 
2.1.4


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] compaction: fix isolate_migratepages_block() for THP=n
  2015-06-03  9:14         ` Vlastimil Babka
@ 2015-06-03 11:24           ` Kirill A. Shutemov
  0 siblings, 0 replies; 8+ messages in thread
From: Kirill A. Shutemov @ 2015-06-03 11:24 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Kirill A. Shutemov, linux-mm, linux-kernel, Sasha Levin

On Wed, Jun 03, 2015 at 11:14:05AM +0200, Vlastimil Babka wrote:
> On 04/29/2015 12:44 AM, Kirill A. Shutemov wrote:
> >>>
> >>> I wrote this to fix bug I originally attributed to refcounting patchset,
> >>> but Sasha triggered the same bug on -next without the patchset applied:
> >>>
> >>> http://lkml.kernel.org/g/553EB993.7030401@oracle.com
> >>
> >> Well why the heck didn't the changelog tell us this?!?!?
> > 
> > Sasha reported bug in -next after I sent the patch.
> > 
> >>
> >>> Now I think it's related to changing of PageLRU() behaviour on tail page
> >>> by my page flags patchset.
> >>
> >> So this patch is a bugfix against one of
> >>
> >> page-flags-trivial-cleanup-for-pagetrans-helpers.patch
> >> page-flags-introduce-page-flags-policies-wrt-compound-pages.patch
> >> page-flags-define-pg_locked-behavior-on-compound-pages.patch
> >> page-flags-define-behavior-of-fs-io-related-flags-on-compound-pages.patch
> >> page-flags-define-behavior-of-lru-related-flags-on-compound-pages.patch
> > 
> > ^^^ this one is fault, I think.
> 
> So this patch is now:
> http://www.ozlabs.org/~akpm/mmotm/broken-out/page-flags-define-behavior-of-lru-related-flags-on-compound-pages-fix.patch
> 
> I've found a non-fatal but misleading issues.
> 
> First, the "will fail to detect hugetlb pages in this case" part of the
> changelog, and mention of hugetlbfs in the comment is AFAIK moot.
> There's a PageLRU() check preceding the compound check, so hugetlbfs
> pages (which are not PageLRU() AFAIK) are already skipped at that point.
> I want to improve that in another series, but that's out of scope here.
> 
> Second, compound_order(page) returns 0 for a tail page, so the ALIGN()
> trick that's supposed to properly advance pfn from a tail page is
> useless. We could grab a head page, but stumbling on a THP tail page
> should be very rare so it's not worth the trouble - just remove the ALIGN.
> 
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Wed, 3 Jun 2015 11:03:37 +0200
> Subject: [PATCH] page-flags-define-behavior-of-lru-related-flags-on-compound-pages-fix-fix
> 
> Mentioning hugetlbfs is misleading, because PageLRU() checks skip over hugetlb pages.
> The ALIGN() parts are useless, because compound_order(page) returns 0 for tail pages.

ACK.

> 
> ---
>  mm/compaction.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 6ef2fdf..16e1b57 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -733,9 +733,12 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  		 * if PageLRU is set) but the lock is not necessarily taken
>  		 * here and it is wasteful to take it just to check transhuge.
>  		 * Check PageCompound without lock and skip the whole pageblock
> -		 * if it's either a transhuge or hugetlbfs page, as calling
> -		 * compound_order() without preventing THP from splitting the
> -		 * page underneath us may return surprising results.
> +		 * if it's a transhuge page, as calling compound_order()
> +		 * without preventing THP from splitting the page underneath us
> +		 * may return surprising results.
> +		 * If we happen to check a THP tail page, compound_order()
> +		 * returns 0. It should be rare enough to not bother with
> +		 * using compound_head() in that case.
>  		 */
>  		if (PageCompound(page)) {
>  			int nr;
> @@ -743,7 +746,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  				nr = 1 << compound_order(page);
>  			else
>  				nr = pageblock_nr_pages;
> -			low_pfn = ALIGN(low_pfn + 1, nr) - 1;
> +			low_pfn += nr - 1;
>  			continue;
>  		}
>  
> @@ -768,7 +771,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  				continue;
>  			if (PageCompound(page)) {
>  				int nr = 1 << compound_order(page);
> -				low_pfn = ALIGN(low_pfn + 1, nr) - 1;
> +				low_pfn += nr - 1;
>  				continue;
>  			}
>  		}
> -- 
> 2.1.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2015-06-03 11:24 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-27 11:26 [PATCH] compaction: fix isolate_migratepages_block() for THP=n Kirill A. Shutemov
2015-04-27 11:50 ` Vlastimil Babka
2015-04-28 22:14 ` Andrew Morton
2015-04-28 22:28   ` Kirill A. Shutemov
2015-04-28 22:37     ` Andrew Morton
2015-04-28 22:44       ` Kirill A. Shutemov
2015-06-03  9:14         ` Vlastimil Babka
2015-06-03 11:24           ` Kirill A. Shutemov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox