* [PATCH] mm,page_alloc: PF_WQ_WORKER should always sleep at should_reclaim_retry().
@ 2018-07-08 10:35 Tetsuo Handa
2018-07-09 7:57 ` Michal Hocko
0 siblings, 1 reply; 5+ messages in thread
From: Tetsuo Handa @ 2018-07-08 10:35 UTC (permalink / raw)
To: akpm
Cc: linux-mm, Michal Hocko, Tetsuo Handa, David Rientjes,
Hillf Danton, Johannes Weiner, Joonsoo Kim, Mel Gorman,
Vladimir Davydov, Vlastimil Babka
From: Michal Hocko <mhocko@suse.com>
should_reclaim_retry() should be a natural reschedule point. PF_WQ_WORKER
is a special case which needs a stronger rescheduling policy. However,
since schedule_timeout_uninterruptible(1) for PF_WQ_WORKER depends on
__zone_watermark_ok() == true, PF_WQ_WORKER is currently counting on
mutex_trylock(&oom_lock) == 0 in __alloc_pages_may_oom() which is a bad
expectation.
Doing schedule_timeout_uninterruptible(1) at should_reclaim_retry()
unconditionally seems more straightforward than depending on a zone being
a good candidate for a further reclaim.
Signed-off-by: Michal Hocko <mhocko@suse.com>
Cc: Hillf Danton <hillf.zj@alibaba-inc.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Joonsoo Kim <js1304@gmail.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Vladimir Davydov <vdavydov@virtuozzo.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
---
mm/page_alloc.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1521100..f56cc09 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3922,6 +3922,7 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
{
struct zone *zone;
struct zoneref *z;
+ bool ret = false;
/*
* Costly allocations might have made a progress but this doesn't mean
@@ -3985,25 +3986,26 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
}
}
- /*
- * Memory allocation/reclaim might be called from a WQ
- * context and the current implementation of the WQ
- * concurrency control doesn't recognize that
- * a particular WQ is congested if the worker thread is
- * looping without ever sleeping. Therefore we have to
- * do a short sleep here rather than calling
- * cond_resched().
- */
- if (current->flags & PF_WQ_WORKER)
- schedule_timeout_uninterruptible(1);
- else
- cond_resched();
-
- return true;
+ ret = true;
+ goto out;
}
}
- return false;
+out:
+ /*
+ * Memory allocation/reclaim might be called from a WQ
+ * context and the current implementation of the WQ
+ * concurrency control doesn't recognize that
+ * a particular WQ is congested if the worker thread is
+ * looping without ever sleeping. Therefore we have to
+ * do a short sleep here rather than calling
+ * cond_resched().
+ */
+ if (current->flags & PF_WQ_WORKER)
+ schedule_timeout_uninterruptible(1);
+ else
+ cond_resched();
+ return ret;
}
static inline bool
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mm,page_alloc: PF_WQ_WORKER should always sleep at should_reclaim_retry().
2018-07-08 10:35 [PATCH] mm,page_alloc: PF_WQ_WORKER should always sleep at should_reclaim_retry() Tetsuo Handa
@ 2018-07-09 7:57 ` Michal Hocko
2018-07-09 13:08 ` Tetsuo Handa
0 siblings, 1 reply; 5+ messages in thread
From: Michal Hocko @ 2018-07-09 7:57 UTC (permalink / raw)
To: Tetsuo Handa
Cc: akpm, linux-mm, David Rientjes, Hillf Danton, Johannes Weiner,
Joonsoo Kim, Mel Gorman, Vladimir Davydov, Vlastimil Babka
On Sun 08-07-18 19:35:58, Tetsuo Handa wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> should_reclaim_retry() should be a natural reschedule point. PF_WQ_WORKER
> is a special case which needs a stronger rescheduling policy. However,
> since schedule_timeout_uninterruptible(1) for PF_WQ_WORKER depends on
> __zone_watermark_ok() == true, PF_WQ_WORKER is currently counting on
> mutex_trylock(&oom_lock) == 0 in __alloc_pages_may_oom() which is a bad
> expectation.
I think your reference to the oom_lock is more confusing than helpful
actually. I would simply use the following from your previous [1]
changelog:
: should_reclaim_retry() should be a natural reschedule point. PF_WQ_WORKER
: is a special case which needs a stronger rescheduling policy. Doing that
: unconditionally seems more straightforward than depending on a zone being
: a good candidate for a further reclaim.
:
: Thus, move the short sleep when we are waiting for the owner of oom_lock
: (which coincidentally also serves as a guaranteed sleep for PF_WQ_WORKER
: threads) to should_reclaim_retry().
> unconditionally seems more straightforward than depending on a zone being
> a good candidate for a further reclaim.
[1] http://lkml.kernel.org/r/1528369223-7571-2-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp
[Tetsuo: changelog]
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> Cc: Hillf Danton <hillf.zj@alibaba-inc.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Joonsoo Kim <js1304@gmail.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Vladimir Davydov <vdavydov@virtuozzo.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
Your s-o-b is still missing.
> ---
> mm/page_alloc.c | 34 ++++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 1521100..f56cc09 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3922,6 +3922,7 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
> {
> struct zone *zone;
> struct zoneref *z;
> + bool ret = false;
>
> /*
> * Costly allocations might have made a progress but this doesn't mean
> @@ -3985,25 +3986,26 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
> }
> }
>
> - /*
> - * Memory allocation/reclaim might be called from a WQ
> - * context and the current implementation of the WQ
> - * concurrency control doesn't recognize that
> - * a particular WQ is congested if the worker thread is
> - * looping without ever sleeping. Therefore we have to
> - * do a short sleep here rather than calling
> - * cond_resched().
> - */
> - if (current->flags & PF_WQ_WORKER)
> - schedule_timeout_uninterruptible(1);
> - else
> - cond_resched();
> -
> - return true;
> + ret = true;
> + goto out;
> }
> }
>
> - return false;
> +out:
> + /*
> + * Memory allocation/reclaim might be called from a WQ
> + * context and the current implementation of the WQ
> + * concurrency control doesn't recognize that
> + * a particular WQ is congested if the worker thread is
> + * looping without ever sleeping. Therefore we have to
> + * do a short sleep here rather than calling
> + * cond_resched().
> + */
> + if (current->flags & PF_WQ_WORKER)
> + schedule_timeout_uninterruptible(1);
> + else
> + cond_resched();
> + return ret;
> }
>
> static inline bool
> --
> 1.8.3.1
>
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm,page_alloc: PF_WQ_WORKER should always sleep at should_reclaim_retry().
2018-07-09 7:57 ` Michal Hocko
@ 2018-07-09 13:08 ` Tetsuo Handa
2018-07-09 13:13 ` Michal Hocko
2018-07-09 13:58 ` Matthew Wilcox
0 siblings, 2 replies; 5+ messages in thread
From: Tetsuo Handa @ 2018-07-09 13:08 UTC (permalink / raw)
To: Michal Hocko
Cc: akpm, linux-mm, David Rientjes, Hillf Danton, Johannes Weiner,
Joonsoo Kim, Mel Gorman, Vladimir Davydov, Vlastimil Babka
On 2018/07/09 16:57, Michal Hocko wrote:
> On Sun 08-07-18 19:35:58, Tetsuo Handa wrote:
>> From: Michal Hocko <mhocko@suse.com>
>>
>> should_reclaim_retry() should be a natural reschedule point. PF_WQ_WORKER
>> is a special case which needs a stronger rescheduling policy. However,
>> since schedule_timeout_uninterruptible(1) for PF_WQ_WORKER depends on
>> __zone_watermark_ok() == true, PF_WQ_WORKER is currently counting on
>> mutex_trylock(&oom_lock) == 0 in __alloc_pages_may_oom() which is a bad
>> expectation.
>
> I think your reference to the oom_lock is more confusing than helpful
> actually. I would simply use the following from your previous [1]
> changelog:
Then, you can post yourself because
> : should_reclaim_retry() should be a natural reschedule point. PF_WQ_WORKER
> : is a special case which needs a stronger rescheduling policy. Doing that
> : unconditionally seems more straightforward than depending on a zone being
> : a good candidate for a further reclaim.
> :
> : Thus, move the short sleep when we are waiting for the owner of oom_lock
> : (which coincidentally also serves as a guaranteed sleep for PF_WQ_WORKER
> : threads) to should_reclaim_retry().
>
>> unconditionally seems more straightforward than depending on a zone being
>> a good candidate for a further reclaim.
>
> [1] http://lkml.kernel.org/r/1528369223-7571-2-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp
>
> [Tetsuo: changelog]
>> Signed-off-by: Michal Hocko <mhocko@suse.com>
>> Cc: Hillf Danton <hillf.zj@alibaba-inc.com>
>> Cc: David Rientjes <rientjes@google.com>
>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>> Cc: Joonsoo Kim <js1304@gmail.com>
>> Cc: Mel Gorman <mgorman@suse.de>
>> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> Cc: Vladimir Davydov <vdavydov@virtuozzo.com>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>
> Your s-o-b is still missing.
all code changes in this patch is from you. That is, my s-o-b is not missing.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm,page_alloc: PF_WQ_WORKER should always sleep at should_reclaim_retry().
2018-07-09 13:08 ` Tetsuo Handa
@ 2018-07-09 13:13 ` Michal Hocko
2018-07-09 13:58 ` Matthew Wilcox
1 sibling, 0 replies; 5+ messages in thread
From: Michal Hocko @ 2018-07-09 13:13 UTC (permalink / raw)
To: Tetsuo Handa
Cc: akpm, linux-mm, David Rientjes, Hillf Danton, Johannes Weiner,
Joonsoo Kim, Mel Gorman, Vladimir Davydov, Vlastimil Babka
On Mon 09-07-18 22:08:04, Tetsuo Handa wrote:
> On 2018/07/09 16:57, Michal Hocko wrote:
[...]
> > [Tetsuo: changelog]
> >> Signed-off-by: Michal Hocko <mhocko@suse.com>
> >> Cc: Hillf Danton <hillf.zj@alibaba-inc.com>
> >> Cc: David Rientjes <rientjes@google.com>
> >> Cc: Johannes Weiner <hannes@cmpxchg.org>
> >> Cc: Joonsoo Kim <js1304@gmail.com>
> >> Cc: Mel Gorman <mgorman@suse.de>
> >> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> >> Cc: Vladimir Davydov <vdavydov@virtuozzo.com>
> >> Cc: Vlastimil Babka <vbabka@suse.cz>
> >
> > Your s-o-b is still missing.
>
> all code changes in this patch is from you. That is, my s-o-b is not missing.
You are supposed to add your s-o-b if you are reposting a patch as
non-author to record to sender path properly.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm,page_alloc: PF_WQ_WORKER should always sleep at should_reclaim_retry().
2018-07-09 13:08 ` Tetsuo Handa
2018-07-09 13:13 ` Michal Hocko
@ 2018-07-09 13:58 ` Matthew Wilcox
1 sibling, 0 replies; 5+ messages in thread
From: Matthew Wilcox @ 2018-07-09 13:58 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Michal Hocko, akpm, linux-mm, David Rientjes, Hillf Danton,
Johannes Weiner, Joonsoo Kim, Mel Gorman, Vladimir Davydov,
Vlastimil Babka
On Mon, Jul 09, 2018 at 10:08:04PM +0900, Tetsuo Handa wrote:
> > [Tetsuo: changelog]
> >> Signed-off-by: Michal Hocko <mhocko@suse.com>
> >> Cc: Hillf Danton <hillf.zj@alibaba-inc.com>
> >> Cc: David Rientjes <rientjes@google.com>
> >> Cc: Johannes Weiner <hannes@cmpxchg.org>
> >> Cc: Joonsoo Kim <js1304@gmail.com>
> >> Cc: Mel Gorman <mgorman@suse.de>
> >> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> >> Cc: Vladimir Davydov <vdavydov@virtuozzo.com>
> >> Cc: Vlastimil Babka <vbabka@suse.cz>
> >
> > Your s-o-b is still missing.
>
> all code changes in this patch is from you. That is, my s-o-b is not missing.
12) When to use Acked-by:, Cc:, and Co-Developed-by:
-------------------------------------------------------
The Signed-off-by: tag indicates that the signer was involved in the
development of the patch, or that he/she was in the patch's delivery path.
That is, if you're submitting it, it needs your S-o-b line. That's
written down in Documentation/process/submitting-patches.rst.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-07-09 13:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-08 10:35 [PATCH] mm,page_alloc: PF_WQ_WORKER should always sleep at should_reclaim_retry() Tetsuo Handa
2018-07-09 7:57 ` Michal Hocko
2018-07-09 13:08 ` Tetsuo Handa
2018-07-09 13:13 ` Michal Hocko
2018-07-09 13:58 ` Matthew Wilcox
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.