lkml.org 
[lkml]   [2010]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [BUGFIX][PATCH] memcg: fix oom kill behavior v2
On Tue, 2 Mar 2010 14:37:38 +0900
Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:

> On Tue, 2 Mar 2010 13:55:24 +0900, KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> > Very sorry, mutex_lock is called after prepare_to_wait.
> > This is a fixed one.
> I'm willing to test your patch, but I have one concern.
>
> > +/*
> > + * try to call OOM killer. returns false if we should exit memory-reclaim loop.
> > + */
> > +bool mem_cgroup_handle_oom(struct mem_cgroup *mem, gfp_t mask)
> > {
> > - mem_cgroup_walk_tree(mem, NULL, record_last_oom_cb);
> > + DEFINE_WAIT(wait);
> > + bool locked;
> > +
> > + /* At first, try to OOM lock hierarchy under mem.*/
> > + mutex_lock(&memcg_oom_mutex);
> > + locked = mem_cgroup_oom_lock(mem);
> > + if (!locked)
> > + prepare_to_wait(&memcg_oom_waitq, &wait, TASK_INTERRUPTIBLE);
> > + mutex_unlock(&memcg_oom_mutex);
> > +
> > + if (locked)
> > + mem_cgroup_out_of_memory(mem, mask);
> > + else {
> > + schedule();
> > + finish_wait(&memcg_oom_waitq, &wait);
> > + }
> > + mutex_lock(&memcg_oom_mutex);
> > + mem_cgroup_oom_unlock(mem);
> > + /* TODO: more fine grained waitq ? */
> > + wake_up_all(&memcg_oom_waitq);
> > + mutex_unlock(&memcg_oom_mutex);
> > +
> > + if (test_thread_flag(TIF_MEMDIE) || fatal_signal_pending(current))
> > + return false;
> > + /* Give chance to dying process */
> > + schedule_timeout(1);
> > + return true;
> > }
> >
> Isn't there such race conditions ?
>
> context A context B
> mutex_lock(&memcg_oom_mutex)
> mem_cgroup_oom_lock()
> ->success
> mutex_unlock(&memcg_oom_mutex)
> mem_cgroup_out_of_memory()
> mutex_lock(&memcg_oom_mutex)
> mem_cgroup_oom_lock()
> ->fail
> prepare_to_wait()
> mutex_unlock(&memcg_oom_mutex)
> mutex_lock(&memcg_oom_mutex)
> mem_cgroup_oom_unlock()
> wake_up_all()
> mutex_unlocklock(&memcg_oom_mutex)
> schedule()
> finish_wait()
>
> In this case, context B will not be waken up, right?
>

No.
prerape_to_wait();
schedule();
finish_wait();
call sequence is for this kind of waiting.


1. Thread B. call prepare_to_wait(), then, wait is queued and task's status
is changed to be TASK_INTERRUPTIBLE
2. Thread A. wake_up_all() check all waiters in queue and change their status
to be TASK_RUNNING.
3. Thread B. calles schedule() but it's status is TASK_RUNNING,
it will be scheduled soon, no sleep.

Then, mutex_lock after prepare_to_wait() is bad ;)

Thanks,
-Kame






\
 
 \ /
  Last update: 2010-03-02 07:03    [W:0.052 / U:2.908 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site