Skip to contentSkip to conversation

cgroup: use a hash table for css_set finding

PATCH In review Li Zefan 4 participants 9 messages Apr 3, 2008 3 messages not in the archive cgroup

lore
Share & tools
lore

Li Zefan

author
#0 lore raw

PATCH 1/3 cgroup: use a hash table for css_set finding

When we attach a process to a different cgroup, the css_set linked-list will be run through to find a suitable existing css_set to use. This patch implements a hash table for better performance.

The following benmarks has been tested:

For N in 1, 5, 10, 50, 100, 500, 1000, create N cgroups with one sleeping task in each, and then move an additional task through each cgroup in turn.

Here is a test result:

N Loop orig - Time(s) hash - Time(s)
----------------------------------------------
1 10000 1.201231728 1.196311177
5 2000 1.065743872 1.040566424
10 1000 0.991054735 0.986876440
50 200 0.976554203 0.969608733
100 100 0.998504680 0.969218270
500 20 1.157347764 0.962602963
1000 10 1.619521852 1.085140172

Signed-off-by Li Zefan <[email protected]>
Reviewed-by Paul Menage <[email protected]>
---
 include/linux/cgroup.h |    7 +++++-
 kernel/cgroup.c        |   58 ++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 52 insertions(+), 13 deletions(-)
include/linux/cgroup.h +6 -1
index 2d1d151..f585b7c 100644
@@ -156,6 +156,12 @@ struct css_set {
 	struct list_head list;
 
 	/*
+	 * List running through all cgroup groups in the same hash
+	 * slot. Protected by css_set_lock
+	 */
+	struct hlist_node hlist;
+
+	/*
 	 * List running through all tasks using this cgroup
 	 * group. Protected by css_set_lock
 	 */

@@ -174,7 +180,6 @@ struct css_set {
 	 * during subsystem registration (at boot time).
 	 */
 	struct cgroup_subsys_state *subsys[CGROUP_SUBSYS_COUNT];
-
 };
 
 /*
kernel/cgroup.c +46 -12

140 lines not on this page · raw message

Signature · 1 line
1.5.4.rc3
#1 lore raw

kernel/cgroup.c static struct cg_cgroup_link init_css_set_link; · line 201

Quoted 5 lines · On Thu, 03 Apr 2008 13:52:43 +0800 Li Zefan <[email protected]> wrote:
kernel/cgroup.c +5 -0
+/* hash table for cgroup groups. This improves the performance to
+ * find an existing css_set */
+#define CSS_SET_HASH_BITS	7
+#define CSS_SET_TABLE_SIZE	(1 << CSS_SET_HASH_BITS)
+static struct hlist_head css_set_table[CSS_SET_TABLE_SIZE];

How above number is selected ?

kernel/cgroup.c static struct cg_cgroup_link init_css_set_link; · line 211

Quoted 9 lines
kernel/cgroup.c +9 -0
+static struct hlist_head *css_set_hash(struct cgroup_subsys_state *css[])
+{
+	int i;
+	int index;
+	unsigned long tmp = 0UL;
+
+	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++)
+		tmp += (unsigned long)css[i];
+

maybe css[i]'s lower 2-3 bits will be ignored. because thery are always 0.

And I don't like "+" for hash. how about
==

	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++)
	unsigned long x;
	x = (unsigned long)css[i];
	tmp = (x >> 16) ^ (x & 0xffff)

==
or some func, which uses full bits.

Quoted 9 lines
kernel/cgroup.c +5 -0
+	index = hash_long(tmp, CSS_SET_HASH_BITS);
+
+	return &css_set_table[index];
+}
+
 /* We don't maintain the lists running through each css_set to its
  * task until after the first call to cgroup_iter_start(). This
  * reduces the fork()/exit() overhead for people who have cgroups

kernel/cgroup.c static int use_task_css_set_links; · line 245

Quoted 6 lines
kernel/cgroup.c +1 -0
@@ -219,6 +240,7 @@ static int use_task_css_set_links;
 static void unlink_css_set(struct css_set *cg)
 {
 	write_lock(&css_set_lock);
+	hlist_del(&cg->hlist);
 	list_del(&cg->list);
 	css_set_count--;

This css_set_lock is worth to be rwlock ?
how about per hashline spinlock ? (but per-hashline seems overkill..)

Thanks,
-Kame

A message that is not in the archive (sent off-list or not yet received): 20080403162459.4a90d574.kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org

#3 lore raw
Quoted 10 lines · KAMEZAWA Hiroyuki

kernel/cgroup.c static struct cg_cgroup_link init_css_set_link; · line 201

Quoted 5 lines · On Thu, 03 Apr 2008 13:52:43 +0800 Li Zefan…
kernel/cgroup.c +5 -0
+/* hash table for cgroup groups. This improves the performance to
+ * find an existing css_set */
+#define CSS_SET_HASH_BITS	7
+#define CSS_SET_TABLE_SIZE	(1 << CSS_SET_HASH_BITS)
+static struct hlist_head css_set_table[CSS_SET_TABLE_SIZE];

