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

[Thomas Renninger]

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...

Thanks,

    Thomas

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
 	 */
 	lock_runqueue(cpu);
 	data = *cpuusage;
 	unlock_runqueue(cpu);
-#else
-	data = *cpuusage;
-#endif
 
 	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
 	 */
 	lock_runqueue(cpu);
 	*cpuusage = val;
 	unlock_runqueue(cpu);
-#else
-	*cpuusage = val;
-#endif
 }
 
 /* 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  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/