From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753391AbcD0VfW (ORCPT ); Wed, 27 Apr 2016 17:35:22 -0400 Received: from mout.kundenserver.de ([212.227.17.10]:53944 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751992AbcD0VfV (ORCPT ); Wed, 27 Apr 2016 17:35:21 -0400 From: Arnd Bergmann To: Martin Jambor Cc: "Martin K. Petersen" , James Bottomley , Josh Poimboeuf , linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, Denys Vlasenko , Thomas Graf , Peter Zijlstra , David Rientjes , Andrew Morton , Ingo Molnar , Himanshu Madhani , qla2xxx-upstream@qlogic.com, Jan Hubicka , Chris Metcalf Subject: Re: [PATCH] scsi: fc: force inlining of wwn conversion functions Date: Wed, 27 Apr 2016 23:34:14 +0200 Message-ID: <4940273.pi2bWg8tAl@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: <20160427110503.GB24887@virgil.suse.cz> References: <20160419085221.GA29087@gmail.com> <5298237.1Guzp0G04x@wuerfel> <20160427110503.GB24887@virgil.suse.cz> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:Z8UMh4XIFpp6PAx1TV1q4BNo9LhU3CxThM2Q4K5UWUNbj7e3SOn LRM5LHPC5X7LDIoIRZLpoaOQn+fE99VubIjJQG4iGL0HkFEz1fjGAosKoW8gfB8MKdcnDUr Y636zQI1W2/Q4xZWaQpBdq1YBnOx8EassJ+qBRJbazz5fqs3YzPVC+Bw28JOKDTdJLepOEi j/+Jq3ay1Ppd4Ncu/0Osg== X-UI-Out-Filterresults: notjunk:1;V01:K0:MWw3z6ssyWA=:g2jWNstE8FsiVPwpvR6V8i MjCHIlgmAiTR4ENna0aIecgJqdHoeOLTZ1zgdKNezAGz+FPJ6yvd4UfNc3xXQ/yew1TXAV7w8 N27FyQkCRVFbqDovz5yMcOklxmLxFMEb3YGGIHbm8kXH328tsQOcW2R7sF7LLN8vhK982VTh0 VqxW0JOiMx9NSlKvbMdEAb8yiZ/mV3j3yqGzZ731ts8QX9WIxBwsm0pBMhPTN7jJy7PHT3fRZ 3h2VdIdBr/+FCdHKpEsQE0GH8UpllA/A+7S9N4ldywpAi9XCHVrGMJhgKwnmWxC5ZSaUPUmPb Nb5yhquk00ahYP6fa08Pw9nY7135P1E/BmEfXypoiuyZpQbDQ45gFJd1OpEqQ48Kq0VqFJjMm /zBeRnxwZHo+35r3SIPXTgPDf3/A72RjfZd7HrMgRvHdjpRIook+W7ON6Y3r55naHeTQHacHN aCu6cWvIF5yznFwKt/gE6+3U/1ZW2IKXup/tKHujnLoMXiWRQYwgb1TyhJHrl1toNRx0SZNzh YqVOYFOGEnkMkG2oSwrGhjJ2LSHNgbkwTmoCDgjgd3rRsFh+Js23H7lHs7wzLI5Yfyl9CuOvX U0/s7dUjuAiFr5Qc9RI7JhUYopxX8HiDgbk5ak970LW2RsgUTzH3fMosapHhxnrQHHz0YmnuM AY4FKQqbl0GrKD+3oSnshHe2hCNUQyqKGkZ9qFJPK6Tg0JXY2d9ibpbQg2c+4+TW7WQ6Csoa5 b0ll10/8FgxmulMz Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wednesday 27 April 2016 13:05:03 Martin Jambor wrote: > On Tue, Apr 26, 2016 at 05:58:20PM +0200, Arnd Bergmann wrote: > > On Tuesday 26 April 2016 09:06:54 Martin K. Petersen wrote: > > > >>>>> "Arnd" == Arnd Bergmann writes: > > > > > > Arnd> I don't think we can realistically blacklist gcc-4.9.{0,1,2,3}, > > > Arnd> gcc-5.{0,1,2,3}.* and gcc-6.0 and require everyone to upgrade to > > > Arnd> compilers that have not been released yet in order to build a > > > Arnd> linux-4.6 kernel. > > > > > > I agree that compiler blacklisting is problematic and I'd like to avoid > > > it. The question is how far we go in the kernel to accommodate various > > > levels of brokenness. > > > > > > In any case. Sticking compiler workarounds in device driver code is akin > > > to putting demolition orders on display on Alpha Centauri. At the very > > > minimum the patch should put a fat comment in the code stating that > > > these wrapper functions or #defines should not be changed in the future > > > because that'll break builds using gcc XYZ. But that does not solve the > > > problem for anybody else that might be doing something similar. > > > Converting between u64 and $RANDOM_TYPE in an inline wrapper does not > > > seem like a rare and unusual programming pattern. > > > > It's not the driver really, it's the core scsi/fc layer, which makes > > it a little dangerous that a random driver. > > > > I agree that putting a comment in would also help. What I understand > > from the bug report is that to trigger this bug you need these elements: > > > > 1. an inline function marked __always_inline > > 2. another inline function that is automatically inlined (not __always_inline) > > 3. CONFIG_OPTIMIZE_INLINING=y to guarantee 2 > > 4. __builtin_compatible_p inside that inline function > > The __always_inline requirement is not true. In fact, if you look at > the example testcase filed in > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c7 you'll see it > uses __builtin_compatible_p in an __always inline function that is > called from one that is not tagged with that attribute. > > And generally speaking, always inline is never a requirement, any call > or chain of calls that the inliner can decide to inline can lead to > the bug (if it complies with the condition below). Ok, thanks for the clarification, I thought you always had to have both kinds of inline functions. > What is a requirement, though, is that __builtin_compatible_p is > called on something passed in an argument by reference or in an > aggregate (i.e. struct or array) argument. > > So, > > int foo1 (unsigned long *ref) > { > if (__builtin_constant (*ref)) > ... > else > /* wrongly unreachable code */ > } > > } > > cannot, and is fine. But please note that wrapping a foo[12]-like > function into a dereferencing wrapper might not help if foo[12] would > be early-inlined into such wrapper (GCC has two inliners, a very > simple early-inliner that only handles simple cases and a full-blown > IPA inliner that contains the bug). I believe this can be ensured by > making the wrapper always_inline and never calling it indirectly (via > a pointer). Honza (CCed), you know inlining heuristics better, please > correct me if my last statement is somehow inaccurate (or indeed if > you have a better idea how kernel developers can make sure they do not > hit the bug). I guess that means that any user of this code in the kernel: static inline __attribute_const__ __u64 __fswab64(__u64 val) { #ifdef __HAVE_BUILTIN_BSWAP64__ return __builtin_bswap64(val); #elif defined (__arch_swab64) return __arch_swab64(val); #elif defined(__SWAB_64_THRU_32__) __u32 h = val >> 32; __u32 l = val & ((1ULL << 32) - 1); return (((__u64)__fswab32(l)) << 32) | ((__u64)(__fswab32(h))); #else return ___constant_swab64(val); #endif } #define __swab64(x) \ (__builtin_constant_p((__u64)(x)) ? \ ___constant_swab64(x) : \ __fswab64(x)) static __always_inline __u64 __swab64p(const __u64 *p) { #ifdef __arch_swab64p return __arch_swab64p(p); #else return __swab64(*p); #endif } has a chance of running into the same problem, and we may want to solve it at the root. For architectures that define __HAVE_BUILTIN_BSWAP64__ (i.e. ARM, MIPS, POWERPC, S390, and x86 with gcc-4.4 or higher, 4.8 for __HAVE_BUILTIN_BSWAP16__), we can probably just change the logic to avoid __builtin_constant_p() and always use __builtin_bswap64(). This won't help on TILE, which is the one architecture that sets ARCH_SUPPORTS_OPTIMIZED_INLINING but does not set ARCH_USE_BUILTIN_BSWAP. Chris Metcalf should be able to figure out whether we can just set ARCH_USE_BUILTIN_BSWAP for tile as well. Arnd