Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933511AbcCOCoU (ORCPT ); Mon, 14 Mar 2016 22:44:20 -0400 Received: from mail-ig0-f175.google.com ([209.85.213.175]:35646 "EHLO mail-ig0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753388AbcCOCoP (ORCPT ); Mon, 14 Mar 2016 22:44:15 -0400 MIME-Version: 1.0 In-Reply-To: <20160314123200.GA15971@gmail.com> References: <20160314123200.GA15971@gmail.com> Date: Mon, 14 Mar 2016 19:44:14 -0700 X-Google-Sender-Auth: Kv3uxwrpRdnNMtCoP9AOnDQ3BKY Message-ID: Subject: Re: [GIT PULL] NOHZ updates for v4.6 From: Linus Torvalds To: Ingo Molnar Cc: Linux Kernel Mailing List , =?UTF-8?B?RnLDqWTDqXJpYyBXZWlzYmVja2Vy?= , Thomas Gleixner , Peter Zijlstra , Andrew Morton Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2618 Lines: 62 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. This macro re-uses the "mask" argument potentially many many times, so semantically it's very dubious. 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. 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. 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. 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. 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. Linus