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@xxxxxxxxxx]
Sent: Wednesday, December 17, 2014 5:40 PM
To: akpm@xxxxxxxxxxxxxxxxxxxx
Cc: linux-mm@xxxxxxxxx; James Custer; isimatu.yasuaki@xxxxxxxxxxxxxx; kamezawa.hiroyu@xxxxxxxxxxxxxx; Russ Anderson; stable@xxxxxxxxxxxxxxx
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@xxxxxxxxxxxxxxxxxxxx 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 from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html