[PATCH] sched, time: cmpxchg does not work on 64-bit variable

[PATCH] sched, time: cmpxchg does not work on 64-bit variable

[Arnd Bergmann]

A recent change to update the stime/utime members of task_struct
using atomic cmpxchg broke configurations on 32-bit machines with
CONFIG_VIRT_CPU_ACCOUNTING_GEN set, because that uses 64-bit
nanoseconds, leading to a link-time error:

kernel/built-in.o: In function `cputime_adjust':
:(.text+0x25234): undefined reference to `__bad_cmpxchg'

This reverts the change that caused the problem, I suspect the real fix
is to conditionally use cmpxchg64 instead, but I have not checked if
that will work on all architectures.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: eb1b4af0a64a ("sched, time: Atomically increment stime & utime")
---
found in ARM randconfig builds on linux-next

diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index 64492dff8a81..e99e7e54131c 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -603,12 +603,9 @@ static void cputime_adjust(struct task_cputime *curr,
 	 * If the tick based count grows faster than the scheduler one,
 	 * the result of the scaling may go backward.
 	 * Let's enforce monotonicity.
-	 * Atomic exchange protects against concurrent cputime_adjust().
 	 */
-	while (stime > (rtime = ACCESS_ONCE(prev->stime)))
-		cmpxchg(&prev->stime, rtime, stime);
-	while (utime > (rtime = ACCESS_ONCE(prev->utime)))
-		cmpxchg(&prev->utime, rtime, utime);
+	prev->stime = max(prev->stime, stime);
+	prev->utime = max(prev->utime, utime);
 
 out:
 	*ut = prev->utime;

[PATCH] sched, time: cmpxchg does not work on 64-bit variable

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/30/2014 07:56 AM, Arnd Bergmann wrote:
> A recent change to update the stime/utime members of task_struct 
> using atomic cmpxchg broke configurations on 32-bit machines with 
> CONFIG_VIRT_CPU_ACCOUNTING_GEN set, because that uses 64-bit 
> nanoseconds, leading to a link-time error:
> 
> kernel/built-in.o: In function `cputime_adjust': :(.text+0x25234):
> undefined reference to `__bad_cmpxchg'
> 
> This reverts the change that caused the problem, I suspect the real
> fix is to conditionally use cmpxchg64 instead, but I have not
> checked if that will work on all architectures.

I see that kernel/sched/clock.c uses cmpxchg64 in a non
architecture, non 64 bit specific piece of code, and
nobody complained about that file not building, so I have
to assume cmpxchg64 works :)

The revert seems like a bad idea, since it will reintroduce
a race condition with sys_times().

One problem is that include/asm-generic/cputime_nsecs.h
defines cputime_t as u64, while cputime_jiffies.h defines
cputime_t as a long...

Will anybody barf at a cmpxchg_cputime, or is the solution
to fix cmpxchg on architectures where it does not accept a
64 bit type?  Not quite sure how to do the latter...

Arnd, on which architecture are you seeing a build failure?
Is it just 32 bit arm?

