From: Rik van Riel <riel@redhat.com> To: Arnd Bergmann <arnd@arndb.de> Cc: Peter Zijlstra <peterz@infradead.org>, Linus Torvalds <torvalds@linux-foundation.org>, umgwanakikbuti@gmail.com, fweisbec@gmail.com, akpm@linux-foundation.org, srao@redhat.com, lwoodman@redhat.com, atheurer@redhat.com, oleg@redhat.com, Ingo Molnar <mingo@kernel.org>, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] sched, time: cmpxchg does not work on 64-bit variable Date: Tue, 30 Sep 2014 08:34:06 -0400 [thread overview] Message-ID: <542AA33E.2050008@redhat.com> (raw) In-Reply-To: <2547036.UshV4pXvhf@wuerfel> -----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-----
WARNING: multiple messages have this Message-ID (diff)
From: riel@redhat.com (Rik van Riel) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] sched, time: cmpxchg does not work on 64-bit variable Date: Tue, 30 Sep 2014 08:34:06 -0400 [thread overview] Message-ID: <542AA33E.2050008@redhat.com> (raw) In-Reply-To: <2547036.UshV4pXvhf@wuerfel> -----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-----
next prev parent reply other threads:[~2014-09-30 12:35 UTC|newest] Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-09-30 11:56 [PATCH] sched, time: cmpxchg does not work on 64-bit variable Arnd Bergmann 2014-09-30 11:56 ` Arnd Bergmann 2014-09-30 12:34 ` Rik van Riel [this message] 2014-09-30 12:34 ` Rik van Riel 2014-09-30 12:43 ` Dietmar Eggemann 2014-09-30 12:43 ` Dietmar Eggemann 2014-09-30 13:26 ` Arnd Bergmann 2014-09-30 13:26 ` Arnd Bergmann 2014-09-30 13:37 ` Peter Zijlstra 2014-09-30 13:37 ` Peter Zijlstra 2014-09-30 19:59 ` [PATCH v2] sched, time: fix build error with 64 bit cputime_t on 32 bit systems Rik van Riel 2014-09-30 19:59 ` Rik van Riel 2014-09-30 20:07 ` Arnd Bergmann 2014-09-30 20:07 ` Arnd Bergmann 2014-10-03 5:27 ` [tip:sched/core] sched, time: Fix " tip-bot for Rik van Riel 2014-09-30 17:40 ` [PATCH] sched, time: fix " Rik van Riel 2014-09-30 17:40 ` Rik van Riel 2014-09-30 18:49 ` Arnd Bergmann 2014-09-30 18:49 ` Arnd Bergmann 2014-09-30 19:19 ` Rik van Riel 2014-09-30 19:19 ` Rik van Riel 2014-09-30 19:06 ` Frederic Weisbecker 2014-09-30 19:06 ` Frederic Weisbecker 2014-09-30 19:08 ` Peter Zijlstra 2014-09-30 19:08 ` Peter Zijlstra
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=542AA33E.2050008@redhat.com \ --to=riel@redhat.com \ --cc=akpm@linux-foundation.org \ --cc=arnd@arndb.de \ --cc=atheurer@redhat.com \ --cc=fweisbec@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=lwoodman@redhat.com \ --cc=mingo@kernel.org \ --cc=oleg@redhat.com \ --cc=peterz@infradead.org \ --cc=srao@redhat.com \ --cc=torvalds@linux-foundation.org \ --cc=umgwanakikbuti@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.