From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759812AbZF3JPW (ORCPT ); Tue, 30 Jun 2009 05:15:22 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753033AbZF3JPJ (ORCPT ); Tue, 30 Jun 2009 05:15:09 -0400 Received: from smtp-out.google.com ([216.239.33.17]:29042 "EHLO smtp-out.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751700AbZF3JPI convert rfc822-to-8bit (ORCPT ); Tue, 30 Jun 2009 05:15:08 -0400 DomainKey-Signature: a=rsa-sha1; s=beta; d=google.com; c=nofws; q=dns; h=mime-version:in-reply-to:references:date:message-id:subject:from:to: cc:content-type:content-transfer-encoding:x-system-of-record; b=JjFLs2/5pPMnW8TUtRONBQXeSY5T6MqpCXwxTlB1+Sz9AzcaqHChT9SR+f6Ffw9UJ nuRWs8RqUjySPaBDHAH4w== MIME-Version: 1.0 In-Reply-To: <20090630180344.d7274644.kamezawa.hiroyu@jp.fujitsu.com> References: <20090630180109.f137c10e.kamezawa.hiroyu@jp.fujitsu.com> <20090630180344.d7274644.kamezawa.hiroyu@jp.fujitsu.com> Date: Tue, 30 Jun 2009 02:15:03 -0700 Message-ID: <6599ad830906300215q56bda5ccnc99862211dc65289@mail.gmail.com> Subject: Re: [PATCH 2/2] cgroup: exlclude release rmdir From: Paul Menage To: KAMEZAWA Hiroyuki Cc: "linux-mm@kvack.org" , "linux-kernel@vger.kernel.org" , "balbir@linux.vnet.ibm.com" , "nishimura@mxp.nes.nec.co.jp" Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT X-System-Of-Record: true Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 30, 2009 at 2:03 AM, KAMEZAWA Hiroyuki wrote: > From: KAMEZAWA Hiroyuki > > Paul Menage pointed out that css_get()/put() only for avoiding race with > rmdir() is complicated and these should be treated as it is for. > > This adds >   - cgroup_exclude_rmdir() ....prevent rmdir() for a while. >   - cgroup_release_rmdir() ....rerun rmdir() if necessary. > And hides cgroup_wakeup_rmdir_waiter() into kernel/cgroup.c, again. Wouldn't it be better to merge these into a single patch? Having one patch that exposes complexity only to take it away in the following patch seems unnecessary; the combined patch would be simpler than the constituents. Paul > > Signed-off-by: KAMEZAWA Hiroyuki > > --- >  include/linux/cgroup.h |   21 +++++++++++---------- >  kernel/cgroup.c        |   17 +++++++++++++++-- >  mm/memcontrol.c        |   12 ++++-------- >  3 files changed, 30 insertions(+), 20 deletions(-) > > Index: mmotm-2.6.31-Jun25/include/linux/cgroup.h > =================================================================== > --- mmotm-2.6.31-Jun25.orig/include/linux/cgroup.h > +++ mmotm-2.6.31-Jun25/include/linux/cgroup.h > @@ -366,17 +366,18 @@ int cgroup_task_count(const struct cgrou >  int cgroup_is_descendant(const struct cgroup *cgrp, struct task_struct *task); > >  /* > - * Allow to use CGRP_WAIT_ON_RMDIR flag to check race with rmdir() for subsys. > - * Subsys can call this function if it's necessary to call pre_destroy() again > - * because it adds not-temporary refs to css after or while pre_destroy(). > - * The caller of this function should use css_tryget(), too. > + * When the subsys has to access css and may add permanent refcnt to css, > + * it should take care of racy conditions with rmdir(). Following set of > + * functions, is for stop/restart rmdir if necessary. > + * Because these will call css_get/put, "css" should be alive css. > + * > + *  cgroup_exclude_rmdir(); > + *  ...do some jobs which may access arbitrary empty cgroup > + *  cgroup_release_rmdir(); >  */ > -void __cgroup_wakeup_rmdir_waiters(void); > -static inline void cgroup_wakeup_rmdir_waiter(struct cgroup *cgrp) > -{ > -       if (unlikely(test_and_clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags))) > -               __cgroup_wakeup_rmdir_waiters(); > -} > + > +void cgroup_exclude_rmdir(struct cgroup_subsys_state *css); > +void cgroup_release_rmdir(struct cgroup_subsys_state *css); > >  /* >  * Control Group subsystem type. > Index: mmotm-2.6.31-Jun25/kernel/cgroup.c > =================================================================== > --- mmotm-2.6.31-Jun25.orig/kernel/cgroup.c > +++ mmotm-2.6.31-Jun25/kernel/cgroup.c > @@ -738,11 +738,24 @@ static void cgroup_d_remove_dir(struct d >  */ >  DECLARE_WAIT_QUEUE_HEAD(cgroup_rmdir_waitq); > > -void __cgroup_wakeup_rmdir_waiters(void) > +static void cgroup_wakeup_rmdir_waiter(struct cgroup *cgrp) >  { > -       wake_up_all(&cgroup_rmdir_waitq); > +       if (unlikely(test_and_clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags))) > +               wake_up_all(&cgroup_rmdir_waitq); >  } > > +void cgroup_exclude_rmdir(struct cgroup_subsys_state *css) > +{ > +       css_get(css); > +} > + > +void cgroup_release_rmdir(struct cgroup_subsys_state *css) > +{ > +       cgroup_wakeup_rmdir_waiter(css->cgroup); > +       css_put(css); > +} > + > + >  static int rebind_subsystems(struct cgroupfs_root *root, >                              unsigned long final_bits) >  { > Index: mmotm-2.6.31-Jun25/mm/memcontrol.c > =================================================================== > --- mmotm-2.6.31-Jun25.orig/mm/memcontrol.c > +++ mmotm-2.6.31-Jun25/mm/memcontrol.c > @@ -1461,7 +1461,7 @@ __mem_cgroup_commit_charge_swapin(struct >                return; >        if (!ptr) >                return; > -       css_get(&ptr->css); > +       cgroup_exclude_rmdir(&ptr->css); >        pc = lookup_page_cgroup(page); >        mem_cgroup_lru_del_before_commit_swapcache(page); >        __mem_cgroup_commit_charge(ptr, pc, ctype); > @@ -1496,9 +1496,7 @@ __mem_cgroup_commit_charge_swapin(struct >         * So, rmdir()->pre_destroy() can be called while we do this charge. >         * In that case, we need to call pre_destroy() again. check it here. >         */ > -       cgroup_wakeup_rmdir_waiter(ptr->css.cgroup); > -       css_put(&ptr->css); > - > +       cgroup_release_rmdir(&ptr->css); >  } > >  void mem_cgroup_commit_charge_swapin(struct page *page, struct mem_cgroup *ptr) > @@ -1704,7 +1702,7 @@ void mem_cgroup_end_migration(struct mem > >        if (!mem) >                return; > -       css_get(&mem->css); > +       cgroup_exclude_rmdir(&mem->css); >        /* at migration success, oldpage->mapping is NULL. */ >        if (oldpage->mapping) { >                target = oldpage; > @@ -1749,9 +1747,7 @@ void mem_cgroup_end_migration(struct mem >         * So, rmdir()->pre_destroy() can be called while we do this charge. >         * In that case, we need to call pre_destroy() again. check it here. >         */ > -       cgroup_wakeup_rmdir_waiter(mem->css.cgroup); > -       css_put(&mem->css); > - > +       cgroup_release_rmdir(&mem->css); >  } > >  /* > >