Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756947AbYGJDWO (ORCPT ); Wed, 9 Jul 2008 23:22:14 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752326AbYGJDV6 (ORCPT ); Wed, 9 Jul 2008 23:21:58 -0400 Received: from cn.fujitsu.com ([222.73.24.84]:64462 "EHLO song.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1751310AbYGJDV5 (ORCPT ); Wed, 9 Jul 2008 23:21:57 -0400 Message-ID: <48757FEC.3030906@cn.fujitsu.com> Date: Thu, 10 Jul 2008 11:20:12 +0800 From: Li Zefan User-Agent: Thunderbird 2.0.0.9 (X11/20071115) MIME-Version: 1.0 To: Matt Helsley CC: KAMEZAWA Hiroyuki , Linux Containers , Linux-Kernel , "Rafael J. Wysocki" , Cedric Le Goater , Pavel Machek , Paul Menage , linux-pm@lists.linux-foundation.org Subject: Re: [RFC][PATCH] Container Freezer: Don't Let Frozen Stuff Change References: <20080707225823.502032693@us.ibm.com> <20080708123150.3034d83f.kamezawa.hiroyu@jp.fujitsu.com> <1215545954.9023.120.camel@localhost.localdomain> <6599ad830807081306l61622eadk215a25d02ee94b00@mail.gmail.com> <6599ad830807081307j811689aldf0f6cb38579c450@mail.gmail.com> <1215640723.7149.35.camel@localhost.localdomain> <20080710094231.58d9e1b9.kamezawa.hiroyu@jp.fujitsu.com> <1215656309.32029.10.camel@localhost.localdomain> In-Reply-To: <1215656309.32029.10.camel@localhost.localdomain> Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5613 Lines: 152 Matt Helsley wrote: > On Thu, 2008-07-10 at 09:42 +0900, KAMEZAWA Hiroyuki wrote: >> On Wed, 09 Jul 2008 14:58:43 -0700 >> Matt Helsley wrote: >> >>> On Tue, 2008-07-08 at 13:07 -0700, Paul Menage wrote: >>>> On Tue, Jul 8, 2008 at 1:06 PM, Paul Menage wrote: >>>>> On Tue, Jul 8, 2008 at 12:39 PM, Matt Helsley wrote: >>>>>> One is to try and disallow users from moving frozen tasks. That doesn't >>>>>> seem like a good approach since it would require a new cgroups interface >>>>>> "can_detach()". >>>>> Detaching from the old cgroup happens at the same time as attaching to >>>>> the new cgroup, so can_attach() would work here. >>> Update: I've made a patch implementing this. However it might be better >>> to just modify attach() to thaw the moving task rather than disallow >>> moving the frozen task. Serge, Cedric, Kame-san, do you have any >>> thoughts on which is more useful and/or intuitive? >>> >> Thank you for explanation in previous mail. >> >> Hmm, just thawing seems atractive but it will confuse people (I think). >> >> I think some kind of process-group is freezed by this freezer and "moving >> freezed task" is wrong(unexpected) operation in general. And there will >> be no demand to do that from users. >> I think just taking "moving freezed task" as error-operation and returning >> -EBUSY is better. > > Kame-san, > > I've been working on changes to the can_attach() code so it was pretty > easy to try this out. > > Don't let frozen tasks or cgroups change. This means frozen tasks can't > leave their current cgroup for another cgroup. It also means that tasks > cannot be added to or removed from a cgroup in the FROZEN state. We > enforce these rules by checking for frozen tasks and cgroups in the > can_attach() function. > > Signed-off-by: Matt Helsley > --- > Builds, boots, passes testing against 2.6.26-rc5-mm2 > > kernel/cgroup_freezer.c | 42 +++++++++++++++++++++++++----------------- > 1 file changed, 25 insertions(+), 17 deletions(-) > > Index: linux-2.6.26-rc5-mm2/kernel/cgroup_freezer.c > =================================================================== > --- linux-2.6.26-rc5-mm2.orig/kernel/cgroup_freezer.c > +++ linux-2.6.26-rc5-mm2/kernel/cgroup_freezer.c > @@ -89,26 +89,43 @@ static void freezer_destroy(struct cgrou > struct cgroup *cgroup) > { > kfree(cgroup_freezer(cgroup)); > } > > +/* Task is frozen or will freeze immediately when next it gets woken */ > +static bool is_task_frozen_enough(struct task_struct *task) > +{ > + return (frozen(task) || (task_is_stopped_or_traced(task) && freezing(task))); > +} > > +/* > + * The call to cgroup_lock() in the freezer.state write method prevents > + * a write to that file racing against an attach, and hence the > + * can_attach() result will remain valid until the attach completes. > + */ > static int freezer_can_attach(struct cgroup_subsys *ss, > struct cgroup *new_cgroup, > struct task_struct *task) > { > struct freezer *freezer; > - int retval = 0; > + int retval; > + > + /* Anything frozen can't move or be moved to/from */ > + > + if (is_task_frozen_enough(task)) > + return -EBUSY; > cgroup_lock() can prevent the state change of old_cgroup and new_cgroup, but will the following racy happen ? 1 2 can_attach(tsk) is_task_frozen_enough(tsk) == false freeze_task(tsk) attach(tsk) i.e., will is_task_frozen_enough(tsk) remain valid through can_attach() and attach()? > - /* > - * The call to cgroup_lock() in the freezer.state write method prevents > - * a write to that file racing against an attach, and hence the > - * can_attach() result will remain valid until the attach completes. > - */ > freezer = cgroup_freezer(new_cgroup); > if (freezer->state == STATE_FROZEN) > + return -EBUSY; > + > + retval = 0; > + task_lock(task); > + freezer = task_freezer(task); > + if (freezer->state == STATE_FROZEN) > retval = -EBUSY; > + task_unlock(task); > return retval; > } > > static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task) > { > @@ -139,16 +156,11 @@ static void check_if_frozen(struct cgrou > unsigned int nfrozen = 0, ntotal = 0; > > cgroup_iter_start(cgroup, &it); > while ((task = cgroup_iter_next(cgroup, &it))) { > ntotal++; > - /* > - * Task is frozen or will freeze immediately when next it gets > - * woken > - */ > - if (frozen(task) || > - (task_is_stopped_or_traced(task) && freezing(task))) > + if (is_task_frozen_enough(task)) > nfrozen++; > } > > /* > * Transition to FROZEN when no new tasks can be added ensures > @@ -195,15 +207,11 @@ static int try_to_freeze_cgroup(struct c > freezer->state = STATE_FREEZING; > cgroup_iter_start(cgroup, &it); > while ((task = cgroup_iter_next(cgroup, &it))) { > if (!freeze_task(task, true)) > continue; > - if (task_is_stopped_or_traced(task) && freezing(task)) > - /* > - * The freeze flag is set so these tasks will > - * immediately go into the fridge upon waking. > - */ > + if (is_task_frozen_enough(task)) > continue; > if (!freezing(task) && !freezer_should_skip(task)) > num_cant_freeze_now++; > } > cgroup_iter_end(cgroup, &it); > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/