Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1464673 > unrolled thread
| Started by | Denys Vlasenko <dvlasenk@redhat.com> |
|---|---|
| First post | 2016-08-17 19:30 +0200 |
| Last post | 2016-08-17 20:40 +0200 |
| Articles | 20 on this page of 23 — 4 participants |
Back to article view | Back to linux.kernel
RFC: Petition Intel/AMD to add POPF_IF insn Denys Vlasenko <dvlasenk@redhat.com> - 2016-08-17 19:30 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-17 20:00 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Denys Vlasenko <dvlasenk@redhat.com> - 2016-08-17 21:00 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Andy Lutomirski <luto@amacapital.net> - 2016-08-17 21:20 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Denys Vlasenko <dvlasenk@redhat.com> - 2016-08-17 21:30 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Denys Vlasenko <dvlasenk@redhat.com> - 2016-08-17 21:40 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-17 22:00 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-17 21:40 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-17 23:30 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-17 23:40 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Andy Lutomirski <luto@amacapital.net> - 2016-08-17 23:50 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-18 00:40 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Denys Vlasenko <dvlasenk@redhat.com> - 2016-08-18 15:30 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Denys Vlasenko <dvlasenk@redhat.com> - 2016-08-19 03:00 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Denys Vlasenko <dvlasenk@redhat.com> - 2016-08-19 03:10 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-19 04:30 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Paolo Bonzini <pbonzini@redhat.com> - 2016-08-19 13:00 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Denys Vlasenko <dvlasenk@redhat.com> - 2016-08-18 11:30 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Denys Vlasenko <dvlasenk@redhat.com> - 2016-08-18 14:20 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Paolo Bonzini <pbonzini@redhat.com> - 2016-08-19 04:10 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-17 21:50 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-17 21:30 +0200
Re: RFC: Petition Intel/AMD to add POPF_IF insn Denys Vlasenko <dvlasenk@redhat.com> - 2016-08-17 20:40 +0200
Page 1 of 2 [1] 2 Next page →
| From | Denys Vlasenko <dvlasenk@redhat.com> |
|---|---|
| Date | 2016-08-17 19:30 +0200 |
| Subject | RFC: Petition Intel/AMD to add POPF_IF insn |
| Message-ID | <s7cP7-469-1@gated-at.bofh.it> |
Last year, a proposal was floated to avoid costly POPF.
In particular, each spin_unlock_irqrestore() does it,
a rather common operation.
https://lkml.org/lkml/2015/4/21/290
[RFC PATCH] x86/asm/irq: Don't use POPF but STI
* Andy Lutomirski <luto@kernel.org> wrote:
> > Another different approach would be to formally state that
> > pv_irq_ops.save_fl() needs to return all the flags, which would
> > make local_irq_save() safe to use in this circumstance, but that
> > makes a hotpath longer for the sake of a single boot time check.
>
> ...which reminds me:
>
> Why does native_restore_fl restore anything other than IF? A branch
> and sti should be considerably faster than popf.
Ingo agreed:
====
Yes, this has come up in the past, something like the patch below?
Totally untested and not signed off yet: because we'd first have to
make sure (via irq flags debugging) that it's not used in reverse, to
re-disable interrupts:
local_irq_save(flags);
local_irq_enable();
...
local_irq_restore(flags); /* effective local_irq_disable() */
I don't think we have many (any?) such patterns left, but it has to be
checked first. If we have such cases then we'll have to use different
primitives there.
====
Linus replied:
=====
"popf" is fast for the "no changes to IF" case, and is a smaller
instruction anyway.
====
This basically shot down the proposal.
But in my measurements POPF is not fast even in the case where restored
flags are not changes at all:
mov $200*1000*1000, %eax
pushf
pop %rbx
.balign 64
loop: push %rbx
popf
dec %eax
jnz loop
# perf stat -r20 ./popf_1g
4,929,012,093 cycles # 3.412 GHz ( +- 0.02% )
835,721,371 instructions # 0.17 insn per cycle ( +- 0.02% )
1.446185359 seconds time elapsed ( +- 0.46% )
If I replace POPF with a pop into an unused register, I get this:
loop: push %rbx
pop %rcx
dec %eax
jnz loop
209,247,645 cycles # 3.209 GHz ( +- 0.11% )
801,898,016 instructions # 3.83 insn per cycle ( +- 0.00% )
0.066210725 seconds time elapsed ( +- 0.59% )
IOW, POPF takes at least 6 cycles.
Linus does have a point that a "test+branch+STI" may end up not a clear win because of the branch.
But the need to restore IF flag exists, it is necessary not only for Linux, but for any OS
running on x86: they all have some sort of spinlock.
The addition of a POPF instruction variant which looks only at IF bit and changes
only that bit in EFLAGS may be a good idea, for all OSes.
I propose that we ask Intel / AMD to do that.
Maybe by the "ignored prefix" trick which was used when LZCNT insn was introduced
as REPZ-prefixed BSR?
Currently, REPZ POPF (f3 9d) insn does execute. Redefine this opcode as
POPF_IF. Then the same kernel will work on old and new CPUs.
CC'ing some @intel and @amd emails...
[toc] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-08-17 20:00 +0200 |
| Message-ID | <s7di9-4hI-7@gated-at.bofh.it> |
| In reply to | #1464673 |
On Wed, Aug 17, 2016 at 10:20 AM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
> Last year, a proposal was floated to avoid costly POPF.
> In particular, each spin_unlock_irqrestore() does it,
> a rather common operation.
Quiet frankly, it takes so long for hw features that I don't think
it's worth it for something like this.
I'd rather see people play around with just the conditional "sti"
model instead and see how well that works. *Particularly* if we inline
that part of the spin_lock_irqrestore(), the branch prediction logic
may work very well. And even if not, it migth work better than popf.
But somebody would need to run the tests on an actual load
(microbenchmarks wouldn't work, because they wouldn't show the
different branch prediction cases).
power has been using "just do IF in software", which is also an
alternative, but it's much more complicated (you take the interrupt,
notice that the sw disable flag is set, don't actually run the irq but
set a flag and disable irqs for _real_ and return. Then you replay the
irq when enabling the software irq flag again)
Linus
[toc] | [prev] | [next] | [standalone]
| From | Denys Vlasenko <dvlasenk@redhat.com> |
|---|---|
| Date | 2016-08-17 21:00 +0200 |
| Message-ID | <s7eee-4TR-1@gated-at.bofh.it> |
| In reply to | #1464693 |
On 08/17/2016 07:32 PM, Linus Torvalds wrote: > On Wed, Aug 17, 2016 at 10:20 AM, Denys Vlasenko <dvlasenk@redhat.com> wrote: >> Last year, a proposal was floated to avoid costly POPF. >> In particular, each spin_unlock_irqrestore() does it, >> a rather common operation. > > Quiet frankly, it takes so long for hw features that I don't think > it's worth it for something like this. True, it will take some ~5 years for new hardware to become widely used. OTOH 5 years will inevitably pass. Just like now 32-bit Linux userspace is commonly compiled to "i686" instruction set, because inevitably enough time has passed that you can safely assume most people run at least Pentium Pro-level CPU, with CMOV, CMPXCHG, etc. If the new instruction will be implemented with "REPZ POPF" encoding, no recompilation or alternatives will be needed for the new kernel to run on an old CPU. On an old CPU, entire EFLAGS will be restored. On a new one, only EFLAGS.IF.
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| Date | 2016-08-17 21:20 +0200 |
| Message-ID | <s7exz-5ga-13@gated-at.bofh.it> |
| In reply to | #1464717 |
On Wed, Aug 17, 2016 at 12:01 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Aug 17, 2016 11:41 AM, "Denys Vlasenko" <dvlasenk@redhat.com> wrote: >> >> OTOH 5 years will inevitably pass. > > Yes. But in five years, maybe we'll have a popf that is faster anyway. > > I'd actually prefer that in the end. The problem with popf right now seems > to be mainly that it's effectively serializing and does stupid things in > microcode. It doesn't have to be that way. It could actually do much better, > but it hasn't been a high enough priority for Intel. > It wouldn't surprise me if that were easier said than done. popf potentially changes AC, and AC affects address translation. popf also potentially changes IOPL, and I don't know whether Intel chips track IOPL in a way that lets them find all the dependent instructions without serializing. But maybe their pipeline is fancy enough. Personally, I still expect that a simple branch-and-sti is the way to go. It wouldn't shock me if even a mispredicted branch and STI is faster than POPF.
[toc] | [prev] | [next] | [standalone]
| From | Denys Vlasenko <dvlasenk@redhat.com> |
|---|---|
| Date | 2016-08-17 21:30 +0200 |
| Message-ID | <s7eHf-5l0-3@gated-at.bofh.it> |
| In reply to | #1464730 |
On 08/17/2016 09:13 PM, Andy Lutomirski wrote: > On Wed, Aug 17, 2016 at 12:01 PM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> On Aug 17, 2016 11:41 AM, "Denys Vlasenko" <dvlasenk@redhat.com> wrote: >>> >>> OTOH 5 years will inevitably pass. >> >> Yes. But in five years, maybe we'll have a popf that is faster anyway. >> >> I'd actually prefer that in the end. The problem with popf right now seems >> to be mainly that it's effectively serializing and does stupid things in >> microcode. It doesn't have to be that way. It could actually do much better, >> but it hasn't been a high enough priority for Intel. >> > > It wouldn't surprise me if that were easier said than done. popf > potentially changes AC, and AC affects address translation. popf also > potentially changes IOPL, and I don't know whether Intel chips track > IOPL in a way that lets them find all the dependent instructions > without serializing. But maybe their pipeline is fancy enough. Exactly. And more: POPF potentially changes TF (and it works even in CPL3). POPD changes DF - must serialize versus string insns. POPF changes NT - must serialize versus IRET insns. POPF changes VIF, from a different bit in a popped value, and under a rather complex conditions. Intel's documentation has a pseudo-code for the instructions. For POPF, that pseudo-code is two pages long...
[toc] | [prev] | [next] | [standalone]
| From | Denys Vlasenko <dvlasenk@redhat.com> |
|---|---|
| Date | 2016-08-17 21:40 +0200 |
| Message-ID | <s7eQV-5ow-5@gated-at.bofh.it> |
| In reply to | #1464732 |
On 08/17/2016 09:32 PM, Linus Torvalds wrote: > On Wed, Aug 17, 2016 at 12:26 PM, Denys Vlasenko <dvlasenk@redhat.com> wrote: >> >> Exactly. And more: > > All of which is ENTIRELY IRRELEVANT. > > The 2-page pseudo-code is about behavior. It's trivial to > short-circuit. There are obvious optimizations, which involve just > making the rare cases go slow and have a trivial "if those bits didn't > change, don't go through the expense of testing them". > > The problem is that IF is almost certainly right now in that rare case > mask, and so popf is stupidly slow for IF. I ran the test, see the first email in the thread. Experimentally, POPF is stupidly slow _always_. 6 cycles even if none of the "scary" flags are changed. Either: * its microcode has no rare case mask or * its microcode is so slow that even fast path is slow, and slow path is worse
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-08-17 22:00 +0200 |
| Message-ID | <s7fai-5wE-31@gated-at.bofh.it> |
| In reply to | #1464745 |
On Wed, Aug 17, 2016 at 12:35 PM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
>
> Experimentally, POPF is stupidly slow _always_. 6 cycles
> even if none of the "scary" flags are changed.
6 cycles is nothing.
That's basically the overhead of "oops, I need to use the microcode sequencer".
One issue is that the intel decoders (AMD too, for that matter) can
only generate a fairly small set of uops for any instruction. Some
instructions are really trivial to decode (popf definitely falls under
that heading), but are more than just a couple of uops, so you end up
having to use the uop sequencer logic.
According to Agner Fog's tables, there's one or two
micro-architectures that actually dot he simple "popf" case with a
single cycle throughput, but that's the very unusual case.
You can't even fit the "pop a value, see if only the arithmetic flags
changed, trap to microcode otherwise" into the three of four uops that
the "complex decoder" can generate directly.
And that "fall back to the uop sequencer engine" tends to just always
cause several cycles regardless. So yes, microcode tends to be slow
even for what would otherwise be trivial operations. You'd think Intel
could do as well as they do for the L0 uop cache, but afaik they
don't.
Anyway, six cycles is fast. I'd *love* for popf to actually be just 6
cycles when IF changes. It's much much worse iirc (although honestly,
I haven't timed it in years - it's much easier to time just the
arithmetic flag changes).
It used to be more like a hundred cycles on Prescott.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-08-17 21:40 +0200 |
| Message-ID | <s7eQV-5ow-23@gated-at.bofh.it> |
| In reply to | #1464732 |
On Wed, Aug 17, 2016 at 12:32 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The 2-page pseudo-code is about behavior. It's trivial to
> short-circuit. There are obvious optimizations, which involve just
> making the rare cases go slow and have a trivial "if those bits didn't
> change, don't go through the expense of testing them".
That said, the first thing we should test is to just see how the
"let's just optimize this in software" thing works.
Replace the "popf" with "if (val & X86_EFLAGS_IF) local_irq_enable();"
and see how that works out. Play with inlining it or not, and see if
the branch predictor matters.
We may simply not need popf optimizations.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-08-17 23:30 +0200 |
| Message-ID | <s7gzo-6R5-31@gated-at.bofh.it> |
| In reply to | #1464750 |
[Multipart message — attachments visible in raw view] — view raw
On Wed, Aug 17, 2016 at 12:37 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Replace the "popf" with "if (val & X86_EFLAGS_IF) local_irq_enable();"
> and see how that works out. Play with inlining it or not, and see if
> the branch predictor matters.
.. actually, thinking a bit more about it, I really don't think the
branch predictor will even matter.
We sure as hell shouldn't have nested irq-safe interrupts in paths
that matter from a performance angle, so the normal case for
spin_unlock_irqrestore() should be to enable interrupts again.
And if interrupts are disabled because the caller is actually in
interrupt context, I don't think the branch prediction is going to
matter, compared to the irq overhead.
So test this trivial patch. It's ENTIRELY UNTESTED. It may be complete
crap and not even compile. But I did test it on
kernel/locking/spinlock.c, and the generated code is beautiful:
_raw_spin_unlock_irqrestore:
testl $512, %esi #, flags
movb $0, (%rdi) #, MEM[(volatile __u8 *)lock_2(D)]
je .L2
sti
.L2:
ret
so maybe the silly popf has always just been stupid.
Of course, if somebody uses native_restore_fl() to actually *disable*
interrupts (when they weren't already disabled), then this untested
patch will just not work. But why would you do somethign so stupid?
Famous last words...
Linus
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-08-17 23:40 +0200 |
| Message-ID | <s7gJ4-6VE-43@gated-at.bofh.it> |
| In reply to | #1464813 |
On Wed, Aug 17, 2016 at 2:26 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Of course, if somebody uses native_restore_fl() to actually *disable*
> interrupts (when they weren't already disabled), then this untested
> patch will just not work. But why would you do somethign so stupid?
> Famous last words...
Looking around, the vsmp code actually uses "native_restore_fl()" to
not just set the interrupt flag, but AC as well.
And the PV spinlock case has that "push;popf" sequence encoded in an alternate.
So that trivial patch may (or may not - still not tested) work for
some quick testing, but needs more effort for any *real* use.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| Date | 2016-08-17 23:50 +0200 |
| Message-ID | <s7gSK-6Zu-23@gated-at.bofh.it> |
| In reply to | #1464819 |
On Wed, Aug 17, 2016 at 2:35 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Wed, Aug 17, 2016 at 2:26 PM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> Of course, if somebody uses native_restore_fl() to actually *disable* >> interrupts (when they weren't already disabled), then this untested >> patch will just not work. But why would you do somethign so stupid? >> Famous last words... > > Looking around, the vsmp code actually uses "native_restore_fl()" to > not just set the interrupt flag, but AC as well. > > And the PV spinlock case has that "push;popf" sequence encoded in an alternate. > > So that trivial patch may (or may not - still not tested) work for > some quick testing, but needs more effort for any *real* use. > It shouldn't be *too* bad, since xen_restore_fl only affects "IF". And even if native_restore_fl needs to be able to turn IRQs off as well as on, we can just do: if (likely(flags & X86_EFLAGS_IF)) sti(); else cli(); at some cost to code size but hopefully little to no runtime cost for the sane cases.
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-08-18 00:40 +0200 |
| Message-ID | <s7hF8-7AW-7@gated-at.bofh.it> |
| In reply to | #1464827 |
On Wed, Aug 17, 2016 at 2:43 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> It shouldn't be *too* bad, since xen_restore_fl only affects "IF".
> And even if native_restore_fl needs to be able to turn IRQs off as
> well as on, we can just do:
>
> if (likely(flags & X86_EFLAGS_IF))
> sti();
> else
> cli();
>
> at some cost to code size but hopefully little to no runtime cost for
> the sane cases.
No, that would be horrible for the case where we had an irq spinlock
in an irq-region. Then we'd have a pointless "cli" there.
So I'd rather just make sure that only the spinlock code actually uses
native_restore_fl().
And yes, the patch works at least minimally, since I'm writing this
with a kernel compiled with that.
Of course, somebody really should do timings on modern CPU's (in cpl0,
comparing native_fl() that enables interrupts with a popf)
Linus
[toc] | [prev] | [next] | [standalone]
| From | Denys Vlasenko <dvlasenk@redhat.com> |
|---|---|
| Date | 2016-08-18 15:30 +0200 |
| Message-ID | <s7vyr-kW-65@gated-at.bofh.it> |
| In reply to | #1464845 |
> Of course, somebody really should do timings on modern CPU's (in cpl0,
> comparing native_fl() that enables interrupts with a popf)
I didn't do CPL0 tests yet. Realized that cli/sti can be tested in userspace
if we set iopl(3) first.
Surprisingly, STI is slower than CLI. A loop with 27 CLI's and one STI
converges to about ~0.5 insn/cycle:
# compile with: gcc -nostartfiles -nostdlib
_start: .globl _start
mov $172, %eax #iopl
mov $3, %edi
syscall
mov $200*1000*1000, %eax
.balign 64
loop:
cli;cli;cli;cli
cli;cli;cli;cli
cli;cli;cli;cli
cli;cli;cli;cli
cli;cli;cli;cli
cli;cli;cli;cli
cli;cli;cli;sti
dec %eax
jnz loop
mov $231, %eax #exit_group
syscall
perf stat:
6,015,787,968 instructions # 0.52 insn per cycle
3.355474199 seconds time elapsed
With all CLIs replaced by STIs, it's ~0.25 insn/cycle:
6,030,530,328 instructions # 0.27 insn per cycle
6.547200322 seconds time elapsed
POPF which needs to enable interrupts is not measurably faster than
one which does not change .IF:
Loop with:
400158: fa cli
400159: 53 push %rbx #saved eflags with if=1
40015a: 9d popfq
shows:
8,908,857,324 instructions # 0.11 insn per cycle ( +- 0.00% )
Loop with:
400140: fb sti
400141: 53 push %rbx
400142: 9d popfq
shows:
8,920,243,701 instructions # 0.10 insn per cycle ( +- 0.01% )
Even loop with neither CLI nor STI, only with POPF:
400140: 53 push %rbx
400141: 9d popfq
shows:
6,079,936,714 instructions # 0.10 insn per cycle ( +- 0.00% )
This is on a Skylake CPU.
The gist of it:
CLI is 2 cycles,
STI is 4 cycles,
POPF is 10 cycles
seemingly regardless of prior value of EFLAGS.IF.
[toc] | [prev] | [next] | [standalone]
| From | Denys Vlasenko <dvlasenk@redhat.com> |
|---|---|
| Date | 2016-08-19 03:00 +0200 |
| Message-ID | <s7Gkb-6XF-57@gated-at.bofh.it> |
| In reply to | #1465338 |
On 08/18/2016 07:47 PM, Denys Vlasenko wrote: > On 08/18/2016 07:24 PM, Linus Torvalds wrote: >> That said, your numbers really aren't very convincing. If popf really >> is just 10 cycles on modern Intel hardware, it's already fast enough >> that I really don't think it matters. > > It's 20 cycles. I was wrong in my email, I forgot that the insn count > also counts "push %ebx" insns. > > Since I already made a mistake, let me double-check. > > 200 million iterations of this loop execute under 17 seconds: > > 400100: b8 00 c2 eb 0b mov $0xbebc200,%eax # 1000*1000*1000 Grr. It's 200*1000*1000, not one billion. Sorry.
[toc] | [prev] | [next] | [standalone]
| From | Denys Vlasenko <dvlasenk@redhat.com> |
|---|---|
| Date | 2016-08-19 03:10 +0200 |
| Message-ID | <s7Gkb-6XF-61@gated-at.bofh.it> |
| In reply to | #1465338 |
On 08/18/2016 07:24 PM, Linus Torvalds wrote: > That said, your numbers really aren't very convincing. If popf really > is just 10 cycles on modern Intel hardware, it's already fast enough > that I really don't think it matters. It's 20 cycles. I was wrong in my email, I forgot that the insn count also counts "push %ebx" insns. Since I already made a mistake, let me double-check. 200 million iterations of this loop execute under 17 seconds: 400100: b8 00 c2 eb 0b mov $0xbebc200,%eax # 1000*1000*1000 400105: 9c pushfq 400106: 5b pop %rbx 400107: 90 nop .... 0000000000400140 <loop>: 400140: 53 push %rbx 400141: 9d popfq 400142: 53 push %rbx 400143: 9d popfq 400144: 53 push %rbx 400145: 9d popfq 400146: 53 push %rbx 400147: 9d popfq 400148: 53 push %rbx 400149: 9d popfq 40014a: 53 push %rbx 40014b: 9d popfq 40014c: 53 push %rbx 40014d: 9d popfq 40014e: 53 push %rbx 40014f: 9d popfq 400150: 53 push %rbx 400151: 9d popfq 400152: 53 push %rbx 400153: 9d popfq 400154: 53 push %rbx 400155: 9d popfq 400156: 53 push %rbx 400157: 9d popfq 400158: 53 push %rbx 400159: 9d popfq 40015a: 53 push %rbx 40015b: 9d popfq 40015c: ff c8 dec %eax 40015e: 75 e0 jne 400140 <loop> The loop is exactly 32 bytes, aligned. There are 14 POPFs. Other insns are very fast. No perf, just "time taskset 1 ./test". My CPU frequency hovers around 3500 MHz when loaded. 17 seconds is 17*3500 million cycles. 17*3500 million cycles / 200*14 million cycles = 21.25 Thus, one POPF in CPL3 is ~20 cycles on Skylake.
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-08-19 04:30 +0200 |
| Message-ID | <s7Gkb-6XF-59@gated-at.bofh.it> |
| In reply to | #1465338 |
On Thu, Aug 18, 2016 at 6:26 AM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
>
> I didn't do CPL0 tests yet. Realized that cli/sti can be tested in userspace
> if we set iopl(3) first.
Yes, but it might not be the same. So the timings could be very
different from a cpl0 case.
Also:
> Surprisingly, STI is slower than CLI. A loop with 27 CLI's and one STI
> converges to about ~0.5 insn/cycle:
You really really should not check "sti" together with immediately
following sti or cli.
The sti instruction has an architecturally defined magical
one-instruction window following it when interrupts stay disabled. I
could easily see that resulting in strange special cases - Intel
actually at some point documented that a sequence of 'sti'
instructions are not going to disable interrupts forever (there was a
question of what happens if you start out with interrupts disabled, go
to a 16-bit code segment that is all filled with "sti" instructions so
that the 16-bit EIP will wrap around and continually do an infinite
series of 'sti' - do interrupts ever get enabled?)
I think intel clarified that when you have a sequence of 'sti'
instructions, interrupts will get enabled after the second one, but
the point is that this is all "special" from a front-end angle. So
putting multiple 'sti' instructions in a bunch may be testing the
magical special case more than it would test anything *real*.
So at a minimum, make the sequence be "sti; nop" if you do it in a
loop. It may not change anything, but at least that way you'll know it
doesn't just test the magical case.
Realistically, it's better to instead test a *real* instruction
sequence, ie just compare something like
pushf
cli
.. do a memory operation here or something half-way real ..
pop
sti
and
pushf
cli
.. do the same half-way real memory op here ..
popf
and see which one is faster in a loop.
That said, your numbers really aren't very convincing. If popf really
is just 10 cycles on modern Intel hardware, it's already fast enough
that I really don't think it matters. Especially with "sti" being ~4
cycles, and there being a question about branch overhead anyway. You
win some, you lose some, but on the whole it sounds like "leave it
alone" wins.
Now, I know for a fact that there have been other x86 uarchitectres
that sucked at "popf", but they may suck almost equally at "sti". So
this might well be worth testing out on something that isn't Skylake.
Modern intel cores really are pretty good at even the slow operations.
Things used to be much much worse in the bad old P4 days. I'm very
impressed with the big intel cores.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2016-08-19 13:00 +0200 |
| Message-ID | <s7PGN-4yC-15@gated-at.bofh.it> |
| In reply to | #1465903 |
On 18/08/2016 19:24, Linus Torvalds wrote: >> > I didn't do CPL0 tests yet. Realized that cli/sti can be tested in userspace >> > if we set iopl(3) first. > Yes, but it might not be the same. So the timings could be very > different from a cpl0 case. FWIW I recently measured around 20 cycles for a popf as well on Haswell-EP and CPL=0 (that was for commit f2485b3e0c6c, "KVM: x86: use guest_exit_irqoff", 2016-07-01). Paolo
[toc] | [prev] | [next] | [standalone]
| From | Denys Vlasenko <dvlasenk@redhat.com> |
|---|---|
| Date | 2016-08-18 11:30 +0200 |
| Message-ID | <s7rOa-6fy-31@gated-at.bofh.it> |
| In reply to | #1464819 |
On 08/17/2016 11:35 PM, Linus Torvalds wrote:
> On Wed, Aug 17, 2016 at 2:26 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> Of course, if somebody uses native_restore_fl() to actually *disable*
>> interrupts (when they weren't already disabled), then this untested
>> patch will just not work. But why would you do somethign so stupid?
>> Famous last words...
>
> Looking around, the vsmp code actually uses "native_restore_fl()" to
> not just set the interrupt flag, but AC as well.
>
> And the PV spinlock case has that "push;popf" sequence encoded in an alternate.
>
> So that trivial patch may (or may not - still not tested) work for
> some quick testing, but needs more effort for any *real* use.
I'm going to test the attached patch.
PV and debug maze is daunting... I discovered that Fedora kernels
compile with the set of .config options which result in
spin_unlock_irqrestore which looks like this:
ffffffff818d0f40 <_raw_spin_unlock_irqrestore>:
ffffffff818d0f40: e8 1b 31 00 00 callq ffffffff818d4060 <__fentry__> ffffffff818d0f41: R_X86_64_PC32 __fentry__-0x4
ffffffff818d0f45: 55 push %rbp
ffffffff818d0f46: 48 89 e5 mov %rsp,%rbp
ffffffff818d0f49: 41 54 push %r12
ffffffff818d0f4b: 53 push %rbx
ffffffff818d0f4c: 48 8b 55 08 mov 0x8(%rbp),%rdx
ffffffff818d0f50: 49 89 fc mov %rdi,%r12
ffffffff818d0f53: 48 89 f3 mov %rsi,%rbx
ffffffff818d0f56: 48 83 c7 18 add $0x18,%rdi
ffffffff818d0f5a: be 01 00 00 00 mov $0x1,%esi
ffffffff818d0f5f: e8 8c b8 83 ff callq ffffffff8110c7f0 <lock_release> ffffffff818d0f60: R_X86_64_PC32 lock_release-0x4
ffffffff818d0f64: 4c 89 e7 mov %r12,%rdi
ffffffff818d0f67: e8 d4 fb 83 ff callq ffffffff81110b40 <do_raw_spin_unlock> ffffffff818d0f68: R_X86_64_PC32 do_raw_spin_unlock-0x4
ffffffff818d0f6c: f6 c7 02 test $0x2,%bh
ffffffff818d0f6f: 74 1b je ffffffff818d0f8c <_raw_spin_unlock_irqrestore+0x4c>
ffffffff818d0f71: e8 aa 9d 83 ff callq ffffffff8110ad20 <trace_hardirqs_on> ffffffff818d0f72: R_X86_64_PC32 trace_hardirqs_on-0x4
ffffffff818d0f76: 48 89 df mov %rbx,%rdi
ffffffff818d0f79: ff 14 25 48 32 e2 81 callq *0xffffffff81e23248 ffffffff818d0f7c: R_X86_64_32S pv_irq_ops+0x8
ffffffff818d0f80: 65 ff 0d c9 c4 73 7e decl %gs:0x7e73c4c9(%rip) # d450 <__preempt_count> ffffffff818d0f83: R_X86_64_PC32 __preempt_count-0x4
ffffffff818d0f87: 5b pop %rbx
ffffffff818d0f88: 41 5c pop %r12
ffffffff818d0f8a: 5d pop %rbp
ffffffff818d0f8b: c3 retq
ffffffff818d0f8c: 48 89 df mov %rbx,%rdi
ffffffff818d0f8f: ff 14 25 48 32 e2 81 callq *0xffffffff81e23248 ffffffff818d0f92: R_X86_64_32S pv_irq_ops+0x8
ffffffff818d0f96: e8 35 5b 83 ff callq ffffffff81106ad0 <trace_hardirqs_off> ffffffff818d0f97: R_X86_64_PC32 trace_hardirqs_off-0x4
ffffffff818d0f9b: eb e3 jmp ffffffff818d0f80 <_raw_spin_unlock_irqrestore+0x40>
ffffffff818d0f9d: 0f 1f 00 nopl (%rax)
Gawd... really Fedora? All this is needed?
Testing with _this_ is not going to show any differences, I need to weed
all the cruft out and test a fast, non-debug .config.
Changed it like this:
+# CONFIG_UPROBES is not set
+# CONFIG_SCHED_OMIT_FRAME_POINTER is not set
+# CONFIG_HYPERVISOR_GUEST is not set
+# CONFIG_SYS_HYPERVISOR is not set
+# CONFIG_FRAME_POINTER is not set
+# CONFIG_KMEMCHECK is not set
+# CONFIG_DEBUG_LOCK_ALLOC is not set
+# CONFIG_PROVE_LOCKING is not set
+# CONFIG_LOCK_STAT is not set
+# CONFIG_PROVE_RCU is not set
+# CONFIG_LATENCYTOP is not set
+# CONFIG_FTRACE is not set
+# CONFIG_BINARY_PRINTF is not set
Looks better (it's with the patch already):
00000000000000f0 <_raw_spin_unlock_irqrestore>:
f0: 53 push %rbx
f1: 48 89 f3 mov %rsi,%rbx
f4: e8 00 00 00 00 callq f9 <_raw_spin_unlock_irqrestore+0x9> f5: R_X86_64_PC32 do_raw_spin_unlock-0x4
f9: 80 e7 02 and $0x2,%bh
fc: 74 01 je ff <_raw_spin_unlock_irqrestore+0xf>
fe: fb sti
ff: 65 ff 0d 00 00 00 00 decl %gs:0x0(%rip) # 106 <_raw_spin_unlock_irqrestore+0x16> 102: R_X86_64_PC32 __preempt_count-0x4
106: 5b pop %rbx
107: c3 retq
This also raises questions. Such as "why do_raw_spin_unlock() is not inlined here?"
Anyway... on to more kernel builds and testing.
Please take a look at the below patch. If it's obviously buggy, let me know.
diff -urp linux-4.7.1.org/arch/x86/include/asm/irqflags.h linux-4.7.1.obj/arch/x86/include/asm/irqflags.h
--- linux-4.7.1.org/arch/x86/include/asm/irqflags.h 2016-08-16 09:35:15.000000000 +0200
+++ linux-4.7.1.obj/arch/x86/include/asm/irqflags.h 2016-08-18 10:01:09.514219644 +0200
@@ -44,6 +44,16 @@ static inline void native_irq_enable(voi
asm volatile("sti": : :"memory");
}
+/*
+ * This produces a "test; jz; sti" insn sequence.
+ * It's faster than "push reg; popf". If sti is skipped, it's much faster.
+ */
+static inline void native_cond_irq_enable(unsigned long flags)
+{
+ if (flags & X86_EFLAGS_IF)
+ native_irq_enable();
+}
+
static inline void native_safe_halt(void)
{
asm volatile("sti; hlt": : :"memory");
@@ -69,7 +79,8 @@ static inline notrace unsigned long arch
static inline notrace void arch_local_irq_restore(unsigned long flags)
{
- native_restore_fl(flags);
+ if (flags & X86_EFLAGS_IF)
+ native_irq_enable();
}
static inline notrace void arch_local_irq_disable(void)
diff -urp linux-4.7.1.org/arch/x86/kernel/paravirt.c linux-4.7.1.obj/arch/x86/kernel/paravirt.c
--- linux-4.7.1.org/arch/x86/kernel/paravirt.c 2016-08-16 09:35:15.000000000 +0200
+++ linux-4.7.1.obj/arch/x86/kernel/paravirt.c 2016-08-18 10:01:24.797155109 +0200
@@ -313,7 +313,7 @@ struct pv_time_ops pv_time_ops = {
__visible struct pv_irq_ops pv_irq_ops = {
.save_fl = __PV_IS_CALLEE_SAVE(native_save_fl),
- .restore_fl = __PV_IS_CALLEE_SAVE(native_restore_fl),
+ .restore_fl = __PV_IS_CALLEE_SAVE(native_cond_irq_enable),
.irq_disable = __PV_IS_CALLEE_SAVE(native_irq_disable),
.irq_enable = __PV_IS_CALLEE_SAVE(native_irq_enable),
.safe_halt = native_safe_halt,
diff -urp linux-4.7.1.org/arch/x86/kernel/paravirt_patch_32.c linux-4.7.1.obj/arch/x86/kernel/paravirt_patch_32.c
--- linux-4.7.1.org/arch/x86/kernel/paravirt_patch_32.c 2016-08-16 09:35:15.000000000 +0200
+++ linux-4.7.1.obj/arch/x86/kernel/paravirt_patch_32.c 2016-08-18 04:43:19.388102755 +0200
@@ -2,7 +2,7 @@
DEF_NATIVE(pv_irq_ops, irq_disable, "cli");
DEF_NATIVE(pv_irq_ops, irq_enable, "sti");
-DEF_NATIVE(pv_irq_ops, restore_fl, "push %eax; popf");
+DEF_NATIVE(pv_irq_ops, restore_fl, "testb $((1<<9)>>8),%ah; jz 1f; sti; 1: ;");
DEF_NATIVE(pv_irq_ops, save_fl, "pushf; pop %eax");
DEF_NATIVE(pv_cpu_ops, iret, "iret");
DEF_NATIVE(pv_mmu_ops, read_cr2, "mov %cr2, %eax");
diff -urp linux-4.7.1.org/arch/x86/kernel/paravirt_patch_64.c linux-4.7.1.obj/arch/x86/kernel/paravirt_patch_64.c
--- linux-4.7.1.org/arch/x86/kernel/paravirt_patch_64.c 2016-08-16 09:35:15.000000000 +0200
+++ linux-4.7.1.obj/arch/x86/kernel/paravirt_patch_64.c 2016-08-18 04:42:56.364545509 +0200
@@ -4,7 +4,7 @@
DEF_NATIVE(pv_irq_ops, irq_disable, "cli");
DEF_NATIVE(pv_irq_ops, irq_enable, "sti");
-DEF_NATIVE(pv_irq_ops, restore_fl, "pushq %rdi; popfq");
+DEF_NATIVE(pv_irq_ops, restore_fl, "testw $(1<<9),%di; jz 1f; sti; 1: ;");
DEF_NATIVE(pv_irq_ops, save_fl, "pushfq; popq %rax");
DEF_NATIVE(pv_mmu_ops, read_cr2, "movq %cr2, %rax");
DEF_NATIVE(pv_mmu_ops, read_cr3, "movq %cr3, %rax");
[toc] | [prev] | [next] | [standalone]
| From | Denys Vlasenko <dvlasenk@redhat.com> |
|---|---|
| Date | 2016-08-18 14:20 +0200 |
| Message-ID | <s7usF-86E-1@gated-at.bofh.it> |
| In reply to | #1465040 |
[Multipart message — attachments visible in raw view] — view raw
On 08/18/2016 11:21 AM, Denys Vlasenko wrote:
>>> Of course, if somebody uses native_restore_fl() to actually *disable*
>>> interrupts (when they weren't already disabled), then this untested
>>> patch will just not work. But why would you do somethign so stupid?
>>> Famous last words...
>>
>> Looking around, the vsmp code actually uses "native_restore_fl()" to
>> not just set the interrupt flag, but AC as well.
>>
>> And the PV spinlock case has that "push;popf" sequence encoded in an alternate.
>>
>> So that trivial patch may (or may not - still not tested) work for
>> some quick testing, but needs more effort for any *real* use.
>
> I'm going to test the attached patch.
...
>
> +# CONFIG_UPROBES is not set
> +# CONFIG_SCHED_OMIT_FRAME_POINTER is not set
> +# CONFIG_HYPERVISOR_GUEST is not set
> +# CONFIG_SYS_HYPERVISOR is not set
> +# CONFIG_FRAME_POINTER is not set
> +# CONFIG_KMEMCHECK is not set
> +# CONFIG_DEBUG_LOCK_ALLOC is not set
> +# CONFIG_PROVE_LOCKING is not set
> +# CONFIG_LOCK_STAT is not set
> +# CONFIG_PROVE_RCU is not set
> +# CONFIG_LATENCYTOP is not set
> +# CONFIG_FTRACE is not set
> +# CONFIG_BINARY_PRINTF is not set
Need also !CONFIG_DEBUG_SPINLOCK, then unpatched irqrestore is:
ffffffff817115a0 <_raw_spin_unlock_irqrestore>:
ffffffff817115a0: c6 07 00 movb $0x0,(%rdi)
ffffffff817115a3: 56 push %rsi
ffffffff817115a4: 9d popfq
ffffffff817115a5: 65 ff 0d e4 ad 8f 7e decl %gs:__preempt_count
ffffffff817115ac: c3 retq
ffffffff817115ad: 0f 1f 00 nopl (%rax)
patched one is
ffffffff81711660 <_raw_spin_unlock_irqrestore>:
ffffffff81711660: f7 c6 00 02 00 00 test $0x200,%esi
ffffffff81711666: c6 07 00 movb $0x0,(%rdi)
ffffffff81711669: 74 01 je ffffffff8171166c <_raw_spin_unlock_irqrestore+0xc>
ffffffff8171166b: fb sti
ffffffff8171166c: 65 ff 0d 1d ad 8f 7e decl %gs:__preempt_count
ffffffff81711673: c3 retq
ffffffff81711674: 66 90 xchg %ax,%ax
ffffffff81711676: 66 2e 0f 1f 84 00 00 00 00 00 nopw %cs:0x0(%rax,%rax,1)
Ran the following twice on a quiesced machine:
taskset 1 perf stat -r60 perf bench sched messaging
taskset 1 perf stat -r60 perf bench sched pipe
Out of these four runs, both "perf bench sched pipe" runs show improvements:
- 2648.279829 task-clock (msec) # 1.000 CPUs utilized ( +- 0.24% )
+ 2483.143469 task-clock (msec) # 0.998 CPUs utilized ( +- 0.31% )
- 2,000,002 context-switches # 0.755 M/sec ( +- 0.00% )
+ 2,000,013 context-switches # 0.805 M/sec ( +- 0.00% )
- 547 page-faults # 0.206 K/sec ( +- 0.04% )
+ 546 page-faults # 0.220 K/sec ( +- 0.04% )
- 8,723,284,926 cycles # 3.294 GHz ( +- 0.06% )
+ 8,157,949,449 cycles # 3.285 GHz ( +- 0.07% )
- 12,286,937,344 instructions # 1.41 insn per cycle ( +- 0.03% )
+ 12,255,616,405 instructions # 1.50 insn per cycle ( +- 0.05% )
- 2,588,839,023 branches # 977.555 M/sec ( +- 0.02% )
+ 2,599,319,615 branches # 1046.786 M/sec ( +- 0.04% )
- 3,620,273 branch-misses # 0.14% of all branches ( +- 0.67% )
+ 3,577,771 branch-misses # 0.14% of all branches ( +- 0.69% )
- 2.648799072 seconds time elapsed ( +- 0.24% )
+ 2.487452268 seconds time elapsed ( +- 0.31% )
Good, we run more insns/cycle, as expected. However, a bit more branches.
But of two "perf bench sched messaging" run, one was slower on a patched kernel,
and perf shows why: more branches, and also branch miss percentage is larger:
- 690.008697 task-clock (msec) # 0.996 CPUs utilized ( +- 0.45% )
+ 699.526509 task-clock (msec) # 0.994 CPUs utilized ( +- 0.28% )
- 26,796 context-switches # 0.039 M/sec ( +- 8.31% )
+ 29,088 context-switches # 0.042 M/sec ( +- 6.62% )
- 35,477 page-faults # 0.051 M/sec ( +- 0.11% )
+ 35,504 page-faults # 0.051 M/sec ( +- 0.14% )
- 2,157,701,609 cycles # 3.127 GHz ( +- 0.35% )
+ 2,143,781,407 cycles # 3.065 GHz ( +- 0.25% )
- 3,115,212,672 instructions # 1.44 insn per cycle ( +- 0.28% )
+ 3,253,499,549 instructions # 1.52 insn per cycle ( +- 0.19% )
- 661,888,593 branches # 959.247 M/sec ( +- 0.36% )
+ 707,862,655 branches # 1011.917 M/sec ( +- 0.20% )
- 2,793,203 branch-misses # 0.42% of all branches ( +- 1.04% )
+ 3,453,397 branch-misses # 0.49% of all branches ( +- 0.32% )
- 0.693004918 seconds time elapsed ( +- 0.45% )
+ 0.703630988 seconds time elapsed ( +- 0.27% )
This tipped the scales, and despite higher insns/cycle, run time is worse.
The other "perf bench sched messaging" run was more lucky:
- 706.944245 task-clock (msec) # 0.995 CPUs utilized ( +- 0.32% )
+ 687.038856 task-clock (msec) # 0.993 CPUs utilized ( +- 0.31% )
- 23,489 context-switches # 0.033 M/sec ( +- 7.02% )
+ 26,644 context-switches # 0.039 M/sec ( +- 7.46% )
- 35,360 page-faults # 0.050 M/sec ( +- 0.12% )
+ 35,417 page-faults # 0.052 M/sec ( +- 0.13% )
- 2,183,639,816 cycles # 3.089 GHz ( +- 0.35% )
+ 2,123,086,753 cycles # 3.090 GHz ( +- 0.27% )
- 3,131,362,238 instructions # 1.43 insn per cycle ( +- 0.19% )
+ 3,236,613,433 instructions # 1.52 insn per cycle ( +- 0.19% )
- 667,874,319 branches # 944.734 M/sec ( +- 0.21% )
+ 703,677,908 branches # 1024.219 M/sec ( +- 0.20% )
- 2,859,521 branch-misses # 0.43% of all branches ( +- 0.56% )
+ 3,462,063 branch-misses # 0.49% of all branches ( +- 0.33% )
- 0.710738536 seconds time elapsed ( +- 0.32% )
+ 0.691908533 seconds time elapsed ( +- 0.31% )
However, it still had more branches (~5% more), and worse branch miss percentage.
The patch seems to work. It also does not bloat the kernel:
text data bss dec hex filename
8199556 5026784 2924544 16150884 f67164 vmlinux
8199897 5026784 2924544 16151225 f672b9 vmlinux.patched
However, a "conditional CLI/STI from r/m" insn could be better still.
The patch is attached.
[toc] | [prev] | [next] | [standalone]
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2016-08-19 04:10 +0200 |
| Message-ID | <s7HpT-7RC-13@gated-at.bofh.it> |
| In reply to | #1465210 |
On 18/08/2016 14:18, Denys Vlasenko wrote: > > - 2,588,839,023 branches # 977.555 > M/sec ( +- 0.02% ) > + 2,599,319,615 branches # 1046.786 > M/sec ( +- 0.04% ) > - 3,620,273 branch-misses # 0.14% of all > branches ( +- 0.67% ) > + 3,577,771 branch-misses # 0.14% of all > branches ( +- 0.69% ) > - 2.648799072 seconds time > elapsed ( +- 0.24% ) > + 2.487452268 seconds time > elapsed ( +- 0.31% ) > > Good, we run more insns/cycle, as expected. However, a bit more branches. Can you see where the missed branches are? Assuming branch misses are the case where IF=0, perhaps there are a few places that can be changed to spin_lock/unlock_irq or local_irq_disable/enable. Paolo
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web