[patch 4/6] mm: fix invalid use of pfn_valid_within in test_pages_in_a_zone

[patch 4/6] mm: fix invalid use of pfn_valid_within in test_pages_in_a_zone

[akpm]

From: James Custer <jcuster@sgi.com>
Subject: mm: fix invalid use of pfn_valid_within in test_pages_in_a_zone

Offlining memory by 'echo 0 > /sys/devices/system/memory/memory#/online'
or reading valid_zones 'cat
/sys/devices/system/memory/memory#/valid_zones' causes BUG: unable to
handle kernel paging request due to invalid use of pfn_valid_within.  This
is due to a bug in test_pages_in_a_zone.

In order to use pfn_valid_within within a MAX_ORDER_NR_PAGES block of
pages, a valid pfn within the block must first be found.  There only needs
to be one valid pfn found in test_pages_in_a_zone in the first place.  So
the fix is to replace pfn_valid_within with pfn_valid such that the first
valid pfn within the pageblock is found (if it exists).  This works
independently of CONFIG_HOLES_IN_ZONE.

Signed-off-by: James Custer <jcuster@sgi.com>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Russ Anderson <rja@sgi.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/memory_hotplug.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff -puN mm/memory_hotplug.c~mm-fix-invalid-use-of-pfn_valid_within-in-test_pages_in_a_zone mm/memory_hotplug.c
--- a/mm/memory_hotplug.c~mm-fix-invalid-use-of-pfn_valid_within-in-test_pages_in_a_zone
+++ a/mm/memory_hotplug.c
@@ -1331,7 +1331,7 @@ int is_mem_section_removable(unsigned lo
 }
 
 /*
- * Confirm all pages in a range [start, end) is belongs to the same zone.
+ * Confirm all pages in a range [start, end) belong to the same zone.
  */
 int test_pages_in_a_zone(unsigned long start_pfn, unsigned long end_pfn)
 {
@@ -1342,10 +1342,11 @@ int test_pages_in_a_zone(unsigned long s
 	for (pfn = start_pfn;
 	     pfn < end_pfn;
 	     pfn += MAX_ORDER_NR_PAGES) {
-		i = 0;
-		/* This is just a CONFIG_HOLES_IN_ZONE check.*/
-		while ((i < MAX_ORDER_NR_PAGES) && !pfn_valid_within(pfn + i))
-			i++;
+		/* Find the first valid pfn in this pageblock */
+		for (i = 0; i < MAX_ORDER_NR_PAGES; i++) {
+			if (pfn_valid(pfn + i))
+				break;
+		}
 		if (i == MAX_ORDER_NR_PAGES)
 			continue;
 		page = pfn_to_page(pfn + i);
_

--
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>

Re: [patch 4/6] mm: fix invalid use of pfn_valid_within in test_pages_in_a_zone

[David Rientjes]

On Mon, 15 Dec 2014, akpm@linux-foundation.org wrote:

> diff -puN mm/memory_hotplug.c~mm-fix-invalid-use-of-pfn_valid_within-in-test_pages_in_a_zone mm/memory_hotplug.c
> --- a/mm/memory_hotplug.c~mm-fix-invalid-use-of-pfn_valid_within-in-test_pages_in_a_zone
> +++ a/mm/memory_hotplug.c
> @@ -1331,7 +1331,7 @@ int is_mem_section_removable(unsigned lo
>  }
>  
>  /*
> - * Confirm all pages in a range [start, end) is belongs to the same zone.
> + * Confirm all pages in a range [start, end) belong to the same zone.
>   */
>  int test_pages_in_a_zone(unsigned long start_pfn, unsigned long end_pfn)
>  {
> @@ -1342,10 +1342,11 @@ int test_pages_in_a_zone(unsigned long s
>  	for (pfn = start_pfn;
>  	     pfn < end_pfn;
>  	     pfn += MAX_ORDER_NR_PAGES) {
> -		i = 0;
> -		/* This is just a CONFIG_HOLES_IN_ZONE check.*/
> -		while ((i < MAX_ORDER_NR_PAGES) && !pfn_valid_within(pfn + i))
> -			i++;
> +		/* Find the first valid pfn in this pageblock */
> +		for (i = 0; i < MAX_ORDER_NR_PAGES; i++) {
> +			if (pfn_valid(pfn + i))
> +				break;
> +		}
>  		if (i == MAX_ORDER_NR_PAGES)
>  			continue;
>  		page = pfn_to_page(pfn + i);

I think it would be much better to implement test_pages_in_a_zone() as a 
wrapper around the logic in memory compaction's pageblock_pfn_to_page() 
that does this exact same check for a pageblock.  It would only need to 
iterate the valid pageblocks in the [start_pfn, end_pfn) range and find 
the zone of the first pfn of the first valid pageblock.  This not only 
removes code, but it also unifies the implementation since your 
implementation above would be slower.

--
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>

RE: [patch 4/6] mm: fix invalid use of pfn_valid_within in test_pages_in_a_zone

[James Custer]

Reading the documentation on pageblock_pfn_to_page it checks to see if all of [start_pfn, end_pfn) is valid and within the same zone. But the validity in the entirety of [start_pfn, end_pfn) doesn't seem to be a requirement of test_pages_in_a_zone, unless I'm missing something.

Disclaimer: I'm very much not familiar with this area of code, and I fixed this bug based off of documentation that I read. 

Regards,
James
________________________________________
From: David Rientjes [rientjes@google.com]
Sent: Wednesday, December 17, 2014 5:40 PM
To: akpm@linux-foundation.org
Cc: linux-mm@kvack.org; James Custer; isimatu.yasuaki@jp.fujitsu.com; kamezawa.hiroyu@jp.fujitsu.com; Russ Anderson; stable@vger.kernel.org
Subject: Re: [patch 4/6] mm: fix invalid use of pfn_valid_within in test_pages_in_a_zone

On Mon, 15 Dec 2014, akpm@linux-foundation.org wrote:

> diff -puN mm/memory_hotplug.c~mm-fix-invalid-use-of-pfn_valid_within-in-test_pages_in_a_zone mm/memory_hotplug.c
> --- a/mm/memory_hotplug.c~mm-fix-invalid-use-of-pfn_valid_within-in-test_pages_in_a_zone
> +++ a/mm/memory_hotplug.c
> @@ -1331,7 +1331,7 @@ int is_mem_section_removable(unsigned lo
>  }
>
>  /*
> - * Confirm all pages in a range [start, end) is belongs to the same zone.
> + * Confirm all pages in a range [start, end) belong to the same zone.
>   */
>  int test_pages_in_a_zone(unsigned long start_pfn, unsigned long end_pfn)
>  {
> @@ -1342,10 +1342,11 @@ int test_pages_in_a_zone(unsigned long s
>       for (pfn = start_pfn;
>            pfn < end_pfn;
>            pfn += MAX_ORDER_NR_PAGES) {
> -             i = 0;
> -             /* This is just a CONFIG_HOLES_IN_ZONE check.*/
> -             while ((i < MAX_ORDER_NR_PAGES) && !pfn_valid_within(pfn + i))
> -                     i++;
> +             /* Find the first valid pfn in this pageblock */
> +             for (i = 0; i < MAX_ORDER_NR_PAGES; i++) {
> +                     if (pfn_valid(pfn + i))
> +                             break;
> +             }
>               if (i == MAX_ORDER_NR_PAGES)
>                       continue;
>               page = pfn_to_page(pfn + i);

I think it would be much better to implement test_pages_in_a_zone() as a
wrapper around the logic in memory compaction's pageblock_pfn_to_page()
that does this exact same check for a pageblock.  It would only need to
iterate the valid pageblocks in the [start_pfn, end_pfn) range and find
the zone of the first pfn of the first valid pageblock.  This not only
removes code, but it also unifies the implementation since your
implementation above would be slower.

--
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>

RE: [patch 4/6] mm: fix invalid use of pfn_valid_within in test_pages_in_a_zone

[James Custer]

Hi David,

I was just wondering if you saw my message. I know I sent it before the holidays, just wanted to make sure it didn't get lost.

Regards,
James
________________________________________
From: James Custer
Sent: Thursday, December 18, 2014 11:16 AM
To: David Rientjes; akpm@linux-foundation.org
Cc: linux-mm@kvack.org; isimatu.yasuaki@jp.fujitsu.com; kamezawa.hiroyu@jp.fujitsu.com; Russ Anderson; stable@vger.kernel.org
Subject: RE: [patch 4/6] mm: fix invalid use of pfn_valid_within in test_pages_in_a_zone

Reading the documentation on pageblock_pfn_to_page it checks to see if all of [start_pfn, end_pfn) is valid and within the same zone. But the validity in the entirety of [start_pfn, end_pfn) doesn't seem to be a requirement of test_pages_in_a_zone, unless I'm missing something.

Disclaimer: I'm very much not familiar with this area of code, and I fixed this bug based off of documentation that I read.

Regards,
James
________________________________________
From: David Rientjes [rientjes@google.com]
Sent: Wednesday, December 17, 2014 5:40 PM
To: akpm@linux-foundation.org
Cc: linux-mm@kvack.org; James Custer; isimatu.yasuaki@jp.fujitsu.com; kamezawa.hiroyu@jp.fujitsu.com; Russ Anderson; stable@vger.kernel.org
Subject: Re: [patch 4/6] mm: fix invalid use of pfn_valid_within in test_pages_in_a_zone

On Mon, 15 Dec 2014, akpm@linux-foundation.org wrote:

> diff -puN mm/memory_hotplug.c~mm-fix-invalid-use-of-pfn_valid_within-in-test_pages_in_a_zone mm/memory_hotplug.c
> --- a/mm/memory_hotplug.c~mm-fix-invalid-use-of-pfn_valid_within-in-test_pages_in_a_zone
> +++ a/mm/memory_hotplug.c
> @@ -1331,7 +1331,7 @@ int is_mem_section_removable(unsigned lo
>  }
>
>  /*
> - * Confirm all pages in a range [start, end) is belongs to the same zone.
> + * Confirm all pages in a range [start, end) belong to the same zone.
>   */
>  int test_pages_in_a_zone(unsigned long start_pfn, unsigned long end_pfn)
>  {
> @@ -1342,10 +1342,11 @@ int test_pages_in_a_zone(unsigned long s
>       for (pfn = start_pfn;
>            pfn < end_pfn;
>            pfn += MAX_ORDER_NR_PAGES) {
> -             i = 0;
> -             /* This is just a CONFIG_HOLES_IN_ZONE check.*/
> -             while ((i < MAX_ORDER_NR_PAGES) && !pfn_valid_within(pfn + i))
> -                     i++;
> +             /* Find the first valid pfn in this pageblock */
> +             for (i = 0; i < MAX_ORDER_NR_PAGES; i++) {
> +                     if (pfn_valid(pfn + i))
> +                             break;
> +             }
>               if (i == MAX_ORDER_NR_PAGES)
>                       continue;
>               page = pfn_to_page(pfn + i);

I think it would be much better to implement test_pages_in_a_zone() as a
wrapper around the logic in memory compaction's pageblock_pfn_to_page()
that does this exact same check for a pageblock.  It would only need to
iterate the valid pageblocks in the [start_pfn, end_pfn) range and find
the zone of the first pfn of the first valid pageblock.  This not only
removes code, but it also unifies the implementation since your
implementation above would be slower.

--
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>

Re: [patch 4/6] mm: fix invalid use of pfn_valid_within in test_pages_in_a_zone

[Vlastimil Babka]

On 12/16/2014 12:03 AM, akpm@linux-foundation.org wrote:
> From: James Custer <jcuster@sgi.com>
> Subject: mm: fix invalid use of pfn_valid_within in test_pages_in_a_zone
> 
> Offlining memory by 'echo 0 > /sys/devices/system/memory/memory#/online'
> or reading valid_zones 'cat
> /sys/devices/system/memory/memory#/valid_zones' causes BUG: unable to
> handle kernel paging request due to invalid use of pfn_valid_within.  This
> is due to a bug in test_pages_in_a_zone.

It would still be useful to show the BUG, and provide /proc/zoneinfo for us to
see where are the corner cases.

> In order to use pfn_valid_within within a MAX_ORDER_NR_PAGES block of
> pages, a valid pfn within the block must first be found.  There only needs
> to be one valid pfn found in test_pages_in_a_zone in the first place.  So
> the fix is to replace pfn_valid_within with pfn_valid such that the first
> valid pfn within the pageblock is found (if it exists).  This works
> independently of CONFIG_HOLES_IN_ZONE.
> 
> Signed-off-by: James Custer <jcuster@sgi.com>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Russ Anderson <rja@sgi.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  mm/memory_hotplug.c |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff -puN mm/memory_hotplug.c~mm-fix-invalid-use-of-pfn_valid_within-in-test_pages_in_a_zone mm/memory_hotplug.c
> --- a/mm/memory_hotplug.c~mm-fix-invalid-use-of-pfn_valid_within-in-test_pages_in_a_zone
> +++ a/mm/memory_hotplug.c
> @@ -1331,7 +1331,7 @@ int is_mem_section_removable(unsigned lo
>  }
>  
>  /*
> - * Confirm all pages in a range [start, end) is belongs to the same zone.
> + * Confirm all pages in a range [start, end) belong to the same zone.
>   */
>  int test_pages_in_a_zone(unsigned long start_pfn, unsigned long end_pfn)
>  {
> @@ -1342,10 +1342,11 @@ int test_pages_in_a_zone(unsigned long s
>  	for (pfn = start_pfn;
>  	     pfn < end_pfn;
>  	     pfn += MAX_ORDER_NR_PAGES) {
> -		i = 0;
> -		/* This is just a CONFIG_HOLES_IN_ZONE check.*/
> -		while ((i < MAX_ORDER_NR_PAGES) && !pfn_valid_within(pfn + i))
> -			i++;
> +		/* Find the first valid pfn in this pageblock */
> +		for (i = 0; i < MAX_ORDER_NR_PAGES; i++) {
> +			if (pfn_valid(pfn + i))
> +				break;
> +		}
>  		if (i == MAX_ORDER_NR_PAGES)
>  			continue;
>  		page = pfn_to_page(pfn + i);
> _
> 
> --
> 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>
> 

--
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>

Re: [patch 4/6] mm: fix invalid use of pfn_valid_within in test_pages_in_a_zone

[Vlastimil Babka]

On 12/18/2014 06:16 PM, James Custer wrote:
> Reading the documentation on pageblock_pfn_to_page it checks to see if all
> of
> [start_pfn, end_pfn) is valid and within the same zone. But the validity in the
> entirety of [start_pfn, end_pfn) doesn't seem to be a requirement of
> test_pages_in_a_zone, unless I'm missing something.

(please don't top-post in reply, it makes further replying harder)

Yes there is a subtle difference you point out. So pageblock_pfn_to_page()
cannot be readily used. But a similar approach could still work, but I fear we
might have to distinguish by CONFIG_HOLES_IN_ZONE

a) CONFIG_HOLES_IN_ZONE is disabled, jut check first/last pfn of each pageblock
for validity. If any is valid, check if it belongs to the zone.

b) CONFIG_HOLES_IN_ZONE is enabled: try the above first, but if first or last is
invalid, we probably have to resort to a full pageblock scan, because there
might be holes containing pageblocks boundaries, and the valid pfn's are in the
middle?

Note that compaction just skips over such pageblocks described in case b) if
such configurations even exist. That might be suboptimal, but not fatal. In case
of memory offlining, it could be I guess.



> Disclaimer: I'm very much not familiar with this area of code, and I fixed
this bug based off of documentation that I read.
> 
> Regards, James ________________________________________
> From: David Rientjes [rientjes@google.com]
> Sent: Wednesday, December 17, 2014 5:40 PM
> To: akpm@linux-foundation.org
> Cc: linux-mm@kvack.org; James Custer; isimatu.yasuaki@jp.fujitsu.com; kamezawa.hiroyu@jp.fujitsu.com; Russ Anderson; stable@vger.kernel.org
> Subject: Re: [patch 4/6] mm: fix invalid use of pfn_valid_within in test_pages_in_a_zone
> 
> On Mon, 15 Dec 2014, akpm@linux-foundation.org wrote:
> 
>> diff -puN mm/memory_hotplug.c~mm-fix-invalid-use-of-pfn_valid_within-in-test_pages_in_a_zone mm/memory_hotplug.c
>> --- a/mm/memory_hotplug.c~mm-fix-invalid-use-of-pfn_valid_within-in-test_pages_in_a_zone
>> +++ a/mm/memory_hotplug.c
>> @@ -1331,7 +1331,7 @@ int is_mem_section_removable(unsigned lo
>>  }
>>
>>  /*
>> - * Confirm all pages in a range [start, end) is belongs to the same zone.
>> + * Confirm all pages in a range [start, end) belong to the same zone.
>>   */
>>  int test_pages_in_a_zone(unsigned long start_pfn, unsigned long end_pfn)
>>  {
>> @@ -1342,10 +1342,11 @@ int test_pages_in_a_zone(unsigned long s
>>       for (pfn = start_pfn;
>>            pfn < end_pfn;
>>            pfn += MAX_ORDER_NR_PAGES) {
>> -             i = 0;
>> -             /* This is just a CONFIG_HOLES_IN_ZONE check.*/
>> -             while ((i < MAX_ORDER_NR_PAGES) && !pfn_valid_within(pfn + i))
>> -                     i++;
>> +             /* Find the first valid pfn in this pageblock */
>> +             for (i = 0; i < MAX_ORDER_NR_PAGES; i++) {
>> +                     if (pfn_valid(pfn + i))
>> +                             break;
>> +             }
>>               if (i == MAX_ORDER_NR_PAGES)
>>                       continue;
>>               page = pfn_to_page(pfn + i);
> 
> I think it would be much better to implement test_pages_in_a_zone() as a
> wrapper around the logic in memory compaction's pageblock_pfn_to_page()
> that does this exact same check for a pageblock.  It would only need to
> iterate the valid pageblocks in the [start_pfn, end_pfn) range and find
> the zone of the first pfn of the first valid pageblock.  This not only
> removes code, but it also unifies the implementation since your
> implementation above would be slower.
> 
> --
> 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=ilto:"dont@kvack.org"> email@kvack.org </a>
> 

--
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>