cpuacct: VIRT_CPU_ACCOUNTING don't prevent percpu cputime count

Changelog:
since v2
- revert using percpu_counter_sum()

since v1
- use percpu_counter_sum() instead percpu_counter_read()

-------------------------------------
Subject: [PATCH v3] cpuacct: VIRT_CPU_ACCOUNTING don't prevent percpu cputime count

cpuacct_update_stats() is called at every tick updating. and it use percpu_counter for avoiding performance degression.

For archs which define VIRT_CPU_ACCOUNTING, every tick would result in >1000 units of cputime updates and since this is much much greater than percpu_batch_counter, we end up taking spinlock on every tick.

This patch change batch rule. now, any cpu can store "percpu_counter_bach * jiffies" cputime in per-cpu cache.
it mean this patch don't have behavior change if VIRT_CPU_ACCOUNTING=n.

Cc Bharata B Rao <[email protected]>

Cc Balaji Rao <[email protected]>

Cc Dhaval Giani <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Balbir Singh <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Martin Schwidefsky <[email protected]>

Signed-off-by KOSAKI Motohiro <[email protected]>

---
 kernel/sched.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: b/kernel/sched.c
===================================================================

Quoted 42 lines

Changelog:
since v2
- revert using percpu_counter_sum()

since v1
- use percpu_counter_sum() instead percpu_counter_read()

-------------------------------------
Subject: [PATCH v3] cpuacct: VIRT_CPU_ACCOUNTING don't prevent percpu cputime count

cpuacct_update_stats() is called at every tick updating. and it use percpu_counter for avoiding performance degression.

For archs which define VIRT_CPU_ACCOUNTING, every tick would result in >1000 units of cputime updates and since this is much much greater than percpu_batch_counter, we end up taking spinlock on every tick.

This patch change batch rule. now, any cpu can store "percpu_counter_bach * jiffies" cputime in per-cpu cache.
it mean this patch don't have behavior change if VIRT_CPU_ACCOUNTING=n.

Cc Bharata B Rao <[email protected]>

Cc Balaji Rao <[email protected]>

Cc Dhaval Giani <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Balbir Singh <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Martin Schwidefsky <[email protected]>

Signed-off-by KOSAKI Motohiro <[email protected]>

---
 kernel/sched.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: b/kernel/sched.c
===================================================================

kernel/sched.c struct cpuacct { · line 10224

Quoted 4 lines

kernel/sched.c +1 -0

@@ -10221,6 +10221,7 @@ struct cpuacct {
 };
 
 struct cgroup_subsys cpuacct_subsys;
+static s32 cpuacct_batch;

should be __read_mostly i guess.

Quoted 7 lines

kernel/sched.c +0 -0

 /* return cpu accounting group corresponding to this container */
 static inline struct cpuacct *cgroup_ca(struct cgroup *cgrp)

@@ -10250,6 +10251,9 @@ static struct cgroup_subsys_state *cpuac
 	if (!ca->cpuusage)
 		goto out_free_ca;
 

kernel/sched.c static struct cgroup_subsys_state *cpuac · line 10255

Quoted 2 lines

kernel/sched.c +2 -0

+	if (!cpuacct_batch)
+		cpuacct_batch = jiffies_to_cputime(percpu_counter_batch);

So this is a once per boot condition? Why not initialize it in sched_init() or so? (instead of incuring this ugly check all the time)

Index: b/kernel/sched.c
===================================================================

kernel/sched.c 2009-05-09 18:16:00.000000000 +0900 renamed from kernel/sched.c 2009-05-09 16:48:08.000000000 +0900 +0 -0

@@ -824,8 +824,12 @@ static struct file_operations sched_feat
        .release        = single_release,

kernel/sched.c 2009-05-09 18:16:00.000000000 +0900 renamed from kernel/sched.c 2009-05-09 16:48:08.000000000 +0900 +2 -1

@@ -10457,7 +10461,8 @@ static void cpuacct_update_stats(struct 
        ca = task_ca(tsk);
 
        do {
-               percpu_counter_add(&ca->cpustat[idx], val);
+               __percpu_counter_add(&ca->cpustat[idx], val, cpuacct_batch);
+
                ca = ca->parent;
        } while (ca);
        rcu_read_unlock();

Quoted 9 lines

Index: b/kernel/sched.c
===================================================================

kernel/sched.c 2009-05-09 18:16:00.000000000 +0900 renamed from kernel/sched.c 2009-05-09 16:48:08.000000000 +0900 +0 -0

@@ -824,8 +824,12 @@ static struct file_operations sched_feat
        .release        = single_release,
 };
 

kernel/sched.c 2009-05-09 18:16:00.000000000 +0900 static struct file_operations sched_feat · line 834

Quoted 8 lines

kernel/sched.c 2009-05-09 18:16:00.000000000 +0900 renamed from kernel/sched.c 2009-05-09 16:48:08.000000000 +0900 +4 -0

+static __read_mostly s32 cpuacct_batch;
+
 static __init int sched_init_debug(void)
 {
+       cpuacct_batch = jiffies_to_cputime(percpu_counter_batch);
+
        debugfs_create_file("sched_features", 0644, NULL, NULL,
                        &sched_feat_fops);

I'm pretty user you can select cpu accounting without SCHED_DEBUG...

Quoted 10 lines · KOSAKI Motohiro

Quoted 9 lines

Index: b/kernel/sched.c
===================================================================

kernel/sched.c 2009-05-09 18:16:00.000000000 +0900 renamed from kernel/sched.c 2009-05-09 16:48:08.000000000 +0900 +0 -0

@@ -824,8 +824,12 @@ static struct file_operations sched_feat
        .release        = single_release,
 };
 

Quoted 10 lines

kernel/sched.c 2009-05-09 18:16:00.000000000 +0900 static struct file_operations sched_feat · line 834

Quoted 8 lines

kernel/sched.c 2009-05-09 18:16:00.000000000 +0900 renamed from kernel/sched.c 2009-05-09 16:48:08.000000000 +0900 +4 -0

+static __read_mostly s32 cpuacct_batch;
+
 static __init int sched_init_debug(void)
 {
+       cpuacct_batch = jiffies_to_cputime(percpu_counter_batch);
+
        debugfs_create_file("sched_features", 0644, NULL, NULL,
                        &sched_feat_fops);

I'm pretty user you can select cpu accounting without SCHED_DEBUG...

Grr, yes, it's idiot ;)
I'll fix it soon.

Commit-ID:  50fbed3bb72cbe38a665512771a91a96d028de46
Gitweb:     http://git.kernel.org/tip/50fbed3bb72cbe38a665512771a91a96d028de46
Author:     KOSAKI Motohiro <[email protected]>
AuthorDate: Sat, 9 May 2009 19:14:58 +0900
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 11 May 2009 14:32:57 +0200

sched: cpuacct: Use bigger percpu counter batch values for stats counters

Quoted 8 lines

Commit-ID:  50fbed3bb72cbe38a665512771a91a96d028de46
Gitweb:     http://git.kernel.org/tip/50fbed3bb72cbe38a665512771a91a96d028de46
Author:     KOSAKI Motohiro <[email protected]>
AuthorDate: Sat, 9 May 2009 19:14:58 +0900
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 11 May 2009 14:32:57 +0200

Quoted 1 line

sched: cpuacct: Use bigger percpu counter batch values for stats counters

-tip testing found a build failure with this patch:

kernel/sched.c:9278: error: ‘percpu_counter_batch’ undeclared (first use in this function) kernel/sched.c:9278: error: (Each undeclared identifier is reported only once kernel/sched.c:9278: error: for each function it appears in.)

+#ifdef CONFIGCONFIG_SMP

Quoted 1 line

+#ifdef CONFIGCONFIG_SMP

typo ?

Quoted 4 lines · KOSAKI Motohiro

Quoted 1 line

+#ifdef CONFIGCONFIG_SMP

typo ?

yes, this week is my memorial stupid one ;)

Quoted 6 lines

Quoted 4 lines · KOSAKI Motohiro

Quoted 1 line

+#ifdef CONFIGCONFIG_SMP

typo ?

yes, this week is my memorial stupid one ;)

