From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S934268AbcJQKXa (ORCPT <rfc822;w@1wt.eu>);
        Mon, 17 Oct 2016 06:23:30 -0400
Received: from mail-lf0-f66.google.com ([209.85.215.66]:33894 "EHLO
        mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S933510AbcJQKXW (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 17 Oct 2016 06:23:22 -0400
MIME-Version: 1.0
In-Reply-To: <938757058.3991379.1476698922290.JavaMail.zimbra@redhat.com>
References: <1476409733-5133-1-git-send-email-wanpeng.li@hotmail.com>
 <20161016133911.GI3142@twins.programming.kicks-ass.net> <CANRm+Cy5yN1Cw6J4OKy91P_n1nvB4KBX09KQN9voFPb_yb+Yqw@mail.gmail.com>
 <20161017082250.GX3568@worktop.programming.kicks-ass.net> <CANRm+CxSHMpF4LUfcF1U_8GXnwAo9beXM6C-3JaTpm7Eb3bxUw@mail.gmail.com>
 <938757058.3991379.1476698922290.JavaMail.zimbra@redhat.com>
From: Wanpeng Li <kernellwp@gmail.com>
Date: Mon, 17 Oct 2016 18:23:19 +0800
Message-ID: <CANRm+Czs_Sg1NPPYmR5tNdvf8z_yYUMX7HTJ0x4aJqrcCq=Jbg@mail.gmail.com>
Subject: Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Wanpeng Li <wanpeng.li@hotmail.com>, Ingo Molnar <mingo@kernel.org>,
        Mike Galbraith <efault@gmx.de>, Thomas Gleixner <tglx@linutronix.de>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

2016-10-17 18:08 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> ----- Original Message -----
>> From: "Wanpeng Li" <kernellwp@gmail.com>
>> To: "Peter Zijlstra" <peterz@infradead.org>
>> Cc: linux-kernel@vger.kernel.org, "Wanpeng Li" <wanpeng.li@hotmail.com>, "Ingo Molnar" <mingo@kernel.org>, "Mike
>> Galbraith" <efault@gmx.de>, "Thomas Gleixner" <tglx@linutronix.de>, "Paolo Bonzini" <pbonzini@redhat.com>
>> Sent: Monday, October 17, 2016 11:45:32 AM
>> Subject: Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()
>>
>> Cc Paolo,
>> 2016-10-17 16:22 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
>> > On Mon, Oct 17, 2016 at 12:19:43PM +0800, Wanpeng Li wrote:
>> >> 2016-10-16 21:39 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
>> >
>> >> >>   [<ffffffff9d492b95>] do_trace_write_msr+0x135/0x140
>> >> >>   [<ffffffff9d06f860>] native_write_msr+0x20/0x30
>> >> >>   [<ffffffff9d065fad>] native_apic_msr_eoi_write+0x1d/0x30
>> >> >>   [<ffffffff9d05bd1d>] smp_reschedule_interrupt+0x1d/0x30
>> >> >>   [<ffffffff9d8daec6>] reschedule_interrupt+0x96/0xa0
>> >
>> >> >>  __visible void smp_reschedule_interrupt(struct pt_regs *regs)
>> >> >>  {
>> >> >> +     irq_enter();
>> >> >>       ack_APIC_irq();
>> >> >>       __smp_reschedule_interrupt();
>> >> >> +     irq_exit();
>> >> >
>> >> > Urgh, I really hate this...
>> >> >
>> >> > So now we're making a very frequent interrupt slower because of debug
>> >> > code :/
>> >>
>> >> Do you have a better idea? :)
>> >
>> > Something like the below avoids all that. Paravirt will still need fixing.
>>
>> kvm_guest_apic_eoi_write
>>  -> native_apic_msr_write
>
> kvm_guest_apic_eoi_write can use native_apic_msr_eoi_write too:
>
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index f5aaf6c83222..9769d76a62c4 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -174,7 +174,7 @@ static inline void disable_local_APIC(void) { }
>  static inline void lapic_update_tsc_freq(void) { }
>  #endif /* !CONFIG_X86_LOCAL_APIC */
>
> -#ifdef CONFIG_X86_X2APIC
> +#if defined CONFIG_X86_X2APIC || defined CONFIG_KVM_GUEST
>  /*
>   * Make previous memory operations globally visible before
>   * sending the IPI through x2apic wrmsr. We need a serializing instruction or
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index edbbfc854e39..61cc6a5e3f44 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -319,7 +319,7 @@ static void kvm_guest_apic_eoi_write(u32 reg, u32 val)
>          */
>         if (__test_and_clear_bit(KVM_PV_EOI_BIT, this_cpu_ptr(&kvm_apic_eoi)))
>                 return;
> -       apic_write(APIC_EOI, APIC_EOI_ACK);
> +       native_apic_msr_eoi_write(APIC_EOI, APIC_EOI_ACK);
>  }
>
>  static void kvm_guest_cpu_init(void)

I see, thanks Paolo and Peterz. :)

