Re: [PATCH 2/2] scheduler: cgroups cpuaccouting: Make cpuusage atomic

From: Thomas Renninger
Date: Thu May 20 2010 - 06:52:10 EST

On Wednesday 19 May 2010 21:02:23 Peter Zijlstra wrote:
> On Wed, 2010-05-19 at 20:58 +0200, Thomas Renninger wrote:
> > and avoid locking on 32 bit.
> > This resolves an ugly dependency in cgroups_cpuaccount.c
> > to per_cpu runqueues lock variable.
> While I totally agree with the sentiments here, atomic64_t isn't
> available on all 32bit archs and is _terribly_ expensive on ARCH=i386.

Please ignore the 2nd patch.
If nobody objects about the creation of kernel/sched.h which should get
used for further cleanups IMO, the first patch should get in.
I thought a bit about it, but taking the per_cpu rq lock is the most sane
thing I could think of.

Here is a fix (bug always was there), for "on top" patching of the first.
It should also be possible to fix this by exchanging:
CONFIG_64BIT to CONFIG_X86_64, but removing the #ifdefs should
be preferred over a lock that is only grabbed via sysfs access...



scheduler: cgroup_cpuaccount - Fix race on cpuusage

A u64 access is not atomic on all 64 bit archs.
Especially this:
*cpuusage += cputime;
should not necessarily be atomic on non X86_64 archs.
As cpuacct_cpuusage_{read,write} is only used during
sysfs access, the lock should be acquired rather rarely
and the code is nicer as well.

Signed-off-by: Thomas Renninger <trenn@xxxxxxx>
CC: linux-kernel@xxxxxxxxxxxxxxx
CC: mike@xxxxxxxxxxx
CC: menage@xxxxxxxxxx
CC: lizf@xxxxxxxxxxxxxx
CC: containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
CC: mingo@xxxxxxx
CC: peterz@xxxxxxxxxxxxx

diff --git a/kernel/cgroup_cpuaccount.c b/kernel/cgroup_cpuaccount.c
index 0ad356a..a9e0345 100644
--- a/kernel/cgroup_cpuaccount.c
+++ b/kernel/cgroup_cpuaccount.c
@@ -95,16 +95,12 @@ static u64 cpuacct_cpuusage_read(struct cpuacct *ca, int cpu)
u64 *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);
u64 data;

-#ifndef CONFIG_64BIT
- * Take rq->lock to make 64-bit read safe on 32-bit platforms.
+ * Take rq->lock to make 64-bit read safe
data = *cpuusage;
- data = *cpuusage;

return data;
@@ -113,16 +109,12 @@ static void cpuacct_cpuusage_write(struct cpuacct *ca, int cpu, u64 val)
u64 *cpuusage = per_cpu_ptr(ca->cpuusage, cpu);

-#ifndef CONFIG_64BIT
- * Take rq->lock to make 64-bit write safe on 32-bit platforms.
+ * Take rq->lock to make 64-bit write safe
*cpuusage = val;
- *cpuusage = val;

/* return total cpu usage (in nanoseconds) of a group */
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at