On Wed, Oct 23, 2019 at 03:23:50PM +0200, Oleg Nesterov wrote:
> On 10/22, Roman Gushchin wrote:
> >
> > On Tue, Oct 22, 2019 at 03:45:55PM +0200, Oleg Nesterov wrote:
> > >
> > > --- x/kernel/cgroup/freezer.c
> > > +++ x/kernel/cgroup/freezer.c
> > > @@ -238,14 +238,14 @@ void cgroup_freezer_migrate_task(struct
> > > if (task->frozen) {
> > > cgroup_inc_frozen_cnt(dst);
> > > cgroup_dec_frozen_cnt(src);
> > > + } else {
> > > + if (test_bit(CGRP_FREEZE, &src->flags))
> > > + cgroup_update_frozen(src);
> > > + if (test_bit(CGRP_FREEZE, &dst->flags)) {
> > > + cgroup_update_frozen(dst);
> > > + cgroup_freeze_task(task, true);
> > > + }
> > > }
> > > - cgroup_update_frozen(dst);
> > > - cgroup_update_frozen(src);
> >
> >
> > > -
> > > - /*
> > > - * Force the task to the desired state.
> > > - */
> > > - cgroup_freeze_task(task, test_bit(CGRP_FREEZE, &dst->flags));
> >
> > Hm, I'm not sure we can skip this part.
>
> Neither me, but
>
> > Imagine A->B migration, A has just been unfrozen, B is frozen.
> >
> > The task has JOBCTL_TRAP_FREEZE cleared, but task->frozen is still set.
> > Now we move the task to B. No one will set JOBCTL_TRAP_FREEZE again, so
> > the task will remain running.
> >
> > Is it a valid concern?
>
> Not sure I understand... The patch doesn't remove cgroup_freeze_task(),
> it shifts it up, under the test_bit(CGRP_FREEZE, &dst).
Yes, but it does remove cgroup_freeze_task(task, false) if the target
cgroup is not frozen. So a much simpler test case will fail: if a frozen
task is moved from a frozen cgroup to a running cgroup, it will remain
frozen. Sorry for the confusion with my original example, it's obviously bad.
Thanks!
