| Message ID | 20140930134011.232bc7bf@annuminas.surriel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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;
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.
-----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-----
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;