ok, assemble list indicate current patch have no typo ;)

ffffffff814ae920:       48 c7 40 30 10 eb 2a    movq   $0xffffffff812aeb10,0x30(%rax)
ffffffff814ae927:       81
ffffffff814ae928:       c7 05 3e 37 fe ff 01    movl   $0x1,-0x1c8c2(%rip)        # ffffffff81492070 <scheduler_running>
ffffffff814ae92f:       00 00 00
ffffffff814ae932:       8b 05 28 52 fe ff       mov    -0x1add8(%rip),%eax        # ffffffff81493b60 <percpu_counter_batch>
ffffffff814ae938:       89 05 36 37 fe ff       mov    %eax,-0x1c8ca(%rip)        # ffffffff81492074 <cpuacct_batch>
ffffffff814ae93e:       41 5c                   pop    %r12

---------------------------------------------------------
Subject: [PATCH] cpuacct: Use bigger percpu counter batch values for stats counters on archs that have VIRT_CPU_ACCOUNTING=y

percpu counters used to accumulate statistics in cpuacct controller use the default batch value [max(2*nr_cpus, 32)] which can be too small for archs that define VIRT_CPU_ACCOUNTING. In such archs, a tick could result in cputime updates in the range of thousands. As a result, cpuacct_update_stats() would end up acquiring the percpu counter spinlock on every tick which is not good for performance.

Let those architectures to have a bigger batch threshold so that percpu counter spinlock isn't taken on every tick. This change doesn't affect the archs which don't define VIRT_CPU_ACCOUNTING and they continue to have the default percpu counter batch value.

Cc Balaji Rao <[email protected]>

Cc Dhaval Giani <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Balbir Singh <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Martin Schwidefsky <[email protected]>

Signed-off-by Bharata B Rao <[email protected]>

Signed-off-by KOSAKI Motohiro <[email protected]>

---
 kernel/sched.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Quoted 9 lines

Cc Balaji Rao <[email protected]>

Cc Dhaval Giani <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Balbir Singh <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Martin Schwidefsky <[email protected]>

Signed-off-by Bharata B Rao <[email protected]>

Signed-off-by KOSAKI Motohiro <[email protected]>

Quoted 3 lines

---
 kernel/sched.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Please reuse the commit log formatting fixes i did when i applied your patch. (you should still have that commit notification email)

Quoted 10 lines · tip-bot for KOSAKI Motohiro

Quoted 8 lines

Commit-ID:  50fbed3bb72cbe38a665512771a91a96d028de46
Gitweb:     http://git.kernel.org/tip/50fbed3bb72cbe38a665512771a91a96d028de46
Author:     KOSAKI Motohiro <[email protected]>
AuthorDate: Sat, 9 May 2009 19:14:58 +0900
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 11 May 2009 14:32:57 +0200

Quoted 7 lines

Quoted 1 line

sched: cpuacct: Use bigger percpu counter batch values for stats counters

-tip testing found a build failure with this patch:

kernel/sched.c:9278: error: ?$B!Fpercpu_counter_batch?$B!G undeclared (first use in this function) kernel/sched.c:9278: error: (Each undeclared identifier is reported only once kernel/sched.c:9278: error: for each function it appears in.)

Oh, my fault again. Good catch!

Changelog:
since V5
- fix build error when UP

since V4
- move cpuacct_batch initialization into sched_init()

since V3
- rewirte patch description (thanks Bharata!)
- append read_mostly to cpuacct_batch
- cpuacct_batch is initialized by sched_init()

since v2
- revert using percpu_counter_sum()

since v1
- use percpu_counter_sum() instead percpu_counter_read()

---------------------------------------------------------
Subject: [PATCH v6] cpuacct: Use bigger percpu counter batch values for stats counters on archs that have VIRT_CPU_ACCOUNTING=y

percpu counters used to accumulate statistics in cpuacct controller use the default batch value [max(2*nr_cpus, 32)] which can be too small for archs that define VIRT_CPU_ACCOUNTING. In such archs, a tick could result in cputime updates in the range of thousands. As a result, cpuacct_update_stats() would end up acquiring the percpu counter spinlock on every tick which is not good for performance.

Let those architectures to have a bigger batch threshold so that percpu counter spinlock isn't taken on every tick. This change doesn't affect the archs which don't define VIRT_CPU_ACCOUNTING and they continue to have the default percpu counter batch value.

