From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934792AbcCOMWN (ORCPT ); Tue, 15 Mar 2016 08:22:13 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:33576 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934751AbcCOMVu (ORCPT ); Tue, 15 Mar 2016 08:21:50 -0400 Date: Tue, 15 Mar 2016 13:21: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 v2] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Message-ID: <20160315122145.GA7225@gmail.com> References: <20160314123200.GA15971@gmail.com> <20160315093245.GA7943@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160315093245.GA7943@gmail.com> 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 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. Also, it uses double-underscore temporary variables - which is dangerous as low level asm ones are using those too and they may alias in unexpected ways. - 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. - Using single-underscore temporary variables. - 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: Andrew Morton Cc: Chris Metcalf Cc: Christoph Lameter Cc: Frederic Weisbecker Cc: Luiz Capitulino Cc: Peter Zijlstra Cc: Peter Zijlstra Cc: Rik van Riel Cc: Thomas Gleixner Cc: Viresh Kumar Link: http://lkml.kernel.org/r/20160315093245.GA7943@gmail.com Signed-off-by: Ingo Molnar --- include/linux/atomic.h | 26 ++++++++++++++++---------- kernel/sched/core.c | 2 +- kernel/time/tick-sched.c | 4 ++-- 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/include/linux/atomic.h b/include/linux/atomic.h index 6c502cb13c95..7bc5297bcca8 100644 --- a/include/linux/atomic.h +++ b/include/linux/atomic.h @@ -549,22 +549,28 @@ 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)); \ - if (__old == __val) \ + _old = cmpxchg(_ptr, _val, _val | _mask); \ + if (_old == _val) \ break; \ - __val = __old; \ + _val = _old; \ } \ - __old; \ + \ + _old; \ }) #endif diff --git a/kernel/sched/core.c b/kernel/sched/core.c index e5725b931bee..bbd347c04826 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -329,7 +329,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 */