| Message ID | 1499964378-24344-1-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 13.07.2017 19:00, Paolo Bonzini wrote: > On 13/07/2017 18:47, Michael S. Tsirkin wrote: >> Speed up tests on host systems with kvm support. >> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Thomas Huth <thuth@redhat.com> >> Cc: Laurent Vivier <lvivier@redhat.com> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> --- >> >> This is on top of my previous patch fixing vmgenid. >> >> PPC PXE test is reported to be especially slow - does anyone care to try >> it and measure whether this helps? >> >> tests/pxe-test.c | 2 +- >> tests/vmgenid-test.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/tests/pxe-test.c b/tests/pxe-test.c >> index 34282d3..cf6e225 100644 >> --- a/tests/pxe-test.c >> +++ b/tests/pxe-test.c >> @@ -25,7 +25,7 @@ static void test_pxe_one(const char *params, bool ipv6) >> { >> char *args; >> >> - args = g_strdup_printf("-machine accel=tcg -nodefaults -boot order=n " >> + args = g_strdup_printf("-machine accel=kvm:tcg -nodefaults -boot order=n " >> "-netdev user,id=" NETNAME ",tftp=./,bootfile=%s," >> "ipv4=%s,ipv6=%s %s", disk, ipv6 ? "off" : "on", >> ipv6 ? "on" : "off", params); >> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c >> index 0556ba2..6477dcc 100644 >> --- a/tests/vmgenid-test.c >> +++ b/tests/vmgenid-test.c >> @@ -132,7 +132,7 @@ static char disk[] = "tests/vmgenid-test-disk-XXXXXX"; >> >> static char *guid_cmd_strdup(const char *guid) >> { >> - return g_strdup_printf("-machine accel=tcg " >> + return g_strdup_printf("-machine accel=kvm:tcg " >> "-device vmgenid,id=testvgid,guid=%s " >> "-drive id=hd0,if=none,file=%s,format=raw " >> "-device ide-hd,drive=hd0 ", > > Acked-by: Paolo Bonzini <pbonzini@redhat.com> NACK. It's not that simple. If you do it like this, you then suddenly get these ugly '"kvm" accelerator not found' messages during "make check" which we really should avoid. Laurent suggested some better ideas a couple of months ago, but I think the common sense was rather "unit tests should always use TCG" at that point in time: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg00400.html Well, now that we've got the "--disable-tcg" configure option, we should maybe start to reconsider that ;-) So I'm fine with the patch if you also include Laurent's patch to shut up the error message: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg00395.html Thomas
> NACK. > > It's not that simple. If you do it like this, you then suddenly get > these ugly '"kvm" accelerator not found' messages during "make check" > which we really should avoid. You're right, I thought these were shut up before with Laurent's patch. I think we should print a single message at the end, like qemu: -machine accel=kvm:xen: No accelerator found Paolo > Laurent suggested some better ideas a couple of months ago, but I think > the common sense was rather "unit tests should always use TCG" at that > point in time: > > https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg00400.html > > Well, now that we've got the "--disable-tcg" configure option, we should > maybe start to reconsider that ;-) > > So I'm fine with the patch if you also include Laurent's patch to shut > up the error message: > > https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg00395.html > > Thomas >
On Fri, Jul 14, 2017 at 02:20:33AM -0400, Paolo Bonzini wrote: > > > NACK. > > > > It's not that simple. If you do it like this, you then suddenly get > > these ugly '"kvm" accelerator not found' messages during "make check" > > which we really should avoid. > > You're right, I thought these were shut up before with Laurent's patch. > I think we should print a single message at the end, like > > qemu: -machine accel=kvm:xen: No accelerator found > > Paolo That's exactly Laurent's patch, isn't it? > > Laurent suggested some better ideas a couple of months ago, but I think > > the common sense was rather "unit tests should always use TCG" at that > > point in time: > > > > https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg00400.html > > > > Well, now that we've got the "--disable-tcg" configure option, we should > > maybe start to reconsider that ;-) > > > > So I'm fine with the patch if you also include Laurent's patch to shut > > up the error message: > > > > https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg00395.html > > > > Thomas > >
On Fri, Jul 14, 2017 at 07:59:54AM +0200, Thomas Huth wrote: > On 13.07.2017 19:00, Paolo Bonzini wrote: > > On 13/07/2017 18:47, Michael S. Tsirkin wrote: > >> Speed up tests on host systems with kvm support. > >> > >> Cc: Paolo Bonzini <pbonzini@redhat.com> > >> Cc: Thomas Huth <thuth@redhat.com> > >> Cc: Laurent Vivier <lvivier@redhat.com> > >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >> --- > >> > >> This is on top of my previous patch fixing vmgenid. > >> > >> PPC PXE test is reported to be especially slow - does anyone care to try > >> it and measure whether this helps? > >> > >> tests/pxe-test.c | 2 +- > >> tests/vmgenid-test.c | 2 +- > >> 2 files changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/tests/pxe-test.c b/tests/pxe-test.c > >> index 34282d3..cf6e225 100644 > >> --- a/tests/pxe-test.c > >> +++ b/tests/pxe-test.c > >> @@ -25,7 +25,7 @@ static void test_pxe_one(const char *params, bool ipv6) > >> { > >> char *args; > >> > >> - args = g_strdup_printf("-machine accel=tcg -nodefaults -boot order=n " > >> + args = g_strdup_printf("-machine accel=kvm:tcg -nodefaults -boot order=n " > >> "-netdev user,id=" NETNAME ",tftp=./,bootfile=%s," > >> "ipv4=%s,ipv6=%s %s", disk, ipv6 ? "off" : "on", > >> ipv6 ? "on" : "off", params); > >> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c > >> index 0556ba2..6477dcc 100644 > >> --- a/tests/vmgenid-test.c > >> +++ b/tests/vmgenid-test.c > >> @@ -132,7 +132,7 @@ static char disk[] = "tests/vmgenid-test-disk-XXXXXX"; > >> > >> static char *guid_cmd_strdup(const char *guid) > >> { > >> - return g_strdup_printf("-machine accel=tcg " > >> + return g_strdup_printf("-machine accel=kvm:tcg " > >> "-device vmgenid,id=testvgid,guid=%s " > >> "-drive id=hd0,if=none,file=%s,format=raw " > >> "-device ide-hd,drive=hd0 ", > > > > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > > NACK. As a rule, don't send nacks please. Skip directly to the technical argument. Nacks add very little to the discussion and tend to scare away contributors. All upper case also looks like shouting. That's unlike acks which include reviewer's name and can be recorded in git history so people know whom to ask questions about a commit. > It's not that simple. If you do it like this, you then suddenly get > these ugly '"kvm" accelerator not found' messages during "make check" > which we really should avoid. > > Laurent suggested some better ideas a couple of months ago, but I think > the common sense was rather "unit tests should always use TCG" at that > point in time: > > https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg00400.html Looks good. This was never posted as a patch. Would you or him care to test it and re-post? I'll include it with my patch. > Well, now that we've got the "--disable-tcg" configure option, we should > maybe start to reconsider that ;-) > > So I'm fine with the patch if you also include Laurent's patch to shut > up the error message: > > https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg00395.html > > Thomas
On 14/07/2017 16:37, Michael S. Tsirkin wrote: > On Fri, Jul 14, 2017 at 02:20:33AM -0400, Paolo Bonzini wrote: >> >>> NACK. >>> >>> It's not that simple. If you do it like this, you then suddenly get >>> these ugly '"kvm" accelerator not found' messages during "make check" >>> which we really should avoid. >> >> You're right, I thought these were shut up before with Laurent's patch. >> I think we should print a single message at the end, like >> >> qemu: -machine accel=kvm:xen: No accelerator found >> >> Paolo > > That's exactly Laurent's patch, isn't it? > Do you want I send an updated and a more formal patch? Thanks, Laurent
On 14.07.2017 16:45, Michael S. Tsirkin wrote: > On Fri, Jul 14, 2017 at 07:59:54AM +0200, Thomas Huth wrote: >> On 13.07.2017 19:00, Paolo Bonzini wrote: >>> On 13/07/2017 18:47, Michael S. Tsirkin wrote: >>>> Speed up tests on host systems with kvm support. [...] >>>> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c >>>> index 0556ba2..6477dcc 100644 >>>> --- a/tests/vmgenid-test.c >>>> +++ b/tests/vmgenid-test.c >>>> @@ -132,7 +132,7 @@ static char disk[] = "tests/vmgenid-test-disk-XXXXXX"; >>>> >>>> static char *guid_cmd_strdup(const char *guid) >>>> { >>>> - return g_strdup_printf("-machine accel=tcg " >>>> + return g_strdup_printf("-machine accel=kvm:tcg " >>>> "-device vmgenid,id=testvgid,guid=%s " >>>> "-drive id=hd0,if=none,file=%s,format=raw " >>>> "-device ide-hd,drive=hd0 ", >>> >>> Acked-by: Paolo Bonzini <pbonzini@redhat.com> >> >> NACK. > > As a rule, don't send nacks please. Skip directly to the technical > argument. Nacks add very little to the discussion and tend to scare away > contributors. I hope I won't scare *you* away that easily ;-) But ok, I got the message, and will try to remember it. >> It's not that simple. If you do it like this, you then suddenly get >> these ugly '"kvm" accelerator not found' messages during "make check" >> which we really should avoid. >> >> Laurent suggested some better ideas a couple of months ago, but I think >> the common sense was rather "unit tests should always use TCG" at that >> point in time: >> >> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg00400.html > > Looks good. This was never posted as a patch. Would you or him care > to test it and re-post? I'll include it with my patch. That URL points to Paolo's "we should always only use TCG mail" ... I guess you are rather referring to the other URL below? Thomas >> Well, now that we've got the "--disable-tcg" configure option, we should >> maybe start to reconsider that ;-) >> >> So I'm fine with the patch if you also include Laurent's patch to shut >> up the error message: >> >> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg00395.html >> >> Thomas
On Mon, Jul 17, 2017 at 09:28:15AM +0200, Laurent Vivier wrote: > On 14/07/2017 16:37, Michael S. Tsirkin wrote: > > On Fri, Jul 14, 2017 at 02:20:33AM -0400, Paolo Bonzini wrote: > >> > >>> NACK. > >>> > >>> It's not that simple. If you do it like this, you then suddenly get > >>> these ugly '"kvm" accelerator not found' messages during "make check" > >>> which we really should avoid. > >> > >> You're right, I thought these were shut up before with Laurent's patch. > >> I think we should print a single message at the end, like > >> > >> qemu: -machine accel=kvm:xen: No accelerator found > >> > >> Paolo > > > > That's exactly Laurent's patch, isn't it? > > > > Do you want I send an updated and a more formal patch? > > Thanks, > Laurent Yes pls.
diff --git a/tests/pxe-test.c b/tests/pxe-test.c index 34282d3..cf6e225 100644 --- a/tests/pxe-test.c +++ b/tests/pxe-test.c @@ -25,7 +25,7 @@ static void test_pxe_one(const char *params, bool ipv6) { char *args; - args = g_strdup_printf("-machine accel=tcg -nodefaults -boot order=n " + args = g_strdup_printf("-machine accel=kvm:tcg -nodefaults -boot order=n " "-netdev user,id=" NETNAME ",tftp=./,bootfile=%s," "ipv4=%s,ipv6=%s %s", disk, ipv6 ? "off" : "on", ipv6 ? "on" : "off", params); diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c index 0556ba2..6477dcc 100644 --- a/tests/vmgenid-test.c +++ b/tests/vmgenid-test.c @@ -132,7 +132,7 @@ static char disk[] = "tests/vmgenid-test-disk-XXXXXX"; static char *guid_cmd_strdup(const char *guid) { - return g_strdup_printf("-machine accel=tcg " + return g_strdup_printf("-machine accel=kvm:tcg " "-device vmgenid,id=testvgid,guid=%s " "-drive id=hd0,if=none,file=%s,format=raw " "-device ide-hd,drive=hd0 ",
Speed up tests on host systems with kvm support. Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Thomas Huth <thuth@redhat.com> Cc: Laurent Vivier <lvivier@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- This is on top of my previous patch fixing vmgenid. PPC PXE test is reported to be especially slow - does anyone care to try it and measure whether this helps? tests/pxe-test.c | 2 +- tests/vmgenid-test.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)