lkml.org 
[lkml]   [2015]   [Oct]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH -mm v2 3/3] mm/oom_kill: fix the wrong task->mm == mm checks in oom_kill_process()
On Wed, 30 Sep 2015 20:24:11 +0200 Oleg Nesterov <oleg@redhat.com> wrote:

> Both "child->mm == mm" and "p->mm != mm" checks in oom_kill_process()
> are wrong. task->mm can be NULL if the task is the exited group leader.
> This means in particular that "kill sharing same memory" loop can miss
> a process with a zombie leader which uses the same ->mm.
>
> Note: the process_has_mm(child, p->mm) check is still not 100% correct,
> p->mm can be NULL too. This is minor, but probably deserves a fix or a
> comment anyway.
>
> ...
>
> +static bool process_shares_mm(struct task_struct *p, struct mm_struct *mm)
> +{
> + struct task_struct *t;
> +
> + for_each_thread(p, t) {
> + struct mm_struct *t_mm = READ_ONCE(t->mm);
> + if (t_mm)
> + return t_mm == mm;
> + }
> + return false;
> +}

Guys, please don't write write-only code. This function is deeply
unobvious and I don't think a typical reader will have a hope of
understanding why things are this way.

I had a lame attempt:

--- a/mm/oom_kill.c~mm-oom_kill-fix-the-wrong-task-mm-==-mm-checks-in-oom_kill_process-fix
+++ a/mm/oom_kill.c
@@ -483,6 +483,12 @@ void oom_killer_enable(void)
oom_killer_disabled = false;
}

+/*
+ * task->mm can be NULL if the task is the exited group leader. So to
+ * determine whether the task is using a particular mm, we examine all the
+ * task's threads: if one of those is using this mm then this task was also
+ * using it.
+ */
static bool process_shares_mm(struct task_struct *p, struct mm_struct *mm)
{
struct task_struct *t;
_
Which makes me wonder if "process_shared_mm" or even
"process_used_to_share_mm" would be better names...



\
 
 \ /
  Last update: 2015-10-02 00:41    [W:0.432 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site