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 Reviewed-by: Vlastimil Babka vbabka@suse.cz Signed-off-by: Joonsoo Kim iamjoonsoo.kim@lge.com --- include/linux/sched/mm.h | 8 +------- mm/page_alloc.c | 31 +++++++++++++++++++++---------- 2 files changed, 22 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..7336e94 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; }
@@ -4620,7 +4632,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
reserve_flags = __gfp_pfmemalloc_flags(gfp_mask); if (reserve_flags) - alloc_flags = reserve_flags; + alloc_flags = current_alloc_flags(gfp_mask, reserve_flags);
/* * Reset the nodemask and zonelist iterators if memory policies can be @@ -4697,7 +4709,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 +4797,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; }
Hi
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag fixing commit: d7fefcc8de91 ("mm/cma: add PF flag to force non cma alloc").
The bot has tested the following trees: v5.7.10, v5.4.53.
v5.7.10: Failed to apply! Possible dependencies: 01c0bfe061f3 ("mm: rename gfpflags_to_migratetype to gfp_migratetype for same convention") 16867664936e ("mm,page_alloc,cma: conditionally prefer cma pageblocks for movable allocations") 3334a45eb9e2 ("mm/page_alloc: use ac->high_zoneidx for classzone_idx") 3f08a302f533 ("mm: remove CONFIG_HAVE_MEMBLOCK_NODE_MAP option") 854e8848c584 ("mm: clean up free_area_init_node() and its helpers") 97a225e69a1f ("mm/page_alloc: integrate classzone_idx and high_zoneidx")
v5.4.53: Failed to apply! Possible dependencies: 01c0bfe061f3 ("mm: rename gfpflags_to_migratetype to gfp_migratetype for same convention") 16867664936e ("mm,page_alloc,cma: conditionally prefer cma pageblocks for movable allocations") 3334a45eb9e2 ("mm/page_alloc: use ac->high_zoneidx for classzone_idx") 34dc0ea6bc96 ("dma-direct: provide mmap and get_sgtable method overrides") 3f08a302f533 ("mm: remove CONFIG_HAVE_MEMBLOCK_NODE_MAP option") 854e8848c584 ("mm: clean up free_area_init_node() and its helpers") 97a225e69a1f ("mm/page_alloc: integrate classzone_idx and high_zoneidx")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
On Thu, 23 Jul 2020 10:49:02 +0900 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.
Whoops.
Currently, there is just one user for these APIs and it has a fallback method to prevent actual problem.
Shouldn't the patch remove the fallback method?
Second, clearing __GFP_MOVABLE in current_gfp_context() has a side effect to exclude the memory on the ZONE_MOVABLE for allocation target.
More whoops.
Could we please have a description of the end-user-visible effects of this change? Very much needed when proposing a -stable backport, I think.
d7fefcc8de9147c is over a year old. Why did we only just discover this? This makes one wonder how serious those end-user-visible effects are?
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.
2020년 7월 24일 (금) 오전 10:08, Andrew Morton akpm@linux-foundation.org님이 작성:
On Thu, 23 Jul 2020 10:49:02 +0900 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.
Whoops.
Currently, there is just one user for these APIs and it has a fallback method to prevent actual problem.
Shouldn't the patch remove the fallback method?
It's not just the fallback but it also has its own functionality. So, we should not remove it.
Second, clearing __GFP_MOVABLE in current_gfp_context() has a side effect to exclude the memory on the ZONE_MOVABLE for allocation target.
More whoops.
Could we please have a description of the end-user-visible effects of this change? Very much needed when proposing a -stable backport, I think.
In fact, there is no noticeable end-user-visible effect since the fallback would cover the problematic case. It's mentioned in the commit description. Perhap, performance would be improved due to reduced retry and more available memory (we can use ZONE_MOVABLE with this patch) but it would be neglectable.
d7fefcc8de9147c is over a year old. Why did we only just discover this? This makes one wonder how serious those end-user-visible effects are?
As mentioned above, there is no visible problem to the end-user.
Thanks.
On Fri, 24 Jul 2020 11:23:52 +0900 Joonsoo Kim js1304@gmail.com wrote:
Second, clearing __GFP_MOVABLE in current_gfp_context() has a side effect to exclude the memory on the ZONE_MOVABLE for allocation target.
More whoops.
Could we please have a description of the end-user-visible effects of this change? Very much needed when proposing a -stable backport, I think.
In fact, there is no noticeable end-user-visible effect since the fallback would cover the problematic case. It's mentioned in the commit description. Perhap, performance would be improved due to reduced retry and more available memory (we can use ZONE_MOVABLE with this patch) but it would be neglectable.
d7fefcc8de9147c is over a year old. Why did we only just discover this? This makes one wonder how serious those end-user-visible effects are?
As mentioned above, there is no visible problem to the end-user.
OK, thanks. In that case, I don't believe that a stable backport is appropriate?
(Documentation/process/stable-kernel-rules.rst)
2020년 7월 24일 (금) 오전 11:36, Andrew Morton akpm@linux-foundation.org님이 작성:
On Fri, 24 Jul 2020 11:23:52 +0900 Joonsoo Kim js1304@gmail.com wrote:
Second, clearing __GFP_MOVABLE in current_gfp_context() has a side effect to exclude the memory on the ZONE_MOVABLE for allocation target.
More whoops.
Could we please have a description of the end-user-visible effects of this change? Very much needed when proposing a -stable backport, I think.
In fact, there is no noticeable end-user-visible effect since the fallback would cover the problematic case. It's mentioned in the commit description. Perhap, performance would be improved due to reduced retry and more available memory (we can use ZONE_MOVABLE with this patch) but it would be neglectable.
d7fefcc8de9147c is over a year old. Why did we only just discover this? This makes one wonder how serious those end-user-visible effects are?
As mentioned above, there is no visible problem to the end-user.
OK, thanks. In that case, I don't believe that a stable backport is appropriate?
(Documentation/process/stable-kernel-rules.rst)
Thanks for the pointer!
Hmm... I'm not sure the correct way to handle this patch. I thought that memalloc_nocma_{save,restore} is an API that is callable from the module. If it is true, it's better to regard this patch as the stable candidate since out-of-tree modules could use it without the fallback and it would cause a problem. But, yes, there is no visible problem to the end-user, at least, within the mainline so it is possibly not a stable candidate.
Please share your opinion about this situation.
Thanks.
On Fri, 24 Jul 2020 12:04:02 +0900 Joonsoo Kim js1304@gmail.com wrote:
2020년 7월 24일 (금) 오전 11:36, Andrew Morton akpm@linux-foundation.org님이 작성:
On Fri, 24 Jul 2020 11:23:52 +0900 Joonsoo Kim js1304@gmail.com wrote:
Second, clearing __GFP_MOVABLE in current_gfp_context() has a side effect to exclude the memory on the ZONE_MOVABLE for allocation target.
More whoops.
Could we please have a description of the end-user-visible effects of this change? Very much needed when proposing a -stable backport, I think.
In fact, there is no noticeable end-user-visible effect since the fallback would cover the problematic case. It's mentioned in the commit description. Perhap, performance would be improved due to reduced retry and more available memory (we can use ZONE_MOVABLE with this patch) but it would be neglectable.
d7fefcc8de9147c is over a year old. Why did we only just discover this? This makes one wonder how serious those end-user-visible effects are?
As mentioned above, there is no visible problem to the end-user.
OK, thanks. In that case, I don't believe that a stable backport is appropriate?
(Documentation/process/stable-kernel-rules.rst)
Thanks for the pointer!
Hmm... I'm not sure the correct way to handle this patch. I thought that memalloc_nocma_{save,restore} is an API that is callable from the module. If it is true, it's better to regard this patch as the stable candidate since out-of-tree modules could use it without the fallback and it would cause a problem. But, yes, there is no visible problem to the end-user, at least, within the mainline so it is possibly not a stable candidate.
Please share your opinion about this situation.
We tend not to care much about out-of-tree modules. I don't think a theoretical concern for out-of-tree code justifies risking the stability of -stable kernels.
2020년 7월 24일 (금) 오후 12:14, Andrew Morton akpm@linux-foundation.org님이 작성:
On Fri, 24 Jul 2020 12:04:02 +0900 Joonsoo Kim js1304@gmail.com wrote:
2020년 7월 24일 (금) 오전 11:36, Andrew Morton akpm@linux-foundation.org님이 작성:
On Fri, 24 Jul 2020 11:23:52 +0900 Joonsoo Kim js1304@gmail.com wrote:
Second, clearing __GFP_MOVABLE in current_gfp_context() has a side effect to exclude the memory on the ZONE_MOVABLE for allocation target.
More whoops.
Could we please have a description of the end-user-visible effects of this change? Very much needed when proposing a -stable backport, I think.
In fact, there is no noticeable end-user-visible effect since the fallback would cover the problematic case. It's mentioned in the commit description. Perhap, performance would be improved due to reduced retry and more available memory (we can use ZONE_MOVABLE with this patch) but it would be neglectable.
d7fefcc8de9147c is over a year old. Why did we only just discover this? This makes one wonder how serious those end-user-visible effects are?
As mentioned above, there is no visible problem to the end-user.
OK, thanks. In that case, I don't believe that a stable backport is appropriate?
(Documentation/process/stable-kernel-rules.rst)
Thanks for the pointer!
Hmm... I'm not sure the correct way to handle this patch. I thought that memalloc_nocma_{save,restore} is an API that is callable from the module. If it is true, it's better to regard this patch as the stable candidate since out-of-tree modules could use it without the fallback and it would cause a problem. But, yes, there is no visible problem to the end-user, at least, within the mainline so it is possibly not a stable candidate.
Please share your opinion about this situation.
We tend not to care much about out-of-tree modules. I don't think a theoretical concern for out-of-tree code justifies risking the stability of -stable kernels.
Okay. It's appreciated if you remove the stable tag. Or, I will send it again without the stable tag.
Thanks.
linux-stable-mirror@lists.linaro.org
-
Andrew Morton -
Joonsoo Kim -
js1304＠gmail.com
-
Sasha Levin