freezer_cg: fix improper BUG_ON() causing oops

Quoted 3 lines · Li Zefan

The BUG_ON() should be protected by freezer->lock, otherwise it can be triggered easily when a task has been unfreezed but the corresponding cgroup hasn't been changed to FROZEN state.

yes. thanks,

Acked-by Cedric Le Goater <clg-NmTC/[email protected]>

Quoted 20 lines

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

kernel/cgroup_freezer.c +2 -1

index e950569..7f54d1c 100644
@@ -190,8 +190,9 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 	freezer = task_freezer(task);
 	task_unlock(task);
 
-	BUG_ON(freezer->state == CGROUP_FROZEN);
 	spin_lock_irq(&freezer->lock);
+	BUG_ON(freezer->state == CGROUP_FROZEN);
+
 	/* Locking avoids race with FREEZING -> THAWED transitions. */
 	if (freezer->state == CGROUP_FREEZING)
 		freeze_task(task, true);

Matt Helsley

Acked-by

2008-10-21 21:39 UTC #4 lore raw

Quoted 5 lines · Li Zefan

The BUG_ON() should be protected by freezer->lock, otherwise it can be triggered easily when a task has been unfreezed but the corresponding cgroup hasn't been changed to FROZEN state.
Signed-off-by Li Zefan <[email protected]>

Acked-by Matt Helsley <matthltc-r/[email protected]>

Quoted 19 lines

---
 kernel/cgroup_freezer.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

kernel/cgroup_freezer.c +2 -1

index e950569..7f54d1c 100644
@@ -190,8 +190,9 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 	freezer = task_freezer(task);
 	task_unlock(task);
 
-	BUG_ON(freezer->state == CGROUP_FROZEN);
 	spin_lock_irq(&freezer->lock);
+	BUG_ON(freezer->state == CGROUP_FROZEN);
+
 	/* Locking avoids race with FREEZING -> THAWED transitions. */
 	if (freezer->state == CGROUP_FREEZING)
 		freeze_task(task, true);

Li Zefan

author

2008-10-21 00:50 UTC #5 lore raw

PATCH 2/4 freezer_cg: remove redundant check in freezer_can_attach()

It is sufficient to check if @task is frozen, and no need to check if the original freezer is frozen.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

kernel/cgroup_freezer.c +7 -9

index 7f54d1c..6fadafe 100644
@@ -162,9 +162,13 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 			      struct task_struct *task)
 {
 	struct freezer *freezer;
-	int retval;
 
-	/* Anything frozen can't move or be moved to/from */
+	/*
+	 * Anything frozen can't move or be moved to/from.
+	 *
+	 * Since orig_freezer->state == FROZEN means that @task has been
+	 * frozen, so it's sufficient to check the latter condition.
+	 */
 
 	if (is_task_frozen_enough(task))
 		return -EBUSY;

@@ -173,13 +177,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 	if (freezer->state == CGROUP_FROZEN)
 		return -EBUSY;
 
-	retval = 0;
-	task_lock(task);
-	freezer = task_freezer(task);
-	if (freezer->state == CGROUP_FROZEN)
-		retval = -EBUSY;
-	task_unlock(task);
-	return retval;
+	return 0;
 }
 
 static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)

Signature · 1 line

1.5.4.rc3

A message that is not in the archive (sent off-list or not yet received): [email protected]

Li Zefan

2008-10-21 00:51 UTC #7 lore raw

PATCH 3/4 freezer_cg: use thaw_process() in unfreeze_cgroup()

Don't duplicate the implementation of thaw_process().

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

kernel/cgroup_freezer.c +4 ~~-11~~

index 6fadafe..3ea57e4 100644
@@ -275,25 +275,18 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 	return num_cant_freeze_now ? -EBUSY : 0;
 }
 
-static int unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
+static void unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 {
 	struct cgroup_iter it;
 	struct task_struct *task;
 
 	cgroup_iter_start(cgroup, &it);
 	while ((task = cgroup_iter_next(cgroup, &it))) {
-		int do_wake;
-
-		task_lock(task);
-		do_wake = __thaw_process(task);
-		task_unlock(task);
-		if (do_wake)
-			wake_up_process(task);
+		thaw_process(task);
 	}
 	cgroup_iter_end(cgroup, &it);
-	freezer->state = CGROUP_THAWED;
 
-	return 0;
+	freezer->state = CGROUP_THAWED;
 }
 
 static int freezer_change_state(struct cgroup *cgroup,

@@ -320,7 +313,7 @@ static int freezer_change_state(struct cgroup *cgroup,
 		}
 		/* state == FREEZING and goal_state == THAWED, so unfreeze */
 	case CGROUP_FROZEN:
-		retval = unfreeze_cgroup(cgroup, freezer);
+		unfreeze_cgroup(cgroup, freezer);
 		break;
 	default:
 		break;

Signature · 1 line

1.5.4.rc3

Cedric Le Goater

2008-10-21 07:32 UTC #8 lore raw

Re: [PATCH 2/4] freezer_cg: remove redundant check in freezer_can_attach()

Quoted 2 lines · Li Zefan

It is sufficient to check if @task is frozen, and no need to check if the original freezer is frozen.

hmm, a task being frozen does not mean that its freezer cgroup is frozen. So the extra check seems valid but looking at the comment :

/*
* The call to cgroup_lock() in the freezer.state write method prevents
* a write to that file racing against an attach, and hence the
* can_attach() result will remain valid until the attach completes. */
static int freezer_can_attach(struct cgroup_subsys *ss,

how do we know that the task_freezer(task), which is not protected by the cgroup_lock(), is not going to change its state to CGROUP_FROZEN it looks racy.

Quoted 40 lines

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

kernel/cgroup_freezer.c +7 -9

index 7f54d1c..6fadafe 100644
@@ -162,9 +162,13 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 			      struct task_struct *task)
 {
 	struct freezer *freezer;
-	int retval;
 
-	/* Anything frozen can't move or be moved to/from */
+	/*
+	 * Anything frozen can't move or be moved to/from.
+	 *
+	 * Since orig_freezer->state == FROZEN means that @task has been
+	 * frozen, so it's sufficient to check the latter condition.
+	 */
 
 	if (is_task_frozen_enough(task))
 		return -EBUSY;

@@ -173,13 +177,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 	if (freezer->state == CGROUP_FROZEN)
 		return -EBUSY;
 
-	retval = 0;
-	task_lock(task);
-	freezer = task_freezer(task);
-	if (freezer->state == CGROUP_FROZEN)
-		retval = -EBUSY;
-	task_unlock(task);
-	return retval;
+	return 0;
 }
 
 static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)

