lkml.org 
[lkml]   [2015]   [Jun]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] compaction: fix isolate_migratepages_block() for THP=n
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



\
 
 \ /
  Last update: 2015-06-03 11:21    [W:0.063 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site