Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1432285 > unrolled thread

Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Sedat Dilek <sedat.dilek@gmail.com> - 2016-06-27 22:00 +0200
    Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Sedat Dilek <sedat.dilek@gmail.com> - 2016-06-27 22:10 +0200
    Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-27 22:20 +0200
      Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Sedat Dilek <sedat.dilek@gmail.com> - 2016-06-27 22:30 +0200
        Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-27 22:40 +0200

#1432285 — Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning

[Multipart message — attachments visible in raw view] — view raw

On Mon, Mar 7, 2016 at 7:30 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Mar 7, 2016 at 10:07 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>>
>> Of course, there are other ways to save a single flag value (such as
>> setz).  It's up to the compiler developers to decide what they think is
>> best.
>
> Using 'setcc' to save eflags somewhere is definitely the right thing to do.
>
> Using pushf/popf in generated code is completely insane (unless done
> very localized in a controlled area).
>
> It is, in fact, insane and wrong even in user space, since eflags does
> contain bits that user space itself might be modifying.
>
> In fact, even IF may be modified with iopl 3 (thing old X server
> setups), but ignoring that flag entirely, you have AC that acts in
> very similar ways (system-wide alignment control) that user space
> might be using to make sure it doesn't have unaligned accesses.
>
> It's rare, yes. But still - this isn't really limited to just the kernel.
>
> But perhaps more importantly, I suspect using pushf/popf isn't just
> semantically the wrong thing to do, it's just plain stupid. It's
> likely slower than the obvious 'setcc' model. Agner Fog's table shows
> it "popf" as being 25-30 uops on several microarchitectures. Looks
> like it's often microcode.
>
> Now, pushf/popf may well be fairly cheap on *some* uarchitectures, but
> it really sounds like a bad idea to use it when not absolutely
> required. And that is completely independent of the fact that is
> screws up the IF bit.
>
> But yeah, for the kernel we at a minimum need a way to disable that
> code generation, even if the clang guys might have some insane reason
> to keep it for other cases.
>

I am testing my new llvm-toolchain v3.8.1 and a pending x86/hweight
fix [1] encouraged me to look at this again.

In [2] I found a simple test program from Michael Hordijk.
( See thread "[LLVMdev] optimizer clobber EFLAGS". )

This is what I see...

$ objdump -S clang-eflag.o

clang-eflag.o:     file format elf64-x86-64


Disassembly of section .text:

0000000000000000 <bar>:
   0:   55                      push   %rbp
   1:   48 89 e5                mov    %rsp,%rbp
   4:   53                      push   %rbx
   5:   50                      push   %rax
   6:   e8 00 00 00 00          callq  b <bar+0xb>
   b:   ff 0d 00 00 00 00       decl   0x0(%rip)        # 11 <bar+0x11>
  11:   9c                      pushfq
  12:   5b                      pop    %rbx
  13:   e8 00 00 00 00          callq  18 <bar+0x18>
  18:   b8 01 00 00 00          mov    $0x1,%eax
  1d:   53                      push   %rbx
  1e:   9d                      popfq
  1f:   75 07                   jne    28 <bar+0x28>
  21:   e8 00 00 00 00          callq  26 <bar+0x26>
  26:   31 c0                   xor    %eax,%eax
  28:   48 83 c4 08             add    $0x8,%rsp
  2c:   5b                      pop    %rbx
  2d:   5d                      pop    %rbp
  2e:   c3                      retq

So, the issue is still alive.

What do you mean by "for the kernel we at a minimum need a way to
disable that code generation"?
Can this be fixed in the Linux-kernel?

I asked parallelly the people involved in [2] if there are any news on that.

- Sedat -

[1] http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?id=f5967101e9de12addcda4510dfbac66d7c5779c3
[2] http://lists.llvm.org/pipermail/llvm-dev/2015-July/088766.html

[toc] | [next] | [standalone]

#1432289