Matt Helsley

Acked-by

2008-10-21 21:40 UTC #9 lore raw

Re: [PATCH 2/4] freezer_cg: remove redundant check in freezer_can_attach()

Quoted 2 lines · Li Zefan

It is sufficient to check if @task is frozen, and no need to check if the original freezer is frozen.

Looks great! Thanks!

Acked-by Matt Helsley <matthltc-r/[email protected]>

Quoted 40 lines

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

kernel/cgroup_freezer.c +7 -9

index 7f54d1c..6fadafe 100644
@@ -162,9 +162,13 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 			      struct task_struct *task)
 {
 	struct freezer *freezer;
-	int retval;
 
-	/* Anything frozen can't move or be moved to/from */
+	/*
+	 * Anything frozen can't move or be moved to/from.
+	 *
+	 * Since orig_freezer->state == FROZEN means that @task has been
+	 * frozen, so it's sufficient to check the latter condition.
+	 */
 
 	if (is_task_frozen_enough(task))
 		return -EBUSY;

@@ -173,13 +177,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 	if (freezer->state == CGROUP_FROZEN)
 		return -EBUSY;
 
-	retval = 0;
-	task_lock(task);
-	freezer = task_freezer(task);
-	if (freezer->state == CGROUP_FROZEN)
-		retval = -EBUSY;
-	task_unlock(task);
-	return retval;
+	return 0;
 }
 
 static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)

Li Zefan

author

2008-10-21 00:51 UTC #10 lore raw

PATCH 3/4 freezer_cg: use thaw_process() in unfreeze_cgroup()

Don't duplicate the implementation of thaw_process().

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

kernel/cgroup_freezer.c +4 ~~-11~~

index 6fadafe..3ea57e4 100644
@@ -275,25 +275,18 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 	return num_cant_freeze_now ? -EBUSY : 0;
 }
 
