Skip to contentSkip to conversation

cgroups: freezer -- Allow to attach a task to a frozen cgroup

RFC In review Cyrill Gorcunov 5 participants 11 messages Nov 28, 2011 – Nov 30, 2011

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

RFC 1/1 cgroups: freezer -- Allow to attach a task to a frozen cgroup

In checkpoint/restore we need an ability to attach pids to a frozen cgroup. Thus once pid reaches a frozen cgroup it is not rejected, but the task get frozen immediately.

---

I would really appreciate complains and comments.

 include/linux/cgroup.h  |    2 
 kernel/cgroup.c         |    6 +-
 kernel/cgroup_freezer.c |  119 +++++++++++++++++++++++++++++++++---------------
 mm/memcontrol.c         |    2 
 4 files changed, 90 insertions(+), 39 deletions(-)
include/linux/cgroup.h +1 -1
Index: linux-2.6.git/include/linux/cgroup.h
===================================================================
@@ -470,7 +470,7 @@ struct cgroup_subsys {
 			  struct task_struct *tsk);
 	int (*can_attach_task)(struct cgroup *cgrp, struct task_struct *tsk);
 	void (*cancel_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
-			      struct task_struct *tsk);
+			      struct cgroup *old_cgrp, struct task_struct *tsk);
 	void (*pre_attach)(struct cgroup *cgrp);
 	void (*attach_task)(struct cgroup *cgrp, struct task_struct *tsk);
 	void (*attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
kernel/cgroup.c +3 -3
Index: linux-2.6.git/kernel/cgroup.c
===================================================================
@@ -1884,7 +1884,7 @@ out:
 				 */
 				break;
 			if (ss->cancel_attach)
-				ss->cancel_attach(ss, cgrp, tsk);
+				ss->cancel_attach(ss, cgrp, oldcgrp, tsk);
 		}
 	}
 	return retval;
@@ -2178,11 +2178,11 @@ out_cancel_attach:
 		for_each_subsys(root, ss) {
 			if (ss == failed_ss) {
 				if (cancel_failed_ss && ss->cancel_attach)
-					ss->cancel_attach(ss, cgrp, leader);
+					ss->cancel_attach(ss, cgrp, oldcgrp, leader);
 				break;
 			}
 			if (ss->cancel_attach)
-				ss->cancel_attach(ss, cgrp, leader);
+				ss->cancel_attach(ss, cgrp, oldcgrp, leader);
 		}
 	}
 	/* clean up the array of referenced threads in the group. */
kernel/cgroup_freezer.c +84 -35

137 lines not on this page · raw message

