Re: [PATCH] x86/smp: Add irq_enter/exit() in smp_reschedule_interrupt()

[Wanpeng Li]

2016-10-17 16:22 GMT+08:00 Peter Zijlstra <peterz@xxxxxxxxxxxxx>:
> On Mon, Oct 17, 2016 at 12:19:43PM +0800, Wanpeng Li wrote:
>> 2016-10-16 21:39 GMT+08:00 Peter Zijlstra <peterz@xxxxxxxxxxxxx>:
>
>> >>   [<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.
>
> 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);
> +}

I move this out of !PARAVIRT since PARAVIRT also will use it.

Regards,
Wanpeng Li

> +
>  #define rdmsrl(msr, val)                       \
>         ((val) = native_read_msr((msr)))
>