Re: RFC: Petition Intel/AMD to add POPF_IF insn

[Denys Vlasenko <dvlasenk@redhat.com>]

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");