Cc Balaji Rao <[email protected]>

Cc Dhaval Giani <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Balbir Singh <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Martin Schwidefsky <[email protected]>

Signed-off-by Bharata B Rao <[email protected]>

Signed-off-by KOSAKI Motohiro <[email protected]>

---
 kernel/sched.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Index: b/kernel/sched.c
===================================================================

kernel/sched.c +2 -0

@@ -870,6 +870,8 @@ static __read_mostly int scheduler_runni
  */
 int sysctl_sched_rt_runtime = 950000;
 
+static __read_mostly s32 cpuacct_batch;
+
 static inline u64 global_rt_period(void)
 {
 	return (u64)sysctl_sched_rt_period * NSEC_PER_USEC;

kernel/sched.c void __init sched_init(void) · line 9291

Quoted 3 lines

kernel/sched.c +3 -0

+#ifdef CONFIGCONFIG_SMP
+	cpuacct_batch = jiffies_to_cputime(percpu_counter_batch);
+#endif

Slow down and compile patches before sending them out.. please. That is a basic expectation if you expect it to be merged.

Quoted 6 lines

kernel/sched.c void __init sched_init(void) · line 9291

Quoted 3 lines

kernel/sched.c +3 -0

+#ifdef CONFIGCONFIG_SMP
+	cpuacct_batch = jiffies_to_cputime(percpu_counter_batch);
+#endif

Slow down and compile patches before sending them out.. please. That is a basic expectation if you expect it to be merged.

Unfortunately, this mistake pass test successfully ;) it because cpuacct_batch=0 works even SMP.

Quoted 7 lines · KOSAKI Motohiro

Quoted 6 lines

kernel/sched.c void __init sched_init(void) · line 9291

Quoted 3 lines

kernel/sched.c +3 -0

+#ifdef CONFIGCONFIG_SMP
+	cpuacct_batch = jiffies_to_cputime(percpu_counter_batch);
+#endif

Slow down and compile patches before sending them out.. please. That is a basic expectation if you expect it to be merged.

Quoted 3 lines

Unfortunately, this mistake pass test successfully ;) it because cpuacct_batch=0 works even SMP.

OK, BTW, using an #ifdef right in the middle of a function makes the code harder to read, can't we use an inline function to abstract out SMP?

Quoted 14 lines · KOSAKI Motohiro

Quoted 11 lines

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +3 -0

+#ifdef CONFIGCONFIG_SMP
+	cpuacct_batch = jiffies_to_cputime(percpu_counter_batch);
+#endif

Slow down and compile patches before sending them out.. please. That is a basic expectation if you expect it to be merged.

Unfortunately, this mistake pass test successfully ;) it because cpuacct_batch=0 works even SMP.

Quoted 3 lines

OK, BTW, using an #ifdef right in the middle of a function makes the code harder to read, can't we use an inline function to abstract out SMP?

or rather, to make cpuacct_batch have a sane value on UP too. (1? 0?)

Quoted 2 lines

or rather, to make cpuacct_batch have a sane value on UP too. (1? 0?)

umm..

I've reviewed my patch again.
but sched_init() already has multiple #ifdef SMP. [...]

[...] Thus I don't think cosmetic changing improve readability largely.

Quoted 8 lines

Quoted 3 lines

or rather, to make cpuacct_batch have a sane value on UP too. (1? 0?)

umm..

I've reviewed my patch again.
but sched_init() already has multiple #ifdef SMP. [...]

Patches are welcome to remove more of them.

Quoted 2 lines

[...] Thus I don't think cosmetic changing improve readability largely.

an avoidable #ifdef should aways be avoided.

2009/5/12 Ingo Molnar <[email protected]>:

Quoted 11 lines · KOSAKI Motohiro

Quoted 8 lines

Quoted 3 lines

or rather, to make cpuacct_batch have a sane value on UP too. (1? 0?)

umm..

I've reviewed my patch again.
but sched_init() already has multiple #ifdef SMP. [...]

Quoted 6 lines

Patches are welcome to remove more of them.

Quoted 2 lines

[...] Thus I don't think cosmetic changing improve readability largely.

an avoidable #ifdef should aways be avoided.

ok. I'll fix it tommorow.

------------------------------------
Subject: [PATCH] sched: cpuacct: Use bigger percpu counter batch values for stats counters

percpu counters used to accumulate statistics in cpuacct controller use the default batch value [max(2*nr_cpus, 32)] which can be too small for archs that define VIRT_CPU_ACCOUNTING. In such archs, a tick could result in cputime updates in the range of thousands. As a result, cpuacct_update_stats() would end up acquiring the percpu counter spinlock on every tick which is not good for performance.

Let those architectures to have a bigger batch threshold so that percpu counter spinlock isn't taken on every tick. This change doesn't affect the archs which don't define VIRT_CPU_ACCOUNTING and they continue to have the default percpu counter batch value.

v7:
- fix typo and changelog

v6:
- fix build error when UP

v5:
- move cpuacct_batch initialization into sched_init()

v4:
- rewrite patch description (thanks Bharata!)
- append read_mostly to cpuacct_batch
- cpuacct_batch is initialized by sched_init_debug()

v3:
- revert using percpu_counter_sum()

v2:
- use percpu_counter_sum() instead percpu_counter_read()

Cc Balaji Rao <[email protected]>

Cc Dhaval Giani <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Balbir Singh <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Martin Schwidefsky <[email protected]>

Signed-off-by Bharata B Rao <[email protected]>

Signed-off-by KOSAKI Motohiro <[email protected]>

---
 kernel/sched.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Index: b/kernel/sched.c
===================================================================

kernel/sched.c +2 -0

@@ -870,6 +870,8 @@ static __read_mostly int scheduler_runni
  */
 int sysctl_sched_rt_runtime = 950000;
 
