[PATCH] mm/page_alloc: don't corrupt pcppage_migratetype

[PATCH] mm/page_alloc: don't corrupt pcppage_migratetype

[Doug Berger]

When placing pages on a pcp list, migratetype values over
MIGRATE_PCPTYPES get added to the MIGRATE_MOVABLE pcp list.

However, the actual migratetype is preserved in the page and
should not be changed to MIGRATE_MOVABLE or the page may end
up on the wrong free_list.

Fixes: df1acc856923 ("mm/page_alloc: avoid conflating IRQs disabled with zone->lock")
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 mm/page_alloc.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 73704e836649..8addb4919f75 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3436,19 +3436,10 @@ void free_unref_page_list(struct list_head *list)
 		 * comment in free_unref_page.
 		 */
 		migratetype = get_pcppage_migratetype(page);
-		if (unlikely(migratetype >= MIGRATE_PCPTYPES)) {
-			if (unlikely(is_migrate_isolate(migratetype))) {
-				list_del(&page->lru);
-				free_one_page(page_zone(page), page, pfn, 0,
-							migratetype, FPI_NONE);
-				continue;
-			}
-
-			/*
-			 * Non-isolated types over MIGRATE_PCPTYPES get added
-			 * to the MIGRATE_MOVABLE pcp list.
-			 */
-			set_pcppage_migratetype(page, MIGRATE_MOVABLE);
+		if (unlikely(is_migrate_isolate(migratetype))) {
+			list_del(&page->lru);
+			free_one_page(page_zone(page), page, pfn, 0, migratetype, FPI_NONE);
+			continue;
 		}
 
 		set_page_private(page, pfn);
@@ -3458,7 +3449,15 @@ void free_unref_page_list(struct list_head *list)
 	list_for_each_entry_safe(page, next, list, lru) {
 		pfn = page_private(page);
 		set_page_private(page, 0);
+
+		/*
+		 * Non-isolated types over MIGRATE_PCPTYPES get added
+		 * to the MIGRATE_MOVABLE pcp list.
+		 */
 		migratetype = get_pcppage_migratetype(page);
+		if (unlikely(migratetype >= MIGRATE_PCPTYPES))
+			migratetype = MIGRATE_MOVABLE;
+
 		trace_mm_page_free_batched(page);
 		free_unref_page_commit(page, pfn, migratetype, 0);
 
-- 
2.25.1

Re: [PATCH] mm/page_alloc: don't corrupt pcppage_migratetype

[Vlastimil Babka]

On 8/11/21 8:29 PM, Doug Berger wrote:
> When placing pages on a pcp list, migratetype values over
> MIGRATE_PCPTYPES get added to the MIGRATE_MOVABLE pcp list.
> 
> However, the actual migratetype is preserved in the page and
> should not be changed to MIGRATE_MOVABLE or the page may end
> up on the wrong free_list.

Nice, how did you find out? Were there any user-visible effects? (Hint: which?
the changelog should say that so that the severity of the bug can be judged).
Otherwise I agree the bug is there and patch is fixing it. Thanks.

> Fixes: df1acc856923 ("mm/page_alloc: avoid conflating IRQs disabled with zone->lock")
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/page_alloc.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 73704e836649..8addb4919f75 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3436,19 +3436,10 @@ void free_unref_page_list(struct list_head *list)
>  		 * comment in free_unref_page.
>  		 */
>  		migratetype = get_pcppage_migratetype(page);
> -		if (unlikely(migratetype >= MIGRATE_PCPTYPES)) {
> -			if (unlikely(is_migrate_isolate(migratetype))) {
> -				list_del(&page->lru);
> -				free_one_page(page_zone(page), page, pfn, 0,
> -							migratetype, FPI_NONE);
> -				continue;
> -			}
> -
> -			/*
> -			 * Non-isolated types over MIGRATE_PCPTYPES get added
> -			 * to the MIGRATE_MOVABLE pcp list.
> -			 */
> -			set_pcppage_migratetype(page, MIGRATE_MOVABLE);
> +		if (unlikely(is_migrate_isolate(migratetype))) {
> +			list_del(&page->lru);
> +			free_one_page(page_zone(page), page, pfn, 0, migratetype, FPI_NONE);
> +			continue;
>  		}
>  
>  		set_page_private(page, pfn);
> @@ -3458,7 +3449,15 @@ void free_unref_page_list(struct list_head *list)
>  	list_for_each_entry_safe(page, next, list, lru) {
>  		pfn = page_private(page);
>  		set_page_private(page, 0);
> +
> +		/*
> +		 * Non-isolated types over MIGRATE_PCPTYPES get added
> +		 * to the MIGRATE_MOVABLE pcp list.
> +		 */
>  		migratetype = get_pcppage_migratetype(page);
> +		if (unlikely(migratetype >= MIGRATE_PCPTYPES))
> +			migratetype = MIGRATE_MOVABLE;
> +
>  		trace_mm_page_free_batched(page);
>  		free_unref_page_commit(page, pfn, migratetype, 0);
>  
> 

Re: [PATCH] mm/page_alloc: don't corrupt pcppage_migratetype

[Mel Gorman]

On Wed, Aug 11, 2021 at 11:29:17AM -0700, Doug Berger wrote:
> When placing pages on a pcp list, migratetype values over
> MIGRATE_PCPTYPES get added to the MIGRATE_MOVABLE pcp list.
> 
> However, the actual migratetype is preserved in the page and
> should not be changed to MIGRATE_MOVABLE or the page may end
> up on the wrong free_list.
> 
> Fixes: df1acc856923 ("mm/page_alloc: avoid conflating IRQs disabled with zone->lock")
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Oops, yes. The impact is that HIGHATOMIC or CMA pages getting bulk freed
from the PCP lists could potentially end up on the wrong buddy list.
There are various consequences but minimally NR_FREE_CMA_PAGES accounting
could get screwed up.

Thanks Doug

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

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm/page_alloc: don't corrupt pcppage_migratetype

[Doug Berger]

On 8/12/2021 1:17 AM, Vlastimil Babka wrote:
> On 8/11/21 8:29 PM, Doug Berger wrote:
>> When placing pages on a pcp list, migratetype values over
>> MIGRATE_PCPTYPES get added to the MIGRATE_MOVABLE pcp list.
>>
>> However, the actual migratetype is preserved in the page and
>> should not be changed to MIGRATE_MOVABLE or the page may end
>> up on the wrong free_list.
> 
> Nice, how did you find out? Were there any user-visible effects? (Hint: which?
> the changelog should say that so that the severity of the bug can be judged).
> Otherwise I agree the bug is there and patch is fixing it. Thanks.

I did not observe the bug "in the wild", but rather noticed it while
reviewing the current CMA implementation. I would imagine in the worst
case that CMA memory could leak onto MIGRATE_MOVABLE free_lists and get
"stolen" through fallback for MIGRATE_UNMOVABLE allocations effectively
breaking CMA.

For full disclosure I should take this opportunity to say that I am
investigating ways to improve the CMA/Movable memory implementations
somewhat in line with what Joonsoo Kim has been trying, which is why I
observed this bug.

However, I largely agree with the proposal Mel Gorman made in his
comments on:
https://lkml.org/lkml/2016/4/28/244

I can appreciate that zones may provide a convenient way of grouping
memory with common attributes, but it is a bastardization of the
original intention of zones. I would prefer a mechanism for designating
blocks of memory as "sticky" movable such that they can only be used to
satisfy movable memory requests and not be converted to other
migratetypes. I've wrestled with the terminology and currently favor
Designated Movable Blocks (as opposed to "sticky") because these are
blocks of memory that have the movable attribute by designation rather
than by default or accident.

While CMA blocks should be a subset of Designated Movable Blocks since
they have the same migration properties, it may be desirable/necessary
to maintain a distinction to accommodate competing allocation
objectives. Specifically, Designated Movable Blocks used by CMA to
satisfy the needs of drivers will likely want the page allocator to be
less aggressive than users of Memory Hot-plug or kernelcore= Designated
Movable Blocks. Perhaps a round-robin scheme with MIGRATE_MOVABLE can
satisfy all users, but I'm not too optimistic.

I don't know if Mel has changed his opinions on this matter, but any
suggestions and encouragement are appreciated.

Thanks,
    Doug

> 
>> Fixes: df1acc856923 ("mm/page_alloc: avoid conflating IRQs disabled with zone->lock")
>> Signed-off-by: Doug Berger <opendmb@gmail.com>
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> 
>> ---
>>  mm/page_alloc.c | 25 ++++++++++++-------------
>>  1 file changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 73704e836649..8addb4919f75 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -3436,19 +3436,10 @@ void free_unref_page_list(struct list_head *list)
>>  		 * comment in free_unref_page.
>>  		 */
>>  		migratetype = get_pcppage_migratetype(page);
>> -		if (unlikely(migratetype >= MIGRATE_PCPTYPES)) {
>> -			if (unlikely(is_migrate_isolate(migratetype))) {
>> -				list_del(&page->lru);
>> -				free_one_page(page_zone(page), page, pfn, 0,
>> -							migratetype, FPI_NONE);
>> -				continue;
>> -			}
>> -
>> -			/*
>> -			 * Non-isolated types over MIGRATE_PCPTYPES get added
>> -			 * to the MIGRATE_MOVABLE pcp list.
>> -			 */
>> -			set_pcppage_migratetype(page, MIGRATE_MOVABLE);
>> +		if (unlikely(is_migrate_isolate(migratetype))) {
>> +			list_del(&page->lru);
>> +			free_one_page(page_zone(page), page, pfn, 0, migratetype, FPI_NONE);
>> +			continue;
>>  		}
>>  
>>  		set_page_private(page, pfn);
>> @@ -3458,7 +3449,15 @@ void free_unref_page_list(struct list_head *list)
>>  	list_for_each_entry_safe(page, next, list, lru) {
>>  		pfn = page_private(page);
>>  		set_page_private(page, 0);
>> +
>> +		/*
>> +		 * Non-isolated types over MIGRATE_PCPTYPES get added
>> +		 * to the MIGRATE_MOVABLE pcp list.
>> +		 */
>>  		migratetype = get_pcppage_migratetype(page);
>> +		if (unlikely(migratetype >= MIGRATE_PCPTYPES))
>> +			migratetype = MIGRATE_MOVABLE;
>> +
>>  		trace_mm_page_free_batched(page);
>>  		free_unref_page_commit(page, pfn, migratetype, 0);
>>  
>>
> 

Re: [PATCH] mm/page_alloc: don't corrupt pcppage_migratetype

[Vlastimil Babka]

On 8/13/21 2:21 AM, Doug Berger wrote:
> On 8/12/2021 1:17 AM, Vlastimil Babka wrote:
>> On 8/11/21 8:29 PM, Doug Berger wrote:
>>> When placing pages on a pcp list, migratetype values over
>>> MIGRATE_PCPTYPES get added to the MIGRATE_MOVABLE pcp list.
>>>
>>> However, the actual migratetype is preserved in the page and
>>> should not be changed to MIGRATE_MOVABLE or the page may end
>>> up on the wrong free_list.
>>
>> Nice, how did you find out? Were there any user-visible effects? (Hint: which?
>> the changelog should say that so that the severity of the bug can be judged).
>> Otherwise I agree the bug is there and patch is fixing it. Thanks.
> 
> I did not observe the bug "in the wild", but rather noticed it while
> reviewing the current CMA implementation. I would imagine in the worst
> case that CMA memory could leak onto MIGRATE_MOVABLE free_lists and get
> "stolen" through fallback for MIGRATE_UNMOVABLE allocations effectively
> breaking CMA.

Right, thanks.

> For full disclosure I should take this opportunity to say that I am
> investigating ways to improve the CMA/Movable memory implementations
> somewhat in line with what Joonsoo Kim has been trying, which is why I
> observed this bug.
> 
> However, I largely agree with the proposal Mel Gorman made in his
> comments on:
> https://lkml.org/lkml/2016/4/28/244
> 
> I can appreciate that zones may provide a convenient way of grouping
> memory with common attributes, but it is a bastardization of the
> original intention of zones. I would prefer a mechanism for designating
> blocks of memory as "sticky" movable such that they can only be used to
> satisfy movable memory requests and not be converted to other
> migratetypes. I've wrestled with the terminology and currently favor
> Designated Movable Blocks (as opposed to "sticky") because these are
> blocks of memory that have the movable attribute by designation rather
> than by default or accident.
> 
> While CMA blocks should be a subset of Designated Movable Blocks since
> they have the same migration properties, it may be desirable/necessary
> to maintain a distinction to accommodate competing allocation
> objectives. Specifically, Designated Movable Blocks used by CMA to
> satisfy the needs of drivers will likely want the page allocator to be
> less aggressive than users of Memory Hot-plug or kernelcore= Designated
> Movable Blocks. Perhaps a round-robin scheme with MIGRATE_MOVABLE can
> satisfy all users, but I'm not too optimistic.

I was, on the other hand, in favor Joonsoo's zone based solution, as
there's always problems with these designated pageblocks and watermarks.
It's also described in the thread you linked above. See
__zone_watermark_unusable_free() how it has to subtract
NR_FREE_CMA_PAGES for non-movable allocations. So you need to track
NR_FREE_CMA_PAGES which is rather fragile, and then reclaim target
depends on whether your goal is movable or unmovable allocation. Either
it's movable and you might end up in free memory only in those
designated-movable (today, MIGRATE_CMA) blocks, and unmovable allocation
has to direct reclaim and hope the reclaimed LRU pages will be in the
"normal" pageblocks, or you make sure that there's enough free pages in
normal pageblocks, and end up with lower memory utilization. A separate
zone solves this problem.

> I don't know if Mel has changed his opinions on this matter, but any
> suggestions and encouragement are appreciated.
> 
> Thanks,
>     Doug