freezer_cg: disable writing freezer.state of root cgroup

PATCH In review Li Zefan 6 participants 11 messages Nov 6, 2008 – Nov 7, 2008 5 messages not in the archive Documentation

lore

Share & tools

lore

Li Zefan

author

2008-11-06 01:21 UTC #0 lore raw

PATCH 1/1 freezer_cg: disable writing freezer.state of root cgroup

With this change, control file 'freezer.state' doesn't exist in root cgroup, making root cgroup unfreezable.

I think it's reasonable to disallow freeze tasks in the root cgroup. And then we can avoid fork overhead when freezer subsystem is compiled but not used.

Also make writing invalid value to freezer.state returns EINVAL rather than EIO. This is more consistent with other cgroup subsystem.

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

---

patch is based on freezer_cg-remove-task_lock-from-freezer_fork.patch

---
 Documentation/cgroups/freezer-subsystem.txt |    3 +++
 kernel/cgroup_freezer.c                     |   11 ++++++++++-
 2 files changed, 13 insertions(+), 1 deletions(-)

Documentation/cgroups/freezer-subsystem.txt +3 -0

index c50ab58..91572b9 100644
@@ -54,6 +54,9 @@ freezer.state. Writing "FROZEN" to the state file will freeze all tasks in the
 cgroup. Subsequently writing "THAWED" will unfreeze the tasks in the cgroup.
 Reading will return the current state.
 
+Note freezer.state doesn't exist in root cgroup, which means root cgroup
+is unfreezable.
+
 * Examples of usage :
 
    # mkdir /containers/freezer

kernel/cgroup_freezer.c +10 -1

index 6605907..cf3fce9 100644
@@ -192,6 +192,13 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 	 */
 	freezer = task_freezer(task);
 
+	/*
+	 * The root cgroup is unfreezable, so we can skip the
+	 * following check.
+	 */
+	if (!freezer->css.cgroup->parent)
+		return;
+
 	spin_lock_irq(&freezer->lock);
 	BUG_ON(freezer->state == CGROUP_FROZEN);