-static int unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
+static void unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 {
 	struct cgroup_iter it;
 	struct task_struct *task;
 
 	cgroup_iter_start(cgroup, &it);
 	while ((task = cgroup_iter_next(cgroup, &it))) {
-		int do_wake;
-
-		task_lock(task);
-		do_wake = __thaw_process(task);
-		task_unlock(task);
-		if (do_wake)
-			wake_up_process(task);
+		thaw_process(task);
 	}
 	cgroup_iter_end(cgroup, &it);
-	freezer->state = CGROUP_THAWED;
 
-	return 0;
+	freezer->state = CGROUP_THAWED;
 }
 
 static int freezer_change_state(struct cgroup *cgroup,

@@ -320,7 +313,7 @@ static int freezer_change_state(struct cgroup *cgroup,
 		}
 		/* state == FREEZING and goal_state == THAWED, so unfreeze */
 	case CGROUP_FROZEN:
-		retval = unfreeze_cgroup(cgroup, freezer);
+		unfreeze_cgroup(cgroup, freezer);
 		break;
 	default:
 		break;

Signature · 1 line

1.5.4.rc3

A message that is not in the archive (sent off-list or not yet received): [email protected]

Li Zefan

2008-10-21 00:52 UTC #12 lore raw

PATCH 4/4 freezer_cg: simplify freezer_change_state()

Just call unfreeze_cgroup() if goal_state == THAWED, and call try_to_freeze_cgroup() if goal_state == FROZEN.

No behavior has been changed.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

kernel/cgroup_freezer.c +7 ~~-12~~

index 3ea57e4..cdef2d8 100644
@@ -296,27 +296,22 @@ static int freezer_change_state(struct cgroup *cgroup,
 	int retval = 0;
 
 	freezer = cgroup_freezer(cgroup);
+
 	spin_lock_irq(&freezer->lock);
+
 	update_freezer_state(cgroup, freezer);
 	if (goal_state == freezer->state)
 		goto out;
-	switch (freezer->state) {
+
+	switch (goal_state) {
 	case CGROUP_THAWED:
-		retval = try_to_freeze_cgroup(cgroup, freezer);
+		unfreeze_cgroup(cgroup, freezer);
 		break;
-	case CGROUP_FREEZING:
-		if (goal_state == CGROUP_FROZEN) {
-			/* Userspace is retrying after
-			 * "/bin/echo FROZEN > freezer.state" returned -EBUSY */
-			retval = try_to_freeze_cgroup(cgroup, freezer);
-			break;
-		}
-		/* state == FREEZING and goal_state == THAWED, so unfreeze */
 	case CGROUP_FROZEN:
-		unfreeze_cgroup(cgroup, freezer);
+		retval = try_to_freeze_cgroup(cgroup, freezer);
 		break;
 	default:
-		break;
+		BUG();
 	}
 out:
 	spin_unlock_irq(&freezer->lock);

Signature · 1 line

1.5.4.rc3

Cedric Le Goater

2008-10-21 07:16 UTC #13 lore raw

Re: [PATCH 3/4] freezer_cg: use thaw_process() in unfreeze_cgroup()

Quoted 1 line · Li Zefan

Don't duplicate the implementation of thaw_process().

looks OK but you should remove __thaw_process() which is unused now.

thanks,

Quoted 47 lines

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

kernel/cgroup_freezer.c +4 ~~-11~~

index 6fadafe..3ea57e4 100644
@@ -275,25 +275,18 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 	return num_cant_freeze_now ? -EBUSY : 0;
 }
 
-static int unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
+static void unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 {
 	struct cgroup_iter it;
 	struct task_struct *task;
 
 	cgroup_iter_start(cgroup, &it);
 	while ((task = cgroup_iter_next(cgroup, &it))) {
-		int do_wake;
-
-		task_lock(task);
-		do_wake = __thaw_process(task);
-		task_unlock(task);
-		if (do_wake)
-			wake_up_process(task);
+		thaw_process(task);
 	}
 	cgroup_iter_end(cgroup, &it);
-	freezer->state = CGROUP_THAWED;
 
-	return 0;
+	freezer->state = CGROUP_THAWED;
 }
 
 static int freezer_change_state(struct cgroup *cgroup,

@@ -320,7 +313,7 @@ static int freezer_change_state(struct cgroup *cgroup,
 		}
 		/* state == FREEZING and goal_state == THAWED, so unfreeze */
 	case CGROUP_FROZEN:
-		retval = unfreeze_cgroup(cgroup, freezer);
+		unfreeze_cgroup(cgroup, freezer);
 		break;
 	default:
 		break;

Matt Helsley

Acked-by

2008-10-21 21:44 UTC #14 lore raw

Re: [PATCH 3/4] freezer_cg: use thaw_process() in unfreeze_cgroup()

Quoted 3 lines · Li Zefan

Don't duplicate the implementation of thaw_process().
Signed-off-by Li Zefan <[email protected]>

Acked-by Matt Helsley <matthltc-r/[email protected]>

Quoted 46 lines

---
 kernel/cgroup_freezer.c |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

kernel/cgroup_freezer.c +4 ~~-11~~

index 6fadafe..3ea57e4 100644
@@ -275,25 +275,18 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 	return num_cant_freeze_now ? -EBUSY : 0;
 }
 
-static int unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
+static void unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 {
 	struct cgroup_iter it;
 	struct task_struct *task;
 
 	cgroup_iter_start(cgroup, &it);
 	while ((task = cgroup_iter_next(cgroup, &it))) {
-		int do_wake;
-
-		task_lock(task);
-		do_wake = __thaw_process(task);
-		task_unlock(task);
-		if (do_wake)
-			wake_up_process(task);
+		thaw_process(task);
 	}
 	cgroup_iter_end(cgroup, &it);
-	freezer->state = CGROUP_THAWED;
 
-	return 0;
+	freezer->state = CGROUP_THAWED;
 }
 
 static int freezer_change_state(struct cgroup *cgroup,

@@ -320,7 +313,7 @@ static int freezer_change_state(struct cgroup *cgroup,
 		}
 		/* state == FREEZING and goal_state == THAWED, so unfreeze */
 	case CGROUP_FROZEN:
-		retval = unfreeze_cgroup(cgroup, freezer);
+		unfreeze_cgroup(cgroup, freezer);
 		break;
 	default:
 		break;

Li Zefan

author

2008-10-21 00:53 UTC #15 lore raw

PATCH 4/4 freezer_cg: simplify freezer_change_state()

Just call unfreeze_cgroup() if goal_state == THAWED, and call try_to_freeze_cgroup() if goal_state == FROZEN.

No behavior has been changed.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

kernel/cgroup_freezer.c +7 ~~-12~~

index 3ea57e4..cdef2d8 100644
@@ -296,27 +296,22 @@ static int freezer_change_state(struct cgroup *cgroup,
 	int retval = 0;
 
 	freezer = cgroup_freezer(cgroup);
+
 	spin_lock_irq(&freezer->lock);
+
 	update_freezer_state(cgroup, freezer);
 	if (goal_state == freezer->state)
 		goto out;
-	switch (freezer->state) {
+
+	switch (goal_state) {
 	case CGROUP_THAWED:
-		retval = try_to_freeze_cgroup(cgroup, freezer);
+		unfreeze_cgroup(cgroup, freezer);
 		break;
-	case CGROUP_FREEZING:
-		if (goal_state == CGROUP_FROZEN) {
-			/* Userspace is retrying after
-			 * "/bin/echo FROZEN > freezer.state" returned -EBUSY */
-			retval = try_to_freeze_cgroup(cgroup, freezer);
-			break;
-		}
-		/* state == FREEZING and goal_state == THAWED, so unfreeze */
 	case CGROUP_FROZEN:
-		unfreeze_cgroup(cgroup, freezer);
+		retval = try_to_freeze_cgroup(cgroup, freezer);
 		break;
 	default:
-		break;
+		BUG();
 	}
 out:
 	spin_unlock_irq(&freezer->lock);

Signature · 1 line

1.5.4.rc3

A message that is not in the archive (sent off-list or not yet received): [email protected]

Cedric Le Goater

Acked-by

2008-10-21 09:53 UTC #17 lore raw

Re: [PATCH 4/4] freezer_cg: simplify freezer_change_state()

Quoted 4 lines · Li Zefan

Just call unfreeze_cgroup() if goal_state == THAWED, and call try_to_freeze_cgroup() if goal_state == FROZEN.
No behavior has been changed.

Looks good.

Acked-by Cedric Le Goater <clg-NmTC/[email protected]>

thanks,

Quoted 45 lines

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

kernel/cgroup_freezer.c +7 ~~-12~~

index 3ea57e4..cdef2d8 100644
@@ -296,27 +296,22 @@ static int freezer_change_state(struct cgroup *cgroup,
 	int retval = 0;
 
 	freezer = cgroup_freezer(cgroup);
+
 	spin_lock_irq(&freezer->lock);
+
 	update_freezer_state(cgroup, freezer);
 	if (goal_state == freezer->state)
 		goto out;
-	switch (freezer->state) {
+
+	switch (goal_state) {
 	case CGROUP_THAWED:
-		retval = try_to_freeze_cgroup(cgroup, freezer);
+		unfreeze_cgroup(cgroup, freezer);
 		break;
-	case CGROUP_FREEZING:
-		if (goal_state == CGROUP_FROZEN) {
-			/* Userspace is retrying after
-			 * "/bin/echo FROZEN > freezer.state" returned -EBUSY */
-			retval = try_to_freeze_cgroup(cgroup, freezer);
-			break;
-		}
-		/* state == FREEZING and goal_state == THAWED, so unfreeze */
 	case CGROUP_FROZEN:
-		unfreeze_cgroup(cgroup, freezer);
+		retval = try_to_freeze_cgroup(cgroup, freezer);
 		break;
 	default:
-		break;
+		BUG();
 	}
 out:
 	spin_unlock_irq(&freezer->lock);

Matt Helsley

Acked-by

2008-10-21 21:45 UTC #18 lore raw

Re: [PATCH 4/4] freezer_cg: simplify freezer_change_state()

Quoted 4 lines · Li Zefan

Just call unfreeze_cgroup() if goal_state == THAWED, and call try_to_freeze_cgroup() if goal_state == FROZEN.
No behavior has been changed.

Another good one. Thanks!

Acked-by Matt Helsley <matthltc-r/[email protected]>

Quoted 44 lines

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

kernel/cgroup_freezer.c +7 ~~-12~~

index 3ea57e4..cdef2d8 100644
@@ -296,27 +296,22 @@ static int freezer_change_state(struct cgroup *cgroup,
 	int retval = 0;
 
 	freezer = cgroup_freezer(cgroup);
+
 	spin_lock_irq(&freezer->lock);
+
 	update_freezer_state(cgroup, freezer);
 	if (goal_state == freezer->state)
 		goto out;
-	switch (freezer->state) {
+
+	switch (goal_state) {
 	case CGROUP_THAWED:
-		retval = try_to_freeze_cgroup(cgroup, freezer);
+		unfreeze_cgroup(cgroup, freezer);
 		break;
-	case CGROUP_FREEZING:
-		if (goal_state == CGROUP_FROZEN) {
-			/* Userspace is retrying after
-			 * "/bin/echo FROZEN > freezer.state" returned -EBUSY */
-			retval = try_to_freeze_cgroup(cgroup, freezer);
-			break;
-		}
-		/* state == FREEZING and goal_state == THAWED, so unfreeze */
 	case CGROUP_FROZEN:
-		unfreeze_cgroup(cgroup, freezer);
+		retval = try_to_freeze_cgroup(cgroup, freezer);
 		break;
 	default:
-		break;
+		BUG();
 	}
 out:
 	spin_unlock_irq(&freezer->lock);

Cedric Le Goater

Acked-by on v1 4/4

2008-10-21 09:53 UTC #19 lore raw

Re: [PATCH 4/4] freezer_cg: simplify freezer_change_state()

Quoted 4 lines · Li Zefan

Just call unfreeze_cgroup() if goal_state == THAWED, and call try_to_freeze_cgroup() if goal_state == FROZEN.
No behavior has been changed.

Looks good.

Acked-by Cedric Le Goater <[email protected]>

thanks,

Quoted 45 lines

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

kernel/cgroup_freezer.c +7 ~~-12~~

index 3ea57e4..cdef2d8 100644
@@ -296,27 +296,22 @@ static int freezer_change_state(struct cgroup *cgroup,
 	int retval = 0;
 
 	freezer = cgroup_freezer(cgroup);
+
 	spin_lock_irq(&freezer->lock);
+
 	update_freezer_state(cgroup, freezer);
 	if (goal_state == freezer->state)
 		goto out;
-	switch (freezer->state) {
+
+	switch (goal_state) {
 	case CGROUP_THAWED:
-		retval = try_to_freeze_cgroup(cgroup, freezer);
+		unfreeze_cgroup(cgroup, freezer);
 		break;
-	case CGROUP_FREEZING:
-		if (goal_state == CGROUP_FROZEN) {
-			/* Userspace is retrying after
-			 * "/bin/echo FROZEN > freezer.state" returned -EBUSY */
-			retval = try_to_freeze_cgroup(cgroup, freezer);
-			break;
-		}
-		/* state == FREEZING and goal_state == THAWED, so unfreeze */
 	case CGROUP_FROZEN:
-		unfreeze_cgroup(cgroup, freezer);
+		retval = try_to_freeze_cgroup(cgroup, freezer);
 		break;
 	default:
-		break;
+		BUG();
 	}
 out:
 	spin_unlock_irq(&freezer->lock);

Matt Helsley

Acked-by on v1 4/4

2008-10-21 21:45 UTC #20 lore raw

Re: [PATCH 4/4] freezer_cg: simplify freezer_change_state()

Quoted 4 lines · Li Zefan

Just call unfreeze_cgroup() if goal_state == THAWED, and call try_to_freeze_cgroup() if goal_state == FROZEN.
No behavior has been changed.

Another good one. Thanks!

Quoted 44 lines

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

kernel/cgroup_freezer.c +7 ~~-12~~

index 3ea57e4..cdef2d8 100644
@@ -296,27 +296,22 @@ static int freezer_change_state(struct cgroup *cgroup,
 	int retval = 0;
 
 	freezer = cgroup_freezer(cgroup);
+
 	spin_lock_irq(&freezer->lock);
+
 	update_freezer_state(cgroup, freezer);
 	if (goal_state == freezer->state)
 		goto out;
-	switch (freezer->state) {
+
+	switch (goal_state) {
 	case CGROUP_THAWED:
-		retval = try_to_freeze_cgroup(cgroup, freezer);
+		unfreeze_cgroup(cgroup, freezer);
 		break;
-	case CGROUP_FREEZING:
-		if (goal_state == CGROUP_FROZEN) {
-			/* Userspace is retrying after
-			 * "/bin/echo FROZEN > freezer.state" returned -EBUSY */
-			retval = try_to_freeze_cgroup(cgroup, freezer);
-			break;
-		}
-		/* state == FREEZING and goal_state == THAWED, so unfreeze */
 	case CGROUP_FROZEN:
-		unfreeze_cgroup(cgroup, freezer);
+		retval = try_to_freeze_cgroup(cgroup, freezer);
 		break;
 	default:
-		break;
+		BUG();
 	}
 out:
 	spin_unlock_irq(&freezer->lock);

Cedric Le Goater

2008-10-21 07:17 UTC #21 lore raw

Re: [PATCH 3/4] freezer_cg: use thaw_process() in unfreeze_cgroup()

Quoted 1 line · Li Zefan

Don't duplicate the implementation of thaw_process().

looks OK but you should remove __thaw_process() which is unused now.

thanks,

Quoted 47 lines

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

kernel/cgroup_freezer.c +4 ~~-11~~

index 6fadafe..3ea57e4 100644
@@ -275,25 +275,18 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 	return num_cant_freeze_now ? -EBUSY : 0;
 }
 
-static int unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
+static void unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 {
 	struct cgroup_iter it;
 	struct task_struct *task;
 
 	cgroup_iter_start(cgroup, &it);
 	while ((task = cgroup_iter_next(cgroup, &it))) {
-		int do_wake;
-
-		task_lock(task);
-		do_wake = __thaw_process(task);
-		task_unlock(task);
-		if (do_wake)
-			wake_up_process(task);
+		thaw_process(task);
 	}
 	cgroup_iter_end(cgroup, &it);
-	freezer->state = CGROUP_THAWED;
 
-	return 0;
+	freezer->state = CGROUP_THAWED;
 }
 
 static int freezer_change_state(struct cgroup *cgroup,

@@ -320,7 +313,7 @@ static int freezer_change_state(struct cgroup *cgroup,
 		}
 		/* state == FREEZING and goal_state == THAWED, so unfreeze */
 	case CGROUP_FROZEN:
-		retval = unfreeze_cgroup(cgroup, freezer);
+		unfreeze_cgroup(cgroup, freezer);
 		break;
 	default:
 		break;

A message that is not in the archive (sent off-list or not yet received): 48FD81B8.5030103-NmTC/[email protected]

Li Zefan

2008-10-21 08:40 UTC #23 lore raw

Re: [PATCH 3/4] freezer_cg: use thaw_process() in unfreeze_cgroup()

Quoted 6 lines · Cedric Le Goater

Quoted 1 line · Li Zefan
Don't duplicate the implementation of thaw_process().
looks OK but you should remove __thaw_process() which is unused now.

Then I'll make it a static function and remove the declaration from .h file.

Andrew Morton

2008-10-21 20:58 UTC #24 lore raw

Re: [PATCH 3/4] freezer_cg: use thaw_process() in unfreeze_cgroup()

In reply to Cedric Le Goater

Quoted 5 lines · On Tue, 21 Oct 2008 09:16:08 +0200 Cedric Le Goater…

Quoted 1 line · Li Zefan
Don't duplicate the implementation of thaw_process().
looks OK but you should remove __thaw_process() which is unused now.

It's called by thaw_process().

But that's the only callsite, I believe, so...

include/linux/freezer.h renamed from include/linux/freezer.h~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix +0 -5

@@ -44,11 +44,6 @@ static inline bool should_send_signal(st
 	return !(p->flags & PF_FREEZER_NOSIG);
 }
 
-/*
- * Wake up a frozen process
- */
-extern int __thaw_process(struct task_struct *p);
-
 /* Takes and releases task alloc lock using task_lock() */
 extern int thaw_process(struct task_struct *p);

kernel/freezer.c renamed from kernel/freezer.c~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix +10 ~~-10~~

diff -puN kernel/freezer.c~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix kernel/freezer.c
@@ -121,16 +121,7 @@ void cancel_freezing(struct task_struct 
 	}
 }
 
-/*
- * Wake up a frozen process
- *
- * task_lock() is needed to prevent the race with refrigerator() which may
- * occur if the freezing of tasks fails.  Namely, without the lock, if the
- * freezing of tasks failed, thaw_tasks() might have run before a task in
- * refrigerator() could call frozen_process(), in which case the task would be
- * frozen and no one would thaw it.
- */
-int __thaw_process(struct task_struct *p)
+static int __thaw_process(struct task_struct *p)
 {
 	if (frozen(p)) {
 		p->flags &= ~PF_FROZEN;

@@ -140,6 +131,15 @@ int __thaw_process(struct task_struct *p
 	return 0;
 }
 
+/*
+ * Wake up a frozen process
+ *
+ * task_lock() is needed to prevent the race with refrigerator() which may
+ * occur if the freezing of tasks fails.  Namely, without the lock, if the
+ * freezing of tasks failed, thaw_tasks() might have run before a task in
+ * refrigerator() could call frozen_process(), in which case the task would be
+ * frozen and no one would thaw it.
+ */
 int thaw_process(struct task_struct *p)
 {
 	task_lock(p);

Li Zefan

author

2008-10-21 08:40 UTC #25 lore raw

Re: [PATCH 3/4] freezer_cg: use thaw_process() in unfreeze_cgroup()

Quoted 6 lines · Cedric Le Goater

Quoted 1 line · Li Zefan
Don't duplicate the implementation of thaw_process().
looks OK but you should remove __thaw_process() which is unused now.

Then I'll make it a static function and remove the declaration from .h file.

Andrew Morton

2008-10-21 20:58 UTC #26 lore raw

Re: [PATCH 3/4] freezer_cg: use thaw_process() in unfreeze_cgroup()

In reply to Cedric Le Goater

Quoted 5 lines · On Tue, 21 Oct 2008 09:16:08 +0200 Cedric Le Goater <[email protected]> wrote:

Quoted 1 line · Li Zefan
Don't duplicate the implementation of thaw_process().
looks OK but you should remove __thaw_process() which is unused now.

It's called by thaw_process().

But that's the only callsite, I believe, so...

include/linux/freezer.h renamed from include/linux/freezer.h~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix +0 -5

@@ -44,11 +44,6 @@ static inline bool should_send_signal(st
 	return !(p->flags & PF_FREEZER_NOSIG);
 }
 
-/*
- * Wake up a frozen process
- */
-extern int __thaw_process(struct task_struct *p);
-
 /* Takes and releases task alloc lock using task_lock() */
 extern int thaw_process(struct task_struct *p);

kernel/freezer.c renamed from kernel/freezer.c~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix +10 ~~-10~~

diff -puN kernel/freezer.c~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix kernel/freezer.c
@@ -121,16 +121,7 @@ void cancel_freezing(struct task_struct 
 	}
 }
 
-/*
- * Wake up a frozen process
- *
- * task_lock() is needed to prevent the race with refrigerator() which may
- * occur if the freezing of tasks fails.  Namely, without the lock, if the
- * freezing of tasks failed, thaw_tasks() might have run before a task in
- * refrigerator() could call frozen_process(), in which case the task would be
- * frozen and no one would thaw it.
- */
-int __thaw_process(struct task_struct *p)
+static int __thaw_process(struct task_struct *p)
 {
 	if (frozen(p)) {
 		p->flags &= ~PF_FROZEN;

@@ -140,6 +131,15 @@ int __thaw_process(struct task_struct *p
 	return 0;
 }
 
+/*
+ * Wake up a frozen process
+ *
+ * task_lock() is needed to prevent the race with refrigerator() which may
+ * occur if the freezing of tasks fails.  Namely, without the lock, if the
+ * freezing of tasks failed, thaw_tasks() might have run before a task in
+ * refrigerator() could call frozen_process(), in which case the task would be
+ * frozen and no one would thaw it.
+ */
 int thaw_process(struct task_struct *p)
 {
 	task_lock(p);

A message that is not in the archive (sent off-list or not yet received): 20081021135813.e77d3015.akpm-de/[email protected]

Cedric Le Goater

2008-10-22 07:37 UTC #28 lore raw

Re: [PATCH 3/4] freezer_cg: use thaw_process() in unfreeze_cgroup()

Quoted 11 lines · Andrew Morton

Quoted 4 lines · On Tue, 21 Oct 2008 09:16:08 +0200 Cedric Le Goater…
Quoted 1 line · Li Zefan
Don't duplicate the implementation of thaw_process().
looks OK but you should remove __thaw_process() which is unused now.
It's called by thaw_process().
But that's the only callsite, I believe, so...

yes it is. it was added by dc52ddc0e6f45b04780b26fc0813509f8e798c42 and was inline before.

thanks,

Quoted 60 lines

include/linux/freezer.h renamed from include/linux/freezer.h~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix +0 -5

@@ -44,11 +44,6 @@ static inline bool should_send_signal(st
 	return !(p->flags & PF_FREEZER_NOSIG);
 }
 
-/*
- * Wake up a frozen process
- */
-extern int __thaw_process(struct task_struct *p);
-
 /* Takes and releases task alloc lock using task_lock() */
 extern int thaw_process(struct task_struct *p);

diff -puN kernel/freezer.c~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix kernel/freezer.c

kernel/freezer.c renamed from kernel/freezer.c~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix +10 ~~-10~~

@@ -121,16 +121,7 @@ void cancel_freezing(struct task_struct 
 	}
 }
 
-/*
- * Wake up a frozen process
- *
- * task_lock() is needed to prevent the race with refrigerator() which may
- * occur if the freezing of tasks fails.  Namely, without the lock, if the
- * freezing of tasks failed, thaw_tasks() might have run before a task in
- * refrigerator() could call frozen_process(), in which case the task would be
- * frozen and no one would thaw it.
- */
-int __thaw_process(struct task_struct *p)
+static int __thaw_process(struct task_struct *p)
 {
 	if (frozen(p)) {
 		p->flags &= ~PF_FROZEN;

@@ -140,6 +131,15 @@ int __thaw_process(struct task_struct *p
 	return 0;
 }
 
+/*
+ * Wake up a frozen process
+ *
+ * task_lock() is needed to prevent the race with refrigerator() which may
+ * occur if the freezing of tasks fails.  Namely, without the lock, if the
+ * freezing of tasks failed, thaw_tasks() might have run before a task in
+ * refrigerator() could call frozen_process(), in which case the task would be
+ * frozen and no one would thaw it.
+ */
 int thaw_process(struct task_struct *p)
 {
 	task_lock(p);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Cedric Le Goater

2008-10-22 07:38 UTC #29 lore raw

Re: [PATCH 3/4] freezer_cg: use thaw_process() in unfreeze_cgroup()

In reply to Andrew Morton

Quoted 11 lines · Andrew Morton

Quoted 4 lines · On Tue, 21 Oct 2008 09:16:08 +0200 Cedric Le Goater <[email protected]> wrote:
Quoted 1 line · Li Zefan
Don't duplicate the implementation of thaw_process().
looks OK but you should remove __thaw_process() which is unused now.
It's called by thaw_process().
But that's the only callsite, I believe, so...

yes it is. it was added by dc52ddc0e6f45b04780b26fc0813509f8e798c42 and was inline before.

thanks,

Quoted 60 lines

include/linux/freezer.h renamed from include/linux/freezer.h~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix +0 -5

@@ -44,11 +44,6 @@ static inline bool should_send_signal(st
 	return !(p->flags & PF_FREEZER_NOSIG);
 }
 
-/*
- * Wake up a frozen process
- */
-extern int __thaw_process(struct task_struct *p);
-
 /* Takes and releases task alloc lock using task_lock() */
 extern int thaw_process(struct task_struct *p);

diff -puN kernel/freezer.c~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix kernel/freezer.c

kernel/freezer.c renamed from kernel/freezer.c~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix +10 ~~-10~~

@@ -121,16 +121,7 @@ void cancel_freezing(struct task_struct 
 	}
 }
 
-/*
- * Wake up a frozen process
- *
- * task_lock() is needed to prevent the race with refrigerator() which may
- * occur if the freezing of tasks fails.  Namely, without the lock, if the
- * freezing of tasks failed, thaw_tasks() might have run before a task in
- * refrigerator() could call frozen_process(), in which case the task would be
- * frozen and no one would thaw it.
- */
-int __thaw_process(struct task_struct *p)
+static int __thaw_process(struct task_struct *p)
 {
 	if (frozen(p)) {
 		p->flags &= ~PF_FROZEN;

@@ -140,6 +131,15 @@ int __thaw_process(struct task_struct *p
 	return 0;
 }
 
+/*
+ * Wake up a frozen process
+ *
+ * task_lock() is needed to prevent the race with refrigerator() which may
+ * occur if the freezing of tasks fails.  Namely, without the lock, if the
+ * freezing of tasks failed, thaw_tasks() might have run before a task in
+ * refrigerator() could call frozen_process(), in which case the task would be
+ * frozen and no one would thaw it.
+ */
 int thaw_process(struct task_struct *p)
 {
 	task_lock(p);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Matt Helsley

Acked-by on v1 3/4

2008-10-21 21:45 UTC #30 lore raw

Re: [PATCH 3/4] freezer_cg: use thaw_process() in unfreeze_cgroup()

Quoted 3 lines · Li Zefan

Don't duplicate the implementation of thaw_process().
Signed-off-by Li Zefan <[email protected]>

Quoted 46 lines

---
 kernel/cgroup_freezer.c |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

kernel/cgroup_freezer.c +4 ~~-11~~

index 6fadafe..3ea57e4 100644
@@ -275,25 +275,18 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 	return num_cant_freeze_now ? -EBUSY : 0;
 }
 
-static int unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
+static void unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 {
 	struct cgroup_iter it;
 	struct task_struct *task;
 
 	cgroup_iter_start(cgroup, &it);
 	while ((task = cgroup_iter_next(cgroup, &it))) {
-		int do_wake;
-
-		task_lock(task);
-		do_wake = __thaw_process(task);
-		task_unlock(task);
-		if (do_wake)
-			wake_up_process(task);
+		thaw_process(task);
 	}
 	cgroup_iter_end(cgroup, &it);
-	freezer->state = CGROUP_THAWED;
 
-	return 0;
+	freezer->state = CGROUP_THAWED;
 }
 
 static int freezer_change_state(struct cgroup *cgroup,

@@ -320,7 +313,7 @@ static int freezer_change_state(struct cgroup *cgroup,
 		}
 		/* state == FREEZING and goal_state == THAWED, so unfreeze */
 	case CGROUP_FROZEN:
-		retval = unfreeze_cgroup(cgroup, freezer);
+		unfreeze_cgroup(cgroup, freezer);
 		break;
 	default:
 		break;

Cedric Le Goater

2008-10-21 07:33 UTC #31 lore raw

Re: [PATCH 2/4] freezer_cg: remove redundant check in freezer_can_attach()

In reply to Cedric Le Goater

Quoted 2 lines · Li Zefan

It is sufficient to check if @task is frozen, and no need to check if the original freezer is frozen.

hmm, a task being frozen does not mean that its freezer cgroup is frozen. So the extra check seems valid but looking at the comment :

how do we know that the task_freezer(task), which is not protected by the cgroup_lock(), is not going to change its state to CGROUP_FROZEN it looks racy.

Quoted 40 lines

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

kernel/cgroup_freezer.c +7 -9

index 7f54d1c..6fadafe 100644
@@ -162,9 +162,13 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 			      struct task_struct *task)
 {
 	struct freezer *freezer;
-	int retval;
 
-	/* Anything frozen can't move or be moved to/from */
+	/*
+	 * Anything frozen can't move or be moved to/from.
+	 *
+	 * Since orig_freezer->state == FROZEN means that @task has been
+	 * frozen, so it's sufficient to check the latter condition.
+	 */
 
 	if (is_task_frozen_enough(task))
 		return -EBUSY;

@@ -173,13 +177,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 	if (freezer->state == CGROUP_FROZEN)
 		return -EBUSY;
 
-	retval = 0;
-	task_lock(task);
-	freezer = task_freezer(task);
-	if (freezer->state == CGROUP_FROZEN)
-		retval = -EBUSY;
-	task_unlock(task);
-	return retval;
+	return 0;
 }
 
 static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)

A message that is not in the archive (sent off-list or not yet received): 48FD85A2.8090605-NmTC/[email protected]

Li Zefan

2008-10-21 09:29 UTC #33 lore raw

Re: [PATCH 2/4] freezer_cg: remove redundant check in freezer_can_attach()

Quoted 6 lines · Cedric Le Goater

Quoted 2 lines · Li Zefan
It is sufficient to check if @task is frozen, and no need to check if the original freezer is frozen.
hmm, a task being frozen does not mean that its freezer cgroup is frozen.

If a task has being frozen, then can_attach() returns -EBUSY at once. If a task isn't frozen, then we have orig_freezer->state == THAWED.

So we need to check the state of the task only.

Quoted 9 lines

So the extra check seems valid but looking at the comment :
/*
* The call to cgroup_lock() in the freezer.state write method prevents
* a write to that file racing against an attach, and hence the
* can_attach() result will remain valid until the attach completes. */
static int freezer_can_attach(struct cgroup_subsys *ss,

Quoted 4 lines

how do we know that the task_freezer(task), which is not protected by the cgroup_lock(), is not going to change its state to CGROUP_FROZEN it looks racy.

Since freezer_can_attach() is called by cgroup core with cgroup_lock held, there is no race with task attaching and state changing.

Quoted 44 lines

Quoted 40 lines

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

kernel/cgroup_freezer.c +7 -9

index 7f54d1c..6fadafe 100644
@@ -162,9 +162,13 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 			      struct task_struct *task)
 {
 	struct freezer *freezer;
-	int retval;
 
-	/* Anything frozen can't move or be moved to/from */
+	/*
+	 * Anything frozen can't move or be moved to/from.
+	 *
+	 * Since orig_freezer->state == FROZEN means that @task has been
+	 * frozen, so it's sufficient to check the latter condition.
+	 */
 
 	if (is_task_frozen_enough(task))
 		return -EBUSY;

@@ -173,13 +177,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 	if (freezer->state == CGROUP_FROZEN)
 		return -EBUSY;
 
-	retval = 0;
-	task_lock(task);
-	freezer = task_freezer(task);
-	if (freezer->state == CGROUP_FROZEN)
-		retval = -EBUSY;
-	task_unlock(task);
-	return retval;
+	return 0;
 }
 
 static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)

Li Zefan

author

2008-10-21 09:29 UTC #34 lore raw

Re: [PATCH 2/4] freezer_cg: remove redundant check in freezer_can_attach()

Quoted 6 lines · Cedric Le Goater

Quoted 2 lines · Li Zefan
It is sufficient to check if @task is frozen, and no need to check if the original freezer is frozen.
hmm, a task being frozen does not mean that its freezer cgroup is frozen.

If a task has being frozen, then can_attach() returns -EBUSY at once. If a task isn't frozen, then we have orig_freezer->state == THAWED.

So we need to check the state of the task only.

Quoted 9 lines

So the extra check seems valid but looking at the comment :
/*
* The call to cgroup_lock() in the freezer.state write method prevents
* a write to that file racing against an attach, and hence the
* can_attach() result will remain valid until the attach completes. */
static int freezer_can_attach(struct cgroup_subsys *ss,

Quoted 4 lines

how do we know that the task_freezer(task), which is not protected by the cgroup_lock(), is not going to change its state to CGROUP_FROZEN it looks racy.

Since freezer_can_attach() is called by cgroup core with cgroup_lock held, there is no race with task attaching and state changing.

Quoted 44 lines

Quoted 40 lines

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

kernel/cgroup_freezer.c +7 -9

index 7f54d1c..6fadafe 100644
@@ -162,9 +162,13 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 			      struct task_struct *task)
 {
 	struct freezer *freezer;
-	int retval;
 
-	/* Anything frozen can't move or be moved to/from */
+	/*
+	 * Anything frozen can't move or be moved to/from.
+	 *
+	 * Since orig_freezer->state == FROZEN means that @task has been
+	 * frozen, so it's sufficient to check the latter condition.
+	 */
 
 	if (is_task_frozen_enough(task))
 		return -EBUSY;

@@ -173,13 +177,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 	if (freezer->state == CGROUP_FROZEN)
 		return -EBUSY;
 
-	retval = 0;
-	task_lock(task);
-	freezer = task_freezer(task);
-	if (freezer->state == CGROUP_FROZEN)
-		retval = -EBUSY;
-	task_unlock(task);
-	return retval;
+	return 0;
 }
 
 static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)

A message that is not in the archive (sent off-list or not yet received): [email protected]

Cedric Le Goater

Acked-by

2008-10-21 09:47 UTC #36 lore raw

Re: [PATCH 2/4] freezer_cg: remove redundant check in freezer_can_attach()

Quoted 21 lines · Li Zefan

Quoted 5 lines · Cedric Le Goater
Quoted 2 lines · Li Zefan
It is sufficient to check if @task is frozen, and no need to check if the original freezer is frozen.
hmm, a task being frozen does not mean that its freezer cgroup is frozen.
If a task has being frozen, then can_attach() returns -EBUSY at once. If a task isn't frozen, then we have orig_freezer->state == THAWED.
So we need to check the state of the task only.
Quoted 9 lines
So the extra check seems valid but looking at the comment :
/*
* The call to cgroup_lock() in the freezer.state write method prevents
* a write to that file racing against an attach, and hence the
* can_attach() result will remain valid until the attach completes. */
static int freezer_can_attach(struct cgroup_subsys *ss,

Quoted 7 lines

Quoted 4 lines
how do we know that the task_freezer(task), which is not protected by the cgroup_lock(), is not going to change its state to CGROUP_FROZEN it looks racy.
Since freezer_can_attach() is called by cgroup core with cgroup_lock held, there is no race with task attaching and state changing.

ok I see. cgroup_mutex is global, I thought it had changed and that we were only locking the cgroup the task was being attached to.

Acked-by Cedric Le Goater <clg-NmTC/[email protected]>

thanks,

Cedric Le Goater

Acked-by

2008-10-21 09:47 UTC #37 lore raw

Re: [PATCH 2/4] freezer_cg: remove redundant check in freezer_can_attach()

Quoted 21 lines · Li Zefan

Quoted 5 lines · Cedric Le Goater
Quoted 2 lines · Li Zefan
It is sufficient to check if @task is frozen, and no need to check if the original freezer is frozen.
hmm, a task being frozen does not mean that its freezer cgroup is frozen.
If a task has being frozen, then can_attach() returns -EBUSY at once. If a task isn't frozen, then we have orig_freezer->state == THAWED.
So we need to check the state of the task only.
Quoted 9 lines
So the extra check seems valid but looking at the comment :
/*
* The call to cgroup_lock() in the freezer.state write method prevents
* a write to that file racing against an attach, and hence the
* can_attach() result will remain valid until the attach completes. */
static int freezer_can_attach(struct cgroup_subsys *ss,

Quoted 7 lines

Quoted 4 lines
how do we know that the task_freezer(task), which is not protected by the cgroup_lock(), is not going to change its state to CGROUP_FROZEN it looks racy.
Since freezer_can_attach() is called by cgroup core with cgroup_lock held, there is no race with task attaching and state changing.

ok I see. cgroup_mutex is global, I thought it had changed and that we were only locking the cgroup the task was being attached to.

Acked-by Cedric Le Goater <[email protected]>

thanks,

Matt Helsley

Acked-by on v1 2/4

2008-10-21 21:40 UTC #38 lore raw

Re: [PATCH 2/4] freezer_cg: remove redundant check in freezer_can_attach()

Quoted 2 lines · Li Zefan

It is sufficient to check if @task is frozen, and no need to check if the original freezer is frozen.

Looks great! Thanks!

Quoted 40 lines

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

kernel/cgroup_freezer.c +7 -9

index 7f54d1c..6fadafe 100644
@@ -162,9 +162,13 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 			      struct task_struct *task)
 {
 	struct freezer *freezer;
-	int retval;
 
-	/* Anything frozen can't move or be moved to/from */
+	/*
+	 * Anything frozen can't move or be moved to/from.
+	 *
+	 * Since orig_freezer->state == FROZEN means that @task has been
+	 * frozen, so it's sufficient to check the latter condition.
+	 */
 
 	if (is_task_frozen_enough(task))
 		return -EBUSY;

@@ -173,13 +177,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 	if (freezer->state == CGROUP_FROZEN)
 		return -EBUSY;
 
-	retval = 0;
-	task_lock(task);
-	freezer = task_freezer(task);
-	if (freezer->state == CGROUP_FROZEN)
-		retval = -EBUSY;
-	task_unlock(task);
-	return retval;
+	return 0;
 }
 
 static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)

Cedric Le Goater

Acked-by on v1 1/4

2008-10-21 07:16 UTC #39 lore raw

Quoted 3 lines · Li Zefan

The BUG_ON() should be protected by freezer->lock, otherwise it can be triggered easily when a task has been unfreezed but the corresponding cgroup hasn't been changed to FROZEN state.

yes. thanks,

Acked-by Cedric Le Goater <[email protected]>

Quoted 20 lines

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup_freezer.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

kernel/cgroup_freezer.c +2 -1

index e950569..7f54d1c 100644
@@ -190,8 +190,9 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 	freezer = task_freezer(task);
 	task_unlock(task);
 
-	BUG_ON(freezer->state == CGROUP_FROZEN);
 	spin_lock_irq(&freezer->lock);
+	BUG_ON(freezer->state == CGROUP_FROZEN);
+
 	/* Locking avoids race with FREEZING -> THAWED transitions. */
 	if (freezer->state == CGROUP_FREEZING)
 		freeze_task(task, true);

Matt Helsley

Acked-by on v1 1/4

2008-10-21 21:39 UTC #40 lore raw

Quoted 5 lines · Li Zefan

The BUG_ON() should be protected by freezer->lock, otherwise it can be triggered easily when a task has been unfreezed but the corresponding cgroup hasn't been changed to FROZEN state.
Signed-off-by Li Zefan <[email protected]>