linux-kernel - Re: [RFC][PATCH 1/3] memcg: wake up filter in oom waitqueue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20100312113838.d6072ae4.kamezawa.hiroyu@jp.fujitsu.com>
Date:	Fri, 12 Mar 2010 11:38:38 +0900
From:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
To:	Daisuke Nishimura <nishimura@....nes.nec.co.jp>
Cc:	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"balbir@...ux.vnet.ibm.com" <balbir@...ux.vnet.ibm.com>,
	kirill@...temov.name
Subject: Re: [RFC][PATCH 1/3] memcg: wake up filter in oom waitqueue

On Fri, 12 Mar 2010 11:30:28 +0900
Daisuke Nishimura <nishimura@....nes.nec.co.jp> wrote:

> On Thu, 11 Mar 2010 16:55:59 +0900, KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com> wrote:
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
> > +	/* check hierarchy */
> > +	if (!css_is_ancestor(&oom_wait_info->mem->css, &wake_mem->css) &&
> > +	    !css_is_ancestor(&wake_mem->css, &oom_wait_info->mem->css))
> > +		return 0;
> > +
> I think these conditions are wrong.
> This can wake up tasks in oom_wait_info->mem when:
> 
>   00/ <- wake_mem: use_hierarchy == false
>     aa/ <- oom_wait_info->mem: use_hierarchy == true;
> 
Hmm. I think this line bails out above case.

> +	if (!oom_wait_info->mem->use_hierarchy || !wake_mem->use_hierarchy)
> +		return 0;

No ?

Thanks,
-Kame

> It should be:
> 
> 	if((oom_wait_info->mem->use_hierarchy &&
> 		css_is_ancestor(&wake_mem->css, &oom_wait_info->mem->css)) ||
> 	   (wake_mem->use_hierarchy &&
> 		css_is_ancestor(&oom_wait_info->mem->css, &wake_mem->css)))
> 		goto wakeup;
> 
> 	return 0;
> 
> But I like the goal of this patch.
> 
> Thanks,
> Daisuke Nishimura.
> 
> > +wakeup:
> > +	return autoremove_wake_function(wait, mode, sync, arg);
> > +}
> > +
> > +static void memcg_wakeup_oom(struct mem_cgroup *mem)
> > +{
> > +	/* for filtering, pass "mem" as argument. */
> > +	__wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, mem);
> > +}
> > +
> >  /*
> >   * 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)
> >  {
> > -	DEFINE_WAIT(wait);
> > +	struct oom_wait_info owait;
> >  	bool locked;
> >  
> > +	owait.mem = mem;
> > +	owait.wait.flags = 0;
> > +	owait.wait.func = memcg_oom_wake_function;
> > +	owait.wait.private = current;
> > +	INIT_LIST_HEAD(&owait.wait.task_list);
> > +
> >  	/* At first, try to OOM lock hierarchy under mem.*/
> >  	mutex_lock(&memcg_oom_mutex);
> >  	locked = mem_cgroup_oom_lock(mem);
> > @@ -1310,31 +1350,18 @@ bool mem_cgroup_handle_oom(struct mem_cg
> >  	 * under OOM is always welcomed, use TASK_KILLABLE here.
> >  	 */
> >  	if (!locked)
> > -		prepare_to_wait(&memcg_oom_waitq, &wait, TASK_KILLABLE);
> > +		prepare_to_wait(&memcg_oom_waitq, &owait.wait, TASK_KILLABLE);
> >  	mutex_unlock(&memcg_oom_mutex);
> >  
> >  	if (locked)
> >  		mem_cgroup_out_of_memory(mem, mask);
> >  	else {
> >  		schedule();
> > -		finish_wait(&memcg_oom_waitq, &wait);
> > +		finish_wait(&memcg_oom_waitq, &owait.wait);
> >  	}
> >  	mutex_lock(&memcg_oom_mutex);
> >  	mem_cgroup_oom_unlock(mem);
> > -	/*
> > -	 * Here, we use global waitq .....more fine grained waitq ?
> > -	 * Assume following hierarchy.
> > -	 * A/
> > -	 *   01
> > -	 *   02
> > -	 * assume OOM happens both in A and 01 at the same time. Tthey are
> > -	 * mutually exclusive by lock. (kill in 01 helps A.)
> > -	 * When we use per memcg waitq, we have to wake up waiters on A and 02
> > -	 * in addtion to waiters on 01. We use global waitq for avoiding mess.
> > -	 * It will not be a big problem.
> > -	 * (And a task may be moved to other groups while it's waiting for OOM.)
> > -	 */
> > -	wake_up_all(&memcg_oom_waitq);
> > +	memcg_wakeup_oom(mem);
> >  	mutex_unlock(&memcg_oom_mutex);
> >  
> >  	if (test_thread_flag(TIF_MEMDIE) || fatal_signal_pending(current))
> > 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/