How above number is selected ?

I suppose 100 will be suitable, so i would like to choose from 6 or 7 bits.

Quoted 12 lines

kernel/cgroup.c static struct cg_cgroup_link init_css_set_link; · line 211

Quoted 9 lines
kernel/cgroup.c +9 -0
+static struct hlist_head *css_set_hash(struct cgroup_subsys_state *css[])
+{
+	int i;
+	int index;
+	unsigned long tmp = 0UL;
+
+	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++)
+		tmp += (unsigned long)css[i];
+

maybe css[i]'s lower 2-3 bits will be ignored. because thery are always 0.

Quoted 9 lines

And I don't like "+" for hash. how about
==

	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++)
	unsigned long x;
	x = (unsigned long)css[i];
	tmp = (x >> 16) ^ (x & 0xffff)

==
or some func, which uses full bits.

I'm using hash_long(), which has been proved to be a good one. And I've tested css_set_hash(), I run the css_set benchmark with N == 1000, the for loop in find_existing_css_set() will break out within 10 iterations for most cases, which is the expected result.

Quoted 11 lines
Quoted 11 lines
kernel/cgroup.c +5 -0
+	index = hash_long(tmp, CSS_SET_HASH_BITS);
+
+	return &css_set_table[index];
+}
+
 /* We don't maintain the lists running through each css_set to its
  * task until after the first call to cgroup_iter_start(). This
  * reduces the fork()/exit() overhead for people who have cgroups

@@ -219,6 +240,7 @@ static int use_task_css_set_links;
 static void unlink_css_set(struct css_set *cg)
Quoted 9 lines

kernel/cgroup.c static int use_task_css_set_links; · line 245

Quoted 5 lines
kernel/cgroup.c +1 -0
 {
 	write_lock(&css_set_lock);
+	hlist_del(&cg->hlist);
 	list_del(&cg->list);
 	css_set_count--;

This css_set_lock is worth to be rwlock ?
how about per hashline spinlock ? (but per-hashline seems overkill..)

I think it's an overkill. And the css_set_lock protects not only the hash table.

Thanks for looking into this. :)

Regards,
Li Zefan

author
#4 lore raw
In reply to KAMEZAWA Hiroyuki
Quoted 10 lines · KAMEZAWA Hiroyuki

kernel/cgroup.c static struct cg_cgroup_link init_css_set_link; · line 201

Quoted 5 lines · On Thu, 03 Apr 2008 13:52:43 +0800 Li Zefan <[email protected]> wrote:
kernel/cgroup.c +5 -0
+/* hash table for cgroup groups. This improves the performance to
+ * find an existing css_set */
+#define CSS_SET_HASH_BITS	7
+#define CSS_SET_TABLE_SIZE	(1 << CSS_SET_HASH_BITS)
+static struct hlist_head css_set_table[CSS_SET_TABLE_SIZE];

How above number is selected ?

I suppose 100 will be suitable, so i would like to choose from 6 or 7 bits.

Quoted 12 lines

kernel/cgroup.c static struct cg_cgroup_link init_css_set_link; · line 211

Quoted 9 lines
kernel/cgroup.c +9 -0
+static struct hlist_head *css_set_hash(struct cgroup_subsys_state *css[])
+{
+	int i;
+	int index;
+	unsigned long tmp = 0UL;
+
+	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++)
+		tmp += (unsigned long)css[i];
+

maybe css[i]'s lower 2-3 bits will be ignored. because thery are always 0.

Quoted 9 lines

And I don't like "+" for hash. how about
==

	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++)
	unsigned long x;
	x = (unsigned long)css[i];
	tmp = (x >> 16) ^ (x & 0xffff)

==
or some func, which uses full bits.

I'm using hash_long(), which has been proved to be a good one. And I've tested css_set_hash(), I run the css_set benchmark with N == 1000, the for loop in find_existing_css_set() will break out within 10 iterations for most cases, which is the expected result.

