linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] fix: decrease NR_FREE_PAGES when isolate page from buddy
@ 2015-07-01  1:17 minkyung88.kim
  2015-07-02  9:52 ` Vlastimil Babka
  0 siblings, 1 reply; 19+ messages in thread
From: minkyung88.kim @ 2015-07-01  1:17 UTC (permalink / raw)
  To: Andrew Morton, linux-mm; +Cc: Seungho Park, kmk3210, minkyung88.kim

From: "minkyung88.kim" <minkyung88.kim@lge.com>

NR_FREEPAGE should be decreased when pages are isolated from buddy.
Therefore fix the count.

Signed-off-by: minkyung88.kim <minkyung88.kim@lge.com>
---
 mm/page_isolation.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index 303c908..16cc172 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -233,10 +233,14 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn,
 			 */
 			if (get_freepage_migratetype(page) != MIGRATE_ISOLATE) {
 				struct page *end_page;
+				struct zone *zone = page_zone(page);
+				int mt = get_freepage_migratetype(page);
+				unsigned long nr_pages;
 
 				end_page = page + (1 << page_order(page)) - 1;
-				move_freepages(page_zone(page), page, end_page,
+				nr_pages = move_freepages(zone, page, end_page,
 						MIGRATE_ISOLATE);
+				__mod_zone_freepage_state(zone, -nr_pages, mt);
 			}
 			pfn += 1 << page_order(page);
 		}
-- 
2.1.4

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH] fix: decrease NR_FREE_PAGES when isolate page from buddy
  2015-07-01  1:17 [PATCH] fix: decrease NR_FREE_PAGES when isolate page from buddy minkyung88.kim
@ 2015-07-02  9:52 ` Vlastimil Babka
  2015-07-03  7:15   ` "김민경/주임연구원/SW Platform(연)AOT팀(minkyung88.kim@lge.com)"
  0 siblings, 1 reply; 19+ messages in thread
From: Vlastimil Babka @ 2015-07-02  9:52 UTC (permalink / raw)
  To: minkyung88.kim, Andrew Morton, linux-mm
  Cc: Seungho Park, kmk3210, Joonsoo Kim, Minchan Kim

[+CC Joonsoo and Minchan]

On 07/01/2015 03:17 AM, minkyung88.kim@lge.com wrote:
> From: "minkyung88.kim" <minkyung88.kim@lge.com>
>
> NR_FREEPAGE should be decreased when pages are isolated from buddy.
> Therefore fix the count.

Did you really observe an accounting bug and this patch fixed it, or is 
it just because of code inspection?

The patched code has this comment:

/*
  * If race between isolatation and allocation happens,
  * some free pages could be in MIGRATE_MOVABLE list
  * although pageblock's migratation type of the page
  * is MIGRATE_ISOLATE. Catch it and move the page into
  * MIGRATE_ISOLATE list.
  */

This is from 2012 and I'm not sure if it still applies. Joonsoo's series 
last year was to eliminate these races, see e.g. 51bb1a4093 
("mm/page_alloc: add freepage on isolate pageblock to correct buddy list").

So I think that this piece of code shouldn't be useful anymore. Well, 
actually I think it can trigger, but it's a false positive and (before 
your patch) result in basically a no-op. The reason is that the value of 
get_freepage_migratetype(page) is a just an optimization used only for 
pages on pcplists. It's not guaranteed to be correct for pages in the 
buddy free lists (and it can get stale even on the pcplists).

Now, the code from Joonsoo's patch mentioned above does this in
free_pcppages_bulk():

mt = get_freepage_migratetype(page);
if (unlikely(has_isolate_pageblock(zone)))
         mt = get_pageblock_migratetype(page);

/* MIGRATE_MOVABLE list may include MIGRATE_RESERVEs */
__free_one_page(page, page_to_pfn(page), zone, 0, mt);

So if get_freepage_migratetype(page) returns e.g. MIGRATE_MOVABLE but 
the pageblock is MIGRATE_ISOLATE, it will catch this and tell 
__free_one_page() the correct migratetype. However, nothing will update 
the freepage's migratetype by set_freepage_migratetype(), because it 
would be a pointless waste of CPU cycles. The page however goes to the 
correct MIGRATE_ISOLATE list. (note that this is likely not the only way 
how freepage_migratetype can be perceived as incorrect)

That means the code you are patching can really find the page where 
get_freepage_migratetype(page) will return MIGRATE_MOVABLE, i.e. != 
MIGRATE_ISOLATE will be true. But the move_freepages() call would be a 
no-op, as the page is already on the correct list and the accounting of 
freepages is correct.

So my conclusion is that after your patch, the freepage accounting could 
actually get broken, not fixed. But I may be wrong. Hopefully Joonsoo 
can verify this :)

If that's true, then the whole test you are patching should be dropped. 
Also we should make it clearer that get_freepage_migratetype() is only 
used for pages on pcplists (and even there it may differ from 
pageblock's migratetype and also from the pcplist the page is actually 
on, in cases of page stealing), as this is not the first confusion.
We should also drop the usage set_freepage_migratetype() from 
move_freepages() while at it.
Now the last usage of get_freepage_migratetype() outside of page_alloc.c 
is the page isolation code and I argue it's wrong. So after that is 
removed, we can actually also make the functions internal to page_alloc.c.

> Signed-off-by: minkyung88.kim <minkyung88.kim@lge.com>
> ---
>   mm/page_isolation.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index 303c908..16cc172 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -233,10 +233,14 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn,
>   			 */
>   			if (get_freepage_migratetype(page) != MIGRATE_ISOLATE) {
>   				struct page *end_page;
> +				struct zone *zone = page_zone(page);
> +				int mt = get_freepage_migratetype(page);
> +				unsigned long nr_pages;
>
>   				end_page = page + (1 << page_order(page)) - 1;
> -				move_freepages(page_zone(page), page, end_page,
> +				nr_pages = move_freepages(zone, page, end_page,
>   						MIGRATE_ISOLATE);
> +				__mod_zone_freepage_state(zone, -nr_pages, mt);
>   			}
>   			pfn += 1 << page_order(page);
>   		}
>

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] fix: decrease NR_FREE_PAGES when isolate page from buddy
  2015-07-02  9:52 ` Vlastimil Babka
@ 2015-07-03  7:15   ` "김민경/주임연구원/SW Platform(연)AOT팀(minkyung88.kim@lge.com)"
  2015-07-03 14:11     ` Vlastimil Babka
  0 siblings, 1 reply; 19+ messages in thread
From: "김민경/주임연구원/SW Platform(연)AOT팀(minkyung88.kim@lge.com)" @ 2015-07-03  7:15 UTC (permalink / raw)
  To: Vlastimil Babka, Andrew Morton, linux-mm
  Cc: Seungho Park, kmk3210, Joonsoo Kim, Minchan Kim

As Vlastimil Babka expalin, this patch is useless and working not correctly.
Thank you for your review :)

