| Message ID | 20191108154035.12913-1-lvivier@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: Fix VSMT mode when it is not supported by the kernel | expand |
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;
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
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 >
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.
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
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 >
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 >> >
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. >
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.
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. > > >
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
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
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.
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
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 >
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
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
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.
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;
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(-)