Re: [PULL 06/28] spapr: Set VSMT to smp_threads by default

[Laurent Vivier]

On 08/11/2019 16:34, Laurent Vivier wrote:
> On 08/11/2019 15:26, David Gibson wrote:
>> On Fri, Nov 08, 2019 at 02:11:03PM +0100, Laurent Vivier wrote:
>>> On 24/10/2019 10:17, David Gibson wrote:
>>>> From: Greg Kurz <address@hidden>
>>>>
>>>> Support for setting VSMT is available in KVM since linux-4.13. Most distros
>>>> that support KVM on POWER already have it. It thus seem reasonable enough
>>>> to have the default machine to set VSMT to smp_threads.
>>>>
>>>> This brings contiguous VCPU ids and thus brings their upper bound down to
>>>> the machine's max_cpus. This is especially useful for XIVE KVM devices,
>>>> which may thus allocate only one VP descriptor per VCPU.
>>>>
>>>> Signed-off-by: Greg Kurz <address@hidden>
>>>> Message-Id: <address@hidden>
>>>> Signed-off-by: David Gibson <address@hidden>
>>>> ---
>>>>  hw/ppc/spapr.c         | 7 ++++++-
>>>>  include/hw/ppc/spapr.h | 1 +
>>>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index 4eb97d3a9b..428b834f30 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -2496,6 +2496,7 @@ static CPUArchId *spapr_find_cpu_slot(MachineState 
>>>> *ms, uint32_t id, int *idx)
>>>>  static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp)
>>>>  {
>>>>      MachineState *ms = MACHINE(spapr);
>>>> +    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>>>>      Error *local_err = NULL;
>>>>      bool vsmt_user = !!spapr->vsmt;
>>>>      int kvm_smt = kvmppc_smt_threads();
>>>> @@ -2522,7 +2523,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState 
>>>> *spapr, Error **errp)
>>>>              goto out;
>>>>          }
>>>>          /* In this case, spapr->vsmt has been set by the command line */
>>>> -    } else {
>>>> +    } else if (!smc->smp_threads_vsmt) {
>>>>          /*
>>>>           * Default VSMT value is tricky, because we need it to be as
>>>>           * consistent as possible (for migration), but this requires
>>>> @@ -2531,6 +2532,8 @@ static void spapr_set_vsmt_mode(SpaprMachineState 
>>>> *spapr, Error **errp)
>>>>           * overwhelmingly common case in production systems.
>>>>           */
>>>>          spapr->vsmt = MAX(8, smp_threads);
>>>> +    } else {
>>>> +        spapr->vsmt = smp_threads;
>>>>      }
>>>>  
>>>>      /* KVM: If necessary, set the SMT mode: */
>>>> @@ -4438,6 +4441,7 @@ static void spapr_machine_class_init(ObjectClass 
>>>> *oc, void *data)
>>>>      smc->irq = &spapr_irq_dual;
>>>>      smc->dr_phb_enabled = true;
>>>>      smc->linux_pci_probe = true;
>>>> +    smc->smp_threads_vsmt = true;
>>>>  }
>>>>  
>>>>  static const TypeInfo spapr_machine_info = {
>>>> @@ -4505,6 +4509,7 @@ static void 
>>>> spapr_machine_4_1_class_options(MachineClass *mc)
>>>>  
>>>>      spapr_machine_4_2_class_options(mc);
>>>>      smc->linux_pci_probe = false;
>>>> +    smc->smp_threads_vsmt = false;
>>>>      compat_props_add(mc->compat_props, hw_compat_4_1, hw_compat_4_1_len);
>>>>      compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
>>>>  }
>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>> index cbd1a4c9f3..2009eb64f9 100644
>>>> --- a/include/hw/ppc/spapr.h
>>>> +++ b/include/hw/ppc/spapr.h
>>>> @@ -122,6 +122,7 @@ struct SpaprMachineClass {
>>>>      bool broken_host_serial_model; /* present real host info to the guest 
>>>> */
>>>>      bool pre_4_1_migration; /* don't migrate hpt-max-page-size */
>>>>      bool linux_pci_probe;
>>>> +    bool smp_threads_vsmt; /* set VSMT to smp_threads by default */
>>>>  
>>>>      void (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
>>>>                            uint64_t *buid, hwaddr *pio, 
>>>>
>>>
>>> This patch breaks tests/migration-test on P8 host with kernel older than
>>> 4.3 because it tries by default to set the VSMT to 1.
>>>
>>> qemu-system-ppc64: Failed to set KVM's VSMT mode to 1 (errno -22)
>>> On PPC, a VM with 1 threads/core on a host with 8 threads/core requires
>>> the use of VSMT mode 1.
>>> This KVM seems to be too old to support VSMT.
>>>
>>> As this is clearly intentional, is there a way to fix migration-test?
>>
>> Hrm.  I believe the argument for this was that the broken kernels were
>> old enough we didn't care.  What platform are you testing on where
>> you're hitting this?
>>
> 
> I'm going to propose a patch to fix this problem.
> (it was on RHEL7)

Patch sent:
[PATCH] spapr: Fix VSMT mode when it is not supported by the kernel
Message-Id: <address@hidden>

Thanks,
Laurent