Prev: [GIT PULL] davinci platform updates for 2.6.35
Next: Added commandline partitions for block devices
From: Thomas Renninger on 19 May 2010 15:20
On Wednesday 19 May 2010 09:02:23 pm 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.
Speed on i386 shouldn't count that much here.
Big machines run x86_64 and cgroup cpu accounting doesn't make much
sense on old i386 single core machines.

Would still be great to see the first patch go in then.
The idea of splitting was to:
a) show the real change
b) make at least the first go in if the snd does not work out

Thanks,

Thomas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Mike Chan on 19 May 2010 15:40
On Wed, May 19, 2010 at 12:13 PM, Thomas Renninger <trenn(a)suse.de> wrote:
> On Wednesday 19 May 2010 09:02:23 pm 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.
> Speed on i386 shouldn't count that much here.
> Big machines run x86_64 and cgroup cpu accounting doesn't make much
> sense on old i386 single core machines.
>

cpuacct can still be useful for single core machines, at least in
Android it tracks the cpu usage if your apps. On ARM I'm not sure
(maybe someone more knowledgeable can chime in) how much overhead this
will incur relative to scheduler / cpuacct overhead that's already
there.

Since this is a per-cpu value already, is it not sufficient to just
disable interrupts on that cpu while doing a normal u64 add for both
MP and UP systems?

-- Mike

> Would still be great to see the first patch go in then.
> The idea of splitting was to:
> �a) show the real change
> �b) make at least the first go in if the snd does not work out
>
> Thanks,
>
> � � Thomas
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Thomas Renninger on 20 May 2010 07:00
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(a)suse.de>
CC: linux-kernel(a)vger.kernel.org
CC: mike(a)android.com
CC: menage(a)google.com
CC: lizf(a)cn.fujitsu.com
CC: containers(a)lists.linux-foundation.org
CC: mingo(a)elte.hu
CC: peterz(a)infradead.org

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(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: [GIT PULL] davinci platform updates for 2.6.35
Next: Added commandline partitions for block devices