> Signed-off-by: Arnd Bergmann <arnd@arndb.de> Fixes: eb1b4af0a64a
> ("sched, time: Atomically increment stime & utime") --- found in
> ARM randconfig builds on linux-next
> 
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c index
> 64492dff8a81..e99e7e54131c 100644 --- a/kernel/sched/cputime.c +++
> b/kernel/sched/cputime.c @@ -603,12 +603,9 @@ static void
> cputime_adjust(struct task_cputime *curr, * If the tick based count
> grows faster than the scheduler one, * the result of the scaling
> may go backward. * Let's enforce monotonicity. -	 * Atomic exchange
> protects against concurrent cputime_adjust(). */ -	while (stime >
> (rtime = ACCESS_ONCE(prev->stime))) -		cmpxchg(&prev->stime, rtime,
> stime); -	while (utime > (rtime = ACCESS_ONCE(prev->utime))) -
> cmpxchg(&prev->utime, rtime, utime); +	prev->stime =
> max(prev->stime, stime); +	prev->utime = max(prev->utime, utime);
> 
> out: *ut = prev->utime;
> 


- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUKqM+AAoJEM553pKExN6Dc8EH/3BuP9ZSvWTXpFI070Wy0uuo
/gqpFqdyLLtJ+i850HW4356ew5SeIYjzKHxDcWFJDYYUBI2/LuUjyaOo02KK/AjX
0G0Qblli94dYB9B1eiMqQc9pU9VLjGHD1Gh5T0IjahTpmxKUCTNw4tv80ykdIDEe
JVrZGNAxFUQXJ+3S2RwhRHRLHGVN3/EGPvhkibPK+xvth17isasv/AdIFkgdDYPY
6UtDuKPtLAilmIEXit82z/OOqInSYPrMzvcB+psnY3NlqHwTWW8IuI5zZmEh2h+a
aQhA/D+4he49Xc2O+7eIFwzLJbBnv0+4mlUQwiw9v2bBXHg1MepqUT8hwveggxE=
=SPyM
-----END PGP SIGNATURE-----

[PATCH] sched, time: cmpxchg does not work on 64-bit variable

[Dietmar Eggemann]

Hi,

On 30/09/14 13:34, Rik van Riel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 09/30/2014 07:56 AM, Arnd Bergmann wrote:
>> A recent change to update the stime/utime members of task_struct 
>> using atomic cmpxchg broke configurations on 32-bit machines with 
>> CONFIG_VIRT_CPU_ACCOUNTING_GEN set, because that uses 64-bit 
>> nanoseconds, leading to a link-time error:
>>
>> kernel/built-in.o: In function `cputime_adjust': :(.text+0x25234):
>> undefined reference to `__bad_cmpxchg'
>>
>> This reverts the change that caused the problem, I suspect the real
>> fix is to conditionally use cmpxchg64 instead, but I have not
>> checked if that will work on all architectures.
> 
> I see that kernel/sched/clock.c uses cmpxchg64 in a non
> architecture, non 64 bit specific piece of code, and
> nobody complained about that file not building, so I have
> to assume cmpxchg64 works :)
> 
> The revert seems like a bad idea, since it will reintroduce
> a race condition with sys_times().
> 
> One problem is that include/asm-generic/cputime_nsecs.h
> defines cputime_t as u64, while cputime_jiffies.h defines
> cputime_t as a long...
> 
> Will anybody barf at a cmpxchg_cputime, or is the solution
> to fix cmpxchg on architectures where it does not accept a
> 64 bit type?  Not quite sure how to do the latter...
> 
> Arnd, on which architecture are you seeing a build failure?
> Is it just 32 bit arm?

I'm seeing it on ARMv7 build too in case I enable 'Full dynticks CPU
time accounting'.

-- Dietmar

> 
[...]

[PATCH] sched, time: cmpxchg does not work on 64-bit variable

[Arnd Bergmann]

On Tuesday 30 September 2014 08:34:06 Rik van Riel wrote:
> 
> Arnd, on which architecture are you seeing a build failure?
> Is it just 32 bit arm?
> 

Yes, I only tested on arm32, but I assume that most other 32-bit
architectures will behave in a similar way.

	Arnd

[PATCH] sched, time: cmpxchg does not work on 64-bit variable

[Peter Zijlstra]

On Tue, Sep 30, 2014 at 08:34:06AM -0400, Rik van Riel wrote:
> On 09/30/2014 07:56 AM, Arnd Bergmann wrote:
> > A recent change to update the stime/utime members of task_struct 
> > using atomic cmpxchg broke configurations on 32-bit machines with 
> > CONFIG_VIRT_CPU_ACCOUNTING_GEN set, because that uses 64-bit 
> > nanoseconds, leading to a link-time error:
> > 
> > kernel/built-in.o: In function `cputime_adjust': :(.text+0x25234):
> > undefined reference to `__bad_cmpxchg'
> > 
> > This reverts the change that caused the problem, I suspect the real
> > fix is to conditionally use cmpxchg64 instead, but I have not
> > checked if that will work on all architectures.
> 
> I see that kernel/sched/clock.c uses cmpxchg64 in a non
> architecture, non 64 bit specific piece of code, and
> nobody complained about that file not building, so I have
> to assume cmpxchg64 works :)

That code is only ever used on x86 and ia64, most other archs have
managed to not mess up their clocks quite as bad.

[PATCH v2] sched, time: fix build error with 64 bit cputime_t on 32 bit systems

[Rik van Riel]

> That code is only ever used on x86 and ia64, most other archs have
> managed to not mess up their clocks quite as bad.

