linux-kernel - Re: [PATCH] mm: vmscam: check page order in isolating lru pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 3 Jan 2012 16:05:05 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Hillf Danton <dhillf@...il.com>
Cc:	KOSAKI Motohiro <kosaki.motohiro@...il.com>, linux-mm@...ck.org,
	David Rientjes <rientjes@...gle.com>,
	Hugh Dickins <hughd@...gle.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Michal Hocko <mhocko@...e.cz>,
	LKML <linux-kernel@...r.kernel.org>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Mel Gorman <mgorman@...e.de>
Subject: Re: [PATCH] mm: vmscam: check page order in isolating lru pages

On Sat, 31 Dec 2011 22:55:22 +0800
Hillf Danton <dhillf@...il.com> wrote:

> From: Hillf Danton <dhillf@...il.com>
> Subject: [PATCH] mm: vmscam: check page order in isolating lru pages
> 
> Before try to isolate physically contiguous pages, check for page order is
> added, and if it is not regular page, we should give up the attempt.

Well..  why?  Neither the changelog nor the code comments explain why
we skip these pages.  They should!

> --- a/mm/vmscan.c	Thu Dec 29 20:20:16 2011
> +++ b/mm/vmscan.c	Sat Dec 31 22:44:16 2011
> @@ -1162,6 +1162,7 @@ static unsigned long isolate_lru_pages(u
>  		unsigned long end_pfn;
>  		unsigned long page_pfn;
>  		int zone_id;
> +		unsigned int isolated_pages = 1;
> 
>  		page = lru_to_page(src);
>  		prefetchw_prev_lru_page(page, src, flags);
> @@ -1172,7 +1173,7 @@ static unsigned long isolate_lru_pages(u
>  		case 0:
>  			mem_cgroup_lru_del(page);
>  			list_move(&page->lru, dst);
> -			nr_taken += hpage_nr_pages(page);
> +			isolated_pages = hpage_nr_pages(page);
>  			break;
> 
>  		case -EBUSY:
> @@ -1184,8 +1185,12 @@ static unsigned long isolate_lru_pages(u
>  			BUG();
>  		}
> 
> +		nr_taken += isolated_pages;
>  		if (!order)
>  			continue;
> +		/* try pfn-based isolation only for regular page */
> +		if (isolated_pages != 1)
> +			continue;
> 
>  		/*
>  		 * Attempt to take all pages in the order aligned region
> @@ -1227,7 +1232,6 @@ static unsigned long isolate_lru_pages(u
>  				break;
> 
>  			if (__isolate_lru_page(cursor_page, mode, file) == 0) {
> -				unsigned int isolated_pages;
> 
>  				mem_cgroup_lru_del(cursor_page);
>  				list_move(&cursor_page->lru, dst);

The code has become rather awkward.

I don't like the trick of reusing a local (isolated_pages) for other
purposes later on in the function.  This introduces risk that someone
will add a usage of the local for its original application after it has
been reused.  And it's a little bit deceiving for readers - they first
have to work out "oh, it's being reused for something else".  It would
be better to use two identifiers.  The compiler is good at reusing
registers (and sometimes stack slots) if the earlier local has gone
dead.

Also, why do we test hpage_nr_pages() here?  Why not directly test
PageTransHuge()?


iow, something like this?

--- a/mm/vmscan.c~mm-vmscam-check-page-order-in-isolating-lru-pages-fix
+++ a/mm/vmscan.c
@@ -1173,7 +1173,6 @@ static unsigned long isolate_lru_pages(u
 		unsigned long end_pfn;
 		unsigned long page_pfn;
 		int zone_id;
-		unsigned int isolated_pages = 1;
 
 		page = lru_to_page(src);
 		prefetchw_prev_lru_page(page, src, flags);
@@ -1184,7 +1183,7 @@ static unsigned long isolate_lru_pages(u
 		case 0:
 			mem_cgroup_lru_del(page);
 			list_move(&page->lru, dst);
-			isolated_pages = hpage_nr_pages(page);
+			nr_taken += hpage_nr_pages(page);
 			break;
 
 		case -EBUSY:
@@ -1196,11 +1195,11 @@ static unsigned long isolate_lru_pages(u
 			BUG();
 		}
 
-		nr_taken += isolated_pages;
 		if (!order)
 			continue;
-		/* try pfn-based isolation only for regular page */
-		if (isolated_pages != 1)
+
+		/* Try pfn-based isolation only for regular pages */
+		if (PageTransHuge(page) != 1)
 			continue;
 
 		/*
@@ -1243,6 +1242,7 @@ static unsigned long isolate_lru_pages(u
 				break;
 
 			if (__isolate_lru_page(cursor_page, mode, file) == 0) {
+				unsigned int isolated_pages;
 
 				mem_cgroup_del_lru(cursor_page);
 				list_move(&cursor_page->lru, dst);


If hpage_nr_pages() is the official way of testing for a thp page
then I guess this is the wrong thing to do!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/