Regards,
Wanpeng Li

>
>
> Thanks,
>
> Paolo
>
>> I think you can replace the wrmsr in native_apic_msr_write() by your
>> wrmsr_notrace().
>>
>> Regards,
>> Wanpeng Li
>>
>> >
>> > The thing is, many many smp_reschedule_interrupt() invocations don't
>> > actually execute anything much at all and are only send to tickle the
>> > return to user path (which does the actual preemption).
>> >
>> > Having to do the whole irq_enter/irq_exit dance just for this unlikely
>> > debug case totally blows.
>> >
>> > ---
>> >  arch/x86/include/asm/apic.h |  2 +-
>> >  arch/x86/include/asm/msr.h  | 15 +++++++++++++++
>> >  2 files changed, 16 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>> > index f5aaf6c83222..b97bfeed6456 100644
>> > --- a/arch/x86/include/asm/apic.h
>> > +++ b/arch/x86/include/asm/apic.h
>> > @@ -196,7 +196,7 @@ static inline void native_apic_msr_write(u32 reg, u32
>> > v)
>> >
>> >  static inline void native_apic_msr_eoi_write(u32 reg, u32 v)
>> >  {
>> > -       wrmsr(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0);
>> > +       wrmsr_notrace(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0);
>> >  }
>> >
>> >  static inline u32 native_apic_msr_read(u32 reg)
>> > diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
>> > index b5fee97813cd..45c080449d5b 100644
>> > --- a/arch/x86/include/asm/msr.h
>> > +++ b/arch/x86/include/asm/msr.h
>> > @@ -127,6 +127,16 @@ notrace static inline void native_write_msr(unsigned
>> > int msr,
>> >  }
>> >
>> >  /* Can be uninlined because referenced by paravirt */
>> > +notrace static inline void native_write_msr_notrace(unsigned int msr,
>> > +                                           unsigned low, unsigned high)
>> > +{
>> > +       asm volatile("1: wrmsr\n"
>> > +                    "2:\n"
>> > +                    _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe)
>> > +                    : : "c" (msr), "a"(low), "d" (high) : "memory");
>> > +}
>> > +
>> > +/* Can be uninlined because referenced by paravirt */
>> >  notrace static inline int native_write_msr_safe(unsigned int msr,
>> >                                         unsigned low, unsigned high)
>> >  {
>> > @@ -228,6 +238,11 @@ static inline void wrmsr(unsigned msr, unsigned low,
>> > unsigned high)
>> >         native_write_msr(msr, low, high);
>> >  }
>> >
>> > +static inline void wrmsr_notrace(unsigned msr, unsigned low, unsigned
>> > high)
>> > +{
>> > +       native_write_msr_notrace(msr, low, high);
>> > +}
>> > +
>> >  #define rdmsrl(msr, val)                       \
>> >         ((val) = native_read_msr((msr)))
>> >
>>