Re: [PATCH v5 4/7] cgroup: cgroup v2 freezer

[Roman Gushchin]

On Tue, Dec 18, 2018 at 06:12:30PM +0100, Oleg Nesterov wrote:
> On 12/18, Roman Gushchin wrote:
> >
> > On Wed, Dec 12, 2018 at 06:49:02PM +0100, Oleg Nesterov wrote:
> > > > > and btw.... what about suspend? try_to_freeze_tasks() will obviously fail
> > > > > if there is a ->frozen thread?
> > > >
> > > > I have to think a bit more here, but something like this will probably work:
> > > >
> > > > diff --git a/kernel/freezer.c b/kernel/freezer.c
> > > > index b162b74611e4..590ac4d10b02 100644
> > > > --- a/kernel/freezer.c
> > > > +++ b/kernel/freezer.c
> > > > @@ -134,7 +134,7 @@ bool freeze_task(struct task_struct *p)
> > > >                 return false;
> > > >
> > > >         spin_lock_irqsave(&freezer_lock, flags);
> > > > -       if (!freezing(p) || frozen(p)) {
> > > > +       if (!freezing(p) || frozen(p) || cgroup_task_frozen()) {
> > > >                 spin_unlock_irqrestore(&freezer_lock, flags);
> > > >                 return false;
> > > >         }
> > > >
> > > > --
> > > >
> > > > If the task is already frozen by the cgroup freezer, we don't have to do
> > > > anything additionally.
> > >
> > > I don't think so. A cgroup_task_frozen() task can be killed after
> > > try_to_freeze_tasks() succeeds, and the exiting task can close files,
> > > do IO, etc. Or it can be thawed by cgroup_freeze_task(false).
> > >
> > > In short, if try_to_freeze_tasks() succeeds, the caller has all rights
> > > to assume that nobody can escape from __refrigerator().
> >
> > But this is what we do with stopped and ptraced tasks, isn't it?
> 
> No,
> 
> > We do use freezable_schedule() and the system freezer just ignores such tasks.
> 
> 	static inline void freezable_schedule(void)
> 	{
> 		freezer_do_not_count();
> 		schedule();
> 		freezer_count();
> 	}
> 
> and note that freezer_count() calls try_to_freeze().
> 
> IOW, the task sleeping in freezable_schedule() doesn't really differ from the
> task sleeping in __refrigerator(). It is not that "the system freezer just
> ignores such tasks", it ignores them because it can safely count them as frozen.

Right, so the task is sleeping peacefully, and we know, that it won't get
anywhere, because we'll catch it in freezer_count(). We allow it to sleep
there, we don't force it to __refrigerator(), and we treat it as frozen.

How's that different to cgroup v2 freezer? If the task is frozen by cgroup v2
freezer, let it sleep there, and catch if it tries to escape. Exactly as it
works for SIGSTOP.

Am I missing something?

> 
> > > And what about TASK_STOPPED/TASK_TRACED tasks? They can not be frozen
> > > or thawed, right? This doesn't look good, and this differs from the
> > > current freezer controller...
> >
> > Good question!
> >
> > It looks like cgroup v1 freezer just ignores them treating as already frozen,
> > which doesn't look nice.
> 
> Not sure I understand you, but see above... cgroup v1 freezer looks fine wrt
> stopped/traced tasks.

So, you think that v2 freezer should follow the same approach, and allow tasks
sleeping on SIGSTOP, for instance, to be treated as frozen?
Hm, maybe. I have to think more here.

Thank you!