Quoted 11 lines
Quoted 11 lines
kernel/cgroup.c +5 -0
+	index = hash_long(tmp, CSS_SET_HASH_BITS);
+
+	return &css_set_table[index];
+}
+
 /* We don't maintain the lists running through each css_set to its
  * task until after the first call to cgroup_iter_start(). This
  * reduces the fork()/exit() overhead for people who have cgroups

@@ -219,6 +240,7 @@ static int use_task_css_set_links;
 static void unlink_css_set(struct css_set *cg)
Quoted 9 lines

kernel/cgroup.c static int use_task_css_set_links; · line 245

Quoted 5 lines
kernel/cgroup.c +1 -0
 {
 	write_lock(&css_set_lock);
+	hlist_del(&cg->hlist);
 	list_del(&cg->list);
 	css_set_count--;

This css_set_lock is worth to be rwlock ?
how about per hashline spinlock ? (but per-hashline seems overkill..)

I think it's an overkill. And the css_set_lock protects not only the hash table.

Thanks for looking into this. :)

Regards,
Li Zefan

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

#6 lore raw
Quoted 21 lines
Quoted 11 lines

kernel/cgroup.c static struct cg_cgroup_link init_css_set_link; · line 211

Quoted 9 lines
kernel/cgroup.c +9 -0
+static struct hlist_head *css_set_hash(struct cgroup_subsys_state *css[])
+{
+	int i;
+	int index;
+	unsigned long tmp = 0UL;
+
+	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++)
+		tmp += (unsigned long)css[i];
+

maybe css[i]'s lower 2-3 bits will be ignored. because thery are always 0.

Quoted 9 lines

And I don't like "+" for hash. how about
==

	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++)
	unsigned long x;
	x = (unsigned long)css[i];
	tmp = (x >> 16) ^ (x & 0xffff)

==
or some func, which uses full bits.

Quoted 5 lines

I'm using hash_long(), which has been proved to be a good one. And I've tested css_set_hash(), I run the css_set benchmark with N == 1000, the for loop in find_existing_css_set() will break out within 10 iterations for most cases, which is the expected result.

I just revised the css_set_hash() a bit, and It turned out we can improve the scalability.

Thanks, KAMEZAWA-san.

Reviewed-by Paul Menage
#7 lore raw
In reply to a message not in the archive

When we attach a process to a different cgroup, the css_set linked-list will be run through to find a suitable existing css_set to use. This patch implements a hash table for better performance.

The following benmarks has been tested:

For N in 1, 5, 10, 50, 100, 500, 1000, create N cgroups with one sleeping task in each, and then move an additional task through each cgroup in turn.

Here is a test result:

N Loop orig - Time(s) hash - Time(s)
----------------------------------------------
1 10000 1.201231728 1.196311177
5 2000 1.065743872 1.040566424
10 1000 0.991054735 0.986876440
50 200 0.976554203 0.969608733
100 100 0.998504680 0.969218270
500 20 1.157347764 0.962602963
1000 10 1.619521852 1.085140172

Signed-off-by Li Zefan <[email protected]>
Reviewed-by Paul Menage <[email protected]>
---
 include/linux/cgroup.h |    7 +++++-
 kernel/cgroup.c        |   59 ++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 53 insertions(+), 13 deletions(-)
include/linux/cgroup.h +6 -1
index 2d1d151..f585b7c 100644
@@ -156,6 +156,12 @@ struct css_set {
 	struct list_head list;
 
 	/*
+	 * List running through all cgroup groups in the same hash
+	 * slot. Protected by css_set_lock
+	 */
+	struct hlist_node hlist;
+
+	/*
 	 * List running through all tasks using this cgroup
 	 * group. Protected by css_set_lock
 	 */