mm/memcontrol.c +2 -0
Index: linux-2.6.git/mm/memcontrol.c
===================================================================
@@ -5337,6 +5337,7 @@ static int mem_cgroup_can_attach(struct
 
 static void mem_cgroup_cancel_attach(struct cgroup_subsys *ss,
 				struct cgroup *cgroup,
+				struct cgroup *old_cgroup,
 				struct task_struct *p)
 {
 	mem_cgroup_clear_mc();
@@ -5477,6 +5478,7 @@ static int mem_cgroup_can_attach(struct
 }
 static void mem_cgroup_cancel_attach(struct cgroup_subsys *ss,
 				struct cgroup *cgroup,
+				struct cgroup *old_cgroup,
 				struct task_struct *p)
 {
 }
#1 lore raw
Quoted 3 lines

       void (*cancel_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
-                             struct task_struct *tsk);
+                             struct cgroup *old_cgrp, struct task_struct *tsk);

I'm not shure, that we need old_cgrp, because when cancel_attach is executed, a task is in old cgroup and old_cgrp = task_cgroup(tsk);

...

Quoted 29 lines
kernel/cgroup_freezer.c +28 -0
+static int freezer_task_transition(struct task_struct *task, int state_to)
+{
+       int retval = 0;
+
+       switch (state_to) {
+       case CGROUP_THAWED:
+               thaw_process(task);
+               break;
+       case CGROUP_FROZEN:
+       case CGROUP_FREEZING:
+               if (freeze_task(task, true)) {
+                       if (!frozen(task)) {
+                               if (!freezing(task) &&
+                                   !freezer_should_skip(task))
+                                       retval = -EBUSY;
+                       }
+               }
+               break;
+       }
+
+       return retval;
+}
+
+static int freezer_can_attach_task(struct cgroup *cgroup, struct task_struct *task)
+{
+       struct freezer *old_freezer;
+       struct freezer *freezer;
+

kernel/cgroup_freezer.c static int freezer_populate(struct cgrou · line 381

Quoted 10 lines
kernel/cgroup_freezer.c +10 -0
+       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.

.....

Quoted 5 lines
kernel/cgroup_freezer.c +5 -0
+
+static void freezer_cancel_attach(struct cgroup_subsys *ss,
+                                 struct cgroup *cgroup,
+                                 struct cgroup *old_cgroup,
+                                 struct task_struct *task)

kernel/cgroup_freezer.c static int freezer_populate(struct cgrou · line 421

Quoted 10 lines
kernel/cgroup_freezer.c +10 -0
+{
+       struct freezer *freezer = cgroup_freezer(old_cgroup);
+       int retval = 0;
+
+       spin_lock_irq(&freezer->lock);
+       retval = freezer_task_transition(task, freezer->state);
+       if (retval)
+               pr_warning("freezer: Can't move task (pid %d) to %s state\n",
+                          task_pid_nr(task),
+                          freezer_state_strs[freezer->state]);

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

Quoted 3 lines

+       spin_unlock_irq(&freezer->lock);

(quoted diff) +2 -0
+}
+
#2 lore raw
Quoted 9 lines · Andrey Vagin
Quoted 3 lines

       void (*cancel_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
-                             struct task_struct *tsk);
+                             struct cgroup *old_cgrp, struct task_struct *tsk);

I'm not shure, that we need old_cgrp, because when cancel_attach is executed, a task is in old cgroup and old_cgrp = task_cgroup(tsk);

...

Yup, thanks for the point. Indeed old_cgrp is redundant and task_cgroup helper will provide all additional information we need.

Quoted 11 lines
Quoted 11 lines
kernel/cgroup_freezer.c +11 -0
+
+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);
Quoted 9 lines

kernel/cgroup_freezer.c static int freezer_populate(struct cgrou · line 381

Quoted 5 lines
kernel/cgroup_freezer.c +5 -0
+
+       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.

Quoted 23 lines
Quoted 6 lines
kernel/cgroup_freezer.c +5 -0
+
+static void freezer_cancel_attach(struct cgroup_subsys *ss,
+                                 struct cgroup *cgroup,
+                                 struct cgroup *old_cgroup,
+                                 struct task_struct *task)

kernel/cgroup_freezer.c static int freezer_populate(struct cgrou · line 421

Quoted 10 lines
kernel/cgroup_freezer.c +10 -0
+{
+       struct freezer *freezer = cgroup_freezer(old_cgroup);
+       int retval = 0;
+
+       spin_lock_irq(&freezer->lock);
+       retval = freezer_task_transition(task, freezer->state);
+       if (retval)
+               pr_warning("freezer: Can't move task (pid %d) to %s state\n",
+                          task_pid_nr(task),
+                          freezer_state_strs[freezer->state]);

It's strange. A rollback can't fail. We have three situations:

frozen -> frozen
thawed -> frozen
frozen -> thawed

Quoted 9 lines

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?

#3 lore raw
Quoted 20 lines
Quoted 10 lines
Quoted 10 lines
kernel/cgroup_freezer.c +10 -0
+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);
Quoted 9 lines

kernel/cgroup_freezer.c static int freezer_populate(struct cgrou · line 381

Quoted 5 lines
kernel/cgroup_freezer.c +5 -0
+
+       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.

Quoted 2 lines

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)
}
Quoted 36 lines
Quoted 24 lines
Quoted 7 lines
kernel/cgroup_freezer.c +5 -0
+
+static void freezer_cancel_attach(struct cgroup_subsys *ss,
+                                 struct cgroup *cgroup,
+                                 struct cgroup *old_cgroup,
+                                 struct task_struct *task)

kernel/cgroup_freezer.c static int freezer_populate(struct cgrou · line 421

Quoted 10 lines
kernel/cgroup_freezer.c +10 -0
+{
+       struct freezer *freezer = cgroup_freezer(old_cgroup);
+       int retval = 0;
+
+       spin_lock_irq(&freezer->lock);
+       retval = freezer_task_transition(task, freezer->state);
+       if (retval)
+               pr_warning("freezer: Can't move task (pid %d) to %s state\n",
+                          task_pid_nr(task),
+                          freezer_state_strs[freezer->state]);

It's strange. A rollback can't fail. We have three situations:

frozen -> frozen
thawed -> frozen
frozen -> thawed

Quoted 9 lines

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

Quoted 7 lines
   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().

#4 lore raw
Quoted 26 lines · Andrew Vagin
Quoted 20 lines
Quoted 10 lines
Quoted 10 lines
kernel/cgroup_freezer.c +10 -0
+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);
Quoted 9 lines

kernel/cgroup_freezer.c static int freezer_populate(struct cgrou · line 381

Quoted 5 lines
kernel/cgroup_freezer.c +5 -0
+
+       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.

Quoted 2 lines

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.

Quoted 8 lines
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?

Quoted 23 lines
Quoted 23 lines
Quoted 7 lines

It's strange. A rollback can't fail. We have three situations:

frozen -> frozen
thawed -> frozen
frozen -> thawed

Quoted 9 lines

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
Quoted 6 lines
Quoted 3 lines

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?

#5 lore raw
Quoted 28 lines
Quoted 28 lines
Quoted 27 lines
Quoted 8 lines

It's strange. A rollback can't fail. We have three situations:

frozen -> frozen
thawed -> frozen
frozen -> thawed

Quoted 9 lines

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?

Quoted 5 lines
Quoted 2 lines

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?

It may become unfreezable...

Quoted 1 line
maintainer
#6 lore raw
In reply to Cyrill Gorcunov
Quoted 8 lines · Cyrill Gorcunov

In checkpoint/restore we need an ability to attach pids to a frozen cgroup. Thus once pid reaches a frozen cgroup it is not rejected, but the task get frozen immediately.

---

I would really appreciate complains and comments.

First of all, both freezer and cgroup have non-trivial pending patchsets (e.g. ->can_attach_task() is scheduled for removal) and I have changes which basically try to achieve about the same thing, so let's slow down a bit. I think the problem is a bit more complex.

Some thoughts I have on cgroup freezer ATM,

* Currently, FROZEN -> FREEZING transition isn't possible. That's why event transition detection by polling is acceptable. ie. once the state is polled to be FROZEN, it stays frozen. Allowing FREEZING -> FROZEN transition would probably require improvements to state transition notification too, at the very least, clarification of rules.

* There are some unclear corner cases and bugs the current cgroup freezer has. e.g. behavior w.r.t. kthreads is outright buggy. It would be great to figure out how to deal with them with or before this change (ie. what happens when you transfer unfreezable kthreads).

* A pending feature request is making the freezing action atomically recursive. We'll probably need to add a param to allow choosing which behavior to take.

* Another improvement that I want to have is allowing cgroup frozen tasks to be killed. I don't think this matters for system freezer but for cgroup freezer both oom killer and systemd want it.

So, while I agree with the direction of this patch, I think this definitely needs a lot more work to go in. I don't think we can do much until the freezer and cgroup changes are settled. The freezer part is now in Rafael's tree, the cgroup part is going under Linus' review. Once they're complete, I'll provide a merged branch that further cgroup works can be based on.

Thanks.

Signature · 1 line
tejun
#7 lore raw
Quoted 17 lines · Tejun Heo
Quoted 8 lines · Cyrill Gorcunov

In checkpoint/restore we need an ability to attach pids to a frozen cgroup. Thus once pid reaches a frozen cgroup it is not rejected, but the task get frozen immediately.

---

I would really appreciate complains and comments.

First of all, both freezer and cgroup have non-trivial pending patchsets (e.g. ->can_attach_task() is scheduled for removal) and I have changes which basically try to achieve about the same thing, so let's slow down a bit. I think the problem is a bit more complex.

Some thoughts I have on cgroup freezer ATM,

...

Quoted 5 lines

* Another improvement that I want to have is allowing cgroup frozen tasks to be killed. I don't think this matters for system freezer but for cgroup freezer both oom killer and systemd want it.

* Run ptrace over frozen tasks ;)

Quoted 7 lines

So, while I agree with the direction of this patch, I think this definitely needs a lot more work to go in. I don't think we can do much until the freezer and cgroup changes are settled. The freezer part is now in Rafael's tree, the cgroup part is going under Linus' review. Once they're complete, I'll provide a merged branch that further cgroup works can be based on.

Thanks a lot for comments, Tejun! For own needs I'll make a stub in our user-space tool, once things are settled down we can move on.

#8 lore raw
In reply to Tejun Heo
Quoted 4 lines · Tejun Heo
Quoted 3 lines · Cyrill Gorcunov

In checkpoint/restore we need an ability to attach pids to a frozen cgroup. Thus once pid reaches a frozen cgroup it is not rejected, but the task get frozen immediately.

Frankly, I don't see the need for this. I think you should explain it with more detail at the very least.

Why can't they be frozen before being checkpointed? Assuming you've used SEIZE and spawned a new parasite thread, just move that thread to a cgroup for the parasitic threads then freeze the old cgroup.

Quoted 6 lines
Quoted 5 lines

---

I would really appreciate complains and comments.

Quoted 8 lines

First of all, both freezer and cgroup have non-trivial pending patchsets (e.g. ->can_attach_task() is scheduled for removal) and I have changes which basically try to achieve about the same thing, so let's slow down a bit. I think the problem is a bit more complex.

Some thoughts I have on cgroup freezer ATM,

* Currently, FROZEN -> FREEZING transition isn't possible. That's why

No, IMO that would consitute breaking the interface. Scripts, tools, applications, and tests have been written to rely on the fact that the cgroup cannot go from FROZEN to FREEZING -- only from FROZEN to THAWED.

Quoted 3 lines

event transition detection by polling is acceptable. ie. once the state is polled to be FROZEN, it stays frozen. Allowing FREEZING -> FROZEN transition would probably require improvements to state

(I'm assuming this last sentence has a thinko in it...)

Quoted 1 line

transition notification too, at the very least, clarification of

Notification is a fine idea. However, not everything that's already written expects them so correct usage of the cgroup freezer should not require them -- IOW allowing the FROZEN -> FREEZING transition isn't made OK just by adding notifications.

Quoted 7 lines

rules.

* There are some unclear corner cases and bugs the current cgroup freezer has. e.g. behavior w.r.t. kthreads is outright buggy. It would be great to figure out how to deal with them with or before this change (ie. what happens when you transfer unfreezable kthreads).

Huh? Shouldn't we just disable moving kthreads between cgroups? Allowing userspace to freeze kthreads via cgroups seems like a *very* bad idea (perhaps it's a thread critical for IO, or some driver, etc.).

Cheers
-Matt Helsley

maintainer
#9 lore raw

Hello, Matt.

Quoted 6 lines · Matt Helsley
Quoted 1 line

transition notification too, at the very least, clarification of

Notification is a fine idea. However, not everything that's already written expects them so correct usage of the cgroup freezer should not require them -- IOW allowing the FROZEN -> FREEZING transition isn't made OK just by adding notifications.

The transition model doesn't have to be modified. ie. we can try to freeze the task/process on migration request and actually moves it only when it actually reaches the target state. Not entirely sure how well that can be implemented with the current cgroup callbacks tho. We can also make the new behavior selectable per-cgroup if all else fail.

Quoted 9 lines
Quoted 5 lines

* There are some unclear corner cases and bugs the current cgroup freezer has. e.g. behavior w.r.t. kthreads is outright buggy. It would be great to figure out how to deal with them with or before this change (ie. what happens when you transfer unfreezable kthreads).

Huh? Shouldn't we just disable moving kthreads between cgroups? Allowing userspace to freeze kthreads via cgroups seems like a *very* bad idea (perhaps it's a thread critical for IO, or some driver, etc.).

That probably is the easiest solution. Different cgroups can be grouped together so we need to be a bit careful but if none of the cgroups makes sense with kthread, we may just ban kthreads from any cgroup.

Thanks.

Signature · 1 line
tejun
#10 lore raw
In reply to Matt Helsley
Quoted 6 lines · Matt Helsley
Quoted 4 lines · Tejun Heo
Quoted 3 lines · Cyrill Gorcunov

In checkpoint/restore we need an ability to attach pids to a frozen cgroup. Thus once pid reaches a frozen cgroup it is not rejected, but the task get frozen immediately.

Quoted 7 lines

Frankly, I don't see the need for this. I think you should explain it with more detail at the very least.

Why can't they be frozen before being checkpointed? Assuming you've used SEIZE and spawned a new parasite thread, just move that thread to a cgroup for the parasitic threads then freeze the old cgroup.

Hi Matt,

At moment there is no way to use ptrace with frozen task (Tejun will take a look into such ability once he get time for, as far as I know).

Ideally we would like to move checkpointing tasks to cgroup, freeze it there and _then_ run ptrace and inject parasite code into it. Finally the parasite code would dump tasks contents, detach itself and unfreeze tasks.

At restore time we would like to move the tasks being restored into frozen cgroup before final jump to the original pt_regs the task had at checkpoint time (it actually takes a few stages, ie once memory and other resources are restored but task is not yet switched to the former pt_regs, we move it into frozen cgroup, and then the task get immediately frozen, finally once all processes being restored reaches this point we unfreeze the cgroup and they all switch to original registers contents and continue execution).

Cyrill