Re: [Qemu-devel] [PATCH v3] spapr: set vsmt to MAX(8, smp_threads)

[David Gibson <david@gibson.dropbear.id.au>]

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

On Fri, Feb 09, 2018 at 03:06:49PM +0100, Greg Kurz wrote:
> On Fri,  9 Feb 2018 09:18:58 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
> > We ignore silently the value of smp_threads when we set
> > the default VSMT value, and if smp_threads is greater than VSMT
> > kernel is going into trouble later.
> > 
> 
> Hi Laurent,
> 
> I've looked a bit more and I'm not sure what kernel troubles you're referring to,
> but several places in QEMU where we use kvm_ppc_smt() later on do assume that
> smp_threads > kvm_ppc_smt(). Basically, everywhere we compute a vCPU id:
> 
> In spapr_init_cpus() when creating DRC connectors:
> 
>         int core_id = i * smp_threads;
> 
>         if (mc->has_hotpluggable_cpus) {
>             spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
>                                    (core_id / smp_threads) * smt);
>         }
> 
> or in spapr_cpu_core_realize() when creating vCPUs:
> 
>         cpu->vcpu_id = (cc->core_id * spapr->vsmt / smp_threads) + i;
> 
> It is visible by adding some printfs in the current code base. This is what
> happens when passing -smp cores=2,threads=16 without your patch:
> 
> DRC connector to vcpu_id 0
> CPU vcpu_id 0
> CPU vcpu_id 1
> CPU vcpu_id 2
> CPU vcpu_id 3
> CPU vcpu_id 4
> CPU vcpu_id 5
> CPU vcpu_id 6
> CPU vcpu_id 7
> CPU vcpu_id 8
> CPU vcpu_id 9
> CPU vcpu_id 10
> CPU vcpu_id 11
> CPU vcpu_id 12
> CPU vcpu_id 13
> CPU vcpu_id 14
> CPU vcpu_id 15
> DRC connector to vcpu_id 8
>                         ^^^
>                      should be 16
> CPU vcpu_id 8
>            ^^^
>    should start numbering at 16
> CPU vcpu_id 9
> CPU vcpu_id 10
> CPU vcpu_id 11
> CPU vcpu_id 12
> CPU vcpu_id 13
> CPU vcpu_id 14
> CPU vcpu_id 15
> CPU vcpu_id 16
> CPU vcpu_id 17
> CPU vcpu_id 18
> CPU vcpu_id 19
> CPU vcpu_id 20
> CPU vcpu_id 21
> CPU vcpu_id 22
> CPU vcpu_id 23
> qemu-system-ppc64: kvm_init_vcpu failed: File exists
>                                               ^^^^
>                                  CPU 8 already created by the first core
> 
> I'm not feeling comfortable with the rest of the code silently depending on
> the fact that spapr_set_vsmt_mode() terminates QEMU if it cannot enforce
> smp_threads <= kvm_ppc_smt().

I'm not quite sure what you're suggesting as an alternative, though.

> 
> Anyway, with your patch, the same command line as above gives:
> 
> qemu-system-ppc64: Failed to set KVM's VSMT mode to 16 (errno -22)
> On PPC, a VM with 16 threads/core on a host with 8 threads/core requires
>  the use of VSMT mode 16.
> This KVM seems to be too old to support VSMT.
> 
> This hammer is big enough to fix the vCPU ids miscalculations, so:
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > Fixes: 8904e5a750
> > ("spapr: Adjust default VSMT value for better migration compatibility")
> > 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > ---
> > 
> > Notes:
> >     v3: use MAX(8, smp_threads) and let KVM to return an error
> >         if nb_threads is too big
> >         update subject to reflect the change
> >     
> >     v2: display a specific error message when the default VSMT is used
> >         fix subject
> > 
> >  hw/ppc/spapr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 32a876be56..c8a1eefa17 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2310,7 +2310,7 @@ static void spapr_set_vsmt_mode(sPAPRMachineState *spapr, Error **errp)
> >           * the value that we'd get with KVM on POWER8, the
> >           * overwhelmingly common case in production systems.
> >           */
> > -        spapr->vsmt = 8;
> > +        spapr->vsmt = MAX(8, smp_threads);
> >      }
> >  
> >      /* KVM: If necessary, set the SMT mode: */
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]