2015-07-02 i??i?? 6:52i?? Vlastimil Babka i?'(e??) i?' e,?:
> [+CC Joonsoo and Minchan]
>
> On 07/01/2015 03:17 AM, minkyung88.kim@lge.com wrote:
>> From: "minkyung88.kim" <minkyung88.kim@lge.com>
>>
>> NR_FREEPAGE should be decreased when pages are isolated from buddy.
>> Therefore fix the count.
>
> Did you really observe an accounting bug and this patch fixed it, or 
> is it just because of code inspection?
>
> The patched code has this comment:
>
> /*
>  * If race between isolatation and allocation happens,
>  * some free pages could be in MIGRATE_MOVABLE list
>  * although pageblock's migratation type of the page
>  * is MIGRATE_ISOLATE. Catch it and move the page into
>  * MIGRATE_ISOLATE list.
>  */
>
> This is from 2012 and I'm not sure if it still applies. Joonsoo's 
> series last year was to eliminate these races, see e.g. 51bb1a4093 
> ("mm/page_alloc: add freepage on isolate pageblock to correct buddy 
> list").
>
> So I think that this piece of code shouldn't be useful anymore. Well, 
> actually I think it can trigger, but it's a false positive and (before 
> your patch) result in basically a no-op. The reason is that the value 
> of get_freepage_migratetype(page) is a just an optimization used only 
> for pages on pcplists. It's not guaranteed to be correct for pages in 
> the buddy free lists (and it can get stale even on the pcplists).
>
> Now, the code from Joonsoo's patch mentioned above does this in
> free_pcppages_bulk():
>
> mt = get_freepage_migratetype(page);
> if (unlikely(has_isolate_pageblock(zone)))
>         mt = get_pageblock_migratetype(page);
>
> /* MIGRATE_MOVABLE list may include MIGRATE_RESERVEs */
> __free_one_page(page, page_to_pfn(page), zone, 0, mt);
>
> So if get_freepage_migratetype(page) returns e.g. MIGRATE_MOVABLE but 
> the pageblock is MIGRATE_ISOLATE, it will catch this and tell 
> __free_one_page() the correct migratetype. However, nothing will 
> update the freepage's migratetype by set_freepage_migratetype(), 
> because it would be a pointless waste of CPU cycles. The page however 
> goes to the correct MIGRATE_ISOLATE list. (note that this is likely 
> not the only way how freepage_migratetype can be perceived as incorrect)
>
> That means the code you are patching can really find the page where 
> get_freepage_migratetype(page) will return MIGRATE_MOVABLE, i.e. != 
> MIGRATE_ISOLATE will be true. But the move_freepages() call would be a 
> no-op, as the page is already on the correct list and the accounting 
> of freepages is correct.
>
> So my conclusion is that after your patch, the freepage accounting 
> could actually get broken, not fixed. But I may be wrong. Hopefully 
> Joonsoo can verify this :)
>
> If that's true, then the whole test you are patching should be 
> dropped. Also we should make it clearer that 
> get_freepage_migratetype() is only used for pages on pcplists (and 
> even there it may differ from pageblock's migratetype and also from 
> the pcplist the page is actually on, in cases of page stealing), as 
> this is not the first confusion.
> We should also drop the usage set_freepage_migratetype() from 
> move_freepages() while at it.
> Now the last usage of get_freepage_migratetype() outside of 
> page_alloc.c is the page isolation code and I argue it's wrong. So 
> after that is removed, we can actually also make the functions 
> internal to page_alloc.c.
>
>> Signed-off-by: minkyung88.kim <minkyung88.kim@lge.com>
>> ---
>>   mm/page_isolation.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>> index 303c908..16cc172 100644
>> --- a/mm/page_isolation.c
>> +++ b/mm/page_isolation.c
>> @@ -233,10 +233,14 @@ __test_page_isolated_in_pageblock(unsigned long 
>> pfn, unsigned long end_pfn,
>>                */
>>               if (get_freepage_migratetype(page) != MIGRATE_ISOLATE) {
>>                   struct page *end_page;
>> +                struct zone *zone = page_zone(page);
>> +                int mt = get_freepage_migratetype(page);
>> +                unsigned long nr_pages;
>>
>>                   end_page = page + (1 << page_order(page)) - 1;
>> -                move_freepages(page_zone(page), page, end_page,
>> +                nr_pages = move_freepages(zone, page, end_page,
>>                           MIGRATE_ISOLATE);
>> +                __mod_zone_freepage_state(zone, -nr_pages, mt);
>>               }
>>               pfn += 1 << page_order(page);
>>           }
>>
>
>

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] fix: decrease NR_FREE_PAGES when isolate page from buddy
  2015-07-03  7:15   ` "김민경/주임연구원/SW Platform(연)AOT팀(minkyung88.kim@lge.com)"
@ 2015-07-03 14:11     ` Vlastimil Babka
  2015-07-21 12:53       ` [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages Vlastimil Babka
  0 siblings, 1 reply; 19+ messages in thread
From: Vlastimil Babka @ 2015-07-03 14:11 UTC (permalink / raw)
  To: "김민경/주임연구원/SW
	Platform(연)AOT팀(minkyung88.kim@lge.com)",
	Andrew Morton, linux-mm
  Cc: Seungho Park, kmk3210, Joonsoo Kim, Minchan Kim

On 3.7.2015 9:15, "e1?e? 1/4 e2 1/2 /iGBP 1/4 i??i??eu!i??/SW Platform(i??)AOTi??
(minkyung88.kim@lge.com)" wrote:
> As Vlastimil Babka expalin, this patch is useless and working not correctly.
> Thank you for your review :)

Thanks for reminding us of the code that should be cleaned up :)
I have the patches almost ready and will send them later on.

Thanks.

> 2015-07-02 i??i?? 6:52i?? Vlastimil Babka i?'(e??) i?' e,?:
>> [+CC Joonsoo and Minchan]
>>
>> On 07/01/2015 03:17 AM, minkyung88.kim@lge.com wrote:
>>> From: "minkyung88.kim" <minkyung88.kim@lge.com>
>>>
>>> NR_FREEPAGE should be decreased when pages are isolated from buddy.
>>> Therefore fix the count.
>>
>> Did you really observe an accounting bug and this patch fixed it, or 
>> is it just because of code inspection?
>>
>> The patched code has this comment:
>>
>> /*
>>  * If race between isolatation and allocation happens,
>>  * some free pages could be in MIGRATE_MOVABLE list
>>  * although pageblock's migratation type of the page
>>  * is MIGRATE_ISOLATE. Catch it and move the page into
>>  * MIGRATE_ISOLATE list.
>>  */
>>
>> This is from 2012 and I'm not sure if it still applies. Joonsoo's 
>> series last year was to eliminate these races, see e.g. 51bb1a4093 
>> ("mm/page_alloc: add freepage on isolate pageblock to correct buddy 
>> list").
>>
>> So I think that this piece of code shouldn't be useful anymore. Well, 
>> actually I think it can trigger, but it's a false positive and (before 
>> your patch) result in basically a no-op. The reason is that the value 
>> of get_freepage_migratetype(page) is a just an optimization used only 
>> for pages on pcplists. It's not guaranteed to be correct for pages in 
>> the buddy free lists (and it can get stale even on the pcplists).
>>
>> Now, the code from Joonsoo's patch mentioned above does this in
>> free_pcppages_bulk():
>>
>> mt = get_freepage_migratetype(page);
>> if (unlikely(has_isolate_pageblock(zone)))
>>         mt = get_pageblock_migratetype(page);
>>
>> /* MIGRATE_MOVABLE list may include MIGRATE_RESERVEs */
>> __free_one_page(page, page_to_pfn(page), zone, 0, mt);
>>
>> So if get_freepage_migratetype(page) returns e.g. MIGRATE_MOVABLE but 
>> the pageblock is MIGRATE_ISOLATE, it will catch this and tell 
>> __free_one_page() the correct migratetype. However, nothing will 
>> update the freepage's migratetype by set_freepage_migratetype(), 
>> because it would be a pointless waste of CPU cycles. The page however 
>> goes to the correct MIGRATE_ISOLATE list. (note that this is likely 
>> not the only way how freepage_migratetype can be perceived as incorrect)
>>
>> That means the code you are patching can really find the page where 
>> get_freepage_migratetype(page) will return MIGRATE_MOVABLE, i.e. != 
>> MIGRATE_ISOLATE will be true. But the move_freepages() call would be a 
>> no-op, as the page is already on the correct list and the accounting 
>> of freepages is correct.
>>
>> So my conclusion is that after your patch, the freepage accounting 
>> could actually get broken, not fixed. But I may be wrong. Hopefully 
>> Joonsoo can verify this :)
>>
>> If that's true, then the whole test you are patching should be 
>> dropped. Also we should make it clearer that 
>> get_freepage_migratetype() is only used for pages on pcplists (and 
>> even there it may differ from pageblock's migratetype and also from 
>> the pcplist the page is actually on, in cases of page stealing), as 
>> this is not the first confusion.
>> We should also drop the usage set_freepage_migratetype() from 
>> move_freepages() while at it.
>> Now the last usage of get_freepage_migratetype() outside of 
>> page_alloc.c is the page isolation code and I argue it's wrong. So 
>> after that is removed, we can actually also make the functions 
>> internal to page_alloc.c.
>>
>>> Signed-off-by: minkyung88.kim <minkyung88.kim@lge.com>
>>> ---
>>>   mm/page_isolation.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>>> index 303c908..16cc172 100644
>>> --- a/mm/page_isolation.c
>>> +++ b/mm/page_isolation.c
>>> @@ -233,10 +233,14 @@ __test_page_isolated_in_pageblock(unsigned long 
>>> pfn, unsigned long end_pfn,
>>>                */
>>>               if (get_freepage_migratetype(page) != MIGRATE_ISOLATE) {
>>>                   struct page *end_page;
>>> +                struct zone *zone = page_zone(page);
>>> +                int mt = get_freepage_migratetype(page);
>>> +                unsigned long nr_pages;
>>>
>>>                   end_page = page + (1 << page_order(page)) - 1;
>>> -                move_freepages(page_zone(page), page, end_page,
>>> +                nr_pages = move_freepages(zone, page, end_page,
>>>                           MIGRATE_ISOLATE);
>>> +                __mod_zone_freepage_state(zone, -nr_pages, mt);
>>>               }
>>>               pfn += 1 << page_order(page);
>>>           }
>>>
>>
>>
> 
> --
> 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>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages
  2015-07-03 14:11     ` Vlastimil Babka
@ 2015-07-21 12:53       ` Vlastimil Babka
  2015-07-21 12:53         ` [PATCH 2/2] mm: rename and move get/set_freepage_migratetype Vlastimil Babka
                           ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Vlastimil Babka @ 2015-07-21 12:53 UTC (permalink / raw)
  To: Andrew Morton, linux-mm
  Cc: linux-kernel, minkyung88.kim, kmk3210, Seungho Park,
	Vlastimil Babka, Joonsoo Kim, Minchan Kim, Michal Nazarewicz,
	Laura Abbott, Naoya Horiguchi, Johannes Weiner,
	Kirill A. Shutemov, Mel Gorman

The __test_page_isolated_in_pageblock() is used to verify whether all pages
in pageblock were either successfully isolated, or are hwpoisoned. Two of the
possible state of pages, that are tested, are however bogus and misleading.

Both tests rely on get_freepage_migratetype(page), which however has no
guarantees about pages on freelists. Specifically, it doesn't guarantee that
the migratetype returned by the function actually matches the migratetype of
the freelist that the page is on. Such guarantee is not its purpose and would
have negative impact on allocator performance.