OK, lets try this again...

Gcc generated an error with the previous patch, because it evaluated
the branch that it was eliminating.  That means we cannot use a
sizeof directed branch, and have to fall back to simply defining which
cmpxchg to call for each cputime_t declaration.

---8<---

Subject: sched,time: fix build error with 64 bit cputime_t on 32 bit systems

On 32 bit systems cmpxchg cannot handle 64 bit values, so
some additional magic is required to allow a 32 bit system
with CONFIG_VIRT_CPU_ACCOUNTING_GEN enabled to build.

Make sure the correct cmpxchg function is used when doing
an atomic swap of a cputime_t.

Signed-off-by: Rik van Riel <riel@redhat.com>
Reported-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/powerpc/include/asm/cputime.h    |  2 ++
 arch/s390/include/asm/cputime.h       |  2 ++
 include/asm-generic/cputime_jiffies.h |  2 ++
 include/asm-generic/cputime_nsecs.h   |  2 ++
 kernel/sched/cputime.c                | 29 +++++++++++++++++++----------
 5 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/include/asm/cputime.h b/arch/powerpc/include/asm/cputime.h
index 607559a..6c840ce 100644
--- a/arch/powerpc/include/asm/cputime.h
+++ b/arch/powerpc/include/asm/cputime.h
@@ -32,6 +32,8 @@ static inline void setup_cputime_one_jiffy(void) { }
 typedef u64 __nocast cputime_t;
 typedef u64 __nocast cputime64_t;
 
+#define cmpxchg_cputime(ptr, old, new) cmpxchg(ptr, old, new)
+
 #ifdef __KERNEL__
 
 /*
diff --git a/arch/s390/include/asm/cputime.h b/arch/s390/include/asm/cputime.h
index f65bd36..3001887 100644
--- a/arch/s390/include/asm/cputime.h
+++ b/arch/s390/include/asm/cputime.h
@@ -18,6 +18,8 @@
 typedef unsigned long long __nocast cputime_t;
 typedef unsigned long long __nocast cputime64_t;
 
+#define cmpxchg_cputime(ptr, old, new) cmpxchg64(ptr, old, new)
+
 static inline unsigned long __div(unsigned long long n, unsigned long base)
 {
 #ifndef CONFIG_64BIT
diff --git a/include/asm-generic/cputime_jiffies.h b/include/asm-generic/cputime_jiffies.h
index d5cb78f5..fe386fc 100644
--- a/include/asm-generic/cputime_jiffies.h
+++ b/include/asm-generic/cputime_jiffies.h
@@ -3,6 +3,8 @@
 
 typedef unsigned long __nocast cputime_t;
 
+#define cmpxchg_cputime(ptr, old, new) cmpxchg(ptr, old, new)
+
 #define cputime_one_jiffy		jiffies_to_cputime(1)
 #define cputime_to_jiffies(__ct)	(__force unsigned long)(__ct)
 #define cputime_to_scaled(__ct)		(__ct)
diff --git a/include/asm-generic/cputime_nsecs.h b/include/asm-generic/cputime_nsecs.h
index 4e81760..0419485 100644
--- a/include/asm-generic/cputime_nsecs.h
+++ b/include/asm-generic/cputime_nsecs.h
@@ -21,6 +21,8 @@
 typedef u64 __nocast cputime_t;
 typedef u64 __nocast cputime64_t;
 
+#define cmpxchg_cputime(ptr, old, new) cmpxchg64(ptr, old, new)
+
 #define cputime_one_jiffy		jiffies_to_cputime(1)
 
 #define cputime_div(__ct, divisor)  div_u64((__force u64)__ct, divisor)
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index 64492df..8394b1e 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -555,6 +555,23 @@ static cputime_t scale_stime(u64 stime, u64 rtime, u64 total)
 }
 
 /*
+ * Atomically advance counter to the new value. Interrupts, vcpu
+ * scheduling, and scaling inaccuracies can cause cputime_advance
+ * to be occasionally called with a new value smaller than counter.
+ * Let's enforce atomicity.
+ *
+ * Normally a caller will only go through this loop once, or not
+ * at all in case a previous caller updated counter the same jiffy.
+ */
+static void cputime_advance(cputime_t *counter, cputime_t new)
+{
+	cputime_t old;
+
+	while (new > (old = ACCESS_ONCE(*counter)))
+		cmpxchg_cputime(counter, old, new);
+}
+
+/*
  * Adjust tick based cputime random precision against scheduler
  * runtime accounting.
  */