+static __read_mostly s32 cpuacct_batch;
+
 static inline u64 global_rt_period(void)
 {
 	return (u64)sysctl_sched_rt_period * NSEC_PER_USEC;

On ppc64, calling jiffies_to_cputime() from sched_init() is too early because jiffies_to_cputime() needs tb_ticks_per_sec which gets initialized only later in time_init(). Because of this I see that cpuacct_batch will always be zero effectively negating what this patch is trying to do.

As explained by you earlier, we too are finding the default batch value to be too low for ppc64 with VIRT_CPU_ACCOUNTING turned on. Hence I guess if this patch is taken in (ofcourse with the above issue fixed), it will benefit ppc64 also.

--

When CONFIG_VIRT_CPU_ACCOUNTING is enabled we can call cpuacct_update_stats with values much larger than percpu_counter_batch. This means the call to percpu_counter_add will always add to the global count which is protected by a spinlock.

Since reading of the CPU accounting cgroup counters is not performance critical, we can use a maximum size batch of INT_MAX and use percpu_counter_sum on the read side which will add all the percpu counters.

With this patch an 8 core POWER6 with CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT shows an improvement in aggregate context switch rate of 397k/sec to 3.9M/sec, a 10x improvement.

Signed-off-by Anton Blanchard <[email protected]>

---

Index: linux.trees.git/kernel/sched.c
===================================================================

kernel/sched.c +2 -2

@@ -10551,7 +10551,7 @@
 	int i;
 
 	for (i = 0; i < CPUACCT_STAT_NSTATS; i++) {
-		s64 val = percpu_counter_read(&ca->cpustat[i]);
+		s64 val = percpu_counter_sum(&ca->cpustat[i]);
 		val = cputime64_to_clock_t(val);
 		cb->fill(cb, cpuacct_stat_desc[i], val);
 	}

@@ -10621,7 +10621,7 @@
 	ca = task_ca(tsk);
 
 	do {
-		percpu_counter_add(&ca->cpustat[idx], val);
+		__percpu_counter_add(&ca->cpustat[idx], val, INT_MAX);
 		ca = ca->parent;
 	} while (ca);
 	rcu_read_unlock();

Hi,

Looks like this issue is still present. I tested on a 32 core box and the patch improved maximum context switch rate from from 76k/sec to 9.5M/sec. Thats over 100x faster, or 50x per line of code. That's got to be some sort of record :)

Any chance we can get a fix in for 2.6.31? Don't make me find an even bigger box so I can break the 200x mark :)

Anton

Quoted 41 lines

--

When CONFIG_VIRT_CPU_ACCOUNTING is enabled we can call cpuacct_update_stats with values much larger than percpu_counter_batch. This means the call to percpu_counter_add will always add to the global count which is protected by a spinlock.

Since reading of the CPU accounting cgroup counters is not performance critical, we can use a maximum size batch of INT_MAX and use percpu_counter_sum on the read side which will add all the percpu counters.

With this patch an 8 core POWER6 with CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT shows an improvement in aggregate context switch rate of 397k/sec to 3.9M/sec, a 10x improvement.

Signed-off-by Anton Blanchard <[email protected]>

---

Index: linux.trees.git/kernel/sched.c
===================================================================

kernel/sched.c +2 -2

@@ -10551,7 +10551,7 @@
 	int i;
 
 	for (i = 0; i < CPUACCT_STAT_NSTATS; i++) {
-		s64 val = percpu_counter_read(&ca->cpustat[i]);
+		s64 val = percpu_counter_sum(&ca->cpustat[i]);
 		val = cputime64_to_clock_t(val);
 		cb->fill(cb, cpuacct_stat_desc[i], val);
 	}

@@ -10621,7 +10621,7 @@
 	ca = task_ca(tsk);
 
 	do {
-		percpu_counter_add(&ca->cpustat[idx], val);
+		__percpu_counter_add(&ca->cpustat[idx], val, INT_MAX);
 		ca = ca->parent;
 	} while (ca);
 	rcu_read_unlock();

Hi,

Looks like this issue is still present. I tested on a 32 core box and the patch improved maximum context switch rate from from 76k/sec to 9.5M/sec. Thats over 100x faster, or 50x per line of code. That's got to be some sort of record :)

Any chance we can get a fix in for 2.6.31? Don't make me find an even bigger box so I can break the 200x mark :)

Anton

Quoted 16 lines

--

When CONFIG_VIRT_CPU_ACCOUNTING is enabled we can call cpuacct_update_stats with values much larger than percpu_counter_batch. This means the call to percpu_counter_add will always add to the global count which is protected by a spinlock.

Since reading of the CPU accounting cgroup counters is not performance critical, we can use a maximum size batch of INT_MAX and use percpu_counter_sum on the read side which will add all the percpu counters.

With this patch an 8 core POWER6 with CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT shows an improvement in aggregate context switch rate of 397k/sec to 3.9M/sec, a 10x improvement.

Quoted 25 lines

Signed-off-by Anton Blanchard <[email protected]>

---

Index: linux.trees.git/kernel/sched.c
===================================================================

kernel/sched.c +2 -2

@@ -10551,7 +10551,7 @@
 	int i;
 
 	for (i = 0; i < CPUACCT_STAT_NSTATS; i++) {
-		s64 val = percpu_counter_read(&ca->cpustat[i]);
+		s64 val = percpu_counter_sum(&ca->cpustat[i]);
 		val = cputime64_to_clock_t(val);
 		cb->fill(cb, cpuacct_stat_desc[i], val);
 	}

@@ -10621,7 +10621,7 @@
 	ca = task_ca(tsk);
 
 	do {
-		percpu_counter_add(&ca->cpustat[idx], val);
+		__percpu_counter_add(&ca->cpustat[idx], val, INT_MAX);
 		ca = ca->parent;
 	} while (ca);
 	rcu_read_unlock();

Looks good overall, why not keep the batch size conditional on CONFIG_VIRT_CPU_ACCOUNTING? I'd still like to stick with percpu_counter_read() on the read side because My concern is that a bad user space application can read cpuacct.stat and bring the kernel to its knees.

Quoted 25 lines

Quoted 25 lines

Signed-off-by Anton Blanchard <[email protected]>

---

Index: linux.trees.git/kernel/sched.c
===================================================================

kernel/sched.c +2 -2

@@ -10551,7 +10551,7 @@
 	int i;
 
 	for (i = 0; i < CPUACCT_STAT_NSTATS; i++) {
-		s64 val = percpu_counter_read(&ca->cpustat[i]);
+		s64 val = percpu_counter_sum(&ca->cpustat[i]);
 		val = cputime64_to_clock_t(val);
 		cb->fill(cb, cpuacct_stat_desc[i], val);
 	}

@@ -10621,7 +10621,7 @@
 	ca = task_ca(tsk);
 
 	do {
-		percpu_counter_add(&ca->cpustat[idx], val);
+		__percpu_counter_add(&ca->cpustat[idx], val, INT_MAX);
 		ca = ca->parent;
 	} while (ca);
 	rcu_read_unlock();

Agreed, the first hunk is dangerous, on the second hunk, I'm not sure about the INT_MAX thing, that's a 4s gap per cpu, that might be a tad much.

Hi,

Quoted 3 lines

Agreed, the first hunk is dangerous, on the second hunk, I'm not sure about the INT_MAX thing, that's a 4s gap per cpu, that might be a tad much.

Yeah I somehow missed the fact we read it every timer tick. I was hoping for an easy fix but I guess we just need to set a reasonable batch size at runtime.

Anton

Could you share contex-switch-test program ?
I'd like to play with it to find out what I can do against percpu counter.

Hi,

Quoted 2 lines

Could you share contex-switch-test program ?
I'd like to play with it to find out what I can do against percpu counter.

Sure:

http://ozlabs.org/~anton/junkcode/context_switch.c

Very simple, just run it once per core:

for i in seq 0 31
do
taskset -c $i ./context_switch &
done

Then look at the context switch rates in vmstat.

Quoted 10 lines · On Thu, 20 Aug 2009 16:24:51 +1000 Anton Blanchard <[email protected]> wrote:

Hi,

Quoted 2 lines

Could you share contex-switch-test program ?
I'd like to play with it to find out what I can do against percpu counter.

Sure:

http://ozlabs.org/~anton/junkcode/context_switch.c

Quoted 9 lines

Very simple, just run it once per core:

for i in seq 0 31
do
taskset -c $i ./context_switch &
done

Then look at the context switch rates in vmstat.

Thank you for test program.

Before adjusting batch counter (I think you should modify it), Could you try this ?

I only have 8cpu(2socket) host but works well. (But...my host is x86-64 and has not virt-cpu-accouting.)

with your program
before patch.
cpuacct off : 414000-416000 ctsw per sec.
cpuacct on : 401000-404000 ctsw per sec.

after patch
cpuacct on : 412000-413000 ctsw per sec.

Maybe I should check cache-miss late ;)