The first test checks whether the freepage_migratetype equals MIGRATE_ISOLATE,
supposedly to catch races between page isolation and allocator activity. These
races should be fixed nowadays with 51bb1a4093 ("mm/page_alloc: add freepage
on isolate pageblock to correct buddy list") and related patches. As explained
above, the check wouldn't be able to catch them reliably anyway. For the same
reason false positives can happen, although they are harmless, as the
move_freepages() call would just move the page to the same freelist it's
already on. So removing the test is not a bug fix, just cleanup. After this
patch, we assume that all PageBuddy pages are on the correct freelist and that
the races were really fixed. A truly reliable verification in the form of e.g.
VM_BUG_ON() would be complicated and is arguably not needed.

The second test (page_count(page) == 0 && get_freepage_migratetype(page)
== MIGRATE_ISOLATE) is probably supposed (the code comes from a big memory
isolation patch from 2007) to catch pages on MIGRATE_ISOLATE pcplists.
However, pcplists don't contain MIGRATE_ISOLATE freepages nowadays, those are
freed directly to free lists, so the check is obsolete. Remove it as well.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Laura Abbott <lauraa@codeaurora.org>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/page_isolation.c | 30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index 0e69d25..9eaa489c 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -226,34 +226,16 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn,
 			continue;
 		}
 		page = pfn_to_page(pfn);
-		if (PageBuddy(page)) {
+		if (PageBuddy(page))
 			/*
-			 * If race between isolatation and allocation happens,
-			 * some free pages could be in MIGRATE_MOVABLE list
-			 * although pageblock's migratation type of the page
-			 * is MIGRATE_ISOLATE. Catch it and move the page into
-			 * MIGRATE_ISOLATE list.
+			 * If the page is on a free list, it has to be on
+			 * the correct MIGRATE_ISOLATE freelist. There is no
+			 * simple way to verify that as VM_BUG_ON(), though.
 			 */
-			if (get_freepage_migratetype(page) != MIGRATE_ISOLATE) {
-				struct page *end_page;
-
-				end_page = page + (1 << page_order(page)) - 1;
-				move_freepages(page_zone(page), page, end_page,
-						MIGRATE_ISOLATE);
-			}
 			pfn += 1 << page_order(page);
-		}
-		else if (page_count(page) == 0 &&
-			get_freepage_migratetype(page) == MIGRATE_ISOLATE)
-			pfn += 1;
-		else if (skip_hwpoisoned_pages && PageHWPoison(page)) {
-			/*
-			 * The HWPoisoned page may be not in buddy
-			 * system, and page_count() is not 0.
-			 */
+		else if (skip_hwpoisoned_pages && PageHWPoison(page))
+			/* A HWPoisoned page cannot be also PageBuddy */
 			pfn++;
-			continue;
-		}
 		else
 			break;
 	}
-- 
2.4.5

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/2] mm: rename and move get/set_freepage_migratetype
  2015-07-21 12:53       ` [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages Vlastimil Babka
@ 2015-07-21 12:53         ` Vlastimil Babka
  2015-07-21 22:47           ` David Rientjes
  2015-07-22 12:29           ` Vlastimil Babka
  2015-07-21 22:43         ` [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages David Rientjes
                           ` (4 subsequent siblings)
  5 siblings, 2 replies; 19+ messages in thread
From: Vlastimil Babka @ 2015-07-21 12:53 UTC (permalink / raw)
  To: Andrew Morton, linux-mm
  Cc: linux-kernel, minkyung88.kim, kmk3210, Seungho Park,
	Vlastimil Babka, Joonsoo Kim, Minchan Kim, Michal Nazarewicz,
	Laura Abbott, Naoya Horiguchi, Kirill A. Shutemov, Mel Gorman,
	Johannes Weiner

The pair of get/set_freepage_migratetype() functions are used to cache
pageblock migratetype for a page put on a pcplist, so that it does not have
to be retrieved again when the page is put on a free list (e.g. when pcplists
become full). Historically it was also assumed that the value is accurate for
pages on freelists (as the functions' names unfortunately suggest), but that
cannot be guaranteed without affecting various allocator fast paths. It is in
fact not needed and all such uses have been removed.

The last remaining (but pointless) usage related to pages of freelists is in
move_freepages(), which this patch removes.

To prevent further confusion, rename the functions to
get/set_pcppage_migratetype() and expand their description. Since all the
users are now in mm/page_alloc.c, move the functions there from the shared
header.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Laura Abbott <lauraa@codeaurora.org>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Johannes Weiner <hannes@cmpxchg.org>
---
 include/linux/mm.h | 12 ------------
 mm/page_alloc.c    | 42 +++++++++++++++++++++++++++++-------------
 2 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index c3a2b37..ce36145 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -310,18 +310,6 @@ struct inode;
 #define page_private(page)		((page)->private)
 #define set_page_private(page, v)	((page)->private = (v))
 
-/* It's valid only if the page is free path or free_list */
-static inline void set_freepage_migratetype(struct page *page, int migratetype)
-{
-	page->index = migratetype;
-}
-
-/* It's valid only if the page is free path or free_list */
-static inline int get_freepage_migratetype(struct page *page)
-{
-	return page->index;
-}
-
 /*
  * FIXME: take this include out, include page-flags.h in
  * files which need it (119 of them)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 41dc650..ab81150 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -125,6 +125,24 @@ unsigned long dirty_balance_reserve __read_mostly;
 int percpu_pagelist_fraction;
 gfp_t gfp_allowed_mask __read_mostly = GFP_BOOT_MASK;
 
+/*
+ * A cached value of the page's pageblock's migratetype, used when the page is
+ * put on a pcplist. Used to avoid the pageblock migratetype lookup when
+ * freeing from pcplists in most cases, at the cost of possibly becoming stale.
+ * Also the migratetype set in the page does not necessarily match the pcplist
+ * index, e.g. page might have MIGRATE_CMA set but be on a pcplist with any
+ * other index - this ensures that it will be put on the correct CMA freelist.
+ */
+static inline int get_pcppage_migratetype(struct page *page)
+{
+	return page->index;
+}
+
+static inline void set_pcppage_migratetype(struct page *page, int migratetype)
+{
+	page->index = migratetype;
+}
+
 #ifdef CONFIG_PM_SLEEP
 /*
  * The following functions are used by the suspend/hibernate code to temporarily
@@ -789,7 +807,7 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 			page = list_entry(list->prev, struct page, lru);
 			/* must delete as __free_one_page list manipulates */
 			list_del(&page->lru);
-			mt = get_freepage_migratetype(page);
+			mt = get_pcppage_migratetype(page);
 			if (unlikely(has_isolate_pageblock(zone)))
 				mt = get_pageblock_migratetype(page);
 
@@ -959,7 +977,7 @@ static void __free_pages_ok(struct page *page, unsigned int order)
 	migratetype = get_pfnblock_migratetype(page, pfn);
 	local_irq_save(flags);
 	__count_vm_events(PGFREE, 1 << order);
-	set_freepage_migratetype(page, migratetype);
+	set_pcppage_migratetype(page, migratetype);
 	free_one_page(page_zone(page), page, pfn, order, migratetype);
 	local_irq_restore(flags);
 }
@@ -1380,7 +1398,7 @@ struct page *__rmqueue_smallest(struct zone *zone, unsigned int order,
 		rmv_page_order(page);
 		area->nr_free--;
 		expand(zone, page, order, current_order, area, migratetype);
-		set_freepage_migratetype(page, migratetype);
+		set_pcppage_migratetype(page, migratetype);
 		return page;
 	}
 
@@ -1457,7 +1475,6 @@ int move_freepages(struct zone *zone,
 		order = page_order(page);
 		list_move(&page->lru,
 			  &zone->free_area[order].free_list[migratetype]);
-		set_freepage_migratetype(page, migratetype);
 		page += 1 << order;
 		pages_moved += 1 << order;
 	}
@@ -1627,14 +1644,13 @@ __rmqueue_fallback(struct zone *zone, unsigned int order, int start_migratetype)
 		expand(zone, page, order, current_order, area,
 					start_migratetype);
 		/*
-		 * The freepage_migratetype may differ from pageblock's
+		 * The pcppage_migratetype may differ from pageblock's
 		 * migratetype depending on the decisions in
-		 * try_to_steal_freepages(). This is OK as long as it
-		 * does not differ for MIGRATE_CMA pageblocks. For CMA
-		 * we need to make sure unallocated pages flushed from
-		 * pcp lists are returned to the correct freelist.
+		 * find_suitable_fallback(). This is OK as long as it does not
+		 * differ for MIGRATE_CMA pageblocks. Those can be used as
+		 * fallback only via special __rmqueue_cma_fallback() function
 		 */
-		set_freepage_migratetype(page, start_migratetype);
+		set_pcppage_migratetype(page, start_migratetype);
 
 		trace_mm_page_alloc_extfrag(page, order, current_order,
 			start_migratetype, fallback_mt);
@@ -1710,7 +1726,7 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
 		else
 			list_add_tail(&page->lru, list);
 		list = &page->lru;
-		if (is_migrate_cma(get_freepage_migratetype(page)))
+		if (is_migrate_cma(get_pcppage_migratetype(page)))
 			__mod_zone_page_state(zone, NR_FREE_CMA_PAGES,
 					      -(1 << order));
 	}
@@ -1907,7 +1923,7 @@ void free_hot_cold_page(struct page *page, bool cold)
 		return;
 
 	migratetype = get_pfnblock_migratetype(page, pfn);