On Mon, Jun 27, 2016 at 9:50 PM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
> On Mon, Mar 7, 2016 at 7:30 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Mon, Mar 7, 2016 at 10:07 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>>>
>>> Of course, there are other ways to save a single flag value (such as
>>> setz).  It's up to the compiler developers to decide what they think is
>>> best.
>>
>> Using 'setcc' to save eflags somewhere is definitely the right thing to do.
>>
>> Using pushf/popf in generated code is completely insane (unless done
>> very localized in a controlled area).
>>
>> It is, in fact, insane and wrong even in user space, since eflags does
>> contain bits that user space itself might be modifying.
>>
>> In fact, even IF may be modified with iopl 3 (thing old X server
>> setups), but ignoring that flag entirely, you have AC that acts in
>> very similar ways (system-wide alignment control) that user space
>> might be using to make sure it doesn't have unaligned accesses.
>>
>> It's rare, yes. But still - this isn't really limited to just the kernel.
>>
>> But perhaps more importantly, I suspect using pushf/popf isn't just
>> semantically the wrong thing to do, it's just plain stupid. It's
>> likely slower than the obvious 'setcc' model. Agner Fog's table shows
>> it "popf" as being 25-30 uops on several microarchitectures. Looks
>> like it's often microcode.
>>
>> Now, pushf/popf may well be fairly cheap on *some* uarchitectures, but
>> it really sounds like a bad idea to use it when not absolutely
>> required. And that is completely independent of the fact that is
>> screws up the IF bit.
>>
>> But yeah, for the kernel we at a minimum need a way to disable that
>> code generation, even if the clang guys might have some insane reason
>> to keep it for other cases.
>>
>
> I am testing my new llvm-toolchain v3.8.1 and a pending x86/hweight
> fix [1] encouraged me to look at this again.

Just for the sake of completeness:

I use the latest Linux v4.4.y LTS for testing (here: v4.4.14) with a
custom llvmlinux-amd64 patchset (on demand I can send it to you).
( With CONFIG_TRACING_SUPPORT=n and CONFIG_PARAVIRT=n )

- Sedat -

[toc] | [prev] | [next] | [standalone]

#1432292

On Mon, Jun 27, 2016 at 12:50 PM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
> $ objdump -S clang-eflag.o
>
> clang-eflag.o:     file format elf64-x86-64
>
>
> Disassembly of section .text:
>
> 0000000000000000 <bar>:
>    0:   55                      push   %rbp
>    1:   48 89 e5                mov    %rsp,%rbp
>    4:   53                      push   %rbx
>    5:   50                      push   %rax
>    6:   e8 00 00 00 00          callq  b <bar+0xb>
>    b:   ff 0d 00 00 00 00       decl   0x0(%rip)        # 11 <bar+0x11>
>   11:   9c                      pushfq
>   12:   5b                      pop    %rbx
>   13:   e8 00 00 00 00          callq  18 <bar+0x18>
>   18:   b8 01 00 00 00          mov    $0x1,%eax
>   1d:   53                      push   %rbx
>   1e:   9d                      popfq
>   1f:   75 07                   jne    28 <bar+0x28>


Yeah, the above is pure garbage.

> So, the issue is still alive.
>
> What do you mean by "for the kernel we at a minimum need a way to
> disable that code generation"?
> Can this be fixed in the Linux-kernel?

No. This will never be fixed in the kernel. It's a compiler bug.

