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

2018-02-12 Thread 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  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  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

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

2018-02-12 Thread Greg Kurz 
On Sat, 10 Feb 2018 20:23:07 +1100
David Gibson  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  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,
- this function should ensure all pre-requisites to compute the vcpu_id are
  met (including trying to set VSMT) or return an error

> > 
> > 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 
> >   
> > > Fixes: 8904e5a750
> > > ("spapr: Adjust default VSMT value for better migration compatibility")
> > > 
> > > Signed-off-by: Laurent Vivier 
> > > ---
> > > 
> > > 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: */  
> >   
>

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

2018-02-10 Thread David Gibson 
On Fri, Feb 09, 2018 at 03:06:49PM +0100, Greg Kurz wrote:
> On Fri,  9 Feb 2018 09:18:58 +0100
> Laurent Vivier  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 
> 
> > Fixes: 8904e5a750
> > ("spapr: Adjust default VSMT value for better migration compatibility")
> > 
> > Signed-off-by: Laurent Vivier 
> > ---
> > 
> > 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


signature.asc
Description: PGP signature

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

2018-02-10 Thread David Gibson 
On Fri, Feb 09, 2018 at 09:18:58AM +0100, Laurent Vivier 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.
> 
> Fixes: 8904e5a750
> ("spapr: Adjust default VSMT value for better migration compatibility")
> 
> Signed-off-by: Laurent Vivier 

Applied, thanks.

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


signature.asc
Description: PGP signature

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

2018-02-09 Thread Greg Kurz 
On Fri,  9 Feb 2018 09:18:58 +0100
Laurent Vivier  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().

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 

> Fixes: 8904e5a750
> ("spapr: Adjust default VSMT value for better migration compatibility")
> 
> Signed-off-by: Laurent Vivier 
> ---
> 
> 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: */

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

2018-02-09 Thread Laurent Vivier 
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.

Fixes: 8904e5a750
("spapr: Adjust default VSMT value for better migration compatibility")

Signed-off-by: Laurent Vivier 
---

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: */
-- 
2.14.3