[PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[Laurent Vivier]

Commit 29cb4187497d sets by default the VSMT to smp_threads,
but older kernels (< 4.13) don't support that.

We can reasonably restore previous behavior with this kernel
to allow to run QEMU as before.

If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
as it is done for previous machine types (< pseries-4.2)

Fixes: 29cb4187497d ("spapr: Set VSMT to smp_threads by default")
Cc: groug@kaod.org
Reported-by: Lukáš Doktor <ldoktor@redhat.com>
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/ppc/spapr.c       | 2 +-
 target/ppc/kvm.c     | 5 +++++
 target/ppc/kvm_ppc.h | 6 ++++++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 94f9d27096af..f6c8ad1eda32 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2522,7 +2522,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 if (!smc->smp_threads_vsmt) {
+    } else if (!smc->smp_threads_vsmt || !kvmppc_check_smt_possible()) {
         /*
          * Default VSMT value is tricky, because we need it to be as
          * consistent as possible (for migration), but this requires
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 7d2e8969ac5f..40ed59881167 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2060,6 +2060,11 @@ void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy)
     }
 }
 
+bool kvmppc_check_smt_possible(void)
+{
+    return kvm_enabled() && cap_ppc_smt_possible;
+}
+
 int kvmppc_smt_threads(void)
 {
     return cap_ppc_smt ? cap_ppc_smt : 1;
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 98bd7d5da6d6..c9629a416b0b 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -27,6 +27,7 @@ void kvmppc_enable_h_page_init(void);
 void kvmppc_set_papr(PowerPCCPU *cpu);
 int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr);
 void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy);
+bool kvmppc_check_smt_possible(void);
 int kvmppc_smt_threads(void);
 void kvmppc_hint_smt_possible(Error **errp);
 int kvmppc_set_smt_threads(int smt);
@@ -159,6 +160,11 @@ static inline void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy)
 {
 }
 
+static inline bool kvmppc_check_smt_possible(void)
+{
+    return false;
+}
+
 static inline int kvmppc_smt_threads(void)
 {
     return 1;
-- 
2.21.0

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[Greg Kurz]

On Fri,  8 Nov 2019 16:40:35 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> Commit 29cb4187497d sets by default the VSMT to smp_threads,
> but older kernels (< 4.13) don't support that.
> 
> We can reasonably restore previous behavior with this kernel
> to allow to run QEMU as before.
> 
> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
> as it is done for previous machine types (< pseries-4.2)
> 

It is usually _bad_ to base the machine behavior on host capabilities.
What happens if we migrate between an older kernel and a recent one ?

I understand this is to fix tests/migration-test on older kernels.
Couldn't this be achieved with migration-test doing some introspection
and maybe pass vsmt=8 on the QEMU command line ?

> Fixes: 29cb4187497d ("spapr: Set VSMT to smp_threads by default")
> Cc: groug@kaod.org
> Reported-by: Lukáš Doktor <ldoktor@redhat.com>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  hw/ppc/spapr.c       | 2 +-
>  target/ppc/kvm.c     | 5 +++++
>  target/ppc/kvm_ppc.h | 6 ++++++
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 94f9d27096af..f6c8ad1eda32 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2522,7 +2522,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 if (!smc->smp_threads_vsmt) {
> +    } else if (!smc->smp_threads_vsmt || !kvmppc_check_smt_possible()) {
>          /*
>           * Default VSMT value is tricky, because we need it to be as
>           * consistent as possible (for migration), but this requires
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 7d2e8969ac5f..40ed59881167 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2060,6 +2060,11 @@ void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy)
>      }
>  }
>  
> +bool kvmppc_check_smt_possible(void)
> +{
> +    return kvm_enabled() && cap_ppc_smt_possible;
> +}
> +
>  int kvmppc_smt_threads(void)
>  {
>      return cap_ppc_smt ? cap_ppc_smt : 1;
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 98bd7d5da6d6..c9629a416b0b 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -27,6 +27,7 @@ void kvmppc_enable_h_page_init(void);
>  void kvmppc_set_papr(PowerPCCPU *cpu);
>  int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr);
>  void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy);
> +bool kvmppc_check_smt_possible(void);
>  int kvmppc_smt_threads(void);
>  void kvmppc_hint_smt_possible(Error **errp);
>  int kvmppc_set_smt_threads(int smt);
> @@ -159,6 +160,11 @@ static inline void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy)
>  {
>  }
>  
> +static inline bool kvmppc_check_smt_possible(void)
> +{
> +    return false;
> +}
> +
>  static inline int kvmppc_smt_threads(void)
>  {
>      return 1;

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[Laurent Vivier]

On 08/11/2019 17:47, Greg Kurz wrote:
> On Fri,  8 Nov 2019 16:40:35 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
>> Commit 29cb4187497d sets by default the VSMT to smp_threads,
>> but older kernels (< 4.13) don't support that.
>>
>> We can reasonably restore previous behavior with this kernel
>> to allow to run QEMU as before.
>>
>> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
>> as it is done for previous machine types (< pseries-4.2)
>>
> 
> It is usually _bad_ to base the machine behavior on host capabilities.

This is already the case, statically: your patch guesses the kernel
always support VSMT. So you can't start the machine (and thus can't
migrate it to/from).

> What happens if we migrate between an older kernel and a recent one ?

I think migration is supported correctly only if parameters are
explicitly set. So this is not our case.

> I understand this is to fix tests/migration-test on older kernels.
> Couldn't this be achieved with migration-test doing some introspection
> and maybe pass vsmt=8 on the QEMU command line ?

It could be a little bit complicated to instrospect this.
We could also set by default vsmt=8 at the test level.

Thanks,
Laurent

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[Greg Kurz]

On Fri, 8 Nov 2019 18:03:08 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 08/11/2019 17:47, Greg Kurz wrote:
> > On Fri,  8 Nov 2019 16:40:35 +0100
> > Laurent Vivier <lvivier@redhat.com> wrote:
> > 
> >> Commit 29cb4187497d sets by default the VSMT to smp_threads,
> >> but older kernels (< 4.13) don't support that.
> >>
> >> We can reasonably restore previous behavior with this kernel
> >> to allow to run QEMU as before.
> >>
> >> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
> >> as it is done for previous machine types (< pseries-4.2)
> >>
> > 
> > It is usually _bad_ to base the machine behavior on host capabilities.
> 
> This is already the case, statically: your patch guesses the kernel
> always support VSMT. So you can't start the machine (and thus can't
> migrate it to/from).
> 

Failing to start the machine is okay. The thing we want to avoid is
to successfully start the guest and discover later on we cannot
migrate it.

> > What happens if we migrate between an older kernel and a recent one ?
> 
> I think migration is supported correctly only if parameters are
> explicitly set. So this is not our case.
> 

Hence my suggestion to introspect...

> > I understand this is to fix tests/migration-test on older kernels.
> > Couldn't this be achieved with migration-test doing some introspection
> > and maybe pass vsmt=8 on the QEMU command line ?
> 
> It could be a little bit complicated to instrospect this.

... ah, you likely know migration-test better than I do :)

> We could also set by default vsmt=8 at the test level.
> 

This would mean that we don't test an exact default setup, but this
would still test most of the migration paths. I guess this could be
an acceptable trade-off only if 'make check' must *really* support
older kernels.

> Thanks,
> Laurent
> 

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[David Gibson]

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

On Fri, Nov 08, 2019 at 05:47:59PM +0100, Greg Kurz wrote:
> On Fri,  8 Nov 2019 16:40:35 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
> > Commit 29cb4187497d sets by default the VSMT to smp_threads,
> > but older kernels (< 4.13) don't support that.
> > 
> > We can reasonably restore previous behavior with this kernel
> > to allow to run QEMU as before.
> > 
> > If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
> > as it is done for previous machine types (< pseries-4.2)
> > 
> 
> It is usually _bad_ to base the machine behavior on host capabilities.
> What happens if we migrate between an older kernel and a recent one ?

Right.  We're really trying to remove instaces of such behaviour.  I'd
prefer to completely revert Greg's original patch than to re-introduce
host configuration dependency into the guest configuration..

> I understand this is to fix tests/migration-test on older kernels.
> Couldn't this be achieved with migration-test doing some introspection
> and maybe pass vsmt=8 on the QEMU command line ?

..adjusting the test case like this might be a better idea, though.

What's the test setup where we're using the old kernel?  I really only
applied the original patch on the guess that we didn't really care
about kernels that old.  The fact you've hit this in practice makes me
doubt that assumption.

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

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[Laurent Vivier]

On 19/11/2019 02:00, David Gibson wrote:
> On Fri, Nov 08, 2019 at 05:47:59PM +0100, Greg Kurz wrote:
>> On Fri,  8 Nov 2019 16:40:35 +0100
>> Laurent Vivier <lvivier@redhat.com> wrote:
>>
>>> Commit 29cb4187497d sets by default the VSMT to smp_threads,
>>> but older kernels (< 4.13) don't support that.
>>>
>>> We can reasonably restore previous behavior with this kernel
>>> to allow to run QEMU as before.
>>>
>>> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
>>> as it is done for previous machine types (< pseries-4.2)
>>>
>>
>> It is usually _bad_ to base the machine behavior on host capabilities.
>> What happens if we migrate between an older kernel and a recent one ?
> 
> Right.  We're really trying to remove instaces of such behaviour.  I'd
> prefer to completely revert Greg's original patch than to re-introduce
> host configuration dependency into the guest configuration..
> 
>> I understand this is to fix tests/migration-test on older kernels.
>> Couldn't this be achieved with migration-test doing some introspection
>> and maybe pass vsmt=8 on the QEMU command line ?
> 
> ..adjusting the test case like this might be a better idea, though.
> 
> What's the test setup where we're using the old kernel?  I really only
> applied the original patch on the guess that we didn't really care
> about kernels that old.  The fact you've hit this in practice makes me
> doubt that assumption.
> 

The way to fix the tests is to add "-smp threads=8" on the command line
(for all tests, so basically in qtest_init_without_qmp_handshake(), and
it will impact all the machine types), and we have to check if it is
ppc64/pseries to do that, and there it becomes a little bit complicated
for a so small piece of code.

So I think the best to do is to revert Greg's patch.

Thanks,
Laurent

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[Greg Kurz]

On Tue, 19 Nov 2019 15:06:51 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 19/11/2019 02:00, David Gibson wrote:
> > On Fri, Nov 08, 2019 at 05:47:59PM +0100, Greg Kurz wrote:
> >> On Fri,  8 Nov 2019 16:40:35 +0100
> >> Laurent Vivier <lvivier@redhat.com> wrote:
> >>
> >>> Commit 29cb4187497d sets by default the VSMT to smp_threads,
> >>> but older kernels (< 4.13) don't support that.
> >>>
> >>> We can reasonably restore previous behavior with this kernel
> >>> to allow to run QEMU as before.
> >>>
> >>> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
> >>> as it is done for previous machine types (< pseries-4.2)
> >>>
> >>
> >> It is usually _bad_ to base the machine behavior on host capabilities.
> >> What happens if we migrate between an older kernel and a recent one ?
> > 
> > Right.  We're really trying to remove instaces of such behaviour.  I'd
> > prefer to completely revert Greg's original patch than to re-introduce
> > host configuration dependency into the guest configuration..
> > 
> >> I understand this is to fix tests/migration-test on older kernels.
> >> Couldn't this be achieved with migration-test doing some introspection
> >> and maybe pass vsmt=8 on the QEMU command line ?
> > 
> > ..adjusting the test case like this might be a better idea, though.
> > 
> > What's the test setup where we're using the old kernel?  I really only
> > applied the original patch on the guess that we didn't really care
> > about kernels that old.  The fact you've hit this in practice makes me
> > doubt that assumption.
> > 
> 
> The way to fix the tests is to add "-smp threads=8" on the command line
> (for all tests, so basically in qtest_init_without_qmp_handshake(), and
> it will impact all the machine types), and we have to check if it is

Ohhh... it isn't possible to initialize Qtest with machine specific
properties ? That's a bit unfortunate :-\

> ppc64/pseries to do that, and there it becomes a little bit complicated
> for a so small piece of code.
> 
> So I think the best to do is to revert Greg's patch.
> 

I'm okay with that since this patch doesn't bring much for the moment.

But soon, ie. linux-5.5 hopefully, KVM will allow to configure the number
of presenters in the XIVE and XICS-on-XIVE devices on POWER9. Combined
with this patch, it will allow to drastically reduce the consumption of
resources in the XIVE HW, which currently limits the number of VMs that
can run concurrently with an in-kernel irqchip. So I hope the 'make check'
you're willing to fix is worth it :-), and BTW you didn't answer David's
question about the test setup.

Cheers,

--
Greg

> Thanks,
> Laurent
> 

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[Lukáš Doktor]

[-- Attachment #1.1: Type: text/plain, Size: 3215 bytes --]

Dne 19. 11. 19 v 16:45 Greg Kurz napsal(a):
> On Tue, 19 Nov 2019 15:06:51 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
>> On 19/11/2019 02:00, David Gibson wrote:
>>> On Fri, Nov 08, 2019 at 05:47:59PM +0100, Greg Kurz wrote:
>>>> On Fri,  8 Nov 2019 16:40:35 +0100
>>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>>
>>>>> Commit 29cb4187497d sets by default the VSMT to smp_threads,
>>>>> but older kernels (< 4.13) don't support that.
>>>>>
>>>>> We can reasonably restore previous behavior with this kernel
>>>>> to allow to run QEMU as before.
>>>>>
>>>>> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
>>>>> as it is done for previous machine types (< pseries-4.2)
>>>>>
>>>>
>>>> It is usually _bad_ to base the machine behavior on host capabilities.
>>>> What happens if we migrate between an older kernel and a recent one ?
>>>
>>> Right.  We're really trying to remove instaces of such behaviour.  I'd
>>> prefer to completely revert Greg's original patch than to re-introduce
>>> host configuration dependency into the guest configuration..
>>>
>>>> I understand this is to fix tests/migration-test on older kernels.
>>>> Couldn't this be achieved with migration-test doing some introspection
>>>> and maybe pass vsmt=8 on the QEMU command line ?
>>>
>>> ..adjusting the test case like this might be a better idea, though.
>>>
>>> What's the test setup where we're using the old kernel?  I really only
>>> applied the original patch on the guess that we didn't really care
>>> about kernels that old.  The fact you've hit this in practice makes me
>>> doubt that assumption.
>>>
>>
>> The way to fix the tests is to add "-smp threads=8" on the command line
>> (for all tests, so basically in qtest_init_without_qmp_handshake(), and
>> it will impact all the machine types), and we have to check if it is
> 
> Ohhh... it isn't possible to initialize Qtest with machine specific
> properties ? That's a bit unfortunate :-\
> 
>> ppc64/pseries to do that, and there it becomes a little bit complicated
>> for a so small piece of code.
>>
>> So I think the best to do is to revert Greg's patch.
>>
> 
> I'm okay with that since this patch doesn't bring much for the moment.
> 
> But soon, ie. linux-5.5 hopefully, KVM will allow to configure the number
> of presenters in the XIVE and XICS-on-XIVE devices on POWER9. Combined
> with this patch, it will allow to drastically reduce the consumption of
> resources in the XIVE HW, which currently limits the number of VMs that
> can run concurrently with an in-kernel irqchip. So I hope the 'make check'
> you're willing to fix is worth it :-), and BTW you didn't answer David's
> question about the test setup.
> 

Hello Greg, guys,

it is P8 machine using RHEL7 host with a stock kernel-3.10.0-XXXX. If you're interested in the whole setup, then:

    ./configure --enable-kvm --block-drv-rw-whitelist=vmdk,null-aio,quorum,null-co,blkverify,file,nbd,raw,blkdebug,host_device,qed,nbd,iscsi,gluster,rbd,qcow2,throttle,copy-on-read
    make -j 13
    SPEED=slow make check

Regards,
Lukáš

> Cheers,
> 
> --
> Greg
> 
>> Thanks,
>> Laurent
>>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[David Gibson]

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

On Tue, Nov 19, 2019 at 05:13:29PM +0100, Lukáš Doktor wrote:
> Dne 19. 11. 19 v 16:45 Greg Kurz napsal(a):
> > On Tue, 19 Nov 2019 15:06:51 +0100
> > Laurent Vivier <lvivier@redhat.com> wrote:
> > 
> >> On 19/11/2019 02:00, David Gibson wrote:
> >>> On Fri, Nov 08, 2019 at 05:47:59PM +0100, Greg Kurz wrote:
> >>>> On Fri,  8 Nov 2019 16:40:35 +0100
> >>>> Laurent Vivier <lvivier@redhat.com> wrote:
> >>>>
> >>>>> Commit 29cb4187497d sets by default the VSMT to smp_threads,
> >>>>> but older kernels (< 4.13) don't support that.
> >>>>>
> >>>>> We can reasonably restore previous behavior with this kernel
> >>>>> to allow to run QEMU as before.
> >>>>>
> >>>>> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
> >>>>> as it is done for previous machine types (< pseries-4.2)
> >>>>>
> >>>>
> >>>> It is usually _bad_ to base the machine behavior on host capabilities.
> >>>> What happens if we migrate between an older kernel and a recent one ?
> >>>
> >>> Right.  We're really trying to remove instaces of such behaviour.  I'd
> >>> prefer to completely revert Greg's original patch than to re-introduce
> >>> host configuration dependency into the guest configuration..
> >>>
> >>>> I understand this is to fix tests/migration-test on older kernels.
> >>>> Couldn't this be achieved with migration-test doing some introspection
> >>>> and maybe pass vsmt=8 on the QEMU command line ?
> >>>
> >>> ..adjusting the test case like this might be a better idea, though.
> >>>
> >>> What's the test setup where we're using the old kernel?  I really only
> >>> applied the original patch on the guess that we didn't really care
> >>> about kernels that old.  The fact you've hit this in practice makes me
> >>> doubt that assumption.
> >>>
> >>
> >> The way to fix the tests is to add "-smp threads=8" on the command line
> >> (for all tests, so basically in qtest_init_without_qmp_handshake(), and
> >> it will impact all the machine types), and we have to check if it is
> > 
> > Ohhh... it isn't possible to initialize Qtest with machine specific
> > properties ? That's a bit unfortunate :-\
> > 
> >> ppc64/pseries to do that, and there it becomes a little bit complicated
> >> for a so small piece of code.
> >>
> >> So I think the best to do is to revert Greg's patch.
> >>
> > 
> > I'm okay with that since this patch doesn't bring much for the moment.
> > 
> > But soon, ie. linux-5.5 hopefully, KVM will allow to configure the number
> > of presenters in the XIVE and XICS-on-XIVE devices on POWER9. Combined
> > with this patch, it will allow to drastically reduce the consumption of
> > resources in the XIVE HW, which currently limits the number of VMs that
> > can run concurrently with an in-kernel irqchip. So I hope the 'make check'
> > you're willing to fix is worth it :-), and BTW you didn't answer David's
> > question about the test setup.
> > 
> 
> Hello Greg, guys,
> 
> it is P8 machine using RHEL7 host with a stock kernel-3.10.0-XXXX. If you're interested in the whole setup, then:
> 
>     ./configure --enable-kvm --block-drv-rw-whitelist=vmdk,null-aio,quorum,null-co,blkverify,file,nbd,raw,blkdebug,host_device,qed,nbd,iscsi,gluster,rbd,qcow2,throttle,copy-on-read
>     make -j 13
>     SPEED=slow make check

Really the only relevant factor here is that it's a RHEL7 host, which
has an old kernel (3.10 is misleading, since it has a *heap* of
backports, but support for the vsmt feature isn't one of them).

So more generally than this specific test, the question really is
whether we thinkg RHEL7 is still in wide enough use that we care to
keep supporting it with the latest upstream machine type.  I'm a bit
undecided on that.

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

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[David Gibson]

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

On Tue, Nov 19, 2019 at 04:45:26PM +0100, Greg Kurz wrote:
> On Tue, 19 Nov 2019 15:06:51 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
> > On 19/11/2019 02:00, David Gibson wrote:
> > > On Fri, Nov 08, 2019 at 05:47:59PM +0100, Greg Kurz wrote:
> > >> On Fri,  8 Nov 2019 16:40:35 +0100
> > >> Laurent Vivier <lvivier@redhat.com> wrote:
> > >>
> > >>> Commit 29cb4187497d sets by default the VSMT to smp_threads,
> > >>> but older kernels (< 4.13) don't support that.
> > >>>
> > >>> We can reasonably restore previous behavior with this kernel
> > >>> to allow to run QEMU as before.
> > >>>
> > >>> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
> > >>> as it is done for previous machine types (< pseries-4.2)
> > >>>
> > >>
> > >> It is usually _bad_ to base the machine behavior on host capabilities.
> > >> What happens if we migrate between an older kernel and a recent one ?
> > > 
> > > Right.  We're really trying to remove instaces of such behaviour.  I'd
> > > prefer to completely revert Greg's original patch than to re-introduce
> > > host configuration dependency into the guest configuration..
> > > 
> > >> I understand this is to fix tests/migration-test on older kernels.
> > >> Couldn't this be achieved with migration-test doing some introspection
> > >> and maybe pass vsmt=8 on the QEMU command line ?
> > > 
> > > ..adjusting the test case like this might be a better idea, though.
> > > 
> > > What's the test setup where we're using the old kernel?  I really only
> > > applied the original patch on the guess that we didn't really care
> > > about kernels that old.  The fact you've hit this in practice makes me
> > > doubt that assumption.
> > > 
> > 
> > The way to fix the tests is to add "-smp threads=8" on the command line
> > (for all tests, so basically in qtest_init_without_qmp_handshake(), and
> > it will impact all the machine types), and we have to check if it is
> 
> Ohhh... it isn't possible to initialize Qtest with machine specific
> properties ? That's a bit unfortunate :-\

Uhh... I don't see why we can't.  Couldn't we just put either -machine
vsmt=8 or -smp 8 into the cmd_src / cmd_dst printfs() in the
strcmp(arch, "ppc64") case?

> > ppc64/pseries to do that, and there it becomes a little bit complicated
> > for a so small piece of code.
> > 
> > So I think the best to do is to revert Greg's patch.
> > 
> 
> I'm okay with that since this patch doesn't bring much for the moment.
> 
> But soon, ie. linux-5.5 hopefully, KVM will allow to configure the number
> of presenters in the XIVE and XICS-on-XIVE devices on POWER9. Combined
> with this patch, it will allow to drastically reduce the consumption of
> resources in the XIVE HW, which currently limits the number of VMs that
> can run concurrently with an in-kernel irqchip. So I hope the 'make check'
> you're willing to fix is worth it :-), and BTW you didn't answer David's
> question about the test setup.
> 

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

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[Greg Kurz]

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

On Wed, 20 Nov 2019 15:36:53 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Nov 19, 2019 at 04:45:26PM +0100, Greg Kurz wrote:
> > On Tue, 19 Nov 2019 15:06:51 +0100
> > Laurent Vivier <lvivier@redhat.com> wrote:
> > 
> > > On 19/11/2019 02:00, David Gibson wrote:
> > > > On Fri, Nov 08, 2019 at 05:47:59PM +0100, Greg Kurz wrote:
> > > >> On Fri,  8 Nov 2019 16:40:35 +0100
> > > >> Laurent Vivier <lvivier@redhat.com> wrote:
> > > >>
> > > >>> Commit 29cb4187497d sets by default the VSMT to smp_threads,
> > > >>> but older kernels (< 4.13) don't support that.
> > > >>>
> > > >>> We can reasonably restore previous behavior with this kernel
> > > >>> to allow to run QEMU as before.
> > > >>>
> > > >>> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
> > > >>> as it is done for previous machine types (< pseries-4.2)
> > > >>>
> > > >>
> > > >> It is usually _bad_ to base the machine behavior on host capabilities.
> > > >> What happens if we migrate between an older kernel and a recent one ?
> > > > 
> > > > Right.  We're really trying to remove instaces of such behaviour.  I'd
> > > > prefer to completely revert Greg's original patch than to re-introduce
> > > > host configuration dependency into the guest configuration..
> > > > 
> > > >> I understand this is to fix tests/migration-test on older kernels.
> > > >> Couldn't this be achieved with migration-test doing some introspection
> > > >> and maybe pass vsmt=8 on the QEMU command line ?
> > > > 
> > > > ..adjusting the test case like this might be a better idea, though.
> > > > 
> > > > What's the test setup where we're using the old kernel?  I really only
> > > > applied the original patch on the guess that we didn't really care
> > > > about kernels that old.  The fact you've hit this in practice makes me
> > > > doubt that assumption.
> > > > 
> > > 
> > > The way to fix the tests is to add "-smp threads=8" on the command line
> > > (for all tests, so basically in qtest_init_without_qmp_handshake(), and
> > > it will impact all the machine types), and we have to check if it is
> > 
> > Ohhh... it isn't possible to initialize Qtest with machine specific
> > properties ? That's a bit unfortunate :-\
> 
> Uhh... I don't see why we can't.  Couldn't we just put either -machine
> vsmt=8 or -smp 8 into the cmd_src / cmd_dst printfs() in the
> strcmp(arch, "ppc64") case?
> 

Yes you seem to be right for migration-test... so I'm not sure what Laurent is
implying with qtest_init_without_qmp_handshake().

> > > ppc64/pseries to do that, and there it becomes a little bit complicated
> > > for a so small piece of code.
> > > 
> > > So I think the best to do is to revert Greg's patch.
> > > 
> > 
> > I'm okay with that since this patch doesn't bring much for the moment.
> > 
> > But soon, ie. linux-5.5 hopefully, KVM will allow to configure the number
> > of presenters in the XIVE and XICS-on-XIVE devices on POWER9. Combined
> > with this patch, it will allow to drastically reduce the consumption of
> > resources in the XIVE HW, which currently limits the number of VMs that
> > can run concurrently with an in-kernel irqchip. So I hope the 'make check'
> > you're willing to fix is worth it :-), and BTW you didn't answer David's
> > question about the test setup.
> > 
> 


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

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[Laurent Vivier]

On 20/11/2019 05:36, David Gibson wrote:
> On Tue, Nov 19, 2019 at 04:45:26PM +0100, Greg Kurz wrote:
>> On Tue, 19 Nov 2019 15:06:51 +0100
>> Laurent Vivier <lvivier@redhat.com> wrote:
>>
>>> On 19/11/2019 02:00, David Gibson wrote:
>>>> On Fri, Nov 08, 2019 at 05:47:59PM +0100, Greg Kurz wrote:
>>>>> On Fri,  8 Nov 2019 16:40:35 +0100
>>>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>>>
>>>>>> Commit 29cb4187497d sets by default the VSMT to smp_threads,
>>>>>> but older kernels (< 4.13) don't support that.
>>>>>>
>>>>>> We can reasonably restore previous behavior with this kernel
>>>>>> to allow to run QEMU as before.
>>>>>>
>>>>>> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
>>>>>> as it is done for previous machine types (< pseries-4.2)
>>>>>>
>>>>>
>>>>> It is usually _bad_ to base the machine behavior on host capabilities.
>>>>> What happens if we migrate between an older kernel and a recent one ?
>>>>
>>>> Right.  We're really trying to remove instaces of such behaviour.  I'd
>>>> prefer to completely revert Greg's original patch than to re-introduce
>>>> host configuration dependency into the guest configuration..
>>>>
>>>>> I understand this is to fix tests/migration-test on older kernels.
>>>>> Couldn't this be achieved with migration-test doing some introspection
>>>>> and maybe pass vsmt=8 on the QEMU command line ?
>>>>
>>>> ..adjusting the test case like this might be a better idea, though.
>>>>
>>>> What's the test setup where we're using the old kernel?  I really only
>>>> applied the original patch on the guess that we didn't really care
>>>> about kernels that old.  The fact you've hit this in practice makes me
>>>> doubt that assumption.
>>>>
>>>
>>> The way to fix the tests is to add "-smp threads=8" on the command line
>>> (for all tests, so basically in qtest_init_without_qmp_handshake(), and
>>> it will impact all the machine types), and we have to check if it is
>>
>> Ohhh... it isn't possible to initialize Qtest with machine specific
>> properties ? That's a bit unfortunate :-\
> 
> Uhh... I don't see why we can't.  Couldn't we just put either -machine
> vsmt=8 or -smp 8 into the cmd_src / cmd_dst printfs() in the
> strcmp(arch, "ppc64") case?

Yes, but we need to do that to all other tests that fail. test-migration
is not the only one impacted by the problem (we have also pxe-test), so
it's why I thought to fix the problem in a generic place.

But it seems there are only this couple of tests that are impacted so I
can modify both instead. I think only tests that really start CPU have
the problem.

I'm going to send a patch to fix that.

Thanks,
Laurent

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[Laurent Vivier]

On 20/11/2019 10:00, Laurent Vivier wrote:
> On 20/11/2019 05:36, David Gibson wrote:
>> On Tue, Nov 19, 2019 at 04:45:26PM +0100, Greg Kurz wrote:
>>> On Tue, 19 Nov 2019 15:06:51 +0100
>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>
>>>> On 19/11/2019 02:00, David Gibson wrote:
>>>>> On Fri, Nov 08, 2019 at 05:47:59PM +0100, Greg Kurz wrote:
>>>>>> On Fri,  8 Nov 2019 16:40:35 +0100
>>>>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>>>>
>>>>>>> Commit 29cb4187497d sets by default the VSMT to smp_threads,
>>>>>>> but older kernels (< 4.13) don't support that.
>>>>>>>
>>>>>>> We can reasonably restore previous behavior with this kernel
>>>>>>> to allow to run QEMU as before.
>>>>>>>
>>>>>>> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
>>>>>>> as it is done for previous machine types (< pseries-4.2)
>>>>>>>
>>>>>>
>>>>>> It is usually _bad_ to base the machine behavior on host capabilities.
>>>>>> What happens if we migrate between an older kernel and a recent one ?
>>>>>
>>>>> Right.  We're really trying to remove instaces of such behaviour.  I'd
>>>>> prefer to completely revert Greg's original patch than to re-introduce
>>>>> host configuration dependency into the guest configuration..
>>>>>
>>>>>> I understand this is to fix tests/migration-test on older kernels.
>>>>>> Couldn't this be achieved with migration-test doing some introspection
>>>>>> and maybe pass vsmt=8 on the QEMU command line ?
>>>>>
>>>>> ..adjusting the test case like this might be a better idea, though.
>>>>>
>>>>> What's the test setup where we're using the old kernel?  I really only
>>>>> applied the original patch on the guess that we didn't really care
>>>>> about kernels that old.  The fact you've hit this in practice makes me
>>>>> doubt that assumption.
>>>>>
>>>>
>>>> The way to fix the tests is to add "-smp threads=8" on the command line
>>>> (for all tests, so basically in qtest_init_without_qmp_handshake(), and
>>>> it will impact all the machine types), and we have to check if it is
>>>
>>> Ohhh... it isn't possible to initialize Qtest with machine specific
>>> properties ? That's a bit unfortunate :-\
>>
>> Uhh... I don't see why we can't.  Couldn't we just put either -machine
>> vsmt=8 or -smp 8 into the cmd_src / cmd_dst printfs() in the
>> strcmp(arch, "ppc64") case?
> 
> Yes, but we need to do that to all other tests that fail. test-migration
> is not the only one impacted by the problem (we have also pxe-test), so
> it's why I thought to fix the problem in a generic place.
> 
> But it seems there are only this couple of tests that are impacted so I
> can modify both instead. I think only tests that really start CPU have
> the problem.
> 
> I'm going to send a patch to fix that.

And again, it's a little bit more complicated than expected: setting
vsmt to 8 works only with kvm_hv, but breaks in case of TCG or kvm_pr.
So the test must check what is in use...

Thanks,
Laurent

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[David Gibson]

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

On Wed, Nov 20, 2019 at 12:28:19PM +0100, Laurent Vivier wrote:
> On 20/11/2019 10:00, Laurent Vivier wrote:
> > On 20/11/2019 05:36, David Gibson wrote:
> >> On Tue, Nov 19, 2019 at 04:45:26PM +0100, Greg Kurz wrote:
> >>> On Tue, 19 Nov 2019 15:06:51 +0100
> >>> Laurent Vivier <lvivier@redhat.com> wrote:
> >>>
> >>>> On 19/11/2019 02:00, David Gibson wrote:
> >>>>> On Fri, Nov 08, 2019 at 05:47:59PM +0100, Greg Kurz wrote:
> >>>>>> On Fri,  8 Nov 2019 16:40:35 +0100
> >>>>>> Laurent Vivier <lvivier@redhat.com> wrote:
> >>>>>>
> >>>>>>> Commit 29cb4187497d sets by default the VSMT to smp_threads,
> >>>>>>> but older kernels (< 4.13) don't support that.
> >>>>>>>
> >>>>>>> We can reasonably restore previous behavior with this kernel
> >>>>>>> to allow to run QEMU as before.
> >>>>>>>
> >>>>>>> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
> >>>>>>> as it is done for previous machine types (< pseries-4.2)
> >>>>>>>
> >>>>>>
> >>>>>> It is usually _bad_ to base the machine behavior on host capabilities.
> >>>>>> What happens if we migrate between an older kernel and a recent one ?
> >>>>>
> >>>>> Right.  We're really trying to remove instaces of such behaviour.  I'd
> >>>>> prefer to completely revert Greg's original patch than to re-introduce
> >>>>> host configuration dependency into the guest configuration..
> >>>>>
> >>>>>> I understand this is to fix tests/migration-test on older kernels.
> >>>>>> Couldn't this be achieved with migration-test doing some introspection
> >>>>>> and maybe pass vsmt=8 on the QEMU command line ?
> >>>>>
> >>>>> ..adjusting the test case like this might be a better idea, though.
> >>>>>
> >>>>> What's the test setup where we're using the old kernel?  I really only
> >>>>> applied the original patch on the guess that we didn't really care
> >>>>> about kernels that old.  The fact you've hit this in practice makes me
> >>>>> doubt that assumption.
> >>>>>
> >>>>
> >>>> The way to fix the tests is to add "-smp threads=8" on the command line
> >>>> (for all tests, so basically in qtest_init_without_qmp_handshake(), and
> >>>> it will impact all the machine types), and we have to check if it is
> >>>
> >>> Ohhh... it isn't possible to initialize Qtest with machine specific
> >>> properties ? That's a bit unfortunate :-\
> >>
> >> Uhh... I don't see why we can't.  Couldn't we just put either -machine
> >> vsmt=8 or -smp 8 into the cmd_src / cmd_dst printfs() in the
> >> strcmp(arch, "ppc64") case?
> > 
> > Yes, but we need to do that to all other tests that fail. test-migration
> > is not the only one impacted by the problem (we have also pxe-test), so
> > it's why I thought to fix the problem in a generic place.
> > 
> > But it seems there are only this couple of tests that are impacted so I
> > can modify both instead. I think only tests that really start CPU have
> > the problem.
> > 
> > I'm going to send a patch to fix that.
> 
> And again, it's a little bit more complicated than expected: setting
> vsmt to 8 works only with kvm_hv, but breaks in case of TCG or kvm_pr.
> So the test must check what is in use...

Ugh, yeah, that's getting too ugly.  I think the feasible options are
either to revert the patch, or just say that upstream qemu no longer
supports a RHEL7 host.

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

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[Laurent Vivier]

On 20/11/2019 12:41, David Gibson wrote:
> On Wed, Nov 20, 2019 at 12:28:19PM +0100, Laurent Vivier wrote:
>> On 20/11/2019 10:00, Laurent Vivier wrote:
>>> On 20/11/2019 05:36, David Gibson wrote:
>>>> On Tue, Nov 19, 2019 at 04:45:26PM +0100, Greg Kurz wrote:
>>>>> On Tue, 19 Nov 2019 15:06:51 +0100
>>>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>>>
>>>>>> On 19/11/2019 02:00, David Gibson wrote:
>>>>>>> On Fri, Nov 08, 2019 at 05:47:59PM +0100, Greg Kurz wrote:
>>>>>>>> On Fri,  8 Nov 2019 16:40:35 +0100
>>>>>>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>>>>>>
>>>>>>>>> Commit 29cb4187497d sets by default the VSMT to smp_threads,
>>>>>>>>> but older kernels (< 4.13) don't support that.
>>>>>>>>>
>>>>>>>>> We can reasonably restore previous behavior with this kernel
>>>>>>>>> to allow to run QEMU as before.
>>>>>>>>>
>>>>>>>>> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
>>>>>>>>> as it is done for previous machine types (< pseries-4.2)
>>>>>>>>>
>>>>>>>>
>>>>>>>> It is usually _bad_ to base the machine behavior on host capabilities.
>>>>>>>> What happens if we migrate between an older kernel and a recent one ?
>>>>>>>
>>>>>>> Right.  We're really trying to remove instaces of such behaviour.  I'd
>>>>>>> prefer to completely revert Greg's original patch than to re-introduce
>>>>>>> host configuration dependency into the guest configuration..
>>>>>>>
>>>>>>>> I understand this is to fix tests/migration-test on older kernels.
>>>>>>>> Couldn't this be achieved with migration-test doing some introspection
>>>>>>>> and maybe pass vsmt=8 on the QEMU command line ?
>>>>>>>
>>>>>>> ..adjusting the test case like this might be a better idea, though.
>>>>>>>
>>>>>>> What's the test setup where we're using the old kernel?  I really only
>>>>>>> applied the original patch on the guess that we didn't really care
>>>>>>> about kernels that old.  The fact you've hit this in practice makes me
>>>>>>> doubt that assumption.
>>>>>>>
>>>>>>
>>>>>> The way to fix the tests is to add "-smp threads=8" on the command line
>>>>>> (for all tests, so basically in qtest_init_without_qmp_handshake(), and
>>>>>> it will impact all the machine types), and we have to check if it is
>>>>>
>>>>> Ohhh... it isn't possible to initialize Qtest with machine specific
>>>>> properties ? That's a bit unfortunate :-\
>>>>
>>>> Uhh... I don't see why we can't.  Couldn't we just put either -machine
>>>> vsmt=8 or -smp 8 into the cmd_src / cmd_dst printfs() in the
>>>> strcmp(arch, "ppc64") case?
>>>
>>> Yes, but we need to do that to all other tests that fail. test-migration
>>> is not the only one impacted by the problem (we have also pxe-test), so
>>> it's why I thought to fix the problem in a generic place.
>>>
>>> But it seems there are only this couple of tests that are impacted so I
>>> can modify both instead. I think only tests that really start CPU have
>>> the problem.
>>>
>>> I'm going to send a patch to fix that.
>>
>> And again, it's a little bit more complicated than expected: setting
>> vsmt to 8 works only with kvm_hv, but breaks in case of TCG or kvm_pr.
>> So the test must check what is in use...
> 
> Ugh, yeah, that's getting too ugly.  I think the feasible options are
> either to revert the patch, or just say that upstream qemu no longer
> supports a RHEL7 host.
> 

I'm going to send my patch, and you'll be able to judge by yourself.

Thanks,
Laurent

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[Laurent Vivier]

On 20/11/2019 12:41, David Gibson wrote:
> On Wed, Nov 20, 2019 at 12:28:19PM +0100, Laurent Vivier wrote:
>> On 20/11/2019 10:00, Laurent Vivier wrote:
>>> On 20/11/2019 05:36, David Gibson wrote:
>>>> On Tue, Nov 19, 2019 at 04:45:26PM +0100, Greg Kurz wrote:
>>>>> On Tue, 19 Nov 2019 15:06:51 +0100
>>>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>>>
>>>>>> On 19/11/2019 02:00, David Gibson wrote:
>>>>>>> On Fri, Nov 08, 2019 at 05:47:59PM +0100, Greg Kurz wrote:
>>>>>>>> On Fri,  8 Nov 2019 16:40:35 +0100
>>>>>>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>>>>>>
>>>>>>>>> Commit 29cb4187497d sets by default the VSMT to smp_threads,
>>>>>>>>> but older kernels (< 4.13) don't support that.
>>>>>>>>>
>>>>>>>>> We can reasonably restore previous behavior with this kernel
>>>>>>>>> to allow to run QEMU as before.
>>>>>>>>>
>>>>>>>>> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
>>>>>>>>> as it is done for previous machine types (< pseries-4.2)
>>>>>>>>>
>>>>>>>>
>>>>>>>> It is usually _bad_ to base the machine behavior on host capabilities.
>>>>>>>> What happens if we migrate between an older kernel and a recent one ?
>>>>>>>
>>>>>>> Right.  We're really trying to remove instaces of such behaviour.  I'd
>>>>>>> prefer to completely revert Greg's original patch than to re-introduce
>>>>>>> host configuration dependency into the guest configuration..
>>>>>>>
>>>>>>>> I understand this is to fix tests/migration-test on older kernels.
>>>>>>>> Couldn't this be achieved with migration-test doing some introspection
>>>>>>>> and maybe pass vsmt=8 on the QEMU command line ?
>>>>>>>
>>>>>>> ..adjusting the test case like this might be a better idea, though.
>>>>>>>
>>>>>>> What's the test setup where we're using the old kernel?  I really only
>>>>>>> applied the original patch on the guess that we didn't really care
>>>>>>> about kernels that old.  The fact you've hit this in practice makes me
>>>>>>> doubt that assumption.
>>>>>>>
>>>>>>
>>>>>> The way to fix the tests is to add "-smp threads=8" on the command line
>>>>>> (for all tests, so basically in qtest_init_without_qmp_handshake(), and
>>>>>> it will impact all the machine types), and we have to check if it is
>>>>>
>>>>> Ohhh... it isn't possible to initialize Qtest with machine specific
>>>>> properties ? That's a bit unfortunate :-\
>>>>
>>>> Uhh... I don't see why we can't.  Couldn't we just put either -machine
>>>> vsmt=8 or -smp 8 into the cmd_src / cmd_dst printfs() in the
>>>> strcmp(arch, "ppc64") case?
>>>
>>> Yes, but we need to do that to all other tests that fail. test-migration
>>> is not the only one impacted by the problem (we have also pxe-test), so
>>> it's why I thought to fix the problem in a generic place.
>>>
>>> But it seems there are only this couple of tests that are impacted so I
>>> can modify both instead. I think only tests that really start CPU have
>>> the problem.
>>>
>>> I'm going to send a patch to fix that.
>>
>> And again, it's a little bit more complicated than expected: setting
>> vsmt to 8 works only with kvm_hv, but breaks in case of TCG or kvm_pr.
>> So the test must check what is in use...
> 
> Ugh, yeah, that's getting too ugly.  I think the feasible options are
> either to revert the patch, or just say that upstream qemu no longer
> supports a RHEL7 host.

In I was mistakenly using "-smp threads=8", with "-M vsmt=8" it works
with TCG and KVM PR (with a warning).

I've sent the patch.

Thanks,
Laurent

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[David Gibson]

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

On Wed, Nov 20, 2019 at 03:28:19PM +0100, Laurent Vivier wrote:
> On 20/11/2019 12:41, David Gibson wrote:
> > On Wed, Nov 20, 2019 at 12:28:19PM +0100, Laurent Vivier wrote:
> >> On 20/11/2019 10:00, Laurent Vivier wrote:
> >>> On 20/11/2019 05:36, David Gibson wrote:
> >>>> On Tue, Nov 19, 2019 at 04:45:26PM +0100, Greg Kurz wrote:
> >>>>> On Tue, 19 Nov 2019 15:06:51 +0100
> >>>>> Laurent Vivier <lvivier@redhat.com> wrote:
> >>>>>
> >>>>>> On 19/11/2019 02:00, David Gibson wrote:
> >>>>>>> On Fri, Nov 08, 2019 at 05:47:59PM +0100, Greg Kurz wrote:
> >>>>>>>> On Fri,  8 Nov 2019 16:40:35 +0100
> >>>>>>>> Laurent Vivier <lvivier@redhat.com> wrote:
> >>>>>>>>
> >>>>>>>>> Commit 29cb4187497d sets by default the VSMT to smp_threads,
> >>>>>>>>> but older kernels (< 4.13) don't support that.
> >>>>>>>>>
> >>>>>>>>> We can reasonably restore previous behavior with this kernel
> >>>>>>>>> to allow to run QEMU as before.
> >>>>>>>>>
> >>>>>>>>> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
> >>>>>>>>> as it is done for previous machine types (< pseries-4.2)
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> It is usually _bad_ to base the machine behavior on host capabilities.
> >>>>>>>> What happens if we migrate between an older kernel and a recent one ?
> >>>>>>>
> >>>>>>> Right.  We're really trying to remove instaces of such behaviour.  I'd
> >>>>>>> prefer to completely revert Greg's original patch than to re-introduce
> >>>>>>> host configuration dependency into the guest configuration..
> >>>>>>>
> >>>>>>>> I understand this is to fix tests/migration-test on older kernels.
> >>>>>>>> Couldn't this be achieved with migration-test doing some introspection
> >>>>>>>> and maybe pass vsmt=8 on the QEMU command line ?
> >>>>>>>
> >>>>>>> ..adjusting the test case like this might be a better idea, though.
> >>>>>>>
> >>>>>>> What's the test setup where we're using the old kernel?  I really only
> >>>>>>> applied the original patch on the guess that we didn't really care
> >>>>>>> about kernels that old.  The fact you've hit this in practice makes me
> >>>>>>> doubt that assumption.
> >>>>>>>
> >>>>>>
> >>>>>> The way to fix the tests is to add "-smp threads=8" on the command line
> >>>>>> (for all tests, so basically in qtest_init_without_qmp_handshake(), and
> >>>>>> it will impact all the machine types), and we have to check if it is
> >>>>>
> >>>>> Ohhh... it isn't possible to initialize Qtest with machine specific
> >>>>> properties ? That's a bit unfortunate :-\
> >>>>
> >>>> Uhh... I don't see why we can't.  Couldn't we just put either -machine
> >>>> vsmt=8 or -smp 8 into the cmd_src / cmd_dst printfs() in the
> >>>> strcmp(arch, "ppc64") case?
> >>>
> >>> Yes, but we need to do that to all other tests that fail. test-migration
> >>> is not the only one impacted by the problem (we have also pxe-test), so
> >>> it's why I thought to fix the problem in a generic place.
> >>>
> >>> But it seems there are only this couple of tests that are impacted so I
> >>> can modify both instead. I think only tests that really start CPU have
> >>> the problem.
> >>>
> >>> I'm going to send a patch to fix that.
> >>
> >> And again, it's a little bit more complicated than expected: setting
> >> vsmt to 8 works only with kvm_hv, but breaks in case of TCG or kvm_pr.
> >> So the test must check what is in use...
> > 
> > Ugh, yeah, that's getting too ugly.  I think the feasible options are
> > either to revert the patch, or just say that upstream qemu no longer
> > supports a RHEL7 host.
> 
> In I was mistakenly using "-smp threads=8", with "-M vsmt=8" it works
> with TCG and KVM PR (with a warning).

Ah, yes, that's not so bad.

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

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[Greg Kurz]

On Wed, 20 Nov 2019 12:28:19 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 20/11/2019 10:00, Laurent Vivier wrote:
> > On 20/11/2019 05:36, David Gibson wrote:
> >> On Tue, Nov 19, 2019 at 04:45:26PM +0100, Greg Kurz wrote:
> >>> On Tue, 19 Nov 2019 15:06:51 +0100
> >>> Laurent Vivier <lvivier@redhat.com> wrote:
> >>>
> >>>> On 19/11/2019 02:00, David Gibson wrote:
> >>>>> On Fri, Nov 08, 2019 at 05:47:59PM +0100, Greg Kurz wrote:
> >>>>>> On Fri,  8 Nov 2019 16:40:35 +0100
> >>>>>> Laurent Vivier <lvivier@redhat.com> wrote:
> >>>>>>
> >>>>>>> Commit 29cb4187497d sets by default the VSMT to smp_threads,
> >>>>>>> but older kernels (< 4.13) don't support that.
> >>>>>>>
> >>>>>>> We can reasonably restore previous behavior with this kernel
> >>>>>>> to allow to run QEMU as before.
> >>>>>>>
> >>>>>>> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
> >>>>>>> as it is done for previous machine types (< pseries-4.2)
> >>>>>>>
> >>>>>>
> >>>>>> It is usually _bad_ to base the machine behavior on host capabilities.
> >>>>>> What happens if we migrate between an older kernel and a recent one ?
> >>>>>
> >>>>> Right.  We're really trying to remove instaces of such behaviour.  I'd
> >>>>> prefer to completely revert Greg's original patch than to re-introduce
> >>>>> host configuration dependency into the guest configuration..
> >>>>>
> >>>>>> I understand this is to fix tests/migration-test on older kernels.
> >>>>>> Couldn't this be achieved with migration-test doing some introspection
> >>>>>> and maybe pass vsmt=8 on the QEMU command line ?
> >>>>>
> >>>>> ..adjusting the test case like this might be a better idea, though.
> >>>>>
> >>>>> What's the test setup where we're using the old kernel?  I really only
> >>>>> applied the original patch on the guess that we didn't really care
> >>>>> about kernels that old.  The fact you've hit this in practice makes me
> >>>>> doubt that assumption.
> >>>>>
> >>>>
> >>>> The way to fix the tests is to add "-smp threads=8" on the command line
> >>>> (for all tests, so basically in qtest_init_without_qmp_handshake(), and
> >>>> it will impact all the machine types), and we have to check if it is
> >>>
> >>> Ohhh... it isn't possible to initialize Qtest with machine specific
> >>> properties ? That's a bit unfortunate :-\
> >>
> >> Uhh... I don't see why we can't.  Couldn't we just put either -machine
> >> vsmt=8 or -smp 8 into the cmd_src / cmd_dst printfs() in the
> >> strcmp(arch, "ppc64") case?
> > 
> > Yes, but we need to do that to all other tests that fail. test-migration
> > is not the only one impacted by the problem (we have also pxe-test), so
> > it's why I thought to fix the problem in a generic place.
> > 
> > But it seems there are only this couple of tests that are impacted so I
> > can modify both instead. I think only tests that really start CPU have
> > the problem.
> > 
> > I'm going to send a patch to fix that.
> 
> And again, it's a little bit more complicated than expected: setting
> vsmt to 8 works only with kvm_hv, but breaks in case of TCG or kvm_pr.
> So the test must check what is in use...
> 

AFAICT, migration-test explicitly skip tests if kvm_hv isn't present.

    /*
     * On ppc64, the test only works with kvm-hv, but not with kvm-pr and TCG
     * is touchy due to race conditions on dirty bits (especially on PPC for
     * some reason)
     */
    if (g_str_equal(qtest_get_arch(), "ppc64") &&
        access("/sys/module/kvm_hv", F_OK)) {
        g_test_message("Skipping test: kvm_hv not available");
        return g_test_run();
    }

and I don't see any error in pxe-test if I force tcg and vsmt=8.

What error do you see with your testing ?

> Thanks,
> Laurent
> 

Re: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel

[Laurent Vivier]

On 20/11/2019 13:47, Greg Kurz wrote:
> On Wed, 20 Nov 2019 12:28:19 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
>> On 20/11/2019 10:00, Laurent Vivier wrote:
>>> On 20/11/2019 05:36, David Gibson wrote:
>>>> On Tue, Nov 19, 2019 at 04:45:26PM +0100, Greg Kurz wrote:
>>>>> On Tue, 19 Nov 2019 15:06:51 +0100
>>>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>>>
>>>>>> On 19/11/2019 02:00, David Gibson wrote:
>>>>>>> On Fri, Nov 08, 2019 at 05:47:59PM +0100, Greg Kurz wrote:
>>>>>>>> On Fri,  8 Nov 2019 16:40:35 +0100
>>>>>>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>>>>>>
>>>>>>>>> Commit 29cb4187497d sets by default the VSMT to smp_threads,
>>>>>>>>> but older kernels (< 4.13) don't support that.
>>>>>>>>>
>>>>>>>>> We can reasonably restore previous behavior with this kernel
>>>>>>>>> to allow to run QEMU as before.
>>>>>>>>>
>>>>>>>>> If VSMT is not supported, VSMT will be set to MAX(8, smp_threads)
>>>>>>>>> as it is done for previous machine types (< pseries-4.2)
>>>>>>>>>
>>>>>>>>
>>>>>>>> It is usually _bad_ to base the machine behavior on host capabilities.
>>>>>>>> What happens if we migrate between an older kernel and a recent one ?
>>>>>>>
>>>>>>> Right.  We're really trying to remove instaces of such behaviour.  I'd
>>>>>>> prefer to completely revert Greg's original patch than to re-introduce
>>>>>>> host configuration dependency into the guest configuration..
>>>>>>>
>>>>>>>> I understand this is to fix tests/migration-test on older kernels.
>>>>>>>> Couldn't this be achieved with migration-test doing some introspection
>>>>>>>> and maybe pass vsmt=8 on the QEMU command line ?
>>>>>>>
>>>>>>> ..adjusting the test case like this might be a better idea, though.
>>>>>>>
>>>>>>> What's the test setup where we're using the old kernel?  I really only
>>>>>>> applied the original patch on the guess that we didn't really care
>>>>>>> about kernels that old.  The fact you've hit this in practice makes me
>>>>>>> doubt that assumption.
>>>>>>>
>>>>>>
>>>>>> The way to fix the tests is to add "-smp threads=8" on the command line
>>>>>> (for all tests, so basically in qtest_init_without_qmp_handshake(), and
>>>>>> it will impact all the machine types), and we have to check if it is
>>>>>
>>>>> Ohhh... it isn't possible to initialize Qtest with machine specific
>>>>> properties ? That's a bit unfortunate :-\
>>>>
>>>> Uhh... I don't see why we can't.  Couldn't we just put either -machine
>>>> vsmt=8 or -smp 8 into the cmd_src / cmd_dst printfs() in the
>>>> strcmp(arch, "ppc64") case?
>>>
>>> Yes, but we need to do that to all other tests that fail. test-migration
>>> is not the only one impacted by the problem (we have also pxe-test), so
>>> it's why I thought to fix the problem in a generic place.
>>>
>>> But it seems there are only this couple of tests that are impacted so I
>>> can modify both instead. I think only tests that really start CPU have
>>> the problem.
>>>
>>> I'm going to send a patch to fix that.
>>
>> And again, it's a little bit more complicated than expected: setting
>> vsmt to 8 works only with kvm_hv, but breaks in case of TCG or kvm_pr.
>> So the test must check what is in use...
>>
> 
> AFAICT, migration-test explicitly skip tests if kvm_hv isn't present.
> 
>     /*
>      * On ppc64, the test only works with kvm-hv, but not with kvm-pr and TCG
>      * is touchy due to race conditions on dirty bits (especially on PPC for
>      * some reason)
>      */
>     if (g_str_equal(qtest_get_arch(), "ppc64") &&
>         access("/sys/module/kvm_hv", F_OK)) {
>         g_test_message("Skipping test: kvm_hv not available");
>         return g_test_run();
>     }
> 
> and I don't see any error in pxe-test if I force tcg and vsmt=8.
> 
> What error do you see with your testing ?

In fact, you're right, it works with vsmt=8 and it's better.

Laurent