Re: [PATCH] compaction: fix isolate_migratepages_block() for THP=n

[Kirill A. Shutemov]

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@xxxxxxxxxx
> >>
> >> 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@xxxxxxx>
> 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@xxxxxxxxxxxxxxx
> 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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/