Re: [PATCH v2 1/2] KVM: X86: Implement PV IPI in linux guest

[Wanpeng Li]

On Mon, 2 Jul 2018 at 18:26, Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote:
>
> Wanpeng Li <kernellwp@xxxxxxxxx> writes:
>
> > From: Wanpeng Li <wanpengli@xxxxxxxxxxx>
> >
> > Implement PV IPIs in guest kernel.
> >
> > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > Cc: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> > Cc: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> > Signed-off-by: Wanpeng Li <wanpengli@xxxxxxxxxxx>
> > ---
> >  arch/x86/kernel/kvm.c         | 99 +++++++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/kvm_para.h |  1 +
> >  2 files changed, 100 insertions(+)
> >
> > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> > index 5b2300b..7e3ee25 100644
> > --- a/arch/x86/kernel/kvm.c
> > +++ b/arch/x86/kernel/kvm.c
> > @@ -47,6 +47,7 @@
> >  #include <asm/hypervisor.h>
> >  #include <asm/kvm_guest.h>
> >
> > +static struct apic orig_apic;
> >  static int kvmapf = 1;
> >
> >  static int __init parse_no_kvmapf(char *arg)
> > @@ -454,6 +455,89 @@ static void __init sev_map_percpu_data(void)
> >  }
> >
> >  #ifdef CONFIG_SMP
> > +
> > +#ifdef CONFIG_X86_64
> > +static bool __send_ipi_mask(const struct cpumask *mask, int vector)
> > +{
> > +     unsigned long flags, ipi_bitmap_low = 0, ipi_bitmap_high = 0, icr = 0;
> > +     int cpu, apic_id, ret = 1;
> > +
> > +     if (cpumask_empty(mask))
> > +             return true;
> > +
> > +     local_irq_save(flags);
> > +
> > +     for_each_cpu(cpu, mask) {
> > +             apic_id = per_cpu(x86_cpu_to_apicid, cpu);
> > +             if (apic_id < BITS_PER_LONG)
> > +                     __set_bit(apic_id, &ipi_bitmap_low);
> > +             else if (apic_id < 2 * BITS_PER_LONG)
> > +                     __set_bit(apic_id - BITS_PER_LONG, &ipi_bitmap_high);
> > +             else
> > +                     goto ipi_mask_done;
>
> Nit:
>
> Both the fact that we don't set 'ret' here and the fact that the label
> is named 'ipi_mask_done' -- which sounds like 'all OK' at least to me --
> contribute to the feeling that we just skip sending IPIs in some cases.
>
> I would prefer to see something like
>
> else {
>    ret = -EFAULT;
>    goto irq_restore_exit;
> }
>
> > +     }
> > +
> > +     switch (vector) {
> > +     default:
> > +             icr = APIC_DM_FIXED | vector;
> > +             break;
> > +     case NMI_VECTOR:
> > +             icr = APIC_DM_NMI;
> > +             break;
> > +     }
> > +
> > +     ret = kvm_hypercall3(KVM_HC_SEND_IPI, ipi_bitmap_low, ipi_bitmap_high, icr);
> > +
> > +ipi_mask_done:
> > +     local_irq_restore(flags);
> > +     return ((ret == 0) ? true : false);
>
> ... and why in the first place do we need to make this function return
> 'bool' then? Let's just make it return 'int'.

Thanks for the comments, will do in v3. Btw, on my haswell desktop(i7
8 HT), there is a 2.5 times performance boot for the IPI
microbenchmark(https://lkml.org/lkml/2017/12/19/141). (8 vCPUs guest,
x2apic physical mode, I will retest on Skylake server w/ 64 vCPUs
x2apic cluster mode guest tomorrow):

Before:
 Dry-run:                         0,            1885493 ns
 Self-IPI:                  7071403,           14711151 ns
 Normal IPI:              204453899,          219896346 ns
 Broadcast IPI:                   0,         2213679722 ns
 Broadcast lock:                  0,         2241226307 ns

After:
 Dry-run:                         0,            1752903 ns
 Self-IPI:                  4944737,           10434149 ns
 Normal IPI:              202351280,          220807969 ns
 Broadcast IPI:                   0,          872991742 ns
=> 2.5 times boost
 Broadcast lock:                  0,          879995113 ns

Regards,
Wanpeng Li