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

[David Gibson]

On Mon, Feb 12, 2018 at 12:11:27PM +0100, Greg Kurz wrote:
> On Sat, 10 Feb 2018 20:23:07 +1100
> David Gibson <address@hidden> wrote:
> 
> > On Fri, Feb 09, 2018 at 03:06:49PM +0100, Greg Kurz wrote:
> > > On Fri,  9 Feb 2018 09:18:58 +0100
> > > Laurent Vivier <address@hidden> 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.
> > 
> 
> I haven't suggested anything yet :)
> 
> But I was thinking of:
> - having a single function to compute the vcpu_id, instead of open-coding
>   the formula in several places like the current code does,

That seems like a good idea.

> - this function should ensure all pre-requisites to compute the vcpu_id are
>   met (including trying to set VSMT) or return an error

I'm much more dubious about this.  Checks that can be performed
passively I'm ok with, but I think something that looks like a simple
calculation helper shouldn't be having side-effects like poking at KVM
state.

-- 
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

signature.asc
Description: PGP signature