The compiler generates shit code. It's absolutely atrociously bad even
if you ignore any kernel issues, because that kind of code just
performs badly (the compiler should have used "setcc" or something
similar to just set the comparison value, not save and restore eflags.

And quite frankly, any compiler writer that thinks it is good code is
not somebody I want touching a compiler that the kernel depends on
anyway.

But it is not just bad code for the kernel, it's actively buggy code,
since it corrupts the IF.

Until this gets fixed in LLVM, there's no way in hell that we will
ever have a kernel compiled with that piece of shit.

Really. If the LLVM developers cannot fix their crap code generation,
it's not worth touching that shit with a ten-foot pole.

I'd love to be able to compile the kernel with LLVM, but the fact that
the broken eflags code apparently _still_ hasn't been fixed makes me
just go "not worth it".

And if the LLVM developers don't see this as an obvious bug, it's even
less worth it - because that shows not just that the compiler is
broken, but that the developers involved with it are broken too.

                  Linus

[toc] | [prev] | [next] | [standalone]

#1432296

On Mon, Jun 27, 2016 at 10:14 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Jun 27, 2016 at 12:50 PM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>>
>> $ objdump -S clang-eflag.o
>>
>> clang-eflag.o:     file format elf64-x86-64
>>
>>
>> Disassembly of section .text:
>>
>> 0000000000000000 <bar>:
>>    0:   55                      push   %rbp
>>    1:   48 89 e5                mov    %rsp,%rbp
>>    4:   53                      push   %rbx
>>    5:   50                      push   %rax
>>    6:   e8 00 00 00 00          callq  b <bar+0xb>
>>    b:   ff 0d 00 00 00 00       decl   0x0(%rip)        # 11 <bar+0x11>
>>   11:   9c                      pushfq
>>   12:   5b                      pop    %rbx
>>   13:   e8 00 00 00 00          callq  18 <bar+0x18>
>>   18:   b8 01 00 00 00          mov    $0x1,%eax
>>   1d:   53                      push   %rbx
>>   1e:   9d                      popfq
>>   1f:   75 07                   jne    28 <bar+0x28>
>
>
> Yeah, the above is pure garbage.
>
>> So, the issue is still alive.
>>
>> What do you mean by "for the kernel we at a minimum need a way to
>> disable that code generation"?
>> Can this be fixed in the Linux-kernel?
>
> No. This will never be fixed in the kernel. It's a compiler bug.
>
> The compiler generates shit code. It's absolutely atrociously bad even
> if you ignore any kernel issues, because that kind of code just
> performs badly (the compiler should have used "setcc" or something
> similar to just set the comparison value, not save and restore eflags.
>
> And quite frankly, any compiler writer that thinks it is good code is
> not somebody I want touching a compiler that the kernel depends on
> anyway.
>
> But it is not just bad code for the kernel, it's actively buggy code,
> since it corrupts the IF.
>
> Until this gets fixed in LLVM, there's no way in hell that we will
> ever have a kernel compiled with that piece of shit.
>
> Really. If the LLVM developers cannot fix their crap code generation,
> it's not worth touching that shit with a ten-foot pole.
>
> I'd love to be able to compile the kernel with LLVM, but the fact that
> the broken eflags code apparently _still_ hasn't been fixed makes me
> just go "not worth it".
>
> And if the LLVM developers don't see this as an obvious bug, it's even
> less worth it - because that shows not just that the compiler is
> broken, but that the developers involved with it are broken too.
>

Thanks for the quick answer.

I just grepped for some "buzzwords" people gave me in this
email-thread and I was looking at (llvm.git HEAD - upcoming v3.9
release) and found these comments in [1]

[ lib/Target/X86/X86InstrInfo.cpp ]

void X86InstrInfo::copyPhysReg()
...
// PUSHF/POPF is also potentially incorrect because it affects other flags
// such as TF/IF/DF, which LLVM doesn't model.
...

- Sedat -

[1] https://github.com/llvm-mirror/llvm/blob/master/lib/Target/X86/X86InstrInfo.cpp#L4516

[toc] | [prev] | [next] | [standalone]

#1432301

On Mon, Jun 27, 2016 at 1:27 PM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
> I just grepped for some "buzzwords" people gave me in this
> email-thread and I was looking at (llvm.git HEAD - upcoming v3.9
> release) and found these comments in [1]
>
>
> [1] https://github.com/llvm-mirror/llvm/blob/master/lib/Target/X86/X86InstrInfo.cpp#L4516

Christ. That's still pretty bad. Using LAHF/SAHF is just wrong.

But at least it's not semantically buggy any more, it's just stupid and slow.

Apparently the problem is that LLVM doesn't actually track flags as
different conditions, but as a single register, and doesn't know which
bits of it matter.

I guess the SETO + LAHF/SAHF is the best llvm can do then. But it
doesn't speak well of the code generation quality.

                Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web