@@ -174,7 +180,6 @@ struct css_set {
 	 * during subsystem registration (at boot time).
 	 */
 	struct cgroup_subsys_state *subsys[CGROUP_SUBSYS_COUNT];
-
 };
 
 /*
kernel/cgroup.c +47 -12

141 lines not on this page · raw message

Signature · 1 line
1.5.4.rc3
author
#8 lore raw
In reply to Li Zefan
Quoted 21 lines
Quoted 11 lines

kernel/cgroup.c static struct cg_cgroup_link init_css_set_link; · line 211

Quoted 9 lines
kernel/cgroup.c +9 -0
+static struct hlist_head *css_set_hash(struct cgroup_subsys_state *css[])
+{
+	int i;
+	int index;
+	unsigned long tmp = 0UL;
+
+	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++)
+		tmp += (unsigned long)css[i];
+

maybe css[i]'s lower 2-3 bits will be ignored. because thery are always 0.

Quoted 9 lines

And I don't like "+" for hash. how about
==

	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++)
	unsigned long x;
	x = (unsigned long)css[i];
	tmp = (x >> 16) ^ (x & 0xffff)

==
or some func, which uses full bits.

Quoted 5 lines

I'm using hash_long(), which has been proved to be a good one. And I've tested css_set_hash(), I run the css_set benchmark with N == 1000, the for loop in find_existing_css_set() will break out within 10 iterations for most cases, which is the expected result.

I just revised the css_set_hash() a bit, and It turned out we can improve the scalability.

Thanks, KAMEZAWA-san.

author Reviewed-by Paul Menage
#9 lore raw
In reply to Li Zefan

When we attach a process to a different cgroup, the css_set linked-list will be run through to find a suitable existing css_set to use. This patch implements a hash table for better performance.

The following benmarks has been tested:

For N in 1, 5, 10, 50, 100, 500, 1000, create N cgroups with one sleeping task in each, and then move an additional task through each cgroup in turn.

Here is a test result:

N Loop orig - Time(s) hash - Time(s)
----------------------------------------------
1 10000 1.201231728 1.196311177
5 2000 1.065743872 1.040566424
10 1000 0.991054735 0.986876440
50 200 0.976554203 0.969608733
100 100 0.998504680 0.969218270
500 20 1.157347764 0.962602963
1000 10 1.619521852 1.085140172

Signed-off-by Li Zefan <[email protected]>
Reviewed-by Paul Menage <[email protected]>
---
 include/linux/cgroup.h |    7 +++++-
 kernel/cgroup.c        |   59 ++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 53 insertions(+), 13 deletions(-)
include/linux/cgroup.h +6 -1
index 2d1d151..f585b7c 100644
@@ -156,6 +156,12 @@ struct css_set {
 	struct list_head list;
 
 	/*
+	 * List running through all cgroup groups in the same hash
+	 * slot. Protected by css_set_lock
+	 */
+	struct hlist_node hlist;
+
+	/*
 	 * List running through all tasks using this cgroup
 	 * group. Protected by css_set_lock
 	 */

@@ -174,7 +180,6 @@ struct css_set {
 	 * during subsystem registration (at boot time).
 	 */
 	struct cgroup_subsys_state *subsys[CGROUP_SUBSYS_COUNT];
-
 };
 
 /*
kernel/cgroup.c +47 -12

141 lines not on this page · raw message

Signature · 1 line
1.5.4.rc3

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

#11 lore raw

kernel/cgroup.c static struct cg_cgroup_link init_css_set_link; · line 201

Quoted 5 lines · On Thu, 03 Apr 2008 13:52:43 +0800 Li Zefan…
kernel/cgroup.c +5 -0
+/* hash table for cgroup groups. This improves the performance to
+ * find an existing css_set */
+#define CSS_SET_HASH_BITS	7
+#define CSS_SET_TABLE_SIZE	(1 << CSS_SET_HASH_BITS)
+static struct hlist_head css_set_table[CSS_SET_TABLE_SIZE];

How above number is selected ?

kernel/cgroup.c static struct cg_cgroup_link init_css_set_link; · line 211

Quoted 9 lines
kernel/cgroup.c +9 -0
+static struct hlist_head *css_set_hash(struct cgroup_subsys_state *css[])
+{
+	int i;
+	int index;
+	unsigned long tmp = 0UL;
+
+	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++)
+		tmp += (unsigned long)css[i];
+

maybe css[i]'s lower 2-3 bits will be ignored. because thery are always 0.

And I don't like "+" for hash. how about
==

	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++)
	unsigned long x;
	x = (unsigned long)css[i];
	tmp = (x >> 16) ^ (x & 0xffff)

==
or some func, which uses full bits.

Quoted 9 lines
kernel/cgroup.c +5 -0
+	index = hash_long(tmp, CSS_SET_HASH_BITS);
+
+	return &css_set_table[index];
+}
+
 /* We don't maintain the lists running through each css_set to its
  * task until after the first call to cgroup_iter_start(). This
  * reduces the fork()/exit() overhead for people who have cgroups

kernel/cgroup.c static int use_task_css_set_links; · line 245

Quoted 6 lines
kernel/cgroup.c +1 -0
@@ -219,6 +240,7 @@ static int use_task_css_set_links;
 static void unlink_css_set(struct css_set *cg)
 {
 	write_lock(&css_set_lock);
+	hlist_del(&cg->hlist);
 	list_del(&cg->list);
 	css_set_count--;

This css_set_lock is worth to be rwlock ?
how about per hashline spinlock ? (but per-hashline seems overkill..)

Thanks,
-Kame