[PATCH] mm/page_alloc: fix memalloc_nocma_{save/restore} APIs

[PATCH] mm/page_alloc: fix memalloc_nocma_{save/restore} APIs

[js1304]

From: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Currently, memalloc_nocma_{save/restore} API that prevents CMA area
in page allocation is implemented by using current_gfp_context(). However,
there are two problems of this implementation.

First, this doesn't work for allocation fastpath. In the fastpath,
original gfp_mask is used since current_gfp_context() is introduced in
order to control reclaim and it is on slowpath. So, CMA area can be
allocated through the allocation fastpath even if
memalloc_nocma_{save/restore} APIs are used. Currently, there is just
one user for these APIs and it has a fallback method to prevent actual
problem.
Second, clearing __GFP_MOVABLE in current_gfp_context() has a side effect
to exclude the memory on the ZONE_MOVABLE for allocation target.

To fix these problems, this patch changes the implementation to exclude
CMA area in page allocation. Main point of this change is using the
alloc_flags. alloc_flags is mainly used to control allocation so it fits
for excluding CMA area in allocation.

Fixes: d7fefcc8de91 (mm/cma: add PF flag to force non cma alloc)
Cc: <stable@vger.kernel.org>
Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 include/linux/sched/mm.h |  8 +-------
 mm/page_alloc.c          | 33 +++++++++++++++++++++++----------
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
index 480a4d1..17e0c31 100644
--- a/include/linux/sched/mm.h
+++ b/include/linux/sched/mm.h
@@ -177,12 +177,10 @@ static inline bool in_vfork(struct task_struct *tsk)
  * Applies per-task gfp context to the given allocation flags.
  * PF_MEMALLOC_NOIO implies GFP_NOIO
  * PF_MEMALLOC_NOFS implies GFP_NOFS
