lkml.org 
[lkml]   [2011]   [Nov]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC] cgroups: freezer -- Allow to attach a task to a frozen cgroup
On Mon, Nov 28, 2011 at 06:03:56PM +0400, Andrew Vagin wrote:
> > > > +static int freezer_can_attach_task(struct cgroup *cgroup, struct task_struct *task)
> > > > +{
> > > > +       struct freezer *old_freezer;
> > > > +       struct freezer *freezer;
> > > > +
> > > > +       int goal_state, orig_state;
> > > > +       int retval = 0;
> > > > +
> > > > +       old_freezer = task_freezer(task);
> > > > +       freezer = cgroup_freezer(cgroup);
> > > > +
> > > > +       spin_lock_irq(&freezer->lock);
> > > > +
> > > > +       if (!spin_trylock_irq(&old_freezer->lock)) {
> > > > +               retval = -EBUSY;
> > >
> > > I think EBUSY is not a good idea in this place. We can do something
> > > like double_rq_lock.
> > >
> >
> > Could you please elaborate? freezers are guarded with spinlocks so I think
> > we should stick with them instead of poking rq (or whatever) directly.
>
> It's misunderstanding. I want to say that we can avoid dead lock if we
> will take a lock with a smaller address at first.
>
> if (&freezer->lock > &old_freezer->lock) {
> spin_lock_irq(&old_freezer->lock)
> spin_lock_irq(&freezer->lock);
> } else {
> spin_lock_irq(&freezer->lock);
> spin_lock_irq(&old_freezer->lock)
> }
>

This is not applicable here as far as I see. It works for rq because of
per-cpu address allocation, but not for freezers which are allocated via
kzalloc. The second try_lock (note I've overdid with irq disabling, plain
spin_trylock would be enough) is not for escaping deadlock but rather for
not waiting much if target freezer is handling state transition for all
task it has.

I think the better approach would to make this code even less lock contended,
ie something like

local_irq_disable
spin_trylock(new_freezer)
spin_trylock(old_freezer)
...
local_irq_enable

so if both freezers are not handling anything we attach the task then.
Or I miss something obvious?

> > >
> > > It's strange. A rollback can't fail. We have three situations:
> > >
> > > frozen -> frozen
> > > thawed -> frozen
> > > frozen -> thawed
> > >
> > > In first and second cases cancel_request can't fail.
> > > In the third we have a problem, which may be solved if we will call
> > > thaw_process(task) from attach_task(), we can do that, because
> > > thaw_process() can't fail. It solves a problem, because
> > > freezer_cancel_attach will be executed for the first and second cases
> > > only.
> > >
> > > If my suggestion is correct, we can replace pr_warning on BUG_ON
> > >
> >
> > Yes, the case which can fail is
> >
> > frozen->(can_attach_task)->thawed
> > (cgroup_task_migrate failure)
> > thawed->(cancel_attach)->frozen
> >
> > and we should never fail here since otherwise we would not have
> > a "frozen" state before. But I think placing BUG_ON is too severe
> > here, maybe WARN_ON_ONCE(1) would fit better?
>
> It's true, if a task is not being executed between thaw_process() and
> freeze_task().

Hmm... But what the problem it might be if a task get executed between
those stages even for some time?
--
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/

\
 
 \ /
  Last update: 2011-11-28 16:03    [from the cache]
©2003-2011 Jasper Spaans