Quoted 8 lines · KAMEZAWA Hiroyuki

with your program
before patch.
cpuacct off : 414000-416000 ctsw per sec.
cpuacct on : 401000-404000 ctsw per sec.

after patch
cpuacct on : 412000-413000 ctsw per sec.

Quoted 1 line

Maybe I should check cache-miss late ;)

Btw., in latest upstream you can do that via:

cd tools/perf/
make -j install

  perf stat --repeat 5 -- taskset -c 1 ./context_switch

Quoted 13 lines · On Thu, 20 Aug 2009 12:04:03 +0200 Ingo Molnar <[email protected]> wrote:

Quoted 9 lines · KAMEZAWA Hiroyuki

with your program
before patch.
cpuacct off : 414000-416000 ctsw per sec.
cpuacct on : 401000-404000 ctsw per sec.

after patch
cpuacct on : 412000-413000 ctsw per sec.

Maybe I should check cache-miss late ;)

Btw., in latest upstream you can do that via:

Quoted 5 lines

cd tools/perf/
make -j install

  perf stat --repeat 5 -- taskset -c 1 ./context_switch

tried. (on 8cpu/2socket host). It seems cache-miss decreases. But IPC ..?

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 include/linux/percpu_counter.h |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Index: linux-2.6.31-rc6/include/linux/percpu_counter.h
===================================================================

include/linux/percpu_counter.h +8 -2

+#define CACHELINE_PADDING(name)  struct __percpu_counter_padding name
 
 struct percpu_counter {
+	/*
+	 * This pointer is persistent and accessed firstly.
+	 * Then, should not be purged by locking in other cpus.
+	 */
+	s32 *counters;
+	CACHELINE_PADDING(pad);
 	spinlock_t lock;
 	s64 count;
 #ifdef CONFIG_HOTPLUG_CPU
+	/* rarely accessed field */
 	struct list_head list;	/* All percpu_counters are on a list */
 #endif
-	s32 *counters;
 };
 
 extern int percpu_counter_batch;
 
 
--

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/

Hi,

Quoted 2 lines

Could you share contex-switch-test program ?
I'd like to play with it to find out what I can do against percpu counter.

Sure:

http://ozlabs.org/~anton/junkcode/context_switch.c

Very simple, just run it once per core:

for i in seq 0 31
do
taskset -c $i ./context_switch &
done

Then look at the context switch rates in vmstat.

Hi,

Another try at this percpu_counter batch issue with CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT enabled. Thoughts?

--

When CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT are enabled we can call cpuacct_update_stats with values much larger than percpu_counter_batch. This means the call to percpu_counter_add will always add to the global count which is protected by a spinlock and we end up with a global spinlock in the scheduler.

Based on an idea by KOSAKI Motohiro, this patch scales the batch value by cputime_one_jiffy such that we have the same batch limit as we would if CONFIG_VIRT_CPU_ACCOUNTING was disabled. His patch did this once at boot but that initialisation happened too early on PowerPC (before time_init) and it was never updated at runtime as a result of a hotplug cpu add/remove.

This patch instead scales percpu_counter_batch by cputime_one_jiffy at runtime, which keeps the batch correct even after cpu hotplug operations. We cap it at INT_MAX in case of overflow.

For architectures that do not support CONFIG_VIRT_CPU_ACCOUNTING, cputime_one_jiffy is the constant 1 and gcc is smart enough to optimise min(s32 percpu_counter_batch, INT_MAX) to just percpu_counter_batch at least on x86 and PowerPC. So there is no need to add an #ifdef.

On a 64 thread PowerPC box with CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT enabled, a context switch microbenchmark is 234x faster and almost matches a CONFIG_CGROUP_CPUACCT disabled kernel:

CONFIG_CGROUP_CPUACCT disabled: 16906698 ctx switches/sec
CONFIG_CGROUP_CPUACCT enabled: 61720 ctx switches/sec
CONFIG_CGROUP_CPUACCT + patch: 16663217 ctx switches/sec

