From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934882AbcCORIq (ORCPT ); Tue, 15 Mar 2016 13:08:46 -0400 Received: from mail-wm0-f46.google.com ([74.125.82.46]:36660 "EHLO mail-wm0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933150AbcCORIl (ORCPT ); Tue, 15 Mar 2016 13:08:41 -0400 Date: Tue, 15 Mar 2016 18:08:37 +0100 From: Frederic Weisbecker To: Ingo Molnar Cc: Linus Torvalds , Linux Kernel Mailing List , Thomas Gleixner , Peter Zijlstra , Andrew Morton Subject: Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Message-ID: <20160315170835.GA5058@lerouge> References: <20160314123200.GA15971@gmail.com> <20160315093245.GA7943@gmail.com> <20160315122145.GA7225@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160315122145.GA7225@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Thanks a lot for fixing this! Some comments below: On Tue, Mar 15, 2016 at 01:21:45PM +0100, Ingo Molnar wrote: > 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. I wouldn't mind that much having xchg_or() but I think Peterz has good arguments in favour of keeping the original name. > > 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); \ Can we add a comment above to prevent from future mistakes with cmpxchg variables aliasing? I'm suprised that GCC doesn't warn about that actually.