- * PF_MEMALLOC_NOCMA implies no allocation from CMA region.
  */
 static inline gfp_t current_gfp_context(gfp_t flags)
 {
-	if (unlikely(current->flags &
-		     (PF_MEMALLOC_NOIO | PF_MEMALLOC_NOFS | PF_MEMALLOC_NOCMA))) {
+	if (unlikely(current->flags & (PF_MEMALLOC_NOIO | PF_MEMALLOC_NOFS))) {
 		/*
 		 * NOIO implies both NOIO and NOFS and it is a weaker context
 		 * so always make sure it makes precedence
@@ -191,10 +189,6 @@ static inline gfp_t current_gfp_context(gfp_t flags)
 			flags &= ~(__GFP_IO | __GFP_FS);
 		else if (current->flags & PF_MEMALLOC_NOFS)
 			flags &= ~__GFP_FS;
-#ifdef CONFIG_CMA
-		if (current->flags & PF_MEMALLOC_NOCMA)
-			flags &= ~__GFP_MOVABLE;
-#endif
 	}
 	return flags;
 }
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index e028b87c..08cb35c 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2790,7 +2790,7 @@ __rmqueue(struct zone *zone, unsigned int order, int migratetype,
 	 * allocating from CMA when over half of the zone's free memory
 	 * is in the CMA area.
 	 */
-	if (migratetype == MIGRATE_MOVABLE &&
+	if (alloc_flags & ALLOC_CMA &&
 	    zone_page_state(zone, NR_FREE_CMA_PAGES) >
 	    zone_page_state(zone, NR_FREE_PAGES) / 2) {
 		page = __rmqueue_cma_fallback(zone, order);
@@ -2801,7 +2801,7 @@ __rmqueue(struct zone *zone, unsigned int order, int migratetype,
 retry:
 	page = __rmqueue_smallest(zone, order, migratetype);
 	if (unlikely(!page)) {
-		if (migratetype == MIGRATE_MOVABLE)
+		if (alloc_flags & ALLOC_CMA)
 			page = __rmqueue_cma_fallback(zone, order);
 
 		if (!page && __rmqueue_fallback(zone, order, migratetype,
@@ -3671,6 +3671,20 @@ alloc_flags_nofragment(struct zone *zone, gfp_t gfp_mask)
 	return alloc_flags;
 }
 
+static inline unsigned int current_alloc_flags(gfp_t gfp_mask,
+					unsigned int alloc_flags)
+{
+#ifdef CONFIG_CMA
+	unsigned int pflags = current->flags;
+
+	if (!(pflags & PF_MEMALLOC_NOCMA) &&
+		gfp_migratetype(gfp_mask) == MIGRATE_MOVABLE)
+		alloc_flags |= ALLOC_CMA;
+
+#endif
+	return alloc_flags;
+}
+
 /*
  * get_page_from_freelist goes through the zonelist trying to allocate
  * a page.
@@ -4316,10 +4330,8 @@ gfp_to_alloc_flags(gfp_t gfp_mask)
 	} else if (unlikely(rt_task(current)) && !in_interrupt())
 		alloc_flags |= ALLOC_HARDER;
 
-#ifdef CONFIG_CMA
-	if (gfp_migratetype(gfp_mask) == MIGRATE_MOVABLE)
-		alloc_flags |= ALLOC_CMA;
-#endif
+	alloc_flags = current_alloc_flags(gfp_mask, alloc_flags);
+
 	return alloc_flags;
 }
 
@@ -4619,8 +4631,10 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		wake_all_kswapds(order, gfp_mask, ac);
 
 	reserve_flags = __gfp_pfmemalloc_flags(gfp_mask);
-	if (reserve_flags)
+	if (reserve_flags) {
 		alloc_flags = reserve_flags;
+		alloc_flags = current_alloc_flags(gfp_mask, alloc_flags);
+	}
 
 	/*
 	 * Reset the nodemask and zonelist iterators if memory policies can be
@@ -4697,7 +4711,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 
 	/* Avoid allocations with no watermarks from looping endlessly */
 	if (tsk_is_oom_victim(current) &&
-	    (alloc_flags == ALLOC_OOM ||
+	    (alloc_flags & ALLOC_OOM ||
 	     (gfp_mask & __GFP_NOMEMALLOC)))
 		goto nopage;
 
@@ -4785,8 +4799,7 @@ static inline bool prepare_alloc_pages(gfp_t gfp_mask, unsigned int order,
 	if (should_fail_alloc_page(gfp_mask, order))
 		return false;
 
-	if (IS_ENABLED(CONFIG_CMA) && ac->migratetype == MIGRATE_MOVABLE)
-		*alloc_flags |= ALLOC_CMA;
+	*alloc_flags = current_alloc_flags(gfp_mask, *alloc_flags);
 
 	return true;
 }
-- 
2.7.4

Re: [PATCH] mm/page_alloc: fix memalloc_nocma_{save/restore} APIs

[Vlastimil Babka]

On 7/21/20 5:28 AM, js1304@gmail.com wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> Currently, memalloc_nocma_{save/restore} API that prevents CMA area
> in page allocation is implemented by using current_gfp_context(). However,
> there are two problems of this implementation.
> 
> First, this doesn't work for allocation fastpath. In the fastpath,
> original gfp_mask is used since current_gfp_context() is introduced in
> order to control reclaim and it is on slowpath. So, CMA area can be
> allocated through the allocation fastpath even if
> memalloc_nocma_{save/restore} APIs are used. Currently, there is just
> one user for these APIs and it has a fallback method to prevent actual
> problem.
> Second, clearing __GFP_MOVABLE in current_gfp_context() has a side effect
> to exclude the memory on the ZONE_MOVABLE for allocation target.
> 
> To fix these problems, this patch changes the implementation to exclude
> CMA area in page allocation. Main point of this change is using the
> alloc_flags. alloc_flags is mainly used to control allocation so it fits
> for excluding CMA area in allocation.

Moreover, the ALLOC_CMA flag already exists for exactly this purpose.

> Fixes: d7fefcc8de91 (mm/cma: add PF flag to force non cma alloc)
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

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

Thanks!

Re: [PATCH] mm/page_alloc: fix memalloc_nocma_{save/restore} APIs

[Matthew Wilcox]

On Tue, Jul 21, 2020 at 12:28:49PM +0900, js1304@gmail.com wrote:
> +static inline unsigned int current_alloc_flags(gfp_t gfp_mask,
> +					unsigned int alloc_flags)
> +{
> +#ifdef CONFIG_CMA
> +	unsigned int pflags = current->flags;
> +
> +	if (!(pflags & PF_MEMALLOC_NOCMA) &&
> +		gfp_migratetype(gfp_mask) == MIGRATE_MOVABLE)
> +		alloc_flags |= ALLOC_CMA;

Please don't indent by one tab when splitting a line because it looks like
the second line and third line are part of the same block.  Either do
this:

	if (!(pflags & PF_MEMALLOC_NOCMA) &&
	    gfp_migratetype(gfp_mask) == MIGRATE_MOVABLE)
		alloc_flags |= ALLOC_CMA;

or this:
	if (!(pflags & PF_MEMALLOC_NOCMA) &&
			gfp_migratetype(gfp_mask) == MIGRATE_MOVABLE)
		alloc_flags |= ALLOC_CMA;

> @@ -4619,8 +4631,10 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  		wake_all_kswapds(order, gfp_mask, ac);
>  
>  	reserve_flags = __gfp_pfmemalloc_flags(gfp_mask);
> -	if (reserve_flags)
> +	if (reserve_flags) {
>  		alloc_flags = reserve_flags;
> +		alloc_flags = current_alloc_flags(gfp_mask, alloc_flags);
> +	}

Is this right?  Shouldn't you be passing reserve_flags to
current_alloc_flags() here?  Also, there's no need to add { } here.

Re: [PATCH] mm/page_alloc: fix memalloc_nocma_{save/restore} APIs

[Vlastimil Babka]

On 7/21/20 2:05 PM, Matthew Wilcox wrote:
> On Tue, Jul 21, 2020 at 12:28:49PM +0900, js1304@gmail.com wrote:
>> +static inline unsigned int current_alloc_flags(gfp_t gfp_mask,
>> +					unsigned int alloc_flags)
>> +{
>> +#ifdef CONFIG_CMA
>> +	unsigned int pflags = current->flags;
>> +
>> +	if (!(pflags & PF_MEMALLOC_NOCMA) &&
>> +		gfp_migratetype(gfp_mask) == MIGRATE_MOVABLE)
>> +		alloc_flags |= ALLOC_CMA;
> 
> Please don't indent by one tab when splitting a line because it looks like
> the second line and third line are part of the same block.  Either do
> this:
> 
> 	if (!(pflags & PF_MEMALLOC_NOCMA) &&
> 	    gfp_migratetype(gfp_mask) == MIGRATE_MOVABLE)
> 		alloc_flags |= ALLOC_CMA;
> 
> or this:
> 	if (!(pflags & PF_MEMALLOC_NOCMA) &&
> 			gfp_migratetype(gfp_mask) == MIGRATE_MOVABLE)
> 		alloc_flags |= ALLOC_CMA;

Ah, good point.

>> @@ -4619,8 +4631,10 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>>  		wake_all_kswapds(order, gfp_mask, ac);
>>  
>>  	reserve_flags = __gfp_pfmemalloc_flags(gfp_mask);
>> -	if (reserve_flags)
>> +	if (reserve_flags) {
>>  		alloc_flags = reserve_flags;
>> +		alloc_flags = current_alloc_flags(gfp_mask, alloc_flags);
>> +	}
> 
> Is this right?  Shouldn't you be passing reserve_flags to
> current_alloc_flags() here?  Also, there's no need to add { } here.

Note the "alloc_flags = reserve_flags;" line is not being deleted here. But if
it was, your points would be true, and yeah it would be a bit more tidy.

Re: [PATCH] mm/page_alloc: fix memalloc_nocma_{save/restore} APIs

[Matthew Wilcox]

On Tue, Jul 21, 2020 at 02:38:56PM +0200, Vlastimil Babka wrote:
> On 7/21/20 2:05 PM, Matthew Wilcox wrote:
> > On Tue, Jul 21, 2020 at 12:28:49PM +0900, js1304@gmail.com wrote:
> >> @@ -4619,8 +4631,10 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> >>  		wake_all_kswapds(order, gfp_mask, ac);
> >>  
> >>  	reserve_flags = __gfp_pfmemalloc_flags(gfp_mask);
> >> -	if (reserve_flags)
> >> +	if (reserve_flags) {
> >>  		alloc_flags = reserve_flags;
> >> +		alloc_flags = current_alloc_flags(gfp_mask, alloc_flags);
> >> +	}
> > 
> > Is this right?  Shouldn't you be passing reserve_flags to
> > current_alloc_flags() here?  Also, there's no need to add { } here.
> 
> Note the "alloc_flags = reserve_flags;" line is not being deleted here. But if
> it was, your points would be true, and yeah it would be a bit more tidy.

Oh ... I should wake up a little more.

Yeah, I'd recommend just doing this:

-		alloc_flags = reserve_flags;
+		alloc_flags = current_alloc_flags(gfp_mask, reserve_flags);

Re: [PATCH] mm/page_alloc: fix memalloc_nocma_{save/restore} APIs

[Joonsoo Kim]

2020년 7월 21일 (화) 오후 9:39, Vlastimil Babka <vbabka@suse.cz>님이 작성:
>
> On 7/21/20 2:05 PM, Matthew Wilcox wrote:
> > On Tue, Jul 21, 2020 at 12:28:49PM +0900, js1304@gmail.com wrote:
> >> +static inline unsigned int current_alloc_flags(gfp_t gfp_mask,
> >> +                                    unsigned int alloc_flags)
> >> +{
> >> +#ifdef CONFIG_CMA
> >> +    unsigned int pflags = current->flags;
> >> +
> >> +    if (!(pflags & PF_MEMALLOC_NOCMA) &&
> >> +            gfp_migratetype(gfp_mask) == MIGRATE_MOVABLE)
> >> +            alloc_flags |= ALLOC_CMA;
> >
> > Please don't indent by one tab when splitting a line because it looks like
> > the second line and third line are part of the same block.  Either do
> > this:
> >
> >       if (!(pflags & PF_MEMALLOC_NOCMA) &&
> >           gfp_migratetype(gfp_mask) == MIGRATE_MOVABLE)
> >               alloc_flags |= ALLOC_CMA;
> >
> > or this:
> >       if (!(pflags & PF_MEMALLOC_NOCMA) &&
> >                       gfp_migratetype(gfp_mask) == MIGRATE_MOVABLE)
> >               alloc_flags |= ALLOC_CMA;
>
> Ah, good point.

Will change it.

Thanks.

Re: [PATCH] mm/page_alloc: fix memalloc_nocma_{save/restore} APIs

[Joonsoo Kim]

2020년 7월 21일 (화) 오후 9:43, Matthew Wilcox <willy@infradead.org>님이 작성:
>
> On Tue, Jul 21, 2020 at 02:38:56PM +0200, Vlastimil Babka wrote:
> > On 7/21/20 2:05 PM, Matthew Wilcox wrote:
> > > On Tue, Jul 21, 2020 at 12:28:49PM +0900, js1304@gmail.com wrote:
> > >> @@ -4619,8 +4631,10 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> > >>            wake_all_kswapds(order, gfp_mask, ac);
> > >>
> > >>    reserve_flags = __gfp_pfmemalloc_flags(gfp_mask);
> > >> -  if (reserve_flags)
> > >> +  if (reserve_flags) {
> > >>            alloc_flags = reserve_flags;
> > >> +          alloc_flags = current_alloc_flags(gfp_mask, alloc_flags);
> > >> +  }
> > >
> > > Is this right?  Shouldn't you be passing reserve_flags to
> > > current_alloc_flags() here?  Also, there's no need to add { } here.
> >
> > Note the "alloc_flags = reserve_flags;" line is not being deleted here. But if
> > it was, your points would be true, and yeah it would be a bit more tidy.
>
> Oh ... I should wake up a little more.
>
> Yeah, I'd recommend just doing this:
>
> -               alloc_flags = reserve_flags;
> +               alloc_flags = current_alloc_flags(gfp_mask, reserve_flags);

Okay. I will change it. Just note that the reason that I added it
separately is that
I think that separation is more readable since we can easily notice
that alloc_flags
is changed to reserve_flags without inspecting currect_alloc_flags() function.

Thanks.