-	set_freepage_migratetype(page, migratetype);
+	set_pcppage_migratetype(page, migratetype);
 	local_irq_save(flags);
 	__count_vm_event(PGFREE);
 
@@ -2112,7 +2128,7 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
 		if (!page)
 			goto failed;
 		__mod_zone_freepage_state(zone, -(1 << order),
-					  get_freepage_migratetype(page));
+					  get_pcppage_migratetype(page));
 	}
 
 	__mod_zone_page_state(zone, NR_ALLOC_BATCH, -(1 << order));
-- 
2.4.5

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages
  2015-07-21 12:53       ` [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages Vlastimil Babka
  2015-07-21 12:53         ` [PATCH 2/2] mm: rename and move get/set_freepage_migratetype Vlastimil Babka
@ 2015-07-21 22:43         ` David Rientjes
  2015-07-22 12:25           ` Vlastimil Babka
  2015-07-23  5:23         ` Joonsoo Kim
                           ` (3 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: David Rientjes @ 2015-07-21 22:43 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, linux-mm, linux-kernel, minkyung88.kim, kmk3210,
	Seungho Park, Joonsoo Kim, Minchan Kim, Michal Nazarewicz,
	Laura Abbott, Naoya Horiguchi, Johannes Weiner,
	Kirill A. Shutemov, Mel Gorman

On Tue, 21 Jul 2015, Vlastimil Babka wrote:

> The __test_page_isolated_in_pageblock() is used to verify whether all pages
> in pageblock were either successfully isolated, or are hwpoisoned. Two of the
> possible state of pages, that are tested, are however bogus and misleading.
> 
> Both tests rely on get_freepage_migratetype(page), which however has no
> guarantees about pages on freelists. Specifically, it doesn't guarantee that
> the migratetype returned by the function actually matches the migratetype of
> the freelist that the page is on. Such guarantee is not its purpose and would
> have negative impact on allocator performance.
> 
> The first test checks whether the freepage_migratetype equals MIGRATE_ISOLATE,
> supposedly to catch races between page isolation and allocator activity. These
> races should be fixed nowadays with 51bb1a4093 ("mm/page_alloc: add freepage
> on isolate pageblock to correct buddy list") and related patches. As explained
> above, the check wouldn't be able to catch them reliably anyway. For the same
> reason false positives can happen, although they are harmless, as the
> move_freepages() call would just move the page to the same freelist it's
> already on. So removing the test is not a bug fix, just cleanup. After this
> patch, we assume that all PageBuddy pages are on the correct freelist and that
> the races were really fixed. A truly reliable verification in the form of e.g.
> VM_BUG_ON() would be complicated and is arguably not needed.
> 
> The second test (page_count(page) == 0 && get_freepage_migratetype(page)
> == MIGRATE_ISOLATE) is probably supposed (the code comes from a big memory
> isolation patch from 2007) to catch pages on MIGRATE_ISOLATE pcplists.
> However, pcplists don't contain MIGRATE_ISOLATE freepages nowadays, those are
> freed directly to free lists, so the check is obsolete. Remove it as well.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> ---
>  mm/page_isolation.c | 30 ++++++------------------------
>  1 file changed, 6 insertions(+), 24 deletions(-)
> 
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index 0e69d25..9eaa489c 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -226,34 +226,16 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn,
>  			continue;
>  		}
>  		page = pfn_to_page(pfn);
> -		if (PageBuddy(page)) {
> +		if (PageBuddy(page))
>  			/*
> -			 * If race between isolatation and allocation happens,
> -			 * some free pages could be in MIGRATE_MOVABLE list
> -			 * although pageblock's migratation type of the page
> -			 * is MIGRATE_ISOLATE. Catch it and move the page into
> -			 * MIGRATE_ISOLATE list.
> +			 * If the page is on a free list, it has to be on
> +			 * the correct MIGRATE_ISOLATE freelist. There is no
> +			 * simple way to verify that as VM_BUG_ON(), though.
>  			 */
> -			if (get_freepage_migratetype(page) != MIGRATE_ISOLATE) {
> -				struct page *end_page;
> -
> -				end_page = page + (1 << page_order(page)) - 1;
> -				move_freepages(page_zone(page), page, end_page,
> -						MIGRATE_ISOLATE);
> -			}
>  			pfn += 1 << page_order(page);
> -		}
> -		else if (page_count(page) == 0 &&
> -			get_freepage_migratetype(page) == MIGRATE_ISOLATE)
> -			pfn += 1;
> -		else if (skip_hwpoisoned_pages && PageHWPoison(page)) {
> -			/*
> -			 * The HWPoisoned page may be not in buddy
> -			 * system, and page_count() is not 0.
> -			 */
> +		else if (skip_hwpoisoned_pages && PageHWPoison(page))
> +			/* A HWPoisoned page cannot be also PageBuddy */
>  			pfn++;
> -			continue;
> -		}
>  		else
>  			break;
>  	}

You may want to consider stating your assumptions explicitly in the code, 
perhaps with VM_BUG_ON(), such as in free_pcppages_bulk() to ensure things 
like get_freepage_migratetype(page) != MIGRATE_ISOLATE.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] mm: rename and move get/set_freepage_migratetype
  2015-07-21 12:53         ` [PATCH 2/2] mm: rename and move get/set_freepage_migratetype Vlastimil Babka
@ 2015-07-21 22:47           ` David Rientjes
  2015-07-22 12:29           ` Vlastimil Babka
  1 sibling, 0 replies; 19+ messages in thread
From: David Rientjes @ 2015-07-21 22:47 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, linux-mm, linux-kernel, minkyung88.kim, kmk3210,
	Seungho Park, Joonsoo Kim, Minchan Kim, Michal Nazarewicz,
	Laura Abbott, Naoya Horiguchi, Kirill A. Shutemov, Mel Gorman,
	Johannes Weiner

On Tue, 21 Jul 2015, Vlastimil Babka wrote:

> The pair of get/set_freepage_migratetype() functions are used to cache
> pageblock migratetype for a page put on a pcplist, so that it does not have
> to be retrieved again when the page is put on a free list (e.g. when pcplists
> become full). Historically it was also assumed that the value is accurate for
> pages on freelists (as the functions' names unfortunately suggest), but that
> cannot be guaranteed without affecting various allocator fast paths. It is in
> fact not needed and all such uses have been removed.
> 
> The last remaining (but pointless) usage related to pages of freelists is in
> move_freepages(), which this patch removes.
> 
> To prevent further confusion, rename the functions to
> get/set_pcppage_migratetype() and expand their description. Since all the
> users are now in mm/page_alloc.c, move the functions there from the shared
> header.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Johannes Weiner <hannes@cmpxchg.org>

Acked-by: David Rientjes <rientjes@google.com>

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages
  2015-07-21 22:43         ` [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages David Rientjes
@ 2015-07-22 12:25           ` Vlastimil Babka
  2015-07-22 21:42             ` David Rientjes
  0 siblings, 1 reply; 19+ messages in thread
From: Vlastimil Babka @ 2015-07-22 12:25 UTC (permalink / raw)
  To: David Rientjes
  Cc: Andrew Morton, linux-mm, linux-kernel, minkyung88.kim, kmk3210,
	Seungho Park, Joonsoo Kim, Minchan Kim, Michal Nazarewicz,
	Laura Abbott, Naoya Horiguchi, Johannes Weiner,
	Kirill A. Shutemov, Mel Gorman

On 07/22/2015 12:43 AM, David Rientjes wrote:
> On Tue, 21 Jul 2015, Vlastimil Babka wrote:
> 
> 
> You may want to consider stating your assumptions explicitly in the code,
> perhaps with VM_BUG_ON(), such as in free_pcppages_bulk() to ensure things
> like get_freepage_migratetype(page) != MIGRATE_ISOLATE.

Hm, OK here's a fixup. I've pondered others but nothing made sense
unless I would have to devise really twisted ways in which somebody
broke the code in the future, and that's not worth BUG_ON().

But the checking made me realize that one more
set_freepage_migratetype() can be removed in the other patch, so I
will resend it.

------8<------
From: Vlastimil Babka <vbabka@suse.cz>
Date: Wed, 22 Jul 2015 14:16:52 +0200
Subject: [PATCH 2/3] fixup! mm, page_isolation: remove bogus tests for
 isolated pages

---
 mm/page_alloc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 41dc650..c61fef8 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -789,7 +789,11 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 			page = list_entry(list->prev, struct page, lru);
 			/* must delete as __free_one_page list manipulates */
 			list_del(&page->lru);
+
 			mt = get_freepage_migratetype(page);
+			/* MIGRATE_ISOLATE page should not go to pcplists */
+			VM_BUG_ON_PAGE(is_migrate_isolate(mt), page);
+			/* Pageblock could have been isolated meanwhile */
 			if (unlikely(has_isolate_pageblock(zone)))
 				mt = get_pageblock_migratetype(page);
 
-- 
2.4.5


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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] mm: rename and move get/set_freepage_migratetype
  2015-07-21 12:53         ` [PATCH 2/2] mm: rename and move get/set_freepage_migratetype Vlastimil Babka
  2015-07-21 22:47           ` David Rientjes
