From: Joonsoo Kim <[email protected]>
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: <[email protected]>
Reviewed-by: Vlastimil Babka <[email protected]>
Signed-off-by: Joonsoo Kim <[email protected]>
---
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;
}
--
2.7.4
On Thu, 23 Jul 2020 10:49:02 +0900 [email protected] wrote:
> From: Joonsoo Kim <[email protected]>
>
> 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 <[email protected]>님이 작성:
>
> On Thu, 23 Jul 2020 10:49:02 +0900 [email protected] wrote:
>
> > From: Joonsoo Kim <[email protected]>
> >
> > 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 <[email protected]> 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 <[email protected]>님이 작성:
>
> On Fri, 24 Jul 2020 11:23:52 +0900 Joonsoo Kim <[email protected]> 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 <[email protected]> wrote:
> 2020년 7월 24일 (금) 오전 11:36, Andrew Morton <[email protected]>님이 작성:
> >
> > On Fri, 24 Jul 2020 11:23:52 +0900 Joonsoo Kim <[email protected]> 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 <[email protected]>님이 작성:
>
> On Fri, 24 Jul 2020 12:04:02 +0900 Joonsoo Kim <[email protected]> wrote:
>
> > 2020년 7월 24일 (금) 오전 11:36, Andrew Morton <[email protected]>님이 작성:
> > >
> > > On Fri, 24 Jul 2020 11:23:52 +0900 Joonsoo Kim <[email protected]> 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.