Re: [patch 7/7] oom: prevent possible OOM_DISABLE livelock

From: Nick Piggin
Date: Tue May 05 2009 - 06:23:09 EST


On Mon, May 04, 2009 at 05:27:07PM -0700, David Rientjes wrote:
> It is currently possible to livelock the oom killer if a task is chosen
> for oom kill and another thread sharing the same memory has an oom_adj
> value of OOM_DISABLE. This occurs because oom_kill_task() repeatedly

Hmm, but didn't the last patch make it a per-mm value?

> returns 1 and refuses to kill the chosen task while select_bad_process()
> will repeatedly chooses the same task during the next retry.
>
> This moves the check for OOM_DISABLE to the badness heuristic while
> holding task_lock(). Badness scores of 0 are now explicitly prohibited
> from being oom killed and since the oom_adj value is a characteristic of
> an mm and not a task, it is no longer necessary to check the oom_adj
> value for threads sharing the same memory (except when simply issuing
> SIGKILLs for threads in other thread groups).
>
> Cc: Nick Piggin <npiggin@xxxxxxx>
> Signed-off-by: David Rientjes <rientjes@xxxxxxxxxx>
> ---
> mm/oom_kill.c | 40 ++++++++++------------------------------
> 1 files changed, 10 insertions(+), 30 deletions(-)
>
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -67,6 +67,10 @@ unsigned long badness(struct task_struct *p, unsigned long uptime)
> return 0;
> }
> oom_adj = mm->oom_adj;
> + if (oom_adj == OOM_DISABLE) {
> + task_unlock(p);
> + return 0;
> + }
>
> /*
> * The memory size of the process is the basis for the badness.
> @@ -253,13 +257,8 @@ static struct task_struct *select_bad_process(unsigned long *ppoints,
> *ppoints = ULONG_MAX;
> }
>
> - task_lock(p);
> - if (p->mm && p->mm->oom_adj == OOM_DISABLE)
> - continue;
> - task_unlock(p);
> -
> points = badness(p, uptime.tv_sec);
> - if (points > *ppoints || !chosen) {
> + if (points > *ppoints) {
> chosen = p;
> *ppoints = points;
> }
> @@ -352,32 +351,13 @@ static int oom_kill_task(struct task_struct *p)
> struct mm_struct *mm;
> struct task_struct *g, *q;
>
> + task_lock(p);
> mm = p->mm;
> -
> - /* WARNING: mm may not be dereferenced since we did not obtain its
> - * value from get_task_mm(p). This is OK since all we need to do is
> - * compare mm to q->mm below.
> - *
> - * Furthermore, even if mm contains a non-NULL value, p->mm may
> - * change to NULL at any time since we do not hold task_lock(p).
> - * However, this is of no concern to us.
> - */
> -
> - if (mm == NULL)
> + if (!mm || mm->oom_adj == OOM_DISABLE) {
> + task_unlock(p);
> return 1;
> -
> - /*
> - * Don't kill the process if any threads are set to OOM_DISABLE
> - */
> - do_each_thread(g, q) {
> - task_lock(q);
> - if (q->mm == mm && q->mm && q->mm->oom_adj == OOM_DISABLE) {
> - task_unlock(q);
> - return 1;
> - }
> - task_unlock(q);
> - } while_each_thread(g, q);
> -
> + }
> + task_unlock(p);
> __oom_kill_task(p, 1);
>
> /*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/