@ 2015-07-22 12:29           ` Vlastimil Babka
  2015-07-23  5:24             ` Joonsoo Kim
                               ` (3 more replies)
  1 sibling, 4 replies; 19+ messages in thread
From: Vlastimil Babka @ 2015-07-22 12:29 UTC (permalink / raw)
  To: Andrew Morton, linux-mm
  Cc: linux-kernel, minkyung88.kim, kmk3210, Seungho Park, Joonsoo Kim,
	Minchan Kim, Michal Nazarewicz, Laura Abbott, Naoya Horiguchi,
	Kirill A. Shutemov, Mel Gorman, Johannes Weiner

On 07/21/2015 02:53 PM, Vlastimil Babka wrote:
> The pair of get/set_freepage_migratetype() functions are used to cache
> pageblock migratetype for a page put on a pcplist, so that it does not have
> to be retrieved again when the page is put on a free list (e.g. when pcplists
> become full). Historically it was also assumed that the value is accurate for
> pages on freelists (as the functions' names unfortunately suggest), but that
> cannot be guaranteed without affecting various allocator fast paths. It is in
> fact not needed and all such uses have been removed.
> 
> The last remaining (but pointless) usage related to pages of freelists is in
> move_freepages(), which this patch removes.

I realized there's one more callsite that can be removed. Here's
whole updated patch due to different changelog and to cope with
context changed by the fixlet to patch 1/2.

------8<------
From: Vlastimil Babka <vbabka@suse.cz>
Date: Thu, 2 Jul 2015 16:37:06 +0200
Subject: mm: rename and move get/set_freepage_migratetype

The pair of get/set_freepage_migratetype() functions are used to cache
pageblock migratetype for a page put on a pcplist, so that it does not have
to be retrieved again when the page is put on a free list (e.g. when pcplists
become full). Historically it was also assumed that the value is accurate for
pages on freelists (as the functions' names unfortunately suggest), but that
cannot be guaranteed without affecting various allocator fast paths. It is in
fact not needed and all such uses have been removed.

The last two remaining (but pointless) usages related to pages of freelists
are removed by this patch:
- move_freepages() which operates on pages already on freelists
- __free_pages_ok() which puts a page directly to freelist, bypassing pcplists

To prevent further confusion, rename the functions to
get/set_pcppage_migratetype() and expand their description. Since all the
users are now in mm/page_alloc.c, move the functions there from the shared
header.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Laura Abbott <lauraa@codeaurora.org>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Johannes Weiner <hannes@cmpxchg.org>
---
 include/linux/mm.h | 12 ------------
 mm/page_alloc.c    | 41 ++++++++++++++++++++++++++++-------------
 2 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index c3a2b37..ce36145 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -310,18 +310,6 @@ struct inode;
 #define page_private(page)		((page)->private)
 #define set_page_private(page, v)	((page)->private = (v))
 
-/* It's valid only if the page is free path or free_list */
-static inline void set_freepage_migratetype(struct page *page, int migratetype)
-{
-	page->index = migratetype;
-}
-
-/* It's valid only if the page is free path or free_list */
-static inline int get_freepage_migratetype(struct page *page)
-{
-	return page->index;
-}
-
 /*
  * FIXME: take this include out, include page-flags.h in
  * files which need it (119 of them)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c61fef8..4b220cb 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -125,6 +125,24 @@ unsigned long dirty_balance_reserve __read_mostly;
 int percpu_pagelist_fraction;
 gfp_t gfp_allowed_mask __read_mostly = GFP_BOOT_MASK;
 
+/*
+ * A cached value of the page's pageblock's migratetype, used when the page is
+ * put on a pcplist. Used to avoid the pageblock migratetype lookup when
+ * freeing from pcplists in most cases, at the cost of possibly becoming stale.
+ * Also the migratetype set in the page does not necessarily match the pcplist
+ * index, e.g. page might have MIGRATE_CMA set but be on a pcplist with any
+ * other index - this ensures that it will be put on the correct CMA freelist.
+ */
+static inline int get_pcppage_migratetype(struct page *page)
+{
+	return page->index;
+}
+
+static inline void set_pcppage_migratetype(struct page *page, int migratetype)
+{
+	page->index = migratetype;
+}
+
 #ifdef CONFIG_PM_SLEEP
 /*
  * The following functions are used by the suspend/hibernate code to temporarily
@@ -790,7 +808,7 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 			/* must delete as __free_one_page list manipulates */
 			list_del(&page->lru);
 
-			mt = get_freepage_migratetype(page);
+			mt = get_pcppage_migratetype(page);
 			/* MIGRATE_ISOLATE page should not go to pcplists */
 			VM_BUG_ON_PAGE(is_migrate_isolate(mt), page);
 			/* Pageblock could have been isolated meanwhile */
@@ -963,7 +981,6 @@ static void __free_pages_ok(struct page *page, unsigned int order)
 	migratetype = get_pfnblock_migratetype(page, pfn);
 	local_irq_save(flags);
 	__count_vm_events(PGFREE, 1 << order);
-	set_freepage_migratetype(page, migratetype);
 	free_one_page(page_zone(page), page, pfn, order, migratetype);
 	local_irq_restore(flags);
 }
@@ -1384,7 +1401,7 @@ struct page *__rmqueue_smallest(struct zone *zone, unsigned int order,
 		rmv_page_order(page);
 		area->nr_free--;
 		expand(zone, page, order, current_order, area, migratetype);
-		set_freepage_migratetype(page, migratetype);
+		set_pcppage_migratetype(page, migratetype);
 		return page;
 	}
 
@@ -1461,7 +1478,6 @@ int move_freepages(struct zone *zone,
 		order = page_order(page);
 		list_move(&page->lru,
 			  &zone->free_area[order].free_list[migratetype]);
-		set_freepage_migratetype(page, migratetype);
 		page += 1 << order;
 		pages_moved += 1 << order;
 	}
@@ -1631,14 +1647,13 @@ __rmqueue_fallback(struct zone *zone, unsigned int order, int start_migratetype)
 		expand(zone, page, order, current_order, area,
 					start_migratetype);
 		/*
-		 * The freepage_migratetype may differ from pageblock's
+		 * The pcppage_migratetype may differ from pageblock's
 		 * migratetype depending on the decisions in
-		 * try_to_steal_freepages(). This is OK as long as it
-		 * does not differ for MIGRATE_CMA pageblocks. For CMA
-		 * we need to make sure unallocated pages flushed from
-		 * pcp lists are returned to the correct freelist.
+		 * find_suitable_fallback(). This is OK as long as it does not
+		 * differ for MIGRATE_CMA pageblocks. Those can be used as
+		 * fallback only via special __rmqueue_cma_fallback() function
 		 */
-		set_freepage_migratetype(page, start_migratetype);
+		set_pcppage_migratetype(page, start_migratetype);
 
 		trace_mm_page_alloc_extfrag(page, order, current_order,
 			start_migratetype, fallback_mt);
@@ -1714,7 +1729,7 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
 		else
 			list_add_tail(&page->lru, list);
 		list = &page->lru;
-		if (is_migrate_cma(get_freepage_migratetype(page)))
+		if (is_migrate_cma(get_pcppage_migratetype(page)))
 			__mod_zone_page_state(zone, NR_FREE_CMA_PAGES,
 					      -(1 << order));
 	}
@@ -1911,7 +1926,7 @@ void free_hot_cold_page(struct page *page, bool cold)
 		return;
 
 	migratetype = get_pfnblock_migratetype(page, pfn);
-	set_freepage_migratetype(page, migratetype);
+	set_pcppage_migratetype(page, migratetype);
 	local_irq_save(flags);
 	__count_vm_event(PGFREE);
 
@@ -2116,7 +2131,7 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
 		if (!page)
 			goto failed;
 		__mod_zone_freepage_state(zone, -(1 << order),
-					  get_freepage_migratetype(page));
+					  get_pcppage_migratetype(page));
 	}
 
 	__mod_zone_page_state(zone, NR_ALLOC_BATCH, -(1 << order));
-- 
2.4.5



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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages
  2015-07-22 12:25           ` Vlastimil Babka
