Re: [PATCH 2/6] x86: bug: prevent gcc distortions
On May 18, 2018 12:36:20 PM PDT, Nadav Amit wrote: >h...@zytor.com wrote: > >> On May 18, 2018 12:21:00 PM PDT, Linus Torvalds > wrote: >>> On Fri, May 18, 2018 at 12:18 PM Nadav Amit >wrote: >>> Gnu ASM manual says: "Each time you run as it assembles exactly one >>> source program. The source program is made up of one or more files.” >>> >>> Ok, that counts as documentation, although it's confusing as hell. >Is >>> it >>> one source program or multiple files again? I see what they are >trying >>> to >>> say, but it really could be phrased more clearly ;) >>> >>>Linus >> >> At least we have a solution! > >Thanks for all your help. I will try to do it over over the weekend. > >Funny, I see that this “trick” (-Wa,[filename.s]) is already being used >to >include arch/x86/boot/code16gcc.h so I feel “safer” to use it. > >Regards, >Nadav Oh. I don't remember when we did that... might even be my doing and then I just forgot about it :) -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
h...@zytor.com wrote: > On May 18, 2018 12:21:00 PM PDT, Linus Torvalds > wrote: >> On Fri, May 18, 2018 at 12:18 PM Nadav Amit wrote: >> >>> Gnu ASM manual says: "Each time you run as it assembles exactly one >> source >>> program. The source program is made up of one or more files.” >> >> Ok, that counts as documentation, although it's confusing as hell. Is >> it >> one source program or multiple files again? I see what they are trying >> to >> say, but it really could be phrased more clearly ;) >> >>Linus > > At least we have a solution! Thanks for all your help. I will try to do it over over the weekend. Funny, I see that this “trick” (-Wa,[filename.s]) is already being used to include arch/x86/boot/code16gcc.h so I feel “safer” to use it. Regards, Nadav
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
On May 18, 2018 12:21:00 PM PDT, Linus Torvalds wrote: >On Fri, May 18, 2018 at 12:18 PM Nadav Amit wrote: > >> Gnu ASM manual says: "Each time you run as it assembles exactly one >source >> program. The source program is made up of one or more files.” > >Ok, that counts as documentation, although it's confusing as hell. Is >it >one source program or multiple files again? I see what they are trying >to >say, but it really could be phrased more clearly ;) > > Linus At least we have a solution! -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
On Fri, May 18, 2018 at 12:18 PM Nadav Amit wrote: > Gnu ASM manual says: "Each time you run as it assembles exactly one source > program. The source program is made up of one or more files.” Ok, that counts as documentation, although it's confusing as hell. Is it one source program or multiple files again? I see what they are trying to say, but it really could be phrased more clearly ;) Linus
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
Linus Torvalds wrote: > On Fri, May 18, 2018 at 12:02 PM Nadav Amit wrote: > >> I can add a -Wa,[filename.s] switch. It works, but sort of undocumented. > > Oh, if it assembles things together, then that sounds optimal. > > And yes, like hpa says, we should make sure that behavior is acknowledged > by the GNU as people, so that they then don't come back and say "hey, now > we assemble things as separate units". > > That said, the "separate units" model really doesn't make much sense now > that I think about it, because 'as' supports only one output file. So I > guess the as people wouldn't have any issues with just accepting the > "concatenate all input files" syntax. Gnu ASM manual says: "Each time you run as it assembles exactly one source program. The source program is made up of one or more files.”
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
On Fri, May 18, 2018 at 12:02 PM Nadav Amit wrote: > I can add a -Wa,[filename.s] switch. It works, but sort of undocumented. Oh, if it assembles things together, then that sounds optimal. And yes, like hpa says, we should make sure that behavior is acknowledged by the GNU as people, so that they then don't come back and say "hey, now we assemble things as separate units". That said, the "separate units" model really doesn't make much sense now that I think about it, because 'as' supports only one output file. So I guess the as people wouldn't have any issues with just accepting the "concatenate all input files" syntax. Linus
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
On May 18, 2018 12:02:50 PM PDT, Nadav Amit wrote: >h...@zytor.com wrote: > >> On May 18, 2018 11:50:12 AM PDT, Linus Torvalds > wrote: >>> On Fri, May 18, 2018 at 11:34 AM wrote: >>> On May 18, 2018 11:25:32 AM PDT, Linus Torvalds < >>> torva...@linux-foundation.org> wrote: >>> Unfortunately gcc doesn't guarantee that global assembly inlines >will >>> appear at the top of the file. >>> >>> Yeah. It really would be better to do the "asm version of -inline". >>> >>> We already do something like that for real *.S files on some >>> architectures >>> (because their assembly really wants it, eg >>> >>> arch/arm/Makefile: >>> KBUILD_AFLAGS +=$(CFLAGS_ABI) $(AFLAGS_ISA) $(arch-y) $(tune-y) >>> -include >>> asm/unified.h -msoft-float >>> >>> but I do want to point out that KBUILD_AFLAGS is *not* used for >>> compiler-generated assembly, only for actual *.S files. >>> >>> Sadly, I don't actually know any way to make gcc call the 'as' phase >>> with >>> particular options. We can use "-Wa,xyzzy" to pass in xyzzy to the >>> assembler, but there is no "-include" option for GNU as afaik. >>> >>> Can you perhaps define a macro symbol for "--defsym"? Probably not. >>> >>> Linus >> >> I looked at this thing a long time ago; it's not there, and the best >would probably be to get global asm() working properly in gcc. > >I can add a -Wa,[filename.s] switch. It works, but sort of >undocumented. Ah, I thought that would have assembled each file separately. We can request it be documented, that's usually much easier than getting a new feature implemented. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
h...@zytor.com wrote: > On May 18, 2018 11:50:12 AM PDT, Linus Torvalds > wrote: >> On Fri, May 18, 2018 at 11:34 AM wrote: >> >>> On May 18, 2018 11:25:32 AM PDT, Linus Torvalds < >> torva...@linux-foundation.org> wrote: >> >>> Unfortunately gcc doesn't guarantee that global assembly inlines will >> appear at the top of the file. >> >> Yeah. It really would be better to do the "asm version of -inline". >> >> We already do something like that for real *.S files on some >> architectures >> (because their assembly really wants it, eg >> >> arch/arm/Makefile: >> KBUILD_AFLAGS +=$(CFLAGS_ABI) $(AFLAGS_ISA) $(arch-y) $(tune-y) >> -include >> asm/unified.h -msoft-float >> >> but I do want to point out that KBUILD_AFLAGS is *not* used for >> compiler-generated assembly, only for actual *.S files. >> >> Sadly, I don't actually know any way to make gcc call the 'as' phase >> with >> particular options. We can use "-Wa,xyzzy" to pass in xyzzy to the >> assembler, but there is no "-include" option for GNU as afaik. >> >> Can you perhaps define a macro symbol for "--defsym"? Probably not. >> >> Linus > > I looked at this thing a long time ago; it's not there, and the best would > probably be to get global asm() working properly in gcc. I can add a -Wa,[filename.s] switch. It works, but sort of undocumented.
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
On May 18, 2018 11:50:12 AM PDT, Linus Torvalds wrote: >On Fri, May 18, 2018 at 11:34 AM wrote: > >> On May 18, 2018 11:25:32 AM PDT, Linus Torvalds < >torva...@linux-foundation.org> wrote: > >> Unfortunately gcc doesn't guarantee that global assembly inlines will >appear at the top of the file. > >Yeah. It really would be better to do the "asm version of -inline". > >We already do something like that for real *.S files on some >architectures >(because their assembly really wants it, eg > > arch/arm/Makefile: >KBUILD_AFLAGS +=$(CFLAGS_ABI) $(AFLAGS_ISA) $(arch-y) $(tune-y) >-include >asm/unified.h -msoft-float > >but I do want to point out that KBUILD_AFLAGS is *not* used for >compiler-generated assembly, only for actual *.S files. > >Sadly, I don't actually know any way to make gcc call the 'as' phase >with >particular options. We can use "-Wa,xyzzy" to pass in xyzzy to the >assembler, but there is no "-include" option for GNU as afaik. > >Can you perhaps define a macro symbol for "--defsym"? Probably not. > > Linus I looked at this thing a long time ago; it's not there, and the best would probably be to get global asm() working properly in gcc. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
On Fri, May 18, 2018 at 11:34 AM wrote: > On May 18, 2018 11:25:32 AM PDT, Linus Torvalds < torva...@linux-foundation.org> wrote: > Unfortunately gcc doesn't guarantee that global assembly inlines will appear at the top of the file. Yeah. It really would be better to do the "asm version of -inline". We already do something like that for real *.S files on some architectures (because their assembly really wants it, eg arch/arm/Makefile: KBUILD_AFLAGS +=$(CFLAGS_ABI) $(AFLAGS_ISA) $(arch-y) $(tune-y) -include asm/unified.h -msoft-float but I do want to point out that KBUILD_AFLAGS is *not* used for compiler-generated assembly, only for actual *.S files. Sadly, I don't actually know any way to make gcc call the 'as' phase with particular options. We can use "-Wa,xyzzy" to pass in xyzzy to the assembler, but there is no "-include" option for GNU as afaik. Can you perhaps define a macro symbol for "--defsym"? Probably not. Linus
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
On May 18, 2018 11:25:32 AM PDT, Linus Torvalds wrote: >On Fri, May 18, 2018 at 10:24 AM Nadav Amit wrote: > >> Will it be ok just to use a global inline asm to set an “.include” >directive >> that gas would later process? (I can probably wrap it in a C macro so >it >> won’t be too disgusting) > >Maybe. I'd almost prefer it to be the automatic kind of thing that we >already do for C files using "-include". > > Linus Unfortunately gcc doesn't guarantee that global assembly inlines will appear at the top of the file. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
On Fri, May 18, 2018 at 10:24 AM Nadav Amit wrote: > Will it be ok just to use a global inline asm to set an “.include” directive > that gas would later process? (I can probably wrap it in a C macro so it > won’t be too disgusting) Maybe. I'd almost prefer it to be the automatic kind of thing that we already do for C files using "-include". Linus
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
On Fri, 2018-05-18 at 14:22 +, Nadav Amit wrote: > It is hard to make the code readable in C, readable in the generated asm, > and to follow the coding style imposed by checkpatch (e.g., no space between > the newline and the asm argument before it). Ignore checkpatch when you know better.
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
On May 18, 2018 6:29:59 PM GMT+02:00, Nadav Amit wrote: >Funny. I found in my mailbox that you once wrote me: "It is a dumb >idea, it >doesn't bring us anything besides some superficial readability which >you >don't really need.” How about a proper quotation with the Message-id you're referring to? -- Sent from a small device: formatting sux and brevity is inevitable.
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
Linus Torvalds wrote:
> On Fri, May 18, 2018 at 12:59 AM Peter Zijlstra
> wrote:
>
>> This is an awesome hack, but is there really nothing we can do to make
>> it more readable? Esp, that global asm doing the macro definition is a
>> pain to read.
>
> I actually find that macro to be *more* legible than what we do now,
> although I'm not enamored with the pseudo-operation name ("__BUG_FLAGS").
>
> That said, the C header code itself I don't love.
>
> I wonder if we should just introduce a new assembler header file, and get
> it included when processing compiler-generated asm. We already do that for
> our _real_ *.S files, with a number of our header files having constants
> and code for the asm case too, not just C.
>
> But we could have an header file that has these kinds of
> macros (or "pseudo-instructions") for assembly language cases, and then we
> could just rely on them in inline asm.
Will it be ok just to use a global inline asm to set an “.include” directive
that gas would later process? (I can probably wrap it in a C macro so it
won’t be too disgusting)
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
Borislav Petkov wrote: > On Fri, May 18, 2018 at 03:46:33PM +, Nadav Amit wrote: >> In case you didn’t read the cover-letter: the patch-set does give a 2% >> performance improvement for #PF-MADV_DONTNEED microbenchmark loop. > > I saw it but *micro*-benchmark doesn't tell me a whole lot. If that > "improvement" is not visible in real workloads/benchmarks, then I'm just > as unimpressed. Funny. I found in my mailbox that you once wrote me: "It is a dumb idea, it doesn't bring us anything besides some superficial readability which you don't really need.” To the point, I think you exaggerate with the effect of the patch on the code readability: it is not much uglier than it was before, and the change is in a very specific point in the code.
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
On Fri, May 18, 2018 at 12:59 AM Peter Zijlstra
wrote:
> This is an awesome hack, but is there really nothing we can do to make
> it more readable? Esp, that global asm doing the macro definition is a
> pain to read.
I actually find that macro to be *more* legible than what we do now,
although I'm not enamored with the pseudo-operation name ("__BUG_FLAGS").
That said, the C header code itself I don't love.
I wonder if we should just introduce a new assembler header file, and get
it included when processing compiler-generated asm. We already do that for
our _real_ *.S files, with a number of our header files having constants
and code for the asm case too, not just C.
But we could have an header file that has these kinds of
macros (or "pseudo-instructions") for assembly language cases, and then we
could just rely on them in inline asm.
Because if you want to see illegible, look at what we currently generate:
# kernel/exit.c:1761: BUG();
#APP
# 1761 "kernel/exit.c" 1
1: .byte 0x0f, 0x0b
.pushsection __bug_table,"aw"
2: .long 1b - 2b # bug_entry::bug_addr
.long .LC0 - 2b # bug_entry::file #
.word 1761 # bug_entry::line #
.word 0 # bug_entry::flags #
.org 2b+12 #
.popsection
# 0 "" 2
# 1761 "kernel/exit.c" 1
180:#
.pushsection .discard.unreachable
.long 180b - . #
.popsection
# 0 "" 2
#NO_APP
and tell me that's legible.. Of course, I'm probably one of the few people
who actually look at the generated asm fairly regularly.
So a few macros that we can use in inline asm definitely wouldn't hurt
legibility. And if we actually can put them in a header file as legible
code - instead of having to wrap them in a global "asm()" macro in C code,
they'd probably be legible at a source level too.
It's not just the bug_flags cases. It's things like jump labels too:
# ./arch/x86/include/asm/jump_label.h:36: asm_volatile_goto("1:"
#APP
# 36 "./arch/x86/include/asm/jump_label.h" 1
1:.byte 0x0f,0x1f,0x44,0x00,0
.pushsection __jump_table, "aw"
.balign 8
.quad 1b, .L71, __tracepoint_sched_process_free+8 + 0 #,,
.popsection
# 0 "" 2
#NO_APP
and atomics:
# ./arch/x86/include/asm/atomic.h:122: GEN_UNARY_RMWcc(LOCK_PREFIX
"decl", v->counter, "%0", e);
#APP
# 122 "./arch/x86/include/asm/atomic.h" 1
.pushsection .smp_locks,"a"
.balign 4
.long 671f - .
.popsection
671:
lock; decl -2336(%rbp) # _7->counter
/* output condition code e*/
# 0 "" 2
# ./include/linux/sched/task.h:95: if (atomic_dec_and_test(&t->usage))
#NO_APP
where I suspect we could hide the whole "lock" magic in a macro, and make
this much more legible.
Maybe? I think it might be worth trying. It's possible that the macro games
themselves would just cause enough pain to make any gains go away.
Linus
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
On Fri, May 18, 2018 at 03:46:33PM +, Nadav Amit wrote: > In case you didn’t read the cover-letter: the patch-set does give a 2% > performance improvement for #PF-MADV_DONTNEED microbenchmark loop. I saw it but *micro*-benchmark doesn't tell me a whole lot. If that "improvement" is not visible in real workloads/benchmarks, then I'm just as unimpressed. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
Borislav Petkov wrote: > On Fri, May 18, 2018 at 02:36:21PM +, Nadav Amit wrote: >> I didn’t try too hard to find more affected (micro)benchmarks, but I am >> pretty sure there are: > > So you being pretty sure there are, doesn't make me go, oh, ok, then, > this is an uglification we should try to live with. It is still ugly > with no proven benefit from it. In case you didn’t read the cover-letter: the patch-set does give a 2% performance improvement for #PF-MADV_DONTNEED microbenchmark loop.
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
On Fri, May 18, 2018 at 02:36:21PM +, Nadav Amit wrote: > I didn’t try too hard to find more affected (micro)benchmarks, but I am > pretty sure there are: So you being pretty sure there are, doesn't make me go, oh, ok, then, this is an uglification we should try to live with. It is still ugly with no proven benefit from it. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
Borislav Petkov wrote: > On Fri, May 18, 2018 at 10:13:54AM +0200, Ingo Molnar wrote: >> Yes, that's my main worry too about all these inlining changes: >> the very, very marked reduction in the readability of assembly code. > > Same reaction here: the small improvements this brings is simply not > enough to sacrifice readability so much IMO. I didn’t try too hard to find more affected (micro)benchmarks, but I am pretty sure there are: pretty much all the paravirt ops are currently not inlined, which can affect kernel operations that bang on the page-tables. Notice that this is not an x86-specific issue. Other architectures, with not-as-good OOOE for instance, might incur higher overheads. So I don’t think that this issue should be ignored.
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
Ingo Molnar wrote:
>
> * Peter Zijlstra wrote:
>
>> On Thu, May 17, 2018 at 09:13:58AM -0700, Nadav Amit wrote:
>>> +asm(".macro __BUG_FLAGS ins:req file:req line:req flags:req size:req\n"
>>> +"1:\t \\ins\n\t"
>>> +".pushsection __bug_table,\"aw\"\n"
>>> +"2:\t "__BUG_REL(1b) "\t# bug_entry::bug_addr\n\t"
>>> +__BUG_REL(\\file) "\t# bug_entry::file\n\t"
>>> +".word \\line" "\t# bug_entry::line\n\t"
>>> +".word \\flags""\t# bug_entry::flags\n\t"
>>> +".org 2b+\\size\n\t"
>>> +".popsection\n\t"
>>> +".endm");
>>> +
>>> +#define _BUG_FLAGS(ins, flags) \
>>> do {
>>> \
>>> + asm volatile("__BUG_FLAGS \"" ins "\" %c0 %c1 %c2 %c3" \
>>> +: : "i" (__FILE__), "i" (__LINE__),\
>>> +"i" (flags), \
>>> "i" (sizeof(struct bug_entry))); \
>>> } while (0)
>>
>> This is an awesome hack, but is there really nothing we can do to make
>> it more readable? Esp, that global asm doing the macro definition is a
>> pain to read.
>>
>> Also, can we pretty please used named operands in 'new' code?
>
> Yes, that's my main worry too about all these inlining changes:
> the very, very marked reduction in the readability of assembly code.
>
> It's bad to an extent that I'm questioning the wisdom of pandering to a
> compiler
> limitation to begin with?
>
> How about asking GCC for an attribute where we can specify the inlined size
> of an
> asm() function? Even if we'll just approximate it due to some vagaries of
> actual
> code generation related to how arguments interact with GCC, an explicit byte
> value
> will do a heck of a better job of it than some sort of implied, vague 'number
> of
> newlines' heuristics ...
If it were to become a GCC feature, I think it is best to be a builtin that
says: consider the enclosed expression as “free”. The problem of poor
inlining decisions is not specific to inline asm. As I mentioned in the RFC,
when there are two code paths for constants and variables based on
__builtin_constant_p(), you can get the “cost” of the constant path for
variables.
It is not hard to add such a feature to GCC, but I don’t know how easy it is
to get new features into the compiler.
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
Peter Zijlstra wrote:
> On Thu, May 17, 2018 at 09:13:58AM -0700, Nadav Amit wrote:
>> +asm(".macro __BUG_FLAGS ins:req file:req line:req flags:req size:req\n"
>> +"1:\t \\ins\n\t"
>> +".pushsection __bug_table,\"aw\"\n"
>> +"2:\t "__BUG_REL(1b)"\t# bug_entry::bug_addr\n\t"
>> +__BUG_REL(\\file) "\t# bug_entry::file\n\t"
>> +".word \\line" "\t# bug_entry::line\n\t"
>> +".word \\flags" "\t# bug_entry::flags\n\t"
>> +".org 2b+\\size\n\t"
>> +".popsection\n\t"
>> +".endm");
>> +
>> +#define _BUG_FLAGS(ins, flags) \
>> do { \
>> +asm volatile("__BUG_FLAGS \"" ins "\" %c0 %c1 %c2 %c3" \
>> + : : "i" (__FILE__), "i" (__LINE__),\
>> + "i" (flags), \
>> "i" (sizeof(struct bug_entry))); \
>> } while (0)
>
> This is an awesome hack, but is there really nothing we can do to make
> it more readable? Esp, that global asm doing the macro definition is a
> pain to read.
>
> Also, can we pretty please used named operands in 'new' code?
It is hard to make the code readable in C, readable in the generated asm,
and to follow the coding style imposed by checkpatch (e.g., no space between
the newline and the asm argument before it).
I considered wrapping the asm macro in a C macro, but AFAIK C macros cannot
emit backslashes.
I thought of suggesting to change “ins” into a vararg and removing the
escaped double-quotes in the C macro, but you ask to use named operands.
So I am out of ideas. Do you have anything else in mind?
Thanks,
Nadav
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
On Fri, May 18, 2018 at 10:13:54AM +0200, Ingo Molnar wrote: > Yes, that's my main worry too about all these inlining changes: > the very, very marked reduction in the readability of assembly code. Same reaction here: the small improvements this brings is simply not enough to sacrifice readability so much IMO. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
* Peter Zijlstra wrote:
> On Thu, May 17, 2018 at 09:13:58AM -0700, Nadav Amit wrote:
> > +asm(".macro __BUG_FLAGS ins:req file:req line:req flags:req size:req\n"
> > +"1:\t \\ins\n\t"
> > +".pushsection __bug_table,\"aw\"\n"
> > +"2:\t "__BUG_REL(1b) "\t# bug_entry::bug_addr\n\t"
> > +__BUG_REL(\\file) "\t# bug_entry::file\n\t"
> > +".word \\line" "\t# bug_entry::line\n\t"
> > +".word \\flags""\t# bug_entry::flags\n\t"
> > +".org 2b+\\size\n\t"
> > +".popsection\n\t"
> > +".endm");
> > +
> > +#define _BUG_FLAGS(ins, flags) \
> > do {
> > \
> > + asm volatile("__BUG_FLAGS \"" ins "\" %c0 %c1 %c2 %c3" \
> > +: : "i" (__FILE__), "i" (__LINE__),\
> > +"i" (flags), \
> > "i" (sizeof(struct bug_entry))); \
> > } while (0)
>
> This is an awesome hack, but is there really nothing we can do to make
> it more readable? Esp, that global asm doing the macro definition is a
> pain to read.
>
> Also, can we pretty please used named operands in 'new' code?
Yes, that's my main worry too about all these inlining changes:
the very, very marked reduction in the readability of assembly code.
It's bad to an extent that I'm questioning the wisdom of pandering to a
compiler
limitation to begin with?
How about asking GCC for an attribute where we can specify the inlined size of
an
asm() function? Even if we'll just approximate it due to some vagaries of
actual
code generation related to how arguments interact with GCC, an explicit byte
value
will do a heck of a better job of it than some sort of implied, vague 'number
of
newlines' heuristics ...
Thanks,
Ingo
Re: [PATCH 2/6] x86: bug: prevent gcc distortions
On Thu, May 17, 2018 at 09:13:58AM -0700, Nadav Amit wrote:
> +asm(".macro __BUG_FLAGS ins:req file:req line:req flags:req size:req\n"
> +"1:\t \\ins\n\t"
> +".pushsection __bug_table,\"aw\"\n"
> +"2:\t "__BUG_REL(1b) "\t# bug_entry::bug_addr\n\t"
> +__BUG_REL(\\file)"\t# bug_entry::file\n\t"
> +".word \\line" "\t# bug_entry::line\n\t"
> +".word \\flags" "\t# bug_entry::flags\n\t"
> +".org 2b+\\size\n\t"
> +".popsection\n\t"
> +".endm");
> +
> +#define _BUG_FLAGS(ins, flags) \
> do { \
> + asm volatile("__BUG_FLAGS \"" ins "\" %c0 %c1 %c2 %c3" \
> + : : "i" (__FILE__), "i" (__LINE__),\
> + "i" (flags), \
>"i" (sizeof(struct bug_entry))); \
> } while (0)
This is an awesome hack, but is there really nothing we can do to make
it more readable? Esp, that global asm doing the macro definition is a
pain to read.
Also, can we pretty please used named operands in 'new' code?
[PATCH 2/6] x86: bug: prevent gcc distortions
GCC considers the number of statements in inlined assembly blocks,
according to new-lines and semicolons, as an indication to the cost of
the block in time and space. This data is distorted by the kernel code,
which puts information in alternative sections. As a result, the
compiler may perform incorrect inlining and branch optimizations.
The solution is to set an assembly macro and call it from the inlinedv
assembly block. As a result GCC considers the inline assembly block as
a single instruction.
This patch increases the kernel size:
textdata bss dec hex filename
18126824 10067268 2936832 31130924 1db052c ./vmlinux before
18127205 10068388 2936832 31132425 1db0b09 ./vmlinux after (+1501)
But enables more aggressive inlining (and probably branch decisions).
The number of static text symbols in vmlinux is lower.
Before: 40015
After: 39860 (-155)
Cc: Thomas Gleixner
Cc: Ingo Molnar
Cc: "H. Peter Anvin"
Cc: x...@kernel.org
Cc: Peter Zijlstra
Cc: Josh Poimboeuf
Signed-off-by: Nadav Amit
---
arch/x86/include/asm/bug.h | 56 +-
1 file changed, 37 insertions(+), 19 deletions(-)
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index 6804d6642767..1167e4822a34 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -30,33 +30,51 @@
#ifdef CONFIG_DEBUG_BUGVERBOSE
-#define _BUG_FLAGS(ins, flags) \
+/*
+ * Saving the bug data is encapsulated within an assembly macro, which is then
+ * called on each use. This hack is necessary to prevent GCC from considering
+ * the inline assembly blocks as costly in time and space, which can prevent
+ * function inlining and lead to other bad compilation decisions. GCC computes
+ * inline assembly cost according to the number of perceived number of assembly
+ * instruction, based on the number of new-lines and semicolons in the assembly
+ * block. The macro will eventually be compiled into a single instruction (and
+ * some data). This scheme allows GCC to better understand the inline asm cost.
+ */
+asm(".macro __BUG_FLAGS ins:req file:req line:req flags:req size:req\n"
+"1:\t \\ins\n\t"
+".pushsection __bug_table,\"aw\"\n"
+"2:\t "__BUG_REL(1b) "\t# bug_entry::bug_addr\n\t"
+__BUG_REL(\\file) "\t# bug_entry::file\n\t"
+".word \\line" "\t# bug_entry::line\n\t"
+".word \\flags""\t# bug_entry::flags\n\t"
+".org 2b+\\size\n\t"
+".popsection\n\t"
+".endm");
+
+#define _BUG_FLAGS(ins, flags) \
do { \
- asm volatile("1:\t" ins "\n"\
-".pushsection __bug_table,\"aw\"\n"\
-"2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \
-"\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \
-"\t.word %c1""\t# bug_entry::line\n" \
-"\t.word %c2""\t# bug_entry::flags\n" \
-"\t.org 2b+%c3\n" \
-".popsection" \
-: : "i" (__FILE__), "i" (__LINE__),\
-"i" (flags), \
+ asm volatile("__BUG_FLAGS \"" ins "\" %c0 %c1 %c2 %c3" \
+: : "i" (__FILE__), "i" (__LINE__),\
+"i" (flags), \
"i" (sizeof(struct bug_entry))); \
} while (0)
#else /* !CONFIG_DEBUG_BUGVERBOSE */
+asm(".macro __BUG_FLAGS ins:req flags:req size:req\n"
+"1:\t\\ins\n\t"
+".pushsection __bug_table,\"aw\"\n"
+"2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n\t"
+".word \\flags""\t# bug_entry::flags\n\t"
+".org 2b+\\size\n\t"
+".popsection\n\t"
+".endm");
+
#define _BUG_FLAGS(ins, flags) \
do { \
- asm volatile("1:\t" ins "\n"\
-".pushsection __bug_table,\"aw\"\n"\
-"2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \
-"\t.word %c0""\t# bug_entry::flags\n" \
-"\t.org 2b+%c1\n" \
-".popsection" \
-: : "i" (flags), \
-"i" (sizeof(struct bug_entry))); \
+ asm volatile("__BUG_FLAGS \"" ins "\" %c0 %c1" \
+ : : "i" (flags),
