Skip to contentSkip to conversation

cgroup: memory.force_empty can make system slowdown

PATCH In review IKEDA, Munehiro 3 participants 5 messages Aug 16, 2008 – Aug 17, 2008 2 messages not in the archive

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 cgroup: memory.force_empty can make system slowdown

Cgroup's memory controller has a control file "memory.force_empty" to reset usage account charged to a cgroup. The account shouldn't be reset if one or more processes are attached to the cgroup (at least for memory controller, IMHO). So mem_cgroup_force_empty() is implemented to return -EBUSY and do nothing if so. However, cgroup on hierarchy root faultily might be a exception. Even if processes are attached to root cgroup (which is a "default" cgroup for processes), forcing-empty can run by writing something to memory.force_empty and it'll never end.

Following patch prevents this issue.

This patch is for cgroup infrastructure code. The issue can be measured by modifying memory controller code also, namely to change mem_cgroup_force_empty() to see CSS_ROOT bit of css->flags. I believe cgroup->count approach like the patch below is rather generic and reasonable, how does that sound?

Paul, Balbir?

kernel/cgroup.c +3 -1
diff -uNrp linux-2.6.27-rc3.orig/kernel/cgroup.c linux-2.6.27-rc3/kernel/cgroup.c
@@ -2264,8 +2264,10 @@ static void init_cgroup_css(struct cgrou
 	css->cgroup = cgrp;
 	atomic_set(&css->refcnt, 0);
 	css->flags = 0;
-	if (cgrp == dummytop)
+	if (cgrp == dummytop) {
 		set_bit(CSS_ROOT, &css->flags);
+		atomic_set(&css->cgroup->count, 1);
+	}
 	BUG_ON(cgrp->subsys[ss->subsys_id]);
 	cgrp->subsys[ss->subsys_id] = css;
 }
Signature · 3 lines
IKEDA, Munehiro
  NEC Corporation of America
    [email protected]

A message that is not in the archive (sent off-list or not yet received): 48A63AD1.3010907-MDRzhb/z0dd8UrSeD/[email protected]

#2 lore raw
Quoted 10 lines · Munehiro

Cgroup's memory controller has a control file "memory.force_empty" to reset usage account charged to a cgroup. The account shouldn't be reset if one or more processes are attached to the cgroup (at least for memory controller, IMHO). So mem_cgroup_force_empty() is implemented to return -EBUSY and do nothing if so. However, cgroup on hierarchy root faultily might be a exception. Even if processes are attached to root cgroup (which is a "default" cgroup for processes), forcing-empty can run by writing something to memory.force_empty and it'll never end.

I found this bug last week, and I've made patches to fix it, but then I was on vacation. I'll send the patches out soon.

Quoted 8 lines

Following patch prevents this issue.

This patch is for cgroup infrastructure code. The issue can be measured by modifying memory controller code also, namely to change mem_cgroup_force_empty() to see CSS_ROOT bit of css->flags. I believe cgroup->count approach like the patch below is rather generic and reasonable, how does that sound?

It's ok for the top_group's count to be 0 due to the top_cgroup hack. With this patch, the top cgroup's count will be always >0, even if it has no tasks in it, so writing to top_cgroup's force_empty will always return -EBUSY.

Quoted 24 lines

Paul, Balbir?

diff -uNrp linux-2.6.27-rc3.orig/kernel/cgroup.c linux-2.6.27-rc3/kernel/cgroup.c

kernel/cgroup.c +3 -1
@@ -2264,8 +2264,10 @@ static void init_cgroup_css(struct cgrou
 	css->cgroup = cgrp;
 	atomic_set(&css->refcnt, 0);
 	css->flags = 0;
-	if (cgrp == dummytop)
+	if (cgrp == dummytop) {
 		set_bit(CSS_ROOT, &css->flags);
+		atomic_set(&css->cgroup->count, 1);
+	}
 	BUG_ON(cgrp->subsys[ss->subsys_id]);
 	cgrp->subsys[ss->subsys_id] = css;
 }
#3 lore raw
In reply to IKEDA, Munehiro
Quoted 10 lines · Munehiro

Cgroup's memory controller has a control file "memory.force_empty" to reset usage account charged to a cgroup. The account shouldn't be reset if one or more processes are attached to the cgroup (at least for memory controller, IMHO). So mem_cgroup_force_empty() is implemented to return -EBUSY and do nothing if so. However, cgroup on hierarchy root faultily might be a exception. Even if processes are attached to root cgroup (which is a "default" cgroup for processes), forcing-empty can run by writing something to memory.force_empty and it'll never end.

I found this bug last week, and I've made patches to fix it, but then I was on vacation. I'll send the patches out soon.

Quoted 8 lines