@ 2015-07-22 21:42             ` David Rientjes
  0 siblings, 0 replies; 19+ messages in thread
From: David Rientjes @ 2015-07-22 21:42 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, linux-mm, linux-kernel, minkyung88.kim, kmk3210,
	Seungho Park, Joonsoo Kim, Minchan Kim, Michal Nazarewicz,
	Laura Abbott, Naoya Horiguchi, Johannes Weiner,
	Kirill A. Shutemov, Mel Gorman

On Wed, 22 Jul 2015, Vlastimil Babka wrote:

> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Wed, 22 Jul 2015 14:16:52 +0200
> Subject: [PATCH 2/3] fixup! mm, page_isolation: remove bogus tests for
>  isolated pages
> 
> ---
>  mm/page_alloc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 41dc650..c61fef8 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -789,7 +789,11 @@ static void free_pcppages_bulk(struct zone *zone, int count,
>  			page = list_entry(list->prev, struct page, lru);
>  			/* must delete as __free_one_page list manipulates */
>  			list_del(&page->lru);
> +
>  			mt = get_freepage_migratetype(page);
> +			/* MIGRATE_ISOLATE page should not go to pcplists */
> +			VM_BUG_ON_PAGE(is_migrate_isolate(mt), page);
> +			/* Pageblock could have been isolated meanwhile */
>  			if (unlikely(has_isolate_pageblock(zone)))
>  				mt = get_pageblock_migratetype(page);
>  

Looks good, thanks!

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages
  2015-07-21 12:53       ` [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages Vlastimil Babka
  2015-07-21 12:53         ` [PATCH 2/2] mm: rename and move get/set_freepage_migratetype Vlastimil Babka
  2015-07-21 22:43         ` [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages David Rientjes
@ 2015-07-23  5:23         ` Joonsoo Kim
  2015-07-23  5:41         ` Naoya Horiguchi
                           ` (2 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Joonsoo Kim @ 2015-07-23  5:23 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, linux-mm, linux-kernel, minkyung88.kim, kmk3210,
	Seungho Park, Minchan Kim, Michal Nazarewicz, Laura Abbott,
	Naoya Horiguchi, Johannes Weiner, Kirill A. Shutemov, Mel Gorman

On Tue, Jul 21, 2015 at 02:53:37PM +0200, Vlastimil Babka wrote:
> The __test_page_isolated_in_pageblock() is used to verify whether all pages
> in pageblock were either successfully isolated, or are hwpoisoned. Two of the
> possible state of pages, that are tested, are however bogus and misleading.
> 
> Both tests rely on get_freepage_migratetype(page), which however has no
> guarantees about pages on freelists. Specifically, it doesn't guarantee that
> the migratetype returned by the function actually matches the migratetype of
> the freelist that the page is on. Such guarantee is not its purpose and would
> have negative impact on allocator performance.
> 
> The first test checks whether the freepage_migratetype equals MIGRATE_ISOLATE,
> supposedly to catch races between page isolation and allocator activity. These
> races should be fixed nowadays with 51bb1a4093 ("mm/page_alloc: add freepage
> on isolate pageblock to correct buddy list") and related patches. As explained
> above, the check wouldn't be able to catch them reliably anyway. For the same
> reason false positives can happen, although they are harmless, as the
> move_freepages() call would just move the page to the same freelist it's
> already on. So removing the test is not a bug fix, just cleanup. After this
> patch, we assume that all PageBuddy pages are on the correct freelist and that
> the races were really fixed. A truly reliable verification in the form of e.g.
> VM_BUG_ON() would be complicated and is arguably not needed.
> 
> The second test (page_count(page) == 0 && get_freepage_migratetype(page)
> == MIGRATE_ISOLATE) is probably supposed (the code comes from a big memory
> isolation patch from 2007) to catch pages on MIGRATE_ISOLATE pcplists.
> However, pcplists don't contain MIGRATE_ISOLATE freepages nowadays, those are
> freed directly to free lists, so the check is obsolete. Remove it as well.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Thanks for taking care of this.

Thanks.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] mm: rename and move get/set_freepage_migratetype
  2015-07-22 12:29           ` Vlastimil Babka
@ 2015-07-23  5:24             ` Joonsoo Kim
  2015-07-23  6:48             ` Naoya Horiguchi
                               ` (2 subsequent siblings)
  3 siblings, 0 replies; 19+ messages in thread
From: Joonsoo Kim @ 2015-07-23  5:24 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, linux-mm, linux-kernel, minkyung88.kim, kmk3210,
	Seungho Park, Minchan Kim, Michal Nazarewicz, Laura Abbott,
	Naoya Horiguchi, Kirill A. Shutemov, Mel Gorman, Johannes Weiner

On Wed, Jul 22, 2015 at 02:29:08PM +0200, Vlastimil Babka wrote:
> On 07/21/2015 02:53 PM, Vlastimil Babka wrote:
> > The pair of get/set_freepage_migratetype() functions are used to cache
> > pageblock migratetype for a page put on a pcplist, so that it does not have
> > to be retrieved again when the page is put on a free list (e.g. when pcplists
> > become full). Historically it was also assumed that the value is accurate for
> > pages on freelists (as the functions' names unfortunately suggest), but that
> > cannot be guaranteed without affecting various allocator fast paths. It is in
> > fact not needed and all such uses have been removed.
> > 
> > The last remaining (but pointless) usage related to pages of freelists is in
> > move_freepages(), which this patch removes.
> 
> I realized there's one more callsite that can be removed. Here's
> whole updated patch due to different changelog and to cope with
> context changed by the fixlet to patch 1/2.
> 
> ------8<------
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 2 Jul 2015 16:37:06 +0200
> Subject: mm: rename and move get/set_freepage_migratetype
> 
> The pair of get/set_freepage_migratetype() functions are used to cache
> pageblock migratetype for a page put on a pcplist, so that it does not have
> to be retrieved again when the page is put on a free list (e.g. when pcplists
> become full). Historically it was also assumed that the value is accurate for
> pages on freelists (as the functions' names unfortunately suggest), but that
> cannot be guaranteed without affecting various allocator fast paths. It is in
> fact not needed and all such uses have been removed.
> 
> The last two remaining (but pointless) usages related to pages of freelists
> are removed by this patch:
> - move_freepages() which operates on pages already on freelists
> - __free_pages_ok() which puts a page directly to freelist, bypassing pcplists
> 
> To prevent further confusion, rename the functions to
> get/set_pcppage_migratetype() and expand their description. Since all the
> users are now in mm/page_alloc.c, move the functions there from the shared
> header.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Acked-by: David Rientjes <rientjes@google.com>

Acked-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Thanks.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages
  2015-07-21 12:53       ` [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages Vlastimil Babka
                           ` (2 preceding siblings ...)
  2015-07-23  5:23         ` Joonsoo Kim
@ 2015-07-23  5:41         ` Naoya Horiguchi
  2015-07-29 13:55         ` Mel Gorman
  2015-07-30 14:07         ` Michal Nazarewicz
  5 siblings, 0 replies; 19+ messages in thread
From: Naoya Horiguchi @ 2015-07-23  5:41 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, linux-mm, linux-kernel, minkyung88.kim, kmk3210,
	Seungho Park, Joonsoo Kim, Minchan Kim, Michal Nazarewicz,
	Laura Abbott, Johannes Weiner, Kirill A. Shutemov, Mel Gorman

On Tue, Jul 21, 2015 at 02:53:37PM +0200, Vlastimil Babka wrote:
> The __test_page_isolated_in_pageblock() is used to verify whether all pages
> in pageblock were either successfully isolated, or are hwpoisoned. Two of the
> possible state of pages, that are tested, are however bogus and misleading.
> 
> Both tests rely on get_freepage_migratetype(page), which however has no
> guarantees about pages on freelists. Specifically, it doesn't guarantee that
> the migratetype returned by the function actually matches the migratetype of
> the freelist that the page is on. Such guarantee is not its purpose and would
> have negative impact on allocator performance.
> 
> The first test checks whether the freepage_migratetype equals MIGRATE_ISOLATE,
> supposedly to catch races between page isolation and allocator activity. These
> races should be fixed nowadays with 51bb1a4093 ("mm/page_alloc: add freepage
> on isolate pageblock to correct buddy list") and related patches. As explained
> above, the check wouldn't be able to catch them reliably anyway. For the same
> reason false positives can happen, although they are harmless, as the
> move_freepages() call would just move the page to the same freelist it's
> already on. So removing the test is not a bug fix, just cleanup. After this
> patch, we assume that all PageBuddy pages are on the correct freelist and that
> the races were really fixed. A truly reliable verification in the form of e.g.
> VM_BUG_ON() would be complicated and is arguably not needed.
> 
> The second test (page_count(page) == 0 && get_freepage_migratetype(page)
> == MIGRATE_ISOLATE) is probably supposed (the code comes from a big memory
> isolation patch from 2007) to catch pages on MIGRATE_ISOLATE pcplists.
> However, pcplists don't contain MIGRATE_ISOLATE freepages nowadays, those are
> freed directly to free lists, so the check is obsolete. Remove it as well.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

Looks good to me.

Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
--
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>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] mm: rename and move get/set_freepage_migratetype
  2015-07-22 12:29           ` Vlastimil Babka
  2015-07-23  5:24             ` Joonsoo Kim
@ 2015-07-23  6:48             ` Naoya Horiguchi
  2015-07-29 13:57             ` Mel Gorman
  2015-07-30 14:08             ` Michal Nazarewicz
  3 siblings, 0 replies; 19+ messages in thread
From: Naoya Horiguchi @ 2015-07-23  6:48 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, linux-mm, linux-kernel, minkyung88.kim, kmk3210,
	Seungho Park, Joonsoo Kim, Minchan Kim, Michal Nazarewicz,
	Laura Abbott, Kirill A. Shutemov, Mel Gorman, Johannes Weiner

On Wed, Jul 22, 2015 at 02:29:08PM +0200, Vlastimil Babka wrote:
> On 07/21/2015 02:53 PM, Vlastimil Babka wrote:
> > The pair of get/set_freepage_migratetype() functions are used to cache
> > pageblock migratetype for a page put on a pcplist, so that it does not have
> > to be retrieved again when the page is put on a free list (e.g. when pcplists
> > become full). Historically it was also assumed that the value is accurate for
> > pages on freelists (as the functions' names unfortunately suggest), but that
> > cannot be guaranteed without affecting various allocator fast paths. It is in
> > fact not needed and all such uses have been removed.
> > 
> > The last remaining (but pointless) usage related to pages of freelists is in
> > move_freepages(), which this patch removes.
> 
> I realized there's one more callsite that can be removed. Here's
> whole updated patch due to different changelog and to cope with
> context changed by the fixlet to patch 1/2.
> 
> ------8<------
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 2 Jul 2015 16:37:06 +0200
> Subject: mm: rename and move get/set_freepage_migratetype
> 
> The pair of get/set_freepage_migratetype() functions are used to cache
> pageblock migratetype for a page put on a pcplist, so that it does not have
> to be retrieved again when the page is put on a free list (e.g. when pcplists
> become full). Historically it was also assumed that the value is accurate for
> pages on freelists (as the functions' names unfortunately suggest), but that
> cannot be guaranteed without affecting various allocator fast paths. It is in
> fact not needed and all such uses have been removed.
> 
> The last two remaining (but pointless) usages related to pages of freelists
> are removed by this patch:
> - move_freepages() which operates on pages already on freelists
> - __free_pages_ok() which puts a page directly to freelist, bypassing pcplists
> 
> To prevent further confusion, rename the functions to
> get/set_pcppage_migratetype() and expand their description. Since all the
> users are now in mm/page_alloc.c, move the functions there from the shared
> header.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Acked-by: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Johannes Weiner <hannes@cmpxchg.org>

Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
--
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>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages
  2015-07-21 12:53       ` [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages Vlastimil Babka
                           ` (3 preceding siblings ...)
  2015-07-23  5:41         ` Naoya Horiguchi
@ 2015-07-29 13:55         ` Mel Gorman
  2015-07-30 14:07         ` Michal Nazarewicz
  5 siblings, 0 replies; 19+ messages in thread
From: Mel Gorman @ 2015-07-29 13:55 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, linux-mm, linux-kernel, minkyung88.kim, kmk3210,
	Seungho Park, Joonsoo Kim, Minchan Kim, Michal Nazarewicz,
	Laura Abbott, Naoya Horiguchi, Johannes Weiner,
	Kirill A. Shutemov

On Tue, Jul 21, 2015 at 02:53:37PM +0200, Vlastimil Babka wrote:
> The __test_page_isolated_in_pageblock() is used to verify whether all pages
> in pageblock were either successfully isolated, or are hwpoisoned. Two of the
> possible state of pages, that are tested, are however bogus and misleading.
> 
> Both tests rely on get_freepage_migratetype(page), which however has no
> guarantees about pages on freelists. Specifically, it doesn't guarantee that
> the migratetype returned by the function actually matches the migratetype of
> the freelist that the page is on. Such guarantee is not its purpose and would
> have negative impact on allocator performance.
> 
> The first test checks whether the freepage_migratetype equals MIGRATE_ISOLATE,
> supposedly to catch races between page isolation and allocator activity. These
> races should be fixed nowadays with 51bb1a4093 ("mm/page_alloc: add freepage
> on isolate pageblock to correct buddy list") and related patches. As explained
> above, the check wouldn't be able to catch them reliably anyway. For the same
> reason false positives can happen, although they are harmless, as the
> move_freepages() call would just move the page to the same freelist it's
> already on. So removing the test is not a bug fix, just cleanup. After this
> patch, we assume that all PageBuddy pages are on the correct freelist and that
> the races were really fixed. A truly reliable verification in the form of e.g.
> VM_BUG_ON() would be complicated and is arguably not needed.
> 
> The second test (page_count(page) == 0 && get_freepage_migratetype(page)
> == MIGRATE_ISOLATE) is probably supposed (the code comes from a big memory
> isolation patch from 2007) to catch pages on MIGRATE_ISOLATE pcplists.
> However, pcplists don't contain MIGRATE_ISOLATE freepages nowadays, those are
> freed directly to free lists, so the check is obsolete. Remove it as well.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] mm: rename and move get/set_freepage_migratetype
  2015-07-22 12:29           ` Vlastimil Babka
  2015-07-23  5:24             ` Joonsoo Kim
  2015-07-23  6:48             ` Naoya Horiguchi
@ 2015-07-29 13:57             ` Mel Gorman
  2015-07-30 14:08             ` Michal Nazarewicz
  3 siblings, 0 replies; 19+ messages in thread
From: Mel Gorman @ 2015-07-29 13:57 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, linux-mm, linux-kernel, minkyung88.kim, kmk3210,
	Seungho Park, Joonsoo Kim, Minchan Kim, Michal Nazarewicz,
	Laura Abbott, Naoya Horiguchi, Kirill A. Shutemov,
	Johannes Weiner

On Wed, Jul 22, 2015 at 02:29:08PM +0200, Vlastimil Babka wrote:
> Subject: mm: rename and move get/set_freepage_migratetype
> 
> The pair of get/set_freepage_migratetype() functions are used to cache
> pageblock migratetype for a page put on a pcplist, so that it does not have
> to be retrieved again when the page is put on a free list (e.g. when pcplists
> become full). Historically it was also assumed that the value is accurate for
> pages on freelists (as the functions' names unfortunately suggest), but that
> cannot be guaranteed without affecting various allocator fast paths. It is in
> fact not needed and all such uses have been removed.
> 
> The last two remaining (but pointless) usages related to pages of freelists
> are removed by this patch:
> - move_freepages() which operates on pages already on freelists
> - __free_pages_ok() which puts a page directly to freelist, bypassing pcplists
> 
> To prevent further confusion, rename the functions to
> get/set_pcppage_migratetype() and expand their description. Since all the
> users are now in mm/page_alloc.c, move the functions there from the shared
> header.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Acked-by: David Rientjes <rientjes@google.com>

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages
  2015-07-21 12:53       ` [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages Vlastimil Babka
                           ` (4 preceding siblings ...)
  2015-07-29 13:55         ` Mel Gorman
@ 2015-07-30 14:07         ` Michal Nazarewicz
  5 siblings, 0 replies; 19+ messages in thread
From: Michal Nazarewicz @ 2015-07-30 14:07 UTC (permalink / raw)
  To: Vlastimil Babka, Andrew Morton, linux-mm
  Cc: linux-kernel, minkyung88.kim, kmk3210, Seungho Park, Joonsoo Kim,
	Minchan Kim, Laura Abbott, Naoya Horiguchi, Johannes Weiner,
	Kirill A. Shutemov, Mel Gorman

On Tue, Jul 21 2015, Vlastimil Babka wrote:
> The __test_page_isolated_in_pageblock() is used to verify whether all pages
> in pageblock were either successfully isolated, or are hwpoisoned. Two of the
> possible state of pages, that are tested, are however bogus and misleading.
>
> Both tests rely on get_freepage_migratetype(page), which however has no
> guarantees about pages on freelists. Specifically, it doesn't guarantee that
> the migratetype returned by the function actually matches the migratetype of
> the freelist that the page is on. Such guarantee is not its purpose and would
> have negative impact on allocator performance.
>
> The first test checks whether the freepage_migratetype equals MIGRATE_ISOLATE,
> supposedly to catch races between page isolation and allocator activity. These
> races should be fixed nowadays with 51bb1a4093 ("mm/page_alloc: add freepage
> on isolate pageblock to correct buddy list") and related patches. As explained
> above, the check wouldn't be able to catch them reliably anyway. For the same
> reason false positives can happen, although they are harmless, as the
> move_freepages() call would just move the page to the same freelist it's
> already on. So removing the test is not a bug fix, just cleanup. After this
> patch, we assume that all PageBuddy pages are on the correct freelist and that
> the races were really fixed. A truly reliable verification in the form of e.g.
> VM_BUG_ON() would be complicated and is arguably not needed.
>
> The second test (page_count(page) == 0 && get_freepage_migratetype(page)
> == MIGRATE_ISOLATE) is probably supposed (the code comes from a big memory
> isolation patch from 2007) to catch pages on MIGRATE_ISOLATE pcplists.
> However, pcplists don't contain MIGRATE_ISOLATE freepages nowadays, those are
> freed directly to free lists, so the check is obsolete. Remove it as well.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Nazarewicz <mina86@mina86.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> ---
>  mm/page_isolation.c | 30 ++++++------------------------
>  1 file changed, 6 insertions(+), 24 deletions(-)
>
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index 0e69d25..9eaa489c 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -226,34 +226,16 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn,
>  			continue;
>  		}
>  		page = pfn_to_page(pfn);
> -		if (PageBuddy(page)) {
> +		if (PageBuddy(page))
>  			/*
> -			 * If race between isolatation and allocation happens,
> -			 * some free pages could be in MIGRATE_MOVABLE list
> -			 * although pageblock's migratation type of the page
> -			 * is MIGRATE_ISOLATE. Catch it and move the page into
> -			 * MIGRATE_ISOLATE list.
> +			 * If the page is on a free list, it has to be on
> +			 * the correct MIGRATE_ISOLATE freelist. There is no
> +			 * simple way to verify that as VM_BUG_ON(), though.
>  			 */
> -			if (get_freepage_migratetype(page) != MIGRATE_ISOLATE) {
> -				struct page *end_page;
> -
> -				end_page = page + (1 << page_order(page)) - 1;
> -				move_freepages(page_zone(page), page, end_page,
> -						MIGRATE_ISOLATE);
> -			}
>  			pfn += 1 << page_order(page);
> -		}
> -		else if (page_count(page) == 0 &&
> -			get_freepage_migratetype(page) == MIGRATE_ISOLATE)
> -			pfn += 1;
> -		else if (skip_hwpoisoned_pages && PageHWPoison(page)) {
> -			/*
> -			 * The HWPoisoned page may be not in buddy
> -			 * system, and page_count() is not 0.
> -			 */
> +		else if (skip_hwpoisoned_pages && PageHWPoison(page))
> +			/* A HWPoisoned page cannot be also PageBuddy */
>  			pfn++;
> -			continue;
> -		}
>  		else
>  			break;
>  	}
> -- 
> 2.4.5
>

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] mm: rename and move get/set_freepage_migratetype
  2015-07-22 12:29           ` Vlastimil Babka
                               ` (2 preceding siblings ...)
  2015-07-29 13:57             ` Mel Gorman
@ 2015-07-30 14:08             ` Michal Nazarewicz
  3 siblings, 0 replies; 19+ messages in thread
From: Michal Nazarewicz @ 2015-07-30 14:08 UTC (permalink / raw)
  To: Vlastimil Babka, Andrew Morton, linux-mm
  Cc: linux-kernel, minkyung88.kim, kmk3210, Seungho Park, Joonsoo Kim,
	Minchan Kim, Laura Abbott, Naoya Horiguchi, Kirill A. Shutemov,
	Mel Gorman, Johannes Weiner

On Wed, Jul 22 2015, Vlastimil Babka wrote:
> Subject: mm: rename and move get/set_freepage_migratetype
>
> The pair of get/set_freepage_migratetype() functions are used to cache
> pageblock migratetype for a page put on a pcplist, so that it does not have
> to be retrieved again when the page is put on a free list (e.g. when pcplists
> become full). Historically it was also assumed that the value is accurate for
> pages on freelists (as the functions' names unfortunately suggest), but that
> cannot be guaranteed without affecting various allocator fast paths. It is in
> fact not needed and all such uses have been removed.
>
> The last two remaining (but pointless) usages related to pages of freelists
> are removed by this patch:
> - move_freepages() which operates on pages already on freelists
> - __free_pages_ok() which puts a page directly to freelist, bypassing pcplists
>
> To prevent further confusion, rename the functions to
> get/set_pcppage_migratetype() and expand their description. Since all the
> users are now in mm/page_alloc.c, move the functions there from the shared
> header.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Acked-by: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Nazarewicz <mina86@mina86.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  include/linux/mm.h | 12 ------------
>  mm/page_alloc.c    | 41 ++++++++++++++++++++++++++++-------------
>  2 files changed, 28 insertions(+), 25 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index c3a2b37..ce36145 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -310,18 +310,6 @@ struct inode;
>  #define page_private(page)		((page)->private)
>  #define set_page_private(page, v)	((page)->private = (v))
>  
> -/* It's valid only if the page is free path or free_list */
> -static inline void set_freepage_migratetype(struct page *page, int migratetype)
> -{
> -	page->index = migratetype;
> -}
> -
> -/* It's valid only if the page is free path or free_list */
> -static inline int get_freepage_migratetype(struct page *page)
> -{
> -	return page->index;
> -}
> -
>  /*
>   * FIXME: take this include out, include page-flags.h in
>   * files which need it (119 of them)
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c61fef8..4b220cb 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -125,6 +125,24 @@ unsigned long dirty_balance_reserve __read_mostly;
>  int percpu_pagelist_fraction;
>  gfp_t gfp_allowed_mask __read_mostly = GFP_BOOT_MASK;
>  
> +/*
> + * A cached value of the page's pageblock's migratetype, used when the page is
> + * put on a pcplist. Used to avoid the pageblock migratetype lookup when
> + * freeing from pcplists in most cases, at the cost of possibly becoming stale.
> + * Also the migratetype set in the page does not necessarily match the pcplist
> + * index, e.g. page might have MIGRATE_CMA set but be on a pcplist with any
> + * other index - this ensures that it will be put on the correct CMA freelist.
> + */
> +static inline int get_pcppage_migratetype(struct page *page)
> +{
> +	return page->index;
> +}
> +
> +static inline void set_pcppage_migratetype(struct page *page, int migratetype)
> +{
> +	page->index = migratetype;
> +}
> +
>  #ifdef CONFIG_PM_SLEEP
>  /*
>   * The following functions are used by the suspend/hibernate code to temporarily
> @@ -790,7 +808,7 @@ static void free_pcppages_bulk(struct zone *zone, int count,
>  			/* must delete as __free_one_page list manipulates */
>  			list_del(&page->lru);
>  
> -			mt = get_freepage_migratetype(page);
> +			mt = get_pcppage_migratetype(page);
>  			/* MIGRATE_ISOLATE page should not go to pcplists */
>  			VM_BUG_ON_PAGE(is_migrate_isolate(mt), page);
>  			/* Pageblock could have been isolated meanwhile */
> @@ -963,7 +981,6 @@ static void __free_pages_ok(struct page *page, unsigned int order)
>  	migratetype = get_pfnblock_migratetype(page, pfn);
>  	local_irq_save(flags);
>  	__count_vm_events(PGFREE, 1 << order);
> -	set_freepage_migratetype(page, migratetype);
>  	free_one_page(page_zone(page), page, pfn, order, migratetype);
>  	local_irq_restore(flags);
>  }
> @@ -1384,7 +1401,7 @@ struct page *__rmqueue_smallest(struct zone *zone, unsigned int order,
>  		rmv_page_order(page);
>  		area->nr_free--;
>  		expand(zone, page, order, current_order, area, migratetype);
> -		set_freepage_migratetype(page, migratetype);
> +		set_pcppage_migratetype(page, migratetype);
>  		return page;
>  	}
>  
> @@ -1461,7 +1478,6 @@ int move_freepages(struct zone *zone,
>  		order = page_order(page);
>  		list_move(&page->lru,
>  			  &zone->free_area[order].free_list[migratetype]);
> -		set_freepage_migratetype(page, migratetype);
>  		page += 1 << order;
>  		pages_moved += 1 << order;
>  	}
> @@ -1631,14 +1647,13 @@ __rmqueue_fallback(struct zone *zone, unsigned int order, int start_migratetype)
>  		expand(zone, page, order, current_order, area,
>  					start_migratetype);
>  		/*
> -		 * The freepage_migratetype may differ from pageblock's
> +		 * The pcppage_migratetype may differ from pageblock's
>  		 * migratetype depending on the decisions in
> -		 * try_to_steal_freepages(). This is OK as long as it
> -		 * does not differ for MIGRATE_CMA pageblocks. For CMA
> -		 * we need to make sure unallocated pages flushed from
> -		 * pcp lists are returned to the correct freelist.
> +		 * find_suitable_fallback(). This is OK as long as it does not
> +		 * differ for MIGRATE_CMA pageblocks. Those can be used as
> +		 * fallback only via special __rmqueue_cma_fallback() function
>  		 */
> -		set_freepage_migratetype(page, start_migratetype);
> +		set_pcppage_migratetype(page, start_migratetype);
>  
>  		trace_mm_page_alloc_extfrag(page, order, current_order,
>  			start_migratetype, fallback_mt);
> @@ -1714,7 +1729,7 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
>  		else
>  			list_add_tail(&page->lru, list);
>  		list = &page->lru;
> -		if (is_migrate_cma(get_freepage_migratetype(page)))
> +		if (is_migrate_cma(get_pcppage_migratetype(page)))
>  			__mod_zone_page_state(zone, NR_FREE_CMA_PAGES,
>  					      -(1 << order));
>  	}
> @@ -1911,7 +1926,7 @@ void free_hot_cold_page(struct page *page, bool cold)
>  		return;
>  
>  	migratetype = get_pfnblock_migratetype(page, pfn);
> -	set_freepage_migratetype(page, migratetype);
> +	set_pcppage_migratetype(page, migratetype);
>  	local_irq_save(flags);
>  	__count_vm_event(PGFREE);
>  
> @@ -2116,7 +2131,7 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
>  		if (!page)
>  			goto failed;
>  		__mod_zone_freepage_state(zone, -(1 << order),
> -					  get_freepage_migratetype(page));
> +					  get_pcppage_migratetype(page));
>  	}
>  
>  	__mod_zone_page_state(zone, NR_ALLOC_BATCH, -(1 << order));
> -- 
> 2.4.5
>
>
>

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2015-07-30 14:08 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-01  1:17 [PATCH] fix: decrease NR_FREE_PAGES when isolate page from buddy minkyung88.kim
2015-07-02  9:52 ` Vlastimil Babka
2015-07-03  7:15   ` "김민경/주임연구원/SW Platform(연)AOT팀(minkyung88.kim@lge.com)"
2015-07-03 14:11     ` Vlastimil Babka
2015-07-21 12:53       ` [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages Vlastimil Babka
2015-07-21 12:53         ` [PATCH 2/2] mm: rename and move get/set_freepage_migratetype Vlastimil Babka
2015-07-21 22:47           ` David Rientjes
2015-07-22 12:29           ` Vlastimil Babka
2015-07-23  5:24             ` Joonsoo Kim
2015-07-23  6:48             ` Naoya Horiguchi
2015-07-29 13:57             ` Mel Gorman
2015-07-30 14:08             ` Michal Nazarewicz
2015-07-21 22:43         ` [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages David Rientjes
2015-07-22 12:25           ` Vlastimil Babka
2015-07-22 21:42             ` David Rientjes
2015-07-23  5:23         ` Joonsoo Kim
2015-07-23  5:41         ` Naoya Horiguchi
2015-07-29 13:55         ` Mel Gorman
2015-07-30 14:07         ` Michal Nazarewicz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).