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

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]

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

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 1127 bytes --]

 arch/x86/include/asm/irqflags.h | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
index b77f5edb03b0..76c4ebfab0be 100644
--- a/arch/x86/include/asm/irqflags.h
+++ b/arch/x86/include/asm/irqflags.h
@@ -8,6 +8,16 @@
  * Interrupt control:
  */
 
+static inline void native_irq_disable(void)
+{
+	asm volatile("cli": : :"memory");
+}
+
+static inline void native_irq_enable(void)
+{
+	asm volatile("sti": : :"memory");
+}
+
 static inline unsigned long native_save_fl(void)
 {
 	unsigned long flags;
@@ -28,20 +38,8 @@ static inline unsigned long native_save_fl(void)
 
 static inline void native_restore_fl(unsigned long flags)
 {
-	asm volatile("push %0 ; popf"
-		     : /* no output */
-		     :"g" (flags)
-		     :"memory", "cc");
-}
-
-static inline void native_irq_disable(void)
-{
-	asm volatile("cli": : :"memory");
-}
-
-static inline void native_irq_enable(void)
-{
-	asm volatile("sti": : :"memory");
+	if (flags & X86_EFLAGS_IF)
+		native_irq_enable();
 }
 
 static inline void native_safe_halt(void)