Following patch prevents this issue.

This patch is for cgroup infrastructure code. The issue can be measured by modifying memory controller code also, namely to change mem_cgroup_force_empty() to see CSS_ROOT bit of css->flags. I believe cgroup->count approach like the patch below is rather generic and reasonable, how does that sound?

It's ok for the top_group's count to be 0 due to the top_cgroup hack. With this patch, the top cgroup's count will be always >0, even if it has no tasks in it, so writing to top_cgroup's force_empty will always return -EBUSY.

Quoted 24 lines

Paul, Balbir?

diff -uNrp linux-2.6.27-rc3.orig/kernel/cgroup.c linux-2.6.27-rc3/kernel/cgroup.c

kernel/cgroup.c +3 -1
@@ -2264,8 +2264,10 @@ static void init_cgroup_css(struct cgrou
 	css->cgroup = cgrp;
 	atomic_set(&css->refcnt, 0);
 	css->flags = 0;
-	if (cgrp == dummytop)
+	if (cgrp == dummytop) {
 		set_bit(CSS_ROOT, &css->flags);
+		atomic_set(&css->cgroup->count, 1);
+	}
 	BUG_ON(cgrp->subsys[ss->subsys_id]);
 	cgrp->subsys[ss->subsys_id] = css;
 }

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

#5 lore raw
Quoted 24 lines · Li Zefan
Quoted 10 lines · Munehiro

Cgroup's memory controller has a control file "memory.force_empty" to reset usage account charged to a cgroup. The account shouldn't be reset if one or more processes are attached to the cgroup (at least for memory controller, IMHO). So mem_cgroup_force_empty() is implemented to return -EBUSY and do nothing if so. However, cgroup on hierarchy root faultily might be a exception. Even if processes are attached to root cgroup (which is a "default" cgroup for processes), forcing-empty can run by writing something to memory.force_empty and it'll never end.

I found this bug last week, and I've made patches to fix it, but then I was on vacation. I'll send the patches out soon.

Quoted 8 lines

Following patch prevents this issue.

This patch is for cgroup infrastructure code. The issue can be measured by modifying memory controller code also, namely to change mem_cgroup_force_empty() to see CSS_ROOT bit of css->flags. I believe cgroup->count approach like the patch below is rather generic and reasonable, how does that sound?

Quoted 5 lines

It's ok for the top_group's count to be 0 due to the top_cgroup hack. With this patch, the top cgroup's count will be always >0, even if it has no tasks in it, so writing to top_cgroup's force_empty will always return -EBUSY.

I thought cgrp->css_sets will be empty when there are no tasks in the top cgroup, but I was wrong, because init_css_set's refcount will always >0,
so cgroup_task_count() won't return 0 for the top cgroup:

# mount -t cgroup -o debug xxx /mnt
# mkdir /mnt/sub
# for pid in `cat /mnt/tasks`; do echo $pid > /mnt/sub/tasks; done
# cat /mnt/tasks
# cat /mnt/debug.taskcount
3
#6 lore raw
In reply to Li Zefan
Quoted 24 lines · Li Zefan
Quoted 10 lines · Munehiro

Cgroup's memory controller has a control file "memory.force_empty" to reset usage account charged to a cgroup. The account shouldn't be reset if one or more processes are attached to the cgroup (at least for memory controller, IMHO). So mem_cgroup_force_empty() is implemented to return -EBUSY and do nothing if so. However, cgroup on hierarchy root faultily might be a exception. Even if processes are attached to root cgroup (which is a "default" cgroup for processes), forcing-empty can run by writing something to memory.force_empty and it'll never end.

I found this bug last week, and I've made patches to fix it, but then I was on vacation. I'll send the patches out soon.

Quoted 8 lines

Following patch prevents this issue.

This patch is for cgroup infrastructure code. The issue can be measured by modifying memory controller code also, namely to change mem_cgroup_force_empty() to see CSS_ROOT bit of css->flags. I believe cgroup->count approach like the patch below is rather generic and reasonable, how does that sound?

Quoted 5 lines

It's ok for the top_group's count to be 0 due to the top_cgroup hack. With this patch, the top cgroup's count will be always >0, even if it has no tasks in it, so writing to top_cgroup's force_empty will always return -EBUSY.

I thought cgrp->css_sets will be empty when there are no tasks in the top cgroup, but I was wrong, because init_css_set's refcount will always >0,
so cgroup_task_count() won't return 0 for the top cgroup:

# mount -t cgroup -o debug xxx /mnt
# mkdir /mnt/sub
# for pid in `cat /mnt/tasks`; do echo $pid > /mnt/sub/tasks; done
# cat /mnt/tasks
# cat /mnt/debug.taskcount
3