On Tue, Oct 22, 2019 at 03:45:55PM +0200, Oleg Nesterov wrote:
> On 10/21, Honglei Wang wrote:
> >
> > @@ -230,6 +230,15 @@ void cgroup_freezer_migrate_task(struct task_struct *task,
> > + /*
> > + * It's not necessary to do changes if both of the src and dst cgroups
> > + * are not freeze and task is not frozen.
> > + */
> > + if (!test_bit(CGRP_FREEZE, &src->flags) &&
> > + !test_bit(CGRP_FREEZE, &dst->flags) &&
> > + !task->frozen)
> > + return;
> > +
>
> If we want to optimize cgroup_freezer_migrate_task()... I am sure I missed
> something, but can't we do better ?
>
> If a frozen task enters or leaves cgrp, this should never change the state
> of FROZEN bit?
Yes, it's correct.
We probably can, but I'm not sure if we want it: moving a frozen task isn't
a hot path, so code simplicity and correctness are way more important here.
It's not an objection to the proposed change though.
>
> Oleg.
>
> --- 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.
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?
Thank you!
