From: Michal Hocko <mhocko@kernel.org>
To: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: akpm@linux-foundation.org, rientjes@google.com, mgorman@suse.de,
oleg@redhat.com, torvalds@linux-foundation.org, hughd@google.com,
andrea@kernel.org, riel@redhat.com, linux-mm@kvack.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/6] mm,oom: exclude TIF_MEMDIE processes from candidates.
Date: Wed, 17 Feb 2016 18:33:17 +0100 [thread overview]
Message-ID: <20160217173317.GA29370@dhcp22.suse.cz> (raw)
In-Reply-To: <201602180140.IHH21322.OSJFHOMtFFOQVL@I-love.SAKURA.ne.jp>
On Thu 18-02-16 01:40:22, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Wed 17-02-16 19:29:33, Tetsuo Handa wrote:
[...]
> > > victim's memory is shared with OOM-unkillable processes) which will
> > > require manual SysRq-f for making progress.
> >
> > Sharing mm with a task which is hidden from the OOM killer is a clear
> > misconfiguration IMO.
> >
>
> Misconfiguration and/or insane stress is no excuse to leave bugs unfixed.
Such a misconfiguration requires administrator privileges and we do not
do not try really hard to prevent admins from shooting themselves into
foot. Especially if that makes the code much more complicated.
[...]
> > In short I dislike this patch. It makes the code harder to read and the
> > same can be solved more straightforward:
>
> Your patch is not doing the same thing. test_tsk_thread_flag() needs to be
> checked against all threads as with process_shares_mm(). Otherwise,
> find_lock_task_mm() can select a TIF_MEMDIE thread.
>
> Updated patch follows.
[...]
> >From 4d305f92e2527b6d86cd366952d598f9e95f095b Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Thu, 18 Feb 2016 01:16:54 +0900
> Subject: [PATCH v2] mm,oom: exclude TIF_MEMDIE processes from candidates.
>
> It is possible that a TIF_MEMDIE thread gets stuck at
> down_read(&mm->mmap_sem) in exit_mm() called from do_exit() due to
> one of !TIF_MEMDIE threads doing a GFP_KERNEL allocation between
> down_write(&mm->mmap_sem) and up_write(&mm->mmap_sem) (e.g. mmap()).
> In that case, we need to use SysRq-f (manual invocation of the OOM
> killer) for making progress.
>
> However, it is possible that the OOM killer chooses the same OOM victim
> forever which already has TIF_MEMDIE. This is effectively disabling
> SysRq-f. This patch excludes processes which has a TIF_MEMDIE thread
> >from OOM victim candidates.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> mm/oom_kill.c | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 6e6abaf..f6f6b47 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -268,6 +268,21 @@ static enum oom_constraint constrained_alloc(struct oom_control *oc,
> }
> #endif
>
> +/*
> + * To determine whether a task is an OOM victim, we examine all the task's
> + * threads: if one of those has TIF_MEMDIE then the task is an OOM victim.
> + */
> +static bool is_oom_victim(struct task_struct *p)
> +{
> + struct task_struct *t;
> +
> + for_each_thread(p, t) {
> + if (test_tsk_thread_flag(t, TIF_MEMDIE))
> + return true;
> + }
> + return false;
> +}
> +
> enum oom_scan_t oom_scan_process_thread(struct oom_control *oc,
> struct task_struct *task, unsigned long totalpages)
> {
> @@ -278,9 +293,11 @@ enum oom_scan_t oom_scan_process_thread(struct oom_control *oc,
> * This task already has access to memory reserves and is being killed.
> * Don't allow any other task to have access to the reserves.
> */
> - if (test_tsk_thread_flag(task, TIF_MEMDIE)) {
> + if (is_oom_victim(task)) {
This will make the scanning much more time consuming (you will check
all the threads in the same thread group for each scanned thread!). I
do not think this is acceptable and it is not really needed for the
!is_sysrq_oom because we are scanning all the threads anyway.
Regarding the is_sysrq_oom case we might indeed select a thread
which doesn't have TIF_MEMDIE but it has been already (group) killed
but an attempt to catch that case is exactly what has been Nacked
previously when I tried to achieve the same thing and had TIF_MEMDIE ||
fatal_signal_pending check
(http://lkml.kernel.org/r/alpine.DEB.2.10.1601121639450.28831@chino.kir.corp.google.com).
This change will basically achieve the same (just in much more expansive
way) so I am not sure it overcomes the previous feedback.
> if (!is_sysrq_oom(oc))
> return OOM_SCAN_ABORT;
> + else
> + return OOM_SCAN_CONTINUE;
> }
> if (!task->mm || task->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
> return OOM_SCAN_CONTINUE;
> @@ -711,6 +728,8 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>
> if (process_shares_mm(child, p->mm))
> continue;
> + if (is_oom_victim(child))
> + continue;
> /*
> * oom_badness() returns 0 if the thread is unkillable
> */
> --
> 1.8.3.1
--
Michal Hocko
SUSE Labs
next prev parent reply other threads:[~2016-02-17 17:33 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-17 10:28 [PATCH 0/6] preparation for merging the OOM reaper Tetsuo Handa
2016-02-17 10:29 ` [PATCH 1/6] mm,oom: exclude TIF_MEMDIE processes from candidates Tetsuo Handa
2016-02-17 12:41 ` Michal Hocko
2016-02-17 16:40 ` Tetsuo Handa
2016-02-17 17:33 ` Michal Hocko [this message]
2016-02-17 20:55 ` Tetsuo Handa
2016-02-17 10:30 ` [PATCH 2/6] mm,oom: don't abort on exiting processes when selecting a victim Tetsuo Handa
2016-02-17 12:54 ` Michal Hocko
2016-02-17 13:07 ` Tetsuo Handa
2016-02-17 14:00 ` Michal Hocko
2016-02-17 14:39 ` Tetsuo Handa
2016-02-17 15:01 ` Michal Hocko
2016-02-17 15:29 ` Tetsuo Handa
2016-02-17 16:17 ` Michal Hocko
2016-02-18 11:21 ` Tetsuo Handa
2016-02-17 10:32 ` [PATCH 3/6] mm,oom: exclude oom_task_origin processes if they are OOM victims Tetsuo Handa
2016-02-17 13:02 ` Michal Hocko
2016-02-17 10:33 ` [PATCH 4/6] mm,oom: exclude oom_task_origin processes if they are OOM-unkillable Tetsuo Handa
2016-02-17 13:10 ` Michal Hocko
2016-02-17 13:36 ` Tetsuo Handa
2016-02-17 13:44 ` Michal Hocko
2016-02-17 10:34 ` [PATCH 5/6] mm,oom: Re-enable OOM killer using timers Tetsuo Handa
2016-02-17 13:20 ` Michal Hocko
2016-04-09 14:00 ` Tetsuo Handa
2016-04-09 14:04 ` Tetsuo Handa
2016-02-17 10:36 ` [PATCH 6/6] mm,oom: wait for OOM victims when using oom_kill_allocating_task == 1 Tetsuo Handa
2016-02-17 13:32 ` Michal Hocko
2016-02-18 10:45 ` Tetsuo Handa
2016-02-18 12:20 ` Michal Hocko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160217173317.GA29370@dhcp22.suse.cz \
--to=mhocko@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=andrea@kernel.org \
--cc=hughd@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mgorman@suse.de \
--cc=oleg@redhat.com \
--cc=penguin-kernel@I-love.SAKURA.ne.jp \
--cc=riel@redhat.com \
--cc=rientjes@google.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).