From: James Bottomley <James.Bottomley@HansenPartnership.com>
To: Arnd Bergmann <arnd@arndb.de>,
"Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>,
linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
Denys Vlasenko <dvlasenk@redhat.com>, Thomas Graf <tgraf@suug.ch>,
Peter Zijlstra <peterz@infradead.org>,
David Rientjes <rientjes@google.com>,
Andrew Morton <akpm@linux-foundation.org>,
jamborm@gcc.gnu.org, Ingo Molnar <mingo@kernel.org>,
Himanshu Madhani <himanshu.madhani@qlogic.com>,
qla2xxx-upstream@qlogic.com
Subject: Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
Date: Tue, 26 Apr 2016 15:36:58 -0700 [thread overview]
Message-ID: <1461710218.2348.27.camel@HansenPartnership.com> (raw)
In-Reply-To: <5298237.1Guzp0G04x@wuerfel>
On Tue, 2016-04-26 at 17:58 +0200, Arnd Bergmann wrote:
> On Tuesday 26 April 2016 09:06:54 Martin K. Petersen wrote:
> > > > > > > "Arnd" == Arnd Bergmann <arnd@arndb.de> 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.
Well, it's libfc; that's a fibre channel transport class mostly used by
FCoE drivers ... there's few enough of those to call it driver only.
> 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 last point is what Denys introduced in the kernel with
> bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of
> some byteswap operations"). So maybe it's better after all to revert
> that patch, to have a higher confidence in the same bug not appearing
> elsewhere. It's also really a workaround for another quirk of the
> compiler, but that one only results in duplicated functions in object
> code rather than functions that end in the middle.
Yes, I think this is my preferred option. That patch is nothing more
than an attempt to force the compiler to do something it didn't do but
should have. If we apply the general rule that we shouldn't work
around compiler problems in the kernel code, then that should have been
disallowed first. Plus, as the root cause of all of this, reverting
that patch will ensure that nothing else picks up this problem (at
least from the route we got it).
James
next prev parent reply other threads:[~2016-04-26 22:39 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-04 19:45 [PATCH] asm-generic: force inlining of some atomic_long operations Denys Vlasenko
2016-02-04 19:45 ` [PATCH] force inlining of some byteswap operations Denys Vlasenko
2016-02-05 7:28 ` Ingo Molnar
2016-04-13 3:36 ` This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Josh Poimboeuf
2016-04-13 12:12 ` Denys Vlasenko
2016-04-13 12:36 ` Josh Poimboeuf
2016-04-13 15:15 ` Josh Poimboeuf
2016-04-13 16:55 ` James Bottomley
2016-04-13 17:10 ` Josh Poimboeuf
2016-04-14 15:29 ` Denys Vlasenko
2016-04-14 15:57 ` Josh Poimboeuf
2016-04-14 17:09 ` Denys Vlasenko
2016-04-15 5:45 ` Ingo Molnar
2016-04-15 13:47 ` Josh Poimboeuf
2016-04-15 22:20 ` Josh Poimboeuf
2016-04-16 9:03 ` Ingo Molnar
2016-04-18 13:39 ` Josh Poimboeuf
2016-04-18 14:07 ` Arnd Bergmann
2016-04-18 14:12 ` Josh Poimboeuf
2016-04-18 14:21 ` Arnd Bergmann
2016-04-19 8:52 ` Ingo Molnar
2016-04-19 13:56 ` [PATCH] scsi: fc: force inlining of wwn conversion functions Josh Poimboeuf
2016-04-22 23:17 ` Quinn Tran
2016-04-25 16:07 ` Josh Poimboeuf
2016-04-26 2:40 ` Martin K. Petersen
2016-04-26 3:37 ` James Bottomley
2016-04-26 7:22 ` Arnd Bergmann
2016-04-26 8:35 ` Christoph Hellwig
2016-04-26 10:05 ` Arnd Bergmann
2016-04-26 13:06 ` Martin K. Petersen
2016-04-26 15:58 ` Arnd Bergmann
2016-04-26 22:36 ` James Bottomley [this message]
2016-04-27 0:44 ` Martin K. Petersen
2016-04-27 11:05 ` Martin Jambor
2016-04-27 21:34 ` Arnd Bergmann
2016-04-28 14:58 ` Chris Metcalf
2016-04-28 15:23 ` Arnd Bergmann
2016-04-28 15:48 ` Chris Metcalf
2016-04-27 22:00 ` [PATCH, RFT] byteswap: try to avoid __builtin_constant_p gcc bug Arnd Bergmann
2016-04-27 22:11 ` Josh Poimboeuf
2016-04-28 16:27 ` Quinn Tran
2016-04-16 7:42 ` This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Arnd Bergmann
2016-04-18 13:22 ` Josh Poimboeuf
2016-02-04 19:45 ` [PATCH] force inlining of unaligned byteswap operations Denys Vlasenko
2016-02-05 7:28 ` Ingo Molnar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1461710218.2348.27.camel@HansenPartnership.com \
--to=james.bottomley@hansenpartnership.com \
--cc=akpm@linux-foundation.org \
--cc=arnd@arndb.de \
--cc=dvlasenk@redhat.com \
--cc=himanshu.madhani@qlogic.com \
--cc=jamborm@gcc.gnu.org \
--cc=jpoimboe@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=mingo@kernel.org \
--cc=peterz@infradead.org \
--cc=qla2xxx-upstream@qlogic.com \
--cc=rientjes@google.com \
--cc=tgraf@suug.ch \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).