@@ -335,7 +342,7 @@ static int freezer_write(struct cgroup *cgroup,
 	else if (strcmp(buffer, freezer_state_strs[CGROUP_FROZEN]) == 0)
 		goal_state = CGROUP_FROZEN;
 	else
-		return -EIO;
+		return -EINVAL;
 
 	if (!cgroup_lock_live_group(cgroup))
 		return -ENODEV;

@@ -354,6 +361,8 @@ static struct cftype files[] = {
 
 static int freezer_populate(struct cgroup_subsys *ss, struct cgroup *cgroup)
 {
+	if (!cgroup->parent)
+		return 0;
 	return cgroup_add_files(cgroup, ss, files, ARRAY_SIZE(files));
 }

Signature · 1 line

1.5.4.rc3

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

Paul Menage

Acked-by

2008-11-06 01:24 UTC #2 lore raw

Quoted 11 lines · Li Zefan

With this change, control file 'freezer.state' doesn't exist in root cgroup, making root cgroup unfreezable.
I think it's reasonable to disallow freeze tasks in the root cgroup. And then we can avoid fork overhead when freezer subsystem is compiled but not used.
Also make writing invalid value to freezer.state returns EINVAL rather than EIO. This is more consistent with other cgroup subsystem.
Signed-off-by Li Zefan <[email protected]>

Acked-by Paul Menage <[email protected]>

I might use the term "non-freezable" rather than "unfreezable".

Thanks,

Paul

Quoted 63 lines

---

patch is based on freezer_cg-remove-task_lock-from-freezer_fork.patch

---
 Documentation/cgroups/freezer-subsystem.txt |    3 +++
 kernel/cgroup_freezer.c                     |   11 ++++++++++-
 2 files changed, 13 insertions(+), 1 deletions(-)

Documentation/cgroups/freezer-subsystem.txt +3 -0

index c50ab58..91572b9 100644
@@ -54,6 +54,9 @@ freezer.state. Writing "FROZEN" to the state file will freeze all tasks in the
 cgroup. Subsequently writing "THAWED" will unfreeze the tasks in the cgroup.
 Reading will return the current state.
 
+Note freezer.state doesn't exist in root cgroup, which means root cgroup
+is unfreezable.
+
 * Examples of usage :
 
   # mkdir /containers/freezer

kernel/cgroup_freezer.c +10 -2

index 6605907..cf3fce9 100644
@@ -192,6 +192,13 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
        */
       freezer = task_freezer(task);
 
+       /*
+        * The root cgroup is unfreezable, so we can skip the
+        * following check.
+        */
+       if (!freezer->css.cgroup->parent)
+               return;
+
       spin_lock_irq(&freezer->lock);
       BUG_ON(freezer->state == CGROUP_FROZEN);

@@ -335,7 +342,7 @@ static int freezer_write(struct cgroup *cgroup,
       else if (strcmp(buffer, freezer_state_strs[CGROUP_FROZEN]) == 0)
               goal_state = CGROUP_FROZEN;
       else
-               return -EIO;
+               return -EINVAL;
 
       if (!cgroup_lock_live_group(cgroup))
               return -ENODEV;

@@ -354,6 +361,8 @@ static struct cftype files[] = {
 
 static int freezer_populate(struct cgroup_subsys *ss, struct cgroup *cgroup)
 {
+       if (!cgroup->parent)
+               return 0;
       return cgroup_add_files(cgroup, ss, files, ARRAY_SIZE(files));
 }
 
--

1.5.4.rc3

Paul Menage

Acked-by on v1 1/1

2008-11-06 01:25 UTC #3 lore raw

In reply to Li Zefan

Quoted 11 lines · Li Zefan

With this change, control file 'freezer.state' doesn't exist in root cgroup, making root cgroup unfreezable.
I think it's reasonable to disallow freeze tasks in the root cgroup. And then we can avoid fork overhead when freezer subsystem is compiled but not used.
Also make writing invalid value to freezer.state returns EINVAL rather than EIO. This is more consistent with other cgroup subsystem.
Signed-off-by Li Zefan <[email protected]>

Acked-by Paul Menage <[email protected]>

I might use the term "non-freezable" rather than "unfreezable".

Thanks,

Paul

Quoted 63 lines

---

patch is based on freezer_cg-remove-task_lock-from-freezer_fork.patch

---
 Documentation/cgroups/freezer-subsystem.txt |    3 +++
 kernel/cgroup_freezer.c                     |   11 ++++++++++-
 2 files changed, 13 insertions(+), 1 deletions(-)

Documentation/cgroups/freezer-subsystem.txt +3 -0

index c50ab58..91572b9 100644
@@ -54,6 +54,9 @@ freezer.state. Writing "FROZEN" to the state file will freeze all tasks in the
 cgroup. Subsequently writing "THAWED" will unfreeze the tasks in the cgroup.
 Reading will return the current state.
 
+Note freezer.state doesn't exist in root cgroup, which means root cgroup
+is unfreezable.
+
 * Examples of usage :
 
   # mkdir /containers/freezer

kernel/cgroup_freezer.c +10 -2

index 6605907..cf3fce9 100644
@@ -192,6 +192,13 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
        */
       freezer = task_freezer(task);
 
+       /*
+        * The root cgroup is unfreezable, so we can skip the
+        * following check.
+        */
+       if (!freezer->css.cgroup->parent)
+               return;
+
       spin_lock_irq(&freezer->lock);
       BUG_ON(freezer->state == CGROUP_FROZEN);

@@ -335,7 +342,7 @@ static int freezer_write(struct cgroup *cgroup,
       else if (strcmp(buffer, freezer_state_strs[CGROUP_FROZEN]) == 0)
               goal_state = CGROUP_FROZEN;
       else
-               return -EIO;
+               return -EINVAL;
 
       if (!cgroup_lock_live_group(cgroup))
               return -ENODEV;

@@ -354,6 +361,8 @@ static struct cftype files[] = {
 
 static int freezer_populate(struct cgroup_subsys *ss, struct cgroup *cgroup)
 {
+       if (!cgroup->parent)
+               return 0;
       return cgroup_add_files(cgroup, ss, files, ARRAY_SIZE(files));
 }
 
--

1.5.4.rc3

A message that is not in the archive (sent off-list or not yet received): 6599ad830811051724p1f97026fl96be5efe38b33152-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Li Zefan

Acked-by Paul Menage

2008-11-06 01:53 UTC #5 lore raw

Quoted 4 lines

Acked-by Paul Menage <[email protected]>
I might use the term "non-freezable" rather than "unfreezable".

My bad English :(

patch updated. (some annoying leading tabs are also removed in the doc)

========================

From: Li Zefan <[email protected]>
Date: Tue, 4 Nov 2008 15:26:29 +0800
Subject: [PATCH] freezer_cg: disable writing freezer.state of root cgroup

With this change, control file 'freezer.state' doesn't exist in root cgroup, making root cgroup unfreezable.

I think it's reasonable to disallow freeze tasks in the root cgroup. And then we can avoid fork overhead when freezer subsystem is compiled but not used.

Also make writing invalid value to freezer.state returns EINVAL rather than EIO. This is more consistent with other cgroup subsystem.

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

Acked-by Paul Menage <[email protected]>

---

patch is based on freezer_cg-remove-task_lock-from-freezer_fork.patch

---
 Documentation/cgroups/freezer-subsystem.txt |   19 +++++++++++--------
 kernel/cgroup_freezer.c                     |   11 ++++++++++-
 2 files changed, 21 insertions(+), 9 deletions(-)

Documentation/cgroups/freezer-subsystem.txt +11 -8

index c50ab58..8f7e5d1 100644
@@ -1,4 +1,4 @@
-	The cgroup freezer is useful to batch job management system which start
+The cgroup freezer is useful to batch job management system which start
 and stop sets of tasks in order to schedule the resources of a machine
 according to the desires of a system administrator. This sort of program
 is often used on HPC clusters to schedule access to the cluster as a

@@ -6,7 +6,7 @@ whole. The cgroup freezer uses cgroups to describe the set of tasks to
 be started/stopped by the batch job management system. It also provides
 a means to start and stop the tasks composing the job.
 
-	The cgroup freezer will also be useful for checkpointing running groups
+The cgroup freezer will also be useful for checkpointing running groups
 of tasks. The freezer allows the checkpoint code to obtain a consistent
 image of the tasks by attempting to force the tasks in a cgroup into a
 quiescent state. Once the tasks are quiescent another task can

@@ -16,7 +16,7 @@ recoverable error occur. This also allows the checkpointed tasks to be
 migrated between nodes in a cluster by copying the gathered information
 to another node and restarting the tasks there.
 
-	Sequences of SIGSTOP and SIGCONT are not always sufficient for stopping
+Sequences of SIGSTOP and SIGCONT are not always sufficient for stopping
 and resuming tasks in userspace. Both of these signals are observable
 from within the tasks we wish to freeze. While SIGSTOP cannot be caught,
 blocked, or ignored it can be seen by waiting or ptracing parent tasks.

@@ -37,26 +37,29 @@ demonstrate this problem using nested bash shells:
 
 	<at this point 16990 exits and causes 16644 to exit too>
 
-	This happens because bash can observe both signals and choose how it
+This happens because bash can observe both signals and choose how it
 responds to them.
 
-	Another example of a program which catches and responds to these
+Another example of a program which catches and responds to these
 signals is gdb. In fact any program designed to use ptrace is likely to
 have a problem with this method of stopping and resuming tasks.
 
-	 In contrast, the cgroup freezer uses the kernel freezer code to
+In contrast, the cgroup freezer uses the kernel freezer code to
 prevent the freeze/unfreeze cycle from becoming visible to the tasks
 being frozen. This allows the bash example above and gdb to run as
 expected.
 
-	The freezer subsystem in the container filesystem defines a file named
+The freezer subsystem in the container filesystem defines a file named
 freezer.state. Writing "FROZEN" to the state file will freeze all tasks in the
 cgroup. Subsequently writing "THAWED" will unfreeze the tasks in the cgroup.
 Reading will return the current state.
 
+Note freezer.state doesn't exist in root cgroup, which means root cgroup
+is non-freezable.
+
 * Examples of usage :
 
-   # mkdir /containers/freezer
+   # mkdir /containers
    # mount -t cgroup -ofreezer freezer  /containers
    # mkdir /containers/0
    # echo $some_pid > /containers/0/tasks

kernel/cgroup_freezer.c +10 -1

index 6605907..fb249e2 100644
@@ -192,6 +192,13 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 	 */
 	freezer = task_freezer(task);
 
+	/*
+	 * The root cgroup is non-freezable, so we can skip the
+	 * following check.
+	 */
+	if (!freezer->css.cgroup->parent)
+		return;
+
 	spin_lock_irq(&freezer->lock);
 	BUG_ON(freezer->state == CGROUP_FROZEN);

@@ -335,7 +342,7 @@ static int freezer_write(struct cgroup *cgroup,
 	else if (strcmp(buffer, freezer_state_strs[CGROUP_FROZEN]) == 0)
 		goal_state = CGROUP_FROZEN;
 	else
-		return -EIO;
+		return -EINVAL;
 
 	if (!cgroup_lock_live_group(cgroup))
 		return -ENODEV;

@@ -354,6 +361,8 @@ static struct cftype files[] = {
 
 static int freezer_populate(struct cgroup_subsys *ss, struct cgroup *cgroup)
 {
+	if (!cgroup->parent)
+		return 0;
 	return cgroup_add_files(cgroup, ss, files, ARRAY_SIZE(files));
 }

Signature · 1 line

1.5.4.rc3

Li Zefan

author Acked-by Paul Menage

2008-11-06 01:56 UTC #6 lore raw

In reply to Paul Menage

Quoted 4 lines

Acked-by Paul Menage <[email protected]>
I might use the term "non-freezable" rather than "unfreezable".

My bad English :(

patch updated. (some annoying leading tabs are also removed in the doc)

========================

From: Li Zefan <[email protected]>
Date: Tue, 4 Nov 2008 15:26:29 +0800
Subject: [PATCH] freezer_cg: disable writing freezer.state of root cgroup

With this change, control file 'freezer.state' doesn't exist in root cgroup, making root cgroup unfreezable.

I think it's reasonable to disallow freeze tasks in the root cgroup. And then we can avoid fork overhead when freezer subsystem is compiled but not used.

Also make writing invalid value to freezer.state returns EINVAL rather than EIO. This is more consistent with other cgroup subsystem.

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

Acked-by Paul Menage <[email protected]>

---

patch is based on freezer_cg-remove-task_lock-from-freezer_fork.patch

---
 Documentation/cgroups/freezer-subsystem.txt |   19 +++++++++++--------
 kernel/cgroup_freezer.c                     |   11 ++++++++++-
 2 files changed, 21 insertions(+), 9 deletions(-)

Documentation/cgroups/freezer-subsystem.txt +11 -8

index c50ab58..8f7e5d1 100644
@@ -1,4 +1,4 @@
-	The cgroup freezer is useful to batch job management system which start
+The cgroup freezer is useful to batch job management system which start
 and stop sets of tasks in order to schedule the resources of a machine
 according to the desires of a system administrator. This sort of program
 is often used on HPC clusters to schedule access to the cluster as a

@@ -6,7 +6,7 @@ whole. The cgroup freezer uses cgroups to describe the set of tasks to
 be started/stopped by the batch job management system. It also provides
 a means to start and stop the tasks composing the job.
 
-	The cgroup freezer will also be useful for checkpointing running groups
+The cgroup freezer will also be useful for checkpointing running groups
 of tasks. The freezer allows the checkpoint code to obtain a consistent
 image of the tasks by attempting to force the tasks in a cgroup into a
 quiescent state. Once the tasks are quiescent another task can

@@ -16,7 +16,7 @@ recoverable error occur. This also allows the checkpointed tasks to be
 migrated between nodes in a cluster by copying the gathered information
 to another node and restarting the tasks there.
 
-	Sequences of SIGSTOP and SIGCONT are not always sufficient for stopping
+Sequences of SIGSTOP and SIGCONT are not always sufficient for stopping
 and resuming tasks in userspace. Both of these signals are observable
 from within the tasks we wish to freeze. While SIGSTOP cannot be caught,
 blocked, or ignored it can be seen by waiting or ptracing parent tasks.

@@ -37,26 +37,29 @@ demonstrate this problem using nested bash shells:
 
 	<at this point 16990 exits and causes 16644 to exit too>
 
-	This happens because bash can observe both signals and choose how it
+This happens because bash can observe both signals and choose how it
 responds to them.
 
-	Another example of a program which catches and responds to these
+Another example of a program which catches and responds to these
 signals is gdb. In fact any program designed to use ptrace is likely to
 have a problem with this method of stopping and resuming tasks.
 
-	 In contrast, the cgroup freezer uses the kernel freezer code to
+In contrast, the cgroup freezer uses the kernel freezer code to
 prevent the freeze/unfreeze cycle from becoming visible to the tasks
 being frozen. This allows the bash example above and gdb to run as
 expected.
 
-	The freezer subsystem in the container filesystem defines a file named
+The freezer subsystem in the container filesystem defines a file named
 freezer.state. Writing "FROZEN" to the state file will freeze all tasks in the
 cgroup. Subsequently writing "THAWED" will unfreeze the tasks in the cgroup.
 Reading will return the current state.
 
+Note freezer.state doesn't exist in root cgroup, which means root cgroup
+is non-freezable.
+
 * Examples of usage :
 
-   # mkdir /containers/freezer
+   # mkdir /containers
    # mount -t cgroup -ofreezer freezer  /containers
    # mkdir /containers/0
    # echo $some_pid > /containers/0/tasks

kernel/cgroup_freezer.c +10 -1

index 6605907..fb249e2 100644
@@ -192,6 +192,13 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 	 */
 	freezer = task_freezer(task);
 
+	/*
+	 * The root cgroup is non-freezable, so we can skip the
+	 * following check.
+	 */
+	if (!freezer->css.cgroup->parent)
+		return;
+
 	spin_lock_irq(&freezer->lock);
 	BUG_ON(freezer->state == CGROUP_FROZEN);

@@ -335,7 +342,7 @@ static int freezer_write(struct cgroup *cgroup,
 	else if (strcmp(buffer, freezer_state_strs[CGROUP_FROZEN]) == 0)
 		goal_state = CGROUP_FROZEN;
 	else
-		return -EIO;
+		return -EINVAL;
 
 	if (!cgroup_lock_live_group(cgroup))
 		return -ENODEV;

@@ -354,6 +361,8 @@ static struct cftype files[] = {
 
 static int freezer_populate(struct cgroup_subsys *ss, struct cgroup *cgroup)
 {
+	if (!cgroup->parent)
+		return 0;
 	return cgroup_add_files(cgroup, ss, files, ARRAY_SIZE(files));
 }

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-11-06 02:01 UTC #8 lore raw

PATCH 1/1 freezer_cg: disable writing freezer.state of root cgroup

Quoted 9 lines · Li Zefan

Quoted 4 lines
Acked-by Paul Menage <[email protected]>
I might use the term "non-freezable" rather than "unfreezable".
My bad English :(
patch updated. (some annoying leading tabs are also removed in the doc)

And I forgot to s/EIO/EINVAL in the document..

This patch should be the last version.. Sorry for the bothering.

======================

From: Li Zefan <[email protected]>
Date: Tue, 4 Nov 2008 15:26:29 +0800
Subject: [PATCH] freezer_cg: disable writing freezer.state of root cgroup

With this change, control file 'freezer.state' doesn't exist in root cgroup, making root cgroup unfreezable.

I think it's reasonable to disallow freeze tasks in the root cgroup. And then we can avoid fork overhead when freezer subsystem is compiled but not used.

Also make writing invalid value to freezer.state returns EINVAL rather than EIO. This is more consistent with other cgroup subsystem.

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

Acked-by Paul Menage <[email protected]>

---

patch is based on freezer_cg-remove-task_lock-from-freezer_fork.patch

---
 Documentation/cgroups/freezer-subsystem.txt |   21 ++++++++++++---------
 kernel/cgroup_freezer.c                     |   11 ++++++++++-
 2 files changed, 22 insertions(+), 10 deletions(-)

Documentation/cgroups/freezer-subsystem.txt +12 -9

index c50ab58..41f37fe 100644
@@ -1,4 +1,4 @@
-	The cgroup freezer is useful to batch job management system which start
+The cgroup freezer is useful to batch job management system which start
 and stop sets of tasks in order to schedule the resources of a machine
 according to the desires of a system administrator. This sort of program
 is often used on HPC clusters to schedule access to the cluster as a

@@ -6,7 +6,7 @@ whole. The cgroup freezer uses cgroups to describe the set of tasks to
 be started/stopped by the batch job management system. It also provides
 a means to start and stop the tasks composing the job.
 
-	The cgroup freezer will also be useful for checkpointing running groups
+The cgroup freezer will also be useful for checkpointing running groups
 of tasks. The freezer allows the checkpoint code to obtain a consistent
 image of the tasks by attempting to force the tasks in a cgroup into a
 quiescent state. Once the tasks are quiescent another task can

@@ -16,7 +16,7 @@ recoverable error occur. This also allows the checkpointed tasks to be
 migrated between nodes in a cluster by copying the gathered information
 to another node and restarting the tasks there.
 
-	Sequences of SIGSTOP and SIGCONT are not always sufficient for stopping
+Sequences of SIGSTOP and SIGCONT are not always sufficient for stopping
 and resuming tasks in userspace. Both of these signals are observable
 from within the tasks we wish to freeze. While SIGSTOP cannot be caught,
 blocked, or ignored it can be seen by waiting or ptracing parent tasks.

@@ -37,26 +37,29 @@ demonstrate this problem using nested bash shells:
 
 	<at this point 16990 exits and causes 16644 to exit too>
 
-	This happens because bash can observe both signals and choose how it
+This happens because bash can observe both signals and choose how it
 responds to them.
 
-	Another example of a program which catches and responds to these
+Another example of a program which catches and responds to these
 signals is gdb. In fact any program designed to use ptrace is likely to
 have a problem with this method of stopping and resuming tasks.
 
-	 In contrast, the cgroup freezer uses the kernel freezer code to
+In contrast, the cgroup freezer uses the kernel freezer code to
 prevent the freeze/unfreeze cycle from becoming visible to the tasks
 being frozen. This allows the bash example above and gdb to run as
 expected.
 
-	The freezer subsystem in the container filesystem defines a file named
+The freezer subsystem in the container filesystem defines a file named
 freezer.state. Writing "FROZEN" to the state file will freeze all tasks in the
 cgroup. Subsequently writing "THAWED" will unfreeze the tasks in the cgroup.
 Reading will return the current state.
 
+Note freezer.state doesn't exist in root cgroup, which means root cgroup
+is non-freezable.
+
 * Examples of usage :
 
-   # mkdir /containers/freezer
+   # mkdir /containers
    # mount -t cgroup -ofreezer freezer  /containers
    # mkdir /containers/0
    # echo $some_pid > /containers/0/tasks

@@ -94,6 +97,6 @@ things happens:
 		the freezer.state file
 	2) Userspace retries the freezing operation by writing "FROZEN" to
 		the freezer.state file (writing "FREEZING" is not legal
-		and returns EIO)
+		and returns EINVAL)
 	3) The tasks that blocked the cgroup from entering the "FROZEN"
 		state disappear from the cgroup's set of tasks.

kernel/cgroup_freezer.c +10 -1

index 6605907..fb249e2 100644
@@ -192,6 +192,13 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 	 */
 	freezer = task_freezer(task);
 
+	/*
+	 * The root cgroup is non-freezable, so we can skip the
+	 * following check.
+	 */
+	if (!freezer->css.cgroup->parent)
+		return;
+
 	spin_lock_irq(&freezer->lock);
 	BUG_ON(freezer->state == CGROUP_FROZEN);

@@ -335,7 +342,7 @@ static int freezer_write(struct cgroup *cgroup,
 	else if (strcmp(buffer, freezer_state_strs[CGROUP_FROZEN]) == 0)
 		goal_state = CGROUP_FROZEN;
 	else
-		return -EIO;
+		return -EINVAL;
 
 	if (!cgroup_lock_live_group(cgroup))
 		return -ENODEV;

@@ -354,6 +361,8 @@ static struct cftype files[] = {
 
 static int freezer_populate(struct cgroup_subsys *ss, struct cgroup *cgroup)
 {
+	if (!cgroup->parent)
+		return 0;
 	return cgroup_add_files(cgroup, ss, files, ARRAY_SIZE(files));
 }

Signature · 1 line

1.5.4.rc3

Li Zefan

author

2008-11-06 02:04 UTC #9 lore raw

PATCH 1/1 freezer_cg: disable writing freezer.state of root cgroup

In reply to Li Zefan

Quoted 9 lines · Li Zefan

Quoted 4 lines
Acked-by Paul Menage <[email protected]>
I might use the term "non-freezable" rather than "unfreezable".
My bad English :(
patch updated. (some annoying leading tabs are also removed in the doc)

And I forgot to s/EIO/EINVAL in the document..

This patch should be the last version.. Sorry for the bothering.

======================

From: Li Zefan <[email protected]>
Date: Tue, 4 Nov 2008 15:26:29 +0800
Subject: [PATCH] freezer_cg: disable writing freezer.state of root cgroup

With this change, control file 'freezer.state' doesn't exist in root cgroup, making root cgroup unfreezable.

I think it's reasonable to disallow freeze tasks in the root cgroup. And then we can avoid fork overhead when freezer subsystem is compiled but not used.

Also make writing invalid value to freezer.state returns EINVAL rather than EIO. This is more consistent with other cgroup subsystem.

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

Acked-by Paul Menage <[email protected]>

---

patch is based on freezer_cg-remove-task_lock-from-freezer_fork.patch

---
 Documentation/cgroups/freezer-subsystem.txt |   21 ++++++++++++---------
 kernel/cgroup_freezer.c                     |   11 ++++++++++-
 2 files changed, 22 insertions(+), 10 deletions(-)

Documentation/cgroups/freezer-subsystem.txt +12 -9

index c50ab58..41f37fe 100644
@@ -1,4 +1,4 @@
-	The cgroup freezer is useful to batch job management system which start
+The cgroup freezer is useful to batch job management system which start
 and stop sets of tasks in order to schedule the resources of a machine
 according to the desires of a system administrator. This sort of program
 is often used on HPC clusters to schedule access to the cluster as a

@@ -6,7 +6,7 @@ whole. The cgroup freezer uses cgroups to describe the set of tasks to
 be started/stopped by the batch job management system. It also provides
 a means to start and stop the tasks composing the job.
 
-	The cgroup freezer will also be useful for checkpointing running groups
+The cgroup freezer will also be useful for checkpointing running groups
 of tasks. The freezer allows the checkpoint code to obtain a consistent
 image of the tasks by attempting to force the tasks in a cgroup into a
 quiescent state. Once the tasks are quiescent another task can

@@ -16,7 +16,7 @@ recoverable error occur. This also allows the checkpointed tasks to be
 migrated between nodes in a cluster by copying the gathered information
 to another node and restarting the tasks there.
 
-	Sequences of SIGSTOP and SIGCONT are not always sufficient for stopping
+Sequences of SIGSTOP and SIGCONT are not always sufficient for stopping
 and resuming tasks in userspace. Both of these signals are observable
 from within the tasks we wish to freeze. While SIGSTOP cannot be caught,
 blocked, or ignored it can be seen by waiting or ptracing parent tasks.

@@ -37,26 +37,29 @@ demonstrate this problem using nested bash shells:
 
 	<at this point 16990 exits and causes 16644 to exit too>
 
-	This happens because bash can observe both signals and choose how it
+This happens because bash can observe both signals and choose how it
 responds to them.
 
-	Another example of a program which catches and responds to these
+Another example of a program which catches and responds to these
 signals is gdb. In fact any program designed to use ptrace is likely to
 have a problem with this method of stopping and resuming tasks.
 
-	 In contrast, the cgroup freezer uses the kernel freezer code to
+In contrast, the cgroup freezer uses the kernel freezer code to
 prevent the freeze/unfreeze cycle from becoming visible to the tasks
 being frozen. This allows the bash example above and gdb to run as
 expected.
 
-	The freezer subsystem in the container filesystem defines a file named
+The freezer subsystem in the container filesystem defines a file named
 freezer.state. Writing "FROZEN" to the state file will freeze all tasks in the
 cgroup. Subsequently writing "THAWED" will unfreeze the tasks in the cgroup.
 Reading will return the current state.
 
+Note freezer.state doesn't exist in root cgroup, which means root cgroup
+is non-freezable.
+
 * Examples of usage :
 
-   # mkdir /containers/freezer
+   # mkdir /containers
    # mount -t cgroup -ofreezer freezer  /containers
    # mkdir /containers/0
    # echo $some_pid > /containers/0/tasks

@@ -94,6 +97,6 @@ things happens:
 		the freezer.state file
 	2) Userspace retries the freezing operation by writing "FROZEN" to
 		the freezer.state file (writing "FREEZING" is not legal
-		and returns EIO)
+		and returns EINVAL)
 	3) The tasks that blocked the cgroup from entering the "FROZEN"
 		state disappear from the cgroup's set of tasks.

kernel/cgroup_freezer.c +10 -1

index 6605907..fb249e2 100644
@@ -192,6 +192,13 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 	 */
 	freezer = task_freezer(task);
 
+	/*
+	 * The root cgroup is non-freezable, so we can skip the
+	 * following check.
+	 */
+	if (!freezer->css.cgroup->parent)
+		return;
+
 	spin_lock_irq(&freezer->lock);
 	BUG_ON(freezer->state == CGROUP_FROZEN);

@@ -335,7 +342,7 @@ static int freezer_write(struct cgroup *cgroup,
 	else if (strcmp(buffer, freezer_state_strs[CGROUP_FROZEN]) == 0)
 		goal_state = CGROUP_FROZEN;
 	else
-		return -EIO;
+		return -EINVAL;
 
 	if (!cgroup_lock_live_group(cgroup))
 		return -ENODEV;

@@ -354,6 +361,8 @@ static struct cftype files[] = {
 
 static int freezer_populate(struct cgroup_subsys *ss, struct cgroup *cgroup)
 {
+	if (!cgroup->parent)
+		return 0;
 	return cgroup_add_files(cgroup, ss, files, ARRAY_SIZE(files));
 }

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-11-06 08:13 UTC #11 lore raw

Hello Li !

Quoted 28 lines · Li Zefan

Quoted 8 lines · Li Zefan
Quoted 4 lines
Acked-by Paul Menage <[email protected]>
I might use the term "non-freezable" rather than "unfreezable".
My bad English :(
patch updated. (some annoying leading tabs are also removed in the doc)
And I forgot to s/EIO/EINVAL in the document..
This patch should be the last version.. Sorry for the bothering.
======================
From: Li Zefan <[email protected]>
Date: Tue, 4 Nov 2008 15:26:29 +0800
Subject: [PATCH] freezer_cg: disable writing freezer.state of root cgroup
With this change, control file 'freezer.state' doesn't exist in root cgroup, making root cgroup unfreezable.
I think it's reasonable to disallow freeze tasks in the root cgroup. And then we can avoid fork overhead when freezer subsystem is compiled but not used.

Quoted 5 lines

Also make writing invalid value to freezer.state returns EINVAL rather than EIO. This is more consistent with other cgroup subsystem.
Signed-off-by Li Zefan <[email protected]>
Acked-by Paul Menage <[email protected]>

It looks fine

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

I've also tested it.

here's a minor comment,

I would make a small macro is_root_freezer() to show explicitly what we are testing and use the CSS_ROOT bit to test. That's what the CSS_ROOT bit is for Paul ?

if yes, here's a possible result is below.

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

---
 kernel/cgroup_freezer.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

kernel/cgroup_freezer.c +15 -0

Index: 2.6.27-lxc/kernel/cgroup_freezer.c
===================================================================
@@ -47,6 +47,11 @@ static inline struct freezer *task_freez
 			    struct freezer, css);
 }
 
+static inline int is_root_freezer(struct freezer *freezer)
+{
+	return test_bit(CSS_ROOT, &freezer->css.flags);
+}
+
 int cgroup_frozen(struct task_struct *task)
 {
 	struct freezer *freezer;

@@ -190,6 +195,13 @@ static void freezer_fork(struct cgroup_s
 	freezer = task_freezer(task);
 	task_unlock(task);
 
+	/*
+	 * The root cgroup is non-freezable, so we can skip the
+	 * following check.
+	 */
+	if (is_root_freezer(freezer))
+		return;
+
 	BUG_ON(freezer->state == CGROUP_FROZEN);
 	spin_lock_irq(&freezer->lock);
 	/* Locking avoids race with FREEZING -> THAWED transitions. */

@@ -363,6 +375,9 @@ static struct cftype files[] = {
 
 static int freezer_populate(struct cgroup_subsys *ss, struct cgroup *cgroup)
 {
+	if (is_root_freezer(cgroup_freezer(cgroup)))
+		return 0;
+
 	return cgroup_add_files(cgroup, ss, files, ARRAY_SIZE(files));
 }

Cedric Le Goater

Acked-by on v1 1/1

2008-11-06 08:15 UTC #12 lore raw

In reply to Li Zefan

Hello Li !

Quoted 28 lines · Li Zefan

Quoted 8 lines · Li Zefan
Quoted 4 lines
Acked-by Paul Menage <[email protected]>
I might use the term "non-freezable" rather than "unfreezable".
My bad English :(
patch updated. (some annoying leading tabs are also removed in the doc)
And I forgot to s/EIO/EINVAL in the document..
This patch should be the last version.. Sorry for the bothering.
======================
From: Li Zefan <[email protected]>
Date: Tue, 4 Nov 2008 15:26:29 +0800
Subject: [PATCH] freezer_cg: disable writing freezer.state of root cgroup
With this change, control file 'freezer.state' doesn't exist in root cgroup, making root cgroup unfreezable.
I think it's reasonable to disallow freeze tasks in the root cgroup. And then we can avoid fork overhead when freezer subsystem is compiled but not used.

Quoted 5 lines

Also make writing invalid value to freezer.state returns EINVAL rather than EIO. This is more consistent with other cgroup subsystem.
Signed-off-by Li Zefan <[email protected]>
Acked-by Paul Menage <[email protected]>

It looks fine

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

I've also tested it.

here's a minor comment,

I would make a small macro is_root_freezer() to show explicitly what we are testing and use the CSS_ROOT bit to test. That's what the CSS_ROOT bit is for Paul ?

if yes, here's a possible result is below.

Signed-off-by Cedric Le Goater <[email protected]>

---
 kernel/cgroup_freezer.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

kernel/cgroup_freezer.c +15 -0

Index: 2.6.27-lxc/kernel/cgroup_freezer.c
===================================================================
@@ -47,6 +47,11 @@ static inline struct freezer *task_freez
 			    struct freezer, css);
 }
 
+static inline int is_root_freezer(struct freezer *freezer)
+{
+	return test_bit(CSS_ROOT, &freezer->css.flags);
+}
+
 int cgroup_frozen(struct task_struct *task)
 {
 	struct freezer *freezer;

@@ -190,6 +195,13 @@ static void freezer_fork(struct cgroup_s
 	freezer = task_freezer(task);
 	task_unlock(task);
 
+	/*
+	 * The root cgroup is non-freezable, so we can skip the
+	 * following check.
+	 */
+	if (is_root_freezer(freezer))
+		return;
+
 	BUG_ON(freezer->state == CGROUP_FROZEN);
 	spin_lock_irq(&freezer->lock);
 	/* Locking avoids race with FREEZING -> THAWED transitions. */

@@ -363,6 +375,9 @@ static struct cftype files[] = {
 
 static int freezer_populate(struct cgroup_subsys *ss, struct cgroup *cgroup)
 {
+	if (is_root_freezer(cgroup_freezer(cgroup)))
+		return 0;
+
 	return cgroup_add_files(cgroup, ss, files, ARRAY_SIZE(files));
 }

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

Li Zefan

2008-11-07 06:58 UTC #14 lore raw

Quoted 4 lines

I would make a small macro is_root_freezer() to show explicitly what we are testing and use the CSS_ROOT bit to test. That's what the CSS_ROOT bit is for Paul ?

I think yes CSS_ROOT can be used, though currently CSS_ROOT is used in cgroup internal only (in css_get() and css_put()).

Quoted 2 lines

if yes, here's a possible result is below.

is_root_freezer() looks a bit better, but I don't have strong option about this change, the original code and the comment is enough to explain what we are doing.

Quoted 47 lines

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

---
 kernel/cgroup_freezer.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

Index: 2.6.27-lxc/kernel/cgroup_freezer.c
===================================================================

kernel/cgroup_freezer.c +15 -0

@@ -47,6 +47,11 @@ static inline struct freezer *task_freez
 			    struct freezer, css);
 }
 
+static inline int is_root_freezer(struct freezer *freezer)
+{
+	return test_bit(CSS_ROOT, &freezer->css.flags);
+}
+
 int cgroup_frozen(struct task_struct *task)
 {
 	struct freezer *freezer;

@@ -190,6 +195,13 @@ static void freezer_fork(struct cgroup_s
 	freezer = task_freezer(task);
 	task_unlock(task);
 
+	/*
+	 * The root cgroup is non-freezable, so we can skip the
+	 * following check.
+	 */
+	if (is_root_freezer(freezer))
+		return;
+
 	BUG_ON(freezer->state == CGROUP_FROZEN);
 	spin_lock_irq(&freezer->lock);
 	/* Locking avoids race with FREEZING -> THAWED transitions. */

@@ -363,6 +375,9 @@ static struct cftype files[] = {
 
 static int freezer_populate(struct cgroup_subsys *ss, struct cgroup *cgroup)
 {
+	if (is_root_freezer(cgroup_freezer(cgroup)))
+		return 0;
+
 	return cgroup_add_files(cgroup, ss, files, ARRAY_SIZE(files));
 }

Li Zefan

author

2008-11-07 07:01 UTC #15 lore raw

In reply to Cedric Le Goater

Quoted 4 lines

I would make a small macro is_root_freezer() to show explicitly what we are testing and use the CSS_ROOT bit to test. That's what the CSS_ROOT bit is for Paul ?

I think yes CSS_ROOT can be used, though currently CSS_ROOT is used in cgroup internal only (in css_get() and css_put()).

Quoted 2 lines

if yes, here's a possible result is below.

is_root_freezer() looks a bit better, but I don't have strong option about this change, the original code and the comment is enough to explain what we are doing.

Quoted 47 lines

Signed-off-by Cedric Le Goater <[email protected]>

---
 kernel/cgroup_freezer.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

Index: 2.6.27-lxc/kernel/cgroup_freezer.c
===================================================================

kernel/cgroup_freezer.c +15 -0

@@ -47,6 +47,11 @@ static inline struct freezer *task_freez
 			    struct freezer, css);
 }
 
+static inline int is_root_freezer(struct freezer *freezer)
+{
+	return test_bit(CSS_ROOT, &freezer->css.flags);
+}
+
 int cgroup_frozen(struct task_struct *task)
 {
 	struct freezer *freezer;

@@ -190,6 +195,13 @@ static void freezer_fork(struct cgroup_s
 	freezer = task_freezer(task);
 	task_unlock(task);
 
+	/*
+	 * The root cgroup is non-freezable, so we can skip the
+	 * following check.
+	 */
+	if (is_root_freezer(freezer))
+		return;
+
 	BUG_ON(freezer->state == CGROUP_FROZEN);
 	spin_lock_irq(&freezer->lock);
 	/* Locking avoids race with FREEZING -> THAWED transitions. */

@@ -363,6 +375,9 @@ static struct cftype files[] = {
 
 static int freezer_populate(struct cgroup_subsys *ss, struct cgroup *cgroup)
 {
+	if (is_root_freezer(cgroup_freezer(cgroup)))
+		return 0;
+
 	return cgroup_add_files(cgroup, ss, files, ARRAY_SIZE(files));
 }