Tested with:

wget http://ozlabs.org/~anton/junkcode/context_switch.c
make context_switch
for i in seq 0 63; do taskset -c $i ./context_switch & done vmstat 1

Signed-off-by Anton Blanchard <[email protected]>

---

Note: ccing ia64 and s390 who have not yet added code to statically initialise cputime_one_jiffy at boot.
See a42548a18866e87092db93b771e6c5b060d78401 (cputime: Optimize jiffies_to_cputime(1) for details). Adding this would help optimise not only this patch but many other areas of the scheduler when CONFIG_VIRT_CPU_ACCOUNTING is enabled.

Index: linux.trees.git/kernel/sched.c
===================================================================

kernel/sched.c +3 -1

@@ -10894,6 +10894,7 @@ static void cpuacct_update_stats(struct 
 		enum cpuacct_stat_index idx, cputime_t val)
 {
 	struct cpuacct *ca;
+	int batch;
 
 	if (unlikely(!cpuacct_subsys.active))
 		return;

@@ -10901,8 +10902,9 @@ static void cpuacct_update_stats(struct 
 	rcu_read_lock();
 	ca = task_ca(tsk);
 
+	batch = min_t(long, percpu_counter_batch * cputime_one_jiffy, INT_MAX);
 	do {
-		percpu_counter_add(&ca->cpustat[idx], val);
+		__percpu_counter_add(&ca->cpustat[idx], val, batch);
 		ca = ca->parent;
 	} while (ca);
 	rcu_read_unlock();

Hi,

Another try at this percpu_counter batch issue with CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT enabled. Thoughts?

Index: linux.trees.git/kernel/sched.c
===================================================================

kernel/sched.c +3 -1

@@ -10894,6 +10894,7 @@ static void cpuacct_update_stats(struct 
 		enum cpuacct_stat_index idx, cputime_t val)
 {
 	struct cpuacct *ca;
+	int batch;
 
 	if (unlikely(!cpuacct_subsys.active))
 		return;

@@ -10901,8 +10902,9 @@ static void cpuacct_update_stats(struct 
 	rcu_read_lock();
 	ca = task_ca(tsk);
 
+	batch = min_t(long, percpu_counter_batch * cputime_one_jiffy, INT_MAX);
 	do {
-		percpu_counter_add(&ca->cpustat[idx], val);
+		__percpu_counter_add(&ca->cpustat[idx], val, batch);
 		ca = ca->parent;
 	} while (ca);
 	rcu_read_unlock();

Seems like a good idea, but isn't that batch number rather static? If so, computing it in some init path would save that multiply on the actual accounting path.

Hi Peter,

Quoted 3 lines

Seems like a good idea, but isn't that batch number rather static? If so, computing it in some init path would save that multiply on the actual accounting path.

It is mostly static, but percpu_counter_batch does change with hotplug operations. Adding a hotplug notifier felt like a lot of work but I was worried we have issues if we didn't scale with hotplug add operations.

Hi,

Another try at this percpu_counter batch issue with CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT enabled. Thoughts?

--

When CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT are enabled we can call cpuacct_update_stats with values much larger than percpu_counter_batch. This means the call to percpu_counter_add will always add to the global count which is protected by a spinlock and we end up with a global spinlock in the scheduler.

Based on an idea by KOSAKI Motohiro, this patch scales the batch value by cputime_one_jiffy such that we have the same batch limit as we would if CONFIG_VIRT_CPU_ACCOUNTING was disabled. His patch did this once at boot but that initialisation happened too early on PowerPC (before time_init) and it was never updated at runtime as a result of a hotplug cpu add/remove.

This patch instead scales percpu_counter_batch by cputime_one_jiffy at runtime, which keeps the batch correct even after cpu hotplug operations. We cap it at INT_MAX in case of overflow.

For architectures that do not support CONFIG_VIRT_CPU_ACCOUNTING, cputime_one_jiffy is the constant 1 and gcc is smart enough to optimise min(s32 percpu_counter_batch, INT_MAX) to just percpu_counter_batch at least on x86 and PowerPC. So there is no need to add an #ifdef.

On a 64 thread PowerPC box with CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT enabled, a context switch microbenchmark is 234x faster and almost matches a CONFIG_CGROUP_CPUACCT disabled kernel:

CONFIG_CGROUP_CPUACCT disabled: 16906698 ctx switches/sec
CONFIG_CGROUP_CPUACCT enabled: 61720 ctx switches/sec
CONFIG_CGROUP_CPUACCT + patch: 16663217 ctx switches/sec

Tested with:

wget http://ozlabs.org/~anton/junkcode/context_switch.c
make context_switch
for i in seq 0 63; do taskset -c $i ./context_switch & done vmstat 1

Signed-off-by Anton Blanchard <[email protected]>

---

Note: ccing ia64 and s390 who have not yet added code to statically initialise cputime_one_jiffy at boot.
See a42548a18866e87092db93b771e6c5b060d78401 (cputime: Optimize jiffies_to_cputime(1) for details). Adding this would help optimise not only this patch but many other areas of the scheduler when CONFIG_VIRT_CPU_ACCOUNTING is enabled.

Index: linux.trees.git/kernel/sched.c
===================================================================

kernel/sched.c +1 -0

@@ -10894,6 +10894,7 @@ static void cpuacct_update_stats(struct 
 		enum cpuacct_stat_index idx, cputime_t val)
 {
 	struct cpuacct *ca;
+	int batch;
 
 	if (unlikely(!cpuacct_subsys.active))
 		return;

Note: ccing ia64 and s390 who have not yet added code to statically initialise cputime_one_jiffy at boot.
See a42548a18866e87092db93b771e6c5b060d78401 (cputime: Optimize jiffies_to_cputime(1) for details). Adding this would help optimise not only this patch but many other areas of the scheduler when CONFIG_VIRT_CPU_ACCOUNTING is enabled.

Index: linux.trees.git/kernel/sched.c
===================================================================

kernel/sched.c +1 -0

@@ -10894,6 +10894,7 @@ static void cpuacct_update_stats(struct 
 		enum cpuacct_stat_index idx, cputime_t val)
 {
 	struct cpuacct *ca;
+	int batch;
 
 	if (unlikely(!cpuacct_subsys.active))
 		return;

For s390 the jiffies_to_cputime is a compile time constant. No need to initialize it at runtime, no?

The patch itself trades some accuracy (larger cpu accounting value that are stored per-cpu) against runtime overhead (spinlock to transfer the value to the global variable in __percpu_counter_add). Did you calculate how big the loss in accuracy is?

Hi Martin,

Quoted 2 lines

For s390 the jiffies_to_cputime is a compile time constant. No need to initialize it at runtime, no?

Indeed it is, I didn't look closely enough. Same with ia64 so no work to do on either arch :)

Quoted 4 lines

The patch itself trades some accuracy (larger cpu accounting value that are stored per-cpu) against runtime overhead (spinlock to transfer the value to the global variable in __percpu_counter_add). Did you calculate how big the loss in accuracy is?

The idea is we are already batching percpu_counter_batch jiffies, so with CONFIG_VIRT_CPU_ACCOUNTING we batch the equivalent amount in cputime.

Hi Martin,

Quoted 2 lines

For s390 the jiffies_to_cputime is a compile time constant. No need to initialize it at runtime, no?

Indeed it is, I didn't look closely enough. Same with ia64 so no work to do on either arch :)

Quoted 4 lines

The patch itself trades some accuracy (larger cpu accounting value that are stored per-cpu) against runtime overhead (spinlock to transfer the value to the global variable in __percpu_counter_add). Did you calculate how big the loss in accuracy is?

The idea is we are already batching percpu_counter_batch jiffies, so with CONFIG_VIRT_CPU_ACCOUNTING we batch the equivalent amount in cputime.

Could we please pick up the patch for -tip?

When CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT are enabled we can call cpuacct_update_stats with values much larger than percpu_counter_batch. This means the call to percpu_counter_add will always add to the global count which is protected by a spinlock and we end up with a global spinlock in the scheduler.

Quoted 5 lines · On Mon, 18 Jan 2010 15:41:42 +1100 Anton Blanchard <[email protected]> wrote:

When CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT are enabled we can call cpuacct_update_stats with values much larger than percpu_counter_batch. This means the call to percpu_counter_add will always add to the global count which is protected by a spinlock and we end up with a global spinlock in the scheduler.

When one looks at the end result:

Quoted 5 lines · On Mon, 18 Jan 2010 15:41:42 +1100 Anton Blanchard <[email protected]> wrote:

When CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT are enabled we can call cpuacct_update_stats with values much larger than percpu_counter_batch. This means the call to percpu_counter_add will always add to the global count which is protected by a spinlock and we end up with a global spinlock in the scheduler.

When one looks at the end result:

: static void cpuacct_update_stats(struct task_struct *tsk,
: enum cpuacct_stat_index idx, cputime_t val)
: {
: struct cpuacct *ca;
: int batch;
:
: if (unlikely(!cpuacct_subsys.active))
: return;
:
: rcu_read_lock();
: ca = task_ca(tsk);
:
: batch = min_t(long, percpu_counter_batch * cputime_one_jiffy, INT_MAX);
: do {
: __percpu_counter_add(&ca->cpustat[idx], val, batch);
: ca = ca->parent;
: } while (ca);
: rcu_read_unlock();
: }

the code (which used to be quite obvious) becomes pretty unobvious. In fact it looks quite wrong.

Shouldn't there be a comment there explaining wtf is going on?

Quoted 31 lines · Andrew Morton

Quoted 5 lines · On Mon, 18 Jan 2010 15:41:42 +1100 Anton Blanchard <[email protected]> wrote:

When CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT are enabled we can call cpuacct_update_stats with values much larger than percpu_counter_batch. This means the call to percpu_counter_add will always add to the global count which is protected by a spinlock and we end up with a global spinlock in the scheduler.

When one looks at the end result:

: static void cpuacct_update_stats(struct task_struct *tsk,
: enum cpuacct_stat_index idx, cputime_t val)
: {
: struct cpuacct *ca;
: int batch;
:
: if (unlikely(!cpuacct_subsys.active))
: return;
:
: rcu_read_lock();
: ca = task_ca(tsk);
:
: batch = min_t(long, percpu_counter_batch * cputime_one_jiffy, INT_MAX);
: do {
: __percpu_counter_add(&ca->cpustat[idx], val, batch);
: ca = ca->parent;
: } while (ca);
: rcu_read_unlock();
: }

Quoted 4 lines

the code (which used to be quite obvious) becomes pretty unobvious. In fact it looks quite wrong.

Shouldn't there be a comment there explaining wtf is going on?

Andrew,

I guess a lot of the changelog and comments are in the email history,

Why does it look quite wrong to you?

Quoted 1 line

I guess a lot of the changelog and comments are in the email history,

Not a very useful location for it!

Quoted 1 line

Why does it look quite wrong to you?

Because it computes the correct value and then if it's larger than INT_MAX, it inexplicably assigns INT_MAX to it, giving a wrong result!

Does that code actually work, btw? percpu_counter_batch has type int' and cputime_one_jiffy has type int' so their product has type `int'. So by the time min_t performs its comparison, the upper 32 bits of the product are already lost.

Commit-ID:  43f85eb1411905afe5db510fbf9841b516e7e6a
Gitweb:     http://git.kernel.org/tip/43f85eab1411905afe5db510fbf9841b516e7e6a
Author:     Anton Blanchard <[email protected]>
AuthorDate: Mon, 18 Jan 2010 15:41:42 +1100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 27 Jan 2010 08:34:38 +0100

sched: cpuacct: Use bigger percpu counter batch values for stats counters

When CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT are enabled we can call cpuacct_update_stats with values much larger than percpu_counter_batch. This means the call to percpu_counter_add will always add to the global count which is protected by a spinlock and we end up with a global spinlock in the scheduler.

Based on an idea by KOSAKI Motohiro, this patch scales the batch value by cputime_one_jiffy such that we have the same batch limit as we would if CONFIG_VIRT_CPU_ACCOUNTING was disabled. His patch did this once at boot but that initialisation happened too early on PowerPC (before time_init) and it was never updated at runtime as a result of a hotplug cpu add/remove.

This patch instead scales percpu_counter_batch by cputime_one_jiffy at runtime, which keeps the batch correct even after cpu hotplug operations. We cap it at INT_MAX in case of overflow.

For architectures that do not support CONFIG_VIRT_CPU_ACCOUNTING, cputime_one_jiffy is the constant 1 and gcc is smart enough to optimise min(s32 percpu_counter_batch, INT_MAX) to just percpu_counter_batch at least on x86 and PowerPC. So there is no need to add an #ifdef.

On a 64 thread PowerPC box with CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT enabled, a context switch microbenchmark is 234x faster and almost matches a CONFIG_CGROUP_CPUACCT disabled kernel:

CONFIG_CGROUP_CPUACCT disabled:         16906698 ctx switches/sec CONFIG_CGROUP_CPUACCT enabled:             61720 ctx switches/sec CONFIG_CGROUP_CPUACCT + patch:          16663217 ctx switches/sec

Tested with:

 wget http://ozlabs.org/~anton/junkcode/context_switch.c
 make context_switch
 for i in seq 0 63; do taskset -c $i ./context_switch & done vmstat 1

Signed-off-by Anton Blanchard <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

LKML-Reference: <20100118044142.GS12666@kryten>

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/sched.c |    4 +++-

 1 files changed, 3 insertions(+), 1 deletions(-)

kernel/sched.c +1 -0

index 3a8fb30..8f94138 100644
@@ -10906,6 +10906,7 @@ static void cpuacct_update_stats(struct task_struct *tsk,
                enum cpuacct_stat_index idx, cputime_t val)
  {
        struct cpuacct *ca;
+       int batch;
 
        if (unlikely(!cpuacct_subsys.active))
                return;

Quoted 70 lines · tip-bot for Anton Blanchard

Commit-ID: __43f85eb1411905afe5db510fbf9841b516e7e6a
Gitweb: __ __ http://git.kernel.org/tip/43f85eab1411905afe5db510fbf9841b516e7e6a
Author: __ __ Anton Blanchard <[email protected]>
AuthorDate: Mon, 18 Jan 2010 15:41:42 +1100
Committer: __Ingo Molnar <[email protected]>
CommitDate: Wed, 27 Jan 2010 08:34:38 +0100

sched: cpuacct: Use bigger percpu counter batch values for stats counters

When CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT are enabled we can call cpuacct_update_stats with values much larger than percpu_counter_batch. This means the call to percpu_counter_add will always add to the global count which is protected by a spinlock and we end up with a global spinlock in the scheduler.

Based on an idea by KOSAKI Motohiro, this patch scales the batch value by cputime_one_jiffy such that we have the same batch limit as we would if CONFIG_VIRT_CPU_ACCOUNTING was disabled. His patch did this once at boot but that initialisation happened too early on PowerPC (before time_init) and it was never updated at runtime as a result of a hotplug cpu add/remove.

This patch instead scales percpu_counter_batch by cputime_one_jiffy at runtime, which keeps the batch correct even after cpu hotplug operations. We cap it at INT_MAX in case of overflow.

For architectures that do not support CONFIG_VIRT_CPU_ACCOUNTING, cputime_one_jiffy is the constant 1 and gcc is smart enough to optimise min(s32 percpu_counter_batch, INT_MAX) to just percpu_counter_batch at least on x86 and PowerPC. So there is no need to add an #ifdef.

On a 64 thread PowerPC box with CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT enabled, a context switch microbenchmark is 234x faster and almost matches a CONFIG_CGROUP_CPUACCT disabled kernel:

CONFIG_CGROUP_CPUACCT disabled: __ __ __ __ 16906698 ctx switches/sec CONFIG_CGROUP_CPUACCT enabled: __ __ __ __ __ __ 61720 ctx switches/sec CONFIG_CGROUP_CPUACCT + patch: __ __ __ __ __16663217 ctx switches/sec

Tested with:

__wget http://ozlabs.org/~anton/junkcode/context_switch.c
__make context_switch
__for i in seq 0 63; do taskset -c $i ./context_switch & done __vmstat 1

Signed-off-by Anton Blanchard <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

LKML-Reference: <20100118044142.GS12666@kryten>

Signed-off-by Ingo Molnar <[email protected]>

---
__kernel/sched.c | __ __4 +++-
__1 files changed, 3 insertions(+), 1 deletions(-)

kernel/sched.c +1 -0

index 3a8fb30..8f94138 100644
@@ -10906,6 +10906,7 @@ static void cpuacct_update_stats(struct task_struct *tsk,
 __ __ __ __ __ __ __ __enum cpuacct_stat_index idx, cputime_t val)
 __{
 __ __ __ __struct cpuacct *ca;
+ __ __ __ int batch;
 
 __ __ __ __if (unlikely(!cpuacct_subsys.active))
 __ __ __ __ __ __ __ __return;

@@ -10913,8 +10914,9 @@ static void cpuacct_update_stats(struct task_struct *tsk,
 __ __ __ __rcu_read_lock();
 __ __ __ __ca = task_ca(tsk);
 

kernel/sched.c static void cpuacct_update_stats(struct task_struct *tsk, · line 10922

Quoted 7 lines

kernel/sched.c +2 -1

+ __ __ __ batch = min_t(long, percpu_counter_batch * cputime_one_jiffy, INT_MAX);
 __ __ __ __do {
- __ __ __ __ __ __ __ percpu_counter_add(&ca->cpustat[idx], val);
+ __ __ __ __ __ __ __ __percpu_counter_add(&ca->cpustat[idx], val, batch);
 __ __ __ __ __ __ __ __ca = ca->parent;
 __ __ __ __} while (ca);
 __ __ __ __rcu_read_unlock();

IIRC, Andrew picked up this patch as well and applied some checkpatch fixes too..

Last I heard, Anton was "working on a useful comment" and will be redoing the patch.

Quoted 3 lines

Quoted 3 lines

__ __ __ __ __ __ __ __ca = ca->parent;
__ __ __ __} while (ca);
__ __ __ __rcu_read_unlock();

^^ your email client inexplicably fills emails with 0xa0

Quoted 2 lines

IIRC, Andrew picked up this patch as well and applied some checkpatch fixes too..

No, I have no changes.

Last I heard, Anton was "working on a useful comment" and will be redoing the patch.