@@ -599,16 +616,8 @@ static void cputime_adjust(struct task_cputime *curr,
 		utime = rtime - stime;
 	}
 
-	/*
-	 * If the tick based count grows faster than the scheduler one,
-	 * the result of the scaling may go backward.
-	 * Let's enforce monotonicity.
-	 * Atomic exchange protects against concurrent cputime_adjust().
-	 */
-	while (stime > (rtime = ACCESS_ONCE(prev->stime)))
-		cmpxchg(&prev->stime, rtime, stime);
-	while (utime > (rtime = ACCESS_ONCE(prev->utime)))
-		cmpxchg(&prev->utime, rtime, utime);
+	cputime_advance(&prev->stime, stime);
+	cputime_advance(&prev->utime, utime);
 
 out:
 	*ut = prev->utime;

[PATCH v2] sched, time: fix build error with 64 bit cputime_t on 32 bit systems

[Arnd Bergmann]

On Tuesday 30 September 2014 15:59:47 Rik van Riel wrote:
> Subject: sched,time: fix build error with 64 bit cputime_t on 32 bit systems
> 
> On 32 bit systems cmpxchg cannot handle 64 bit values, so
> some additional magic is required to allow a 32 bit system
> with CONFIG_VIRT_CPU_ACCOUNTING_GEN enabled to build.
> 
> Make sure the correct cmpxchg function is used when doing
> an atomic swap of a cputime_t.
> 
> Signed-off-by: Rik van Riel <riel@redhat.com>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> 

Now builds without warnings on both 32-bit and 64-bit cputime_t configurations
on arm32.

Acked-by: Arnd Bergmann <arnd@arndb.de>

Thanks!

	Arnd

[PATCH] sched, time: fix build error with 64 bit cputime_t on 32 bit systems

[Rik van Riel]

On Tue, 30 Sep 2014 13:56:37 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> A recent change to update the stime/utime members of task_struct
> using atomic cmpxchg broke configurations on 32-bit machines with
> CONFIG_VIRT_CPU_ACCOUNTING_GEN set, because that uses 64-bit
> nanoseconds, leading to a link-time error:
> 
> kernel/built-in.o: In function `cputime_adjust':
> :(.text+0x25234): undefined reference to `__bad_cmpxchg'

Arnd, this should fix your problem, while still ensuring that
the cpu time counters only ever go forward.

I do not have cross compiling toolchains set up here, but I assume
this fixes your bug.

Ingo & Peter, if this patch fixes the bug for Arnd, could you please
merge it into -tip?

Linus, the changeset causing the problem is only in -tip right now,
and this patch will not apply to your tree.

---8<---

Subject: sched,time: fix build error with 64 bit cputime_t on 32 bit systems

On 32 bit systems cmpxchg cannot handle 64 bit values, and
cmpxchg64 needs to be used when full dynticks CPU accounting
is enabled, since that turns cputime_t into a u64.

With jiffies based CPU accounting, cputime_t is an unsigned
long. On 64 bit systems, cputime_t is always the size of a
long.

Luckily the compiler can figure out whether we need to call
cmpxchg or cmpxchg64.

Signed-off-by: Rik van Riel <riel@redhat.com>
Reported-by: Arnd Bergmann <arnd@arndb.de>
---
 kernel/sched/cputime.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index 64492df..db239c9 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -555,6 +555,29 @@ static cputime_t scale_stime(u64 stime, u64 rtime, u64 total)
 }
 
 /*
+ * Atomically advance counter to the new value. Interrupts, vcpu
+ * scheduling, and scaling inaccuracies can cause cputime_advance
+ * to be occasionally called with a new value smaller than counter.
+ * Let's enforce atomicity.
+ *
+ * Normally a caller will only go through this loop once, or not
+ * at all in case a previous caller updated counter the same jiffy.
+ */
+static void cputime_advance(cputime_t *counter, cputime_t new)
+{
+	cputime_t old;
+
+	while (new > (old = ACCESS_ONCE(*counter))) {
+		/* The compiler will optimize away this branch.  */
+		if (sizeof(cputime_t) == sizeof(long))
+			cmpxchg(counter, old, new);
+		else
+			/* 64 bit cputime_t on a 32 bit system... */
+			cmpxchg64(counter, old, new);
+	}
+}
+
+/*
  * Adjust tick based cputime random precision against scheduler
  * runtime accounting.
  */
@@ -599,16 +622,8 @@ static void cputime_adjust(struct task_cputime *curr,
 		utime = rtime - stime;
 	}
 
-	/*
-	 * If the tick based count grows faster than the scheduler one,
-	 * the result of the scaling may go backward.
-	 * Let's enforce monotonicity.
-	 * Atomic exchange protects against concurrent cputime_adjust().
-	 */
-	while (stime > (rtime = ACCESS_ONCE(prev->stime)))
-		cmpxchg(&prev->stime, rtime, stime);
-	while (utime > (rtime = ACCESS_ONCE(prev->utime)))
-		cmpxchg(&prev->utime, rtime, utime);
+	cputime_advance(&prev->stime, stime);
+	cputime_advance(&prev->utime, utime);
 
 out:
 	*ut = prev->utime;

[PATCH] sched, time: fix build error with 64 bit cputime_t on 32 bit systems

[Arnd Bergmann]

On Tuesday 30 September 2014 13:40:11 Rik van Riel wrote:
> On Tue, 30 Sep 2014 13:56:37 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > A recent change to update the stime/utime members of task_struct
> > using atomic cmpxchg broke configurations on 32-bit machines with
> > CONFIG_VIRT_CPU_ACCOUNTING_GEN set, because that uses 64-bit
> > nanoseconds, leading to a link-time error:
> > 
> > kernel/built-in.o: In function `cputime_adjust':
> > :(.text+0x25234): undefined reference to `__bad_cmpxchg'
> 
> Arnd, this should fix your problem, while still ensuring that
> the cpu time counters only ever go forward.
> 
> I do not have cross compiling toolchains set up here, but I assume
> this fixes your bug.
> 
> Ingo & Peter, if this patch fixes the bug for Arnd, could you please
> merge it into -tip?
> 
> Linus, the changeset causing the problem is only in -tip right now,
> and this patch will not apply to your tree.

It compiles and links on arm32 with both 32-bit and 64-bit cputime_t,
but I now get a new warning for the former case:

kernel/sched/cputime.c: In function 'cputime_advance':
kernel/sched/cputime.c:576:29: warning: passing argument 1 of '__cmpxchg64_mb' from incompatible pointer type
    cmpxchg64(counter, old, new);
                             ^
In file included from /git/arm-soc/arch/arm/include/asm/atomic.h:19:0,
                 from /git/arm-soc/include/linux/atomic.h:4,
                 from /git/arm-soc/include/linux/debug_locks.h:5,
                 from /git/arm-soc/include/linux/lockdep.h:23,
                 from /git/arm-soc/include/linux/spinlock_types.h:18,
                 from /git/arm-soc/include/linux/spinlock.h:81,
                 from /git/arm-soc/include/linux/seqlock.h:35,
                 from /git/arm-soc/include/linux/time.h:5,
                 from /git/arm-soc/include/uapi/linux/timex.h:56,
                 from /git/arm-soc/include/linux/timex.h:56,
                 from /git/arm-soc/include/linux/sched.h:19,
                 from /git/arm-soc/kernel/sched/cputime.c:2:
/git/arm-soc/arch/arm/include/asm/cmpxchg.h:255:105: note: expected 'long long unsigned int *' but argument is of type 'cputime_t *'
 static inline unsigned long long __cmpxchg64_mb(unsigned long long *ptr,
       kernel/sched/cputime.c:576:5: warning: value computed is not used [-Wunused-value]
    cmpxchg64(counter, old, new);
     ^
                                                                                                  ^
I suspect there is no solution that doesn't involve the preprocessor.
How about adding a cputime_cmpxchg() helper next to the cputime_t
definition?

	Arnd

[PATCH] sched, time: fix build error with 64 bit cputime_t on 32 bit systems

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/30/2014 02:49 PM, Arnd Bergmann wrote:
> On Tuesday 30 September 2014 13:40:11 Rik van Riel wrote:
>> On Tue, 30 Sep 2014 13:56:37 +0200 Arnd Bergmann <arnd@arndb.de>
>> wrote:
>> 
>>> A recent change to update the stime/utime members of
>>> task_struct using atomic cmpxchg broke configurations on 32-bit
>>> machines with CONFIG_VIRT_CPU_ACCOUNTING_GEN set, because that
>>> uses 64-bit nanoseconds, leading to a link-time error:
>>> 
>>> kernel/built-in.o: In function `cputime_adjust': 
>>> :(.text+0x25234): undefined reference to `__bad_cmpxchg'
>> 
>> Arnd, this should fix your problem, while still ensuring that the
>> cpu time counters only ever go forward.
>> 
>> I do not have cross compiling toolchains set up here, but I
>> assume this fixes your bug.
>> 
>> Ingo & Peter, if this patch fixes the bug for Arnd, could you
>> please merge it into -tip?
>> 
>> Linus, the changeset causing the problem is only in -tip right
>> now, and this patch will not apply to your tree.
> 
> It compiles and links on arm32 with both 32-bit and 64-bit
> cputime_t, but I now get a new warning for the former case:
> 
> kernel/sched/cputime.c: In function 'cputime_advance': 
> kernel/sched/cputime.c:576:29: warning: passing argument 1 of
> '__cmpxchg64_mb' from incompatible pointer type cmpxchg64(counter,
> old, new); ^ In file included from
> /git/arm-soc/arch/arm/include/asm/atomic.h:19:0, from
> /git/arm-soc/include/linux/atomic.h:4, from
> /git/arm-soc/include/linux/debug_locks.h:5, from
> /git/arm-soc/include/linux/lockdep.h:23, from
> /git/arm-soc/include/linux/spinlock_types.h:18, from
> /git/arm-soc/include/linux/spinlock.h:81, from
> /git/arm-soc/include/linux/seqlock.h:35, from
> /git/arm-soc/include/linux/time.h:5, from
> /git/arm-soc/include/uapi/linux/timex.h:56, from
> /git/arm-soc/include/linux/timex.h:56, from
> /git/arm-soc/include/linux/sched.h:19, from
> /git/arm-soc/kernel/sched/cputime.c:2: 
> /git/arm-soc/arch/arm/include/asm/cmpxchg.h:255:105: note: expected
> 'long long unsigned int *' but argument is of type 'cputime_t *' 
> static inline unsigned long long __cmpxchg64_mb(unsigned long long
> *ptr, kernel/sched/cputime.c:576:5: warning: value computed is not
> used [-Wunused-value] cmpxchg64(counter, old, new); ^

Is u64 on arm defined to "long long unsigned int", and is the compiler
really blowing up over the difference between that and "unsigned long
long"?

Is u64 on arm really supposed to be "long long unsigned int"?

Am I overlooking something?

> I suspect there is no solution that doesn't involve the
> preprocessor. How about adding a cputime_cmpxchg() helper next to
> the cputime_t definition?

I thought of that first, and quickly dismissed it with "yuck" :)

I guess we could cast cputime_t to unsigned long long in the branch
that calls cmpxchg64.  Is that acceptable/unacceptable to people?

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUKwI2AAoJEM553pKExN6Dr+kIAJMePKlT90h75zAPeVeofpGn
OPZ/V6N7do+pqZ3u56MdxmbCt1Rmbbp9Ka3tWZIJRS8OvyTQsPImmUiYD4KLiDL0
fOrADIiEIqj+hzvX9w6iSRTg8HwQSb8lL1jLSklhfE9bayN9/O0Pl4x80sqtfBfW
LJF+JGzktHQOq+VnfptQuPhCEKXFYgBHcP0JsVadkj/okXYmpQ4Iwk9pVs9WF1AG
+i251YZR0E8H8PEIEY5IPLP/xM64SwyxS6NUSJx4J8mMarIg2onXJuSabisOLARI
2bATM0UYPNLMJfSIO0E0dWCgEi/V8TyR7tzRBETFT1JQW6iWnIk4yVgoYMAaFPY=
=1+D4
-----END PGP SIGNATURE-----

[PATCH] sched, time: fix build error with 64 bit cputime_t on 32 bit systems

[Frederic Weisbecker]

On Tue, Sep 30, 2014 at 01:40:11PM -0400, Rik van Riel wrote:
> On Tue, 30 Sep 2014 13:56:37 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > A recent change to update the stime/utime members of task_struct
> > using atomic cmpxchg broke configurations on 32-bit machines with
> > CONFIG_VIRT_CPU_ACCOUNTING_GEN set, because that uses 64-bit
> > nanoseconds, leading to a link-time error:
> > 
> > kernel/built-in.o: In function `cputime_adjust':
> > :(.text+0x25234): undefined reference to `__bad_cmpxchg'
> 
> Arnd, this should fix your problem, while still ensuring that
> the cpu time counters only ever go forward.
> 
> I do not have cross compiling toolchains set up here, but I assume
> this fixes your bug.
> 
> Ingo & Peter, if this patch fixes the bug for Arnd, could you please
> merge it into -tip?
> 
> Linus, the changeset causing the problem is only in -tip right now,
> and this patch will not apply to your tree.
> 
> ---8<---
> 
> Subject: sched,time: fix build error with 64 bit cputime_t on 32 bit systems
> 
> On 32 bit systems cmpxchg cannot handle 64 bit values, and
> cmpxchg64 needs to be used when full dynticks CPU accounting
> is enabled, since that turns cputime_t into a u64.
> 
> With jiffies based CPU accounting, cputime_t is an unsigned
> long. On 64 bit systems, cputime_t is always the size of a
> long.
> 
> Luckily the compiler can figure out whether we need to call
> cmpxchg or cmpxchg64.
> 
> Signed-off-by: Rik van Riel <riel@redhat.com>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  kernel/sched/cputime.c | 35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index 64492df..db239c9 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -555,6 +555,29 @@ static cputime_t scale_stime(u64 stime, u64 rtime, u64 total)
>  }
>  
>  /*
> + * Atomically advance counter to the new value. Interrupts, vcpu
> + * scheduling, and scaling inaccuracies can cause cputime_advance
> + * to be occasionally called with a new value smaller than counter.
> + * Let's enforce atomicity.
> + *
> + * Normally a caller will only go through this loop once, or not
> + * at all in case a previous caller updated counter the same jiffy.
> + */
> +static void cputime_advance(cputime_t *counter, cputime_t new)
> +{
> +	cputime_t old;
> +
> +	while (new > (old = ACCESS_ONCE(*counter))) {
> +		/* The compiler will optimize away this branch.  */
> +		if (sizeof(cputime_t) == sizeof(long))
> +			cmpxchg(counter, old, new);
> +		else
> +			/* 64 bit cputime_t on a 32 bit system... */
> +			cmpxchg64(counter, old, new);

Maybe cmpxchg() should itself be a wrapper that does this build time choice
between cmpxchg32() and cmpxchg64().

And if it looks too dangerous to convert all users, this could be cmpxchg_t().

> +	}
> +}
> +
> +/*
>   * Adjust tick based cputime random precision against scheduler
>   * runtime accounting.
>   */
> @@ -599,16 +622,8 @@ static void cputime_adjust(struct task_cputime *curr,
>  		utime = rtime - stime;
>  	}
>  
> -	/*
> -	 * If the tick based count grows faster than the scheduler one,
> -	 * the result of the scaling may go backward.
> -	 * Let's enforce monotonicity.
> -	 * Atomic exchange protects against concurrent cputime_adjust().
> -	 */
> -	while (stime > (rtime = ACCESS_ONCE(prev->stime)))
> -		cmpxchg(&prev->stime, rtime, stime);
> -	while (utime > (rtime = ACCESS_ONCE(prev->utime)))
> -		cmpxchg(&prev->utime, rtime, utime);
> +	cputime_advance(&prev->stime, stime);
> +	cputime_advance(&prev->utime, utime);
>  
>  out:
>  	*ut = prev->utime;

[PATCH] sched, time: fix build error with 64 bit cputime_t on 32 bit systems

[Peter Zijlstra]

On Tue, Sep 30, 2014 at 09:06:13PM +0200, Frederic Weisbecker wrote:
> Maybe cmpxchg() should itself be a wrapper that does this build time choice
> between cmpxchg32() and cmpxchg64().
> 
> And if it looks too dangerous to convert all users, this could be cmpxchg_t().

It cannot, not all 32bit archs can do a 64bit cmpxchg.