From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756005AbcCOJc7 (ORCPT ); Tue, 15 Mar 2016 05:32:59 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:34491 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755750AbcCOJcu (ORCPT ); Tue, 15 Mar 2016 05:32:50 -0400 Date: Tue, 15 Mar 2016 10:32:45 +0100 From: Ingo Molnar To: Linus Torvalds Cc: Linux Kernel Mailing List , =?iso-8859-1?Q?Fr=E9d=E9ric?= Weisbecker , Thomas Gleixner , Peter Zijlstra , Andrew Morton Subject: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Message-ID: <20160315093245.GA7943@gmail.com> References: <20160314123200.GA15971@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Linus Torvalds wrote: > On Mon, Mar 14, 2016 at 5:32 AM, Ingo Molnar wrote: > > +/** > > + * fetch_or - perform *ptr |= mask and return old value of *ptr > > + * @ptr: pointer to value > > + * @mask: mask to OR on the value > > + * > > + * cmpxchg based fetch_or, macro so it works for different integer types > > + */ > > +#ifndef fetch_or > > +#define fetch_or(ptr, mask) \ > > +({ typeof(*(ptr)) __old, __val = *(ptr); \ > > + for (;;) { \ > > + __old = cmpxchg((ptr), __val, __val | (mask)); \ > > + if (__old == __val) \ > > + break; \ > > + __val = __old; \ > > + } \ > > + __old; \ > > +}) > > +#endif > > This is garbage. Indeed. > This macro re-uses the "mask" argument potentially many many times, so > semantically it's very dubious. Yes. > That may have been acceptable in the old situation when this was > internal to sched.c, but now the code was moved to a generic header > file. And this kind of broken macro is *not* acceptable in that > context any more. > > It is now in asm-generic/atomic.h, so it should now conform to the > rest of the code there. Try to model it around ATOMIC_OP_RETURN(), > although obviously this fetch_or() function returns the value *before* > the logical 'or' rather than the end result. Ok. I can see two other problems with it as well: 1) 'ptr' may get evaluated multiple times as well, not just 'mask'. 2) its naming sucks. "fetch_or()" does not really signal that it's a fundamentally atomic operation, nor what API family it belongs to. Given that it's in include/linux/atomic.h, and it has _almost_ xchg() semantics - how about we name it xchg_or() and model it after that? From the semantics POV it's really an unconditional xchg()-alike API, even though technically it's a cmpxchg() loop. I've attached a (totally untested) patch that fixes the bugs and does the rename. > It would be lovely if it were piossible to just use an "atomic_t" > type, but it looks like it is used on thread_info->flags. Which > doesn't have a good type, sadly. Indeed, thread_info->flags is 'almost' but not quite 'unsigned long': triton:~/tip> grep -E '\.*;' $(git grep -l 'thread_info {' arch/) | grep -vE 'WARN|return' arch/alpha/include/asm/thread_info.h: unsigned int flags; /* low level flags */ arch/arc/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/arm/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/arm64/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/avr32/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/blackfin/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/c6x/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/cris/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/frv/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/h8300/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/hexagon/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/ia64/include/asm/thread_info.h: __u32 flags; /* thread_info flags (see TIF_*) */ arch/m32r/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/m68k/include/asm/thread_info.h: unsigned long flags; arch/metag/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/microblaze/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/mips/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/mn10300/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/nios2/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/openrisc/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/parisc/include/asm/thread_info.h: unsigned long flags; /* thread_info flags (see TIF_*) */ arch/powerpc/include/asm/thread_info.h: unsigned long flags ____cacheline_aligned_in_smp; arch/s390/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/score/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/sh/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/sparc/include/asm/thread_info_32.h: unsigned long flags; /* low level flags */ arch/sparc/include/asm/thread_info_64.h: unsigned long flags; arch/tile/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/um/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/unicore32/include/asm/thread_info.h: unsigned long flags; /* low level flags */ arch/x86/include/asm/thread_info.h: __u32 flags; /* low level flags */ arch/xtensa/include/asm/thread_info.h: unsigned long flags; /* low level flags */ alpha, ia64 and x86 are the odd ones out with an int ti::flags. > As a result, the code then makes a big deal about how this works with > any integer type, but then the new code that uses it uses a stupid > type that isn't appropriate. Why would it be using an "unsigned long", > when > > - it holds a fixed number of bits that don't depend on architecture > and certainly is not 64 (or even close to 32). > > - the structure it is in was just preceded by an "int", so on 64-bit > it generates pointless padding in addition to the pointless 64-bit > field. Yes, its natural type would be 'unsigned int'. > The only reason to use a "unsigned long" is because "fetch_or()" would > be hardcoded to that type, which doesn't seem to be true. Yes, fetch_or() uses cmpxchg() internally, and AFAICS cmpxchg() is guaranteed to work on two integer types: - 32 bit - the natural word type of the architecture. (32 or 64 bit) Many architectures have wider support for cmpxchg(): 8, 16, 32 and 64 bits, but we cannot rely on that in a generic header. So changing the dependency mask type to 'unsigned int' should be safe. I'll send another patch for that. > Now, in practice, the code looks like it should *work* fine, so I'm > going to pull this, but I do want to lodge a protest on sloppiness and > general and unnecessary uglicity of this code. > > So please get this fixed. Sorry about this, that was quite sloppy on several levels! Thanks, Ingo ===========================> From: Ingo Molnar Date: Tue, 15 Mar 2016 10:19:10 +0100 Subject: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Linus noticed a couple of problems with the fetch_or() implementation introduced by 5fd7a09cfb8c ("atomic: Export fetch_or()"): - Sloppy macro implementation: 'mask' and 'ptr' is evaluated multiple times, which will break if arguments have side effects. - Sloppy semantics: the naming and structure of the macro is ambigious, with no well-defined semantics. It's neither an atomic nor a cmpxchg() interface, but still it lives in include/linux/atomic.h. Solve this by: - Extracting the arguments into helper variables once, and never using the original arguments from that point on. This makes it pretty clear that there can be no unwanted macro evaluation side effects. - Renaming fetch_or() to xchg_or(), recognizing that the semantics are xchg()-alike. Also propagate the rename to the call sites. Reported-by: Linus Torvalds Cc: Chris Metcalf Cc: Frederic Weisbecker Cc: Christoph Lameter Cc: Chris Metcalf Cc: Ingo Molnar Cc: Luiz Capitulino Cc: Peter Zijlstra Cc: Rik van Riel Cc: Thomas Gleixner Cc: Viresh Kumar Signed-off-by: Ingo Molnar --- include/linux/atomic.h | 20 +++++++++++++------- kernel/sched/core.c | 2 +- kernel/time/tick-sched.c | 4 ++-- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/include/linux/atomic.h b/include/linux/atomic.h index 6c502cb13c95..a884f56b882f 100644 --- a/include/linux/atomic.h +++ b/include/linux/atomic.h @@ -549,21 +549,27 @@ static inline int atomic_dec_if_positive(atomic_t *v) #endif /** - * fetch_or - perform *ptr |= mask and return old value of *ptr - * @ptr: pointer to value + * xchg_or - perform *ptr |= mask atomically and return old value of *ptr + * @ptr: pointer to value (cmpxchg() compatible integer pointer type) * @mask: mask to OR on the value * - * cmpxchg based fetch_or, macro so it works for different integer types + * cmpxchg() based, it's a macro so it works for different integer types. */ -#ifndef fetch_or -#define fetch_or(ptr, mask) \ -({ typeof(*(ptr)) __old, __val = *(ptr); \ +#ifndef xchg_or +# define xchg_or(ptr, mask) \ +({ \ + typeof(ptr) __ptr = (ptr); \ + typeof(mask) __mask = (mask); \ + \ + typeof(*(__ptr)) __old, __val = *__ptr; \ + \ for (;;) { \ - __old = cmpxchg((ptr), __val, __val | (mask)); \ + __old = cmpxchg(__ptr, __val, __val | __mask); \ if (__old == __val) \ break; \ __val = __old; \ } \ + \ __old; \ }) #endif diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 1c82ded3e675..de380776d1df 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -330,7 +330,7 @@ static inline void init_hrtick(void) static bool set_nr_and_not_polling(struct task_struct *p) { struct thread_info *ti = task_thread_info(p); - return !(fetch_or(&ti->flags, _TIF_NEED_RESCHED) & _TIF_POLLING_NRFLAG); + return !(xchg_or(&ti->flags, _TIF_NEED_RESCHED) & _TIF_POLLING_NRFLAG); } /* diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 969e6704c3c9..851631899352 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -264,7 +264,7 @@ static void tick_nohz_dep_set_all(unsigned long *dep, { unsigned long prev; - prev = fetch_or(dep, BIT_MASK(bit)); + prev = xchg_or(dep, BIT_MASK(bit)); if (!prev) tick_nohz_full_kick_all(); } @@ -294,7 +294,7 @@ void tick_nohz_dep_set_cpu(int cpu, enum tick_dep_bits bit) ts = per_cpu_ptr(&tick_cpu_sched, cpu); - prev = fetch_or(&ts->tick_dep_mask, BIT_MASK(bit)); + prev = xchg_or(&ts->tick_dep_mask, BIT_MASK(bit)); if (!prev) { preempt_disable(); /* Perf needs local kick that is NMI safe */