[Qemu-devel] [PATCH 0/2] introduce "accel=best"

[Qemu-devel] [PATCH 0/2] introduce "accel=best"

[Laurent Vivier]

This series introduces a new parameter to "accel" to
ask to use the best one.

This allows to remove ugly "kvm accelerator not found"
from the "make check" logs.

Laurent Vivier (2):
  accel: allows to select the "best" accelerator
  tests: use accel=best instead of accel=kvm:tcg

 accel.c                  | 58 ++++++++++++++++++++++++++++++++++++++++--------
 include/sysemu/accel.h   |  1 +
 kvm-all.c                |  1 +
 qemu-options.hx          |  4 +++-
 qtest.c                  |  1 +
 tests/bios-tables-test.c |  2 +-
 tests/postcopy-test.c    |  8 +++----
 xen-common.c             |  1 +
 8 files changed, 61 insertions(+), 15 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] accel: allows to select the "best" accelerator

[Laurent Vivier]

By default, QEMU uses 'tcg' or the one provided with the "accel"
property.

But sometime, user wants to use a real accelerator without knowing
if he really can, with, for instance accel=kvm:tcg.
In this case, and if the accelerator is not available we
have a noisy "XXX accelerator not found".

By allowing the user to ask the "best" accelerator for the given
target, we can avoid this problem.

This patch introduces a new parameter for the "accel" property, the
"best" keyword.

You can ask to use the best accelerator with "-M accel=best",
or if you want to use your favorite accelerator and if it is not
available, the best one, you can use, for instance
"-M accel=kvm:best".

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 accel.c                | 58 ++++++++++++++++++++++++++++++++++++++++++--------
 include/sysemu/accel.h |  1 +
 kvm-all.c              |  1 +
 qemu-options.hx        |  4 +++-
 qtest.c                |  1 +
 xen-common.c           |  1 +
 6 files changed, 56 insertions(+), 10 deletions(-)

diff --git a/accel.c b/accel.c
index 403eb5e..07a738d 100644
--- a/accel.c
+++ b/accel.c
@@ -60,6 +60,38 @@ static AccelClass *accel_find(const char *opt_name)
     return ac;
 }
 
+static void accel_filter_best(ObjectClass *klass, void *opaque)
+{
+    AccelClass **best = opaque;
+    AccelClass *ac = ACCEL_CLASS(klass);
+
+    if (ac->available && !ac->available()) {
+        return;
+    }
+
+    if (ac->priority < 0) {
+        return;
+    }
+
+    if (*best == NULL) {
+        *best = ac;
+        return;
+    }
+
+    if (ac->priority > (*best)->priority) {
+        *best = ac;
+    }
+}
+
+static AccelClass *accel_find_best(void)
+{
+    AccelClass *best = NULL;
+
+    object_class_foreach(accel_filter_best, TYPE_ACCEL, false, &best);
+
+    return ACCEL_CLASS(best);
+}
+
 static int accel_init_machine(AccelClass *acc, MachineState *ms)
 {
     ObjectClass *oc = OBJECT_CLASS(acc);
@@ -97,15 +129,22 @@ void configure_accelerator(MachineState *ms)
             p++;
         }
         p = get_opt_name(buf, sizeof(buf), p, ':');
-        acc = accel_find(buf);
-        if (!acc) {
-            fprintf(stderr, "\"%s\" accelerator not found.\n", buf);
-            continue;
-        }
-        if (acc->available && !acc->available()) {
-            printf("%s not supported for this target\n",
-                   acc->name);
-            continue;
+        if (strcmp(buf, "best") == 0) {
+            acc = accel_find_best();
+            if (acc == NULL) {
+                break;
+            }
+        } else {
+            acc = accel_find(buf);
+            if (!acc) {
+                fprintf(stderr, "\"%s\" accelerator not found.\n", buf);
+                continue;
+            }
+            if (acc->available && !acc->available()) {
+                printf("%s not supported for this target\n",
+                       acc->name);
+                continue;
+            }
         }
         ret = accel_init_machine(acc, ms);
         if (ret < 0) {
@@ -137,6 +176,7 @@ static void tcg_accel_class_init(ObjectClass *oc, void *data)
     ac->name = "tcg";
     ac->init_machine = tcg_init;
     ac->allowed = &tcg_allowed;
+    ac->priority = 10;
 }
 
 #define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index 15944c1..5f2d7c9 100644
--- a/include/sysemu/accel.h
+++ b/include/sysemu/accel.h
@@ -40,6 +40,7 @@ typedef struct AccelClass {
     int (*available)(void);
     int (*init_machine)(MachineState *ms);
     bool *allowed;
+    int priority;
 } AccelClass;
 
 #define TYPE_ACCEL "accel"
diff --git a/kvm-all.c b/kvm-all.c
index fc2898a..1b7d82a 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -2460,6 +2460,7 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
     ac->name = "KVM";
     ac->init_machine = kvm_init;
     ac->allowed = &kvm_allowed;
+    ac->priority = 100;
 }
 
 static const TypeInfo kvm_accel_type = {
diff --git a/qemu-options.hx b/qemu-options.hx
index 01f01df..8ed7454 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -55,7 +55,9 @@ available machines. Supported machine properties are:
 This is used to enable an accelerator. Depending on the target architecture,
 kvm, xen, or tcg can be available. By default, tcg is used. If there is more
 than one accelerator specified, the next one is used if the previous one fails
-to initialize.
+to initialize. You can ask to use the best accelerator with "accel=best". If
+you want to use one accelerator and if it is not available, the best one, you
+can use, for instance, "accel=kvm:best".
 @item kernel_irqchip=on|off
 Controls in-kernel irqchip support for the chosen accelerator when available.
 @item gfx_passthru=on|off
diff --git a/qtest.c b/qtest.c
index 22482cc..4915f51 100644
--- a/qtest.c
+++ b/qtest.c
@@ -698,6 +698,7 @@ static void qtest_accel_class_init(ObjectClass *oc, void *data)
     ac->available = qtest_available;
     ac->init_machine = qtest_init_accel;
     ac->allowed = &qtest_allowed;
+    ac->priority = -1;
 }
 
 #define TYPE_QTEST_ACCEL ACCEL_CLASS_NAME("qtest")
diff --git a/xen-common.c b/xen-common.c
index e641ad1..19848be 100644
--- a/xen-common.c
+++ b/xen-common.c
@@ -140,6 +140,7 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
     ac->name = "Xen";
     ac->init_machine = xen_init;
     ac->allowed = &xen_allowed;
+    ac->priority = 100;
 }
 
 #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] tests: use accel=best instead of accel=kvm:tcg

[Laurent Vivier]

This avoids to have logs polluted by "kvm accelerator not found".

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 tests/bios-tables-test.c | 2 +-
 tests/postcopy-test.c    | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 7e27ea9..497dd56 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -716,7 +716,7 @@ static void test_acpi_one(const char *params, test_data *data)
                            "-net none -display none %s "
                            "-drive id=hd0,if=none,file=%s,format=raw "
                            "-device ide-hd,drive=hd0 ",
-                           data->machine, "kvm:tcg",
+                           data->machine, "best",
                            params ? params : "", disk);
 
     qtest_start(args);
diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index 41ed1a9..d549d5e 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -368,12 +368,12 @@ static void test_migrate(void)
 
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         init_bootfile_x86(bootpath);
-        cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
+        cmd_src = g_strdup_printf("-machine accel=best -m 150M"
                                   " -name pcsource,debug-threads=on"
                                   " -serial file:%s/src_serial"
                                   " -drive file=%s,format=raw",
                                   tmpfs, bootpath);
-        cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
+        cmd_dst = g_strdup_printf("-machine accel=best -m 150M"
                                   " -name pcdest,debug-threads=on"
                                   " -serial file:%s/dest_serial"
                                   " -drive file=%s,format=raw"
@@ -381,12 +381,12 @@ static void test_migrate(void)
                                   tmpfs, bootpath, uri);
     } else if (strcmp(arch, "ppc64") == 0) {
         init_bootfile_ppc(bootpath);
-        cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
+        cmd_src = g_strdup_printf("-machine accel=best -m 256M"
                                   " -name pcsource,debug-threads=on"
                                   " -serial file:%s/src_serial"
                                   " -drive file=%s,if=pflash,format=raw",
                                   tmpfs, bootpath);
-        cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
+        cmd_dst = g_strdup_printf("-machine accel=best -m 256M"
                                   " -name pcdest,debug-threads=on"
                                   " -serial file:%s/dest_serial"
                                   " -incoming %s",
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/2] introduce "accel=best"

[Laurent Vivier]

Sorry I didn't use the good email address for Stefano, cc the one from
MAINTAINERS now...

Laurent

On 04/10/2016 13:30, Laurent Vivier wrote:
> This series introduces a new parameter to "accel" to
> ask to use the best one.
> 
> This allows to remove ugly "kvm accelerator not found"
> from the "make check" logs.
> 
> Laurent Vivier (2):
>   accel: allows to select the "best" accelerator
>   tests: use accel=best instead of accel=kvm:tcg
> 
>  accel.c                  | 58 ++++++++++++++++++++++++++++++++++++++++--------
>  include/sysemu/accel.h   |  1 +
>  kvm-all.c                |  1 +
>  qemu-options.hx          |  4 +++-
>  qtest.c                  |  1 +
>  tests/bios-tables-test.c |  2 +-
>  tests/postcopy-test.c    |  8 +++----
>  xen-common.c             |  1 +
>  8 files changed, 61 insertions(+), 15 deletions(-)
> 

Re: [Qemu-devel] [PATCH 1/2] accel: allows to select the "best" accelerator

[Daniel P. Berrange]

On Tue, Oct 04, 2016 at 01:30:47PM +0200, Laurent Vivier wrote:
> By default, QEMU uses 'tcg' or the one provided with the "accel"
> property.
> 
> But sometime, user wants to use a real accelerator without knowing
> if he really can, with, for instance accel=kvm:tcg.
> In this case, and if the accelerator is not available we
> have a noisy "XXX accelerator not found".

IMHO it would be better to just remove that warning message
and continue using existing accel=kvm:tcg syntax to specify
the preference of which accelerators to use.

Only emit "XXX accelerator not found", if there are not
further accelerators listed. eg

  accel=kvm:tcg

should not print a "KVM accelerator not found" warning
when it falls back to tcg, but a

  accel=kvm

would print a warning, since no fallback is given.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH 0/2] introduce "accel=best"

[Andrew Jones]

On Tue, Oct 04, 2016 at 01:30:46PM +0200, Laurent Vivier wrote:
> This series introduces a new parameter to "accel" to
> ask to use the best one.
> 
> This allows to remove ugly "kvm accelerator not found"
> from the "make check" logs.
> 
> Laurent Vivier (2):
>   accel: allows to select the "best" accelerator
>   tests: use accel=best instead of accel=kvm:tcg
> 
>  accel.c                  | 58 ++++++++++++++++++++++++++++++++++++++++--------
>  include/sysemu/accel.h   |  1 +
>  kvm-all.c                |  1 +
>  qemu-options.hx          |  4 +++-
>  qtest.c                  |  1 +
>  tests/bios-tables-test.c |  2 +-
>  tests/postcopy-test.c    |  8 +++----
>  xen-common.c             |  1 +
>  8 files changed, 61 insertions(+), 15 deletions(-)
> 
> -- 
> 2.7.4
> 
>

How about probing the host for KVM in the tests. When it's
available, then only emit accel=kvm. When it's not (and
it's OK to fallback to tcg), then emit only accel=tcg.
That's what we do in kvm-unit-tests.

Thanks,
drew

Re: [Qemu-devel] [PATCH 0/2] introduce "accel=best"

[Daniel P. Berrange]

On Tue, Oct 04, 2016 at 02:00:46PM +0200, Andrew Jones wrote:
> On Tue, Oct 04, 2016 at 01:30:46PM +0200, Laurent Vivier wrote:
> > This series introduces a new parameter to "accel" to
> > ask to use the best one.
> > 
> > This allows to remove ugly "kvm accelerator not found"
> > from the "make check" logs.
> > 
> > Laurent Vivier (2):
> >   accel: allows to select the "best" accelerator
> >   tests: use accel=best instead of accel=kvm:tcg
> > 
> >  accel.c                  | 58 ++++++++++++++++++++++++++++++++++++++++--------
> >  include/sysemu/accel.h   |  1 +
> >  kvm-all.c                |  1 +
> >  qemu-options.hx          |  4 +++-
> >  qtest.c                  |  1 +
> >  tests/bios-tables-test.c |  2 +-
> >  tests/postcopy-test.c    |  8 +++----
> >  xen-common.c             |  1 +
> >  8 files changed, 61 insertions(+), 15 deletions(-)
> 
> How about probing the host for KVM in the tests. When it's
> available, then only emit accel=kvm. When it's not (and
> it's OK to fallback to tcg), then emit only accel=tcg.
> That's what we do in kvm-unit-tests.

Arguably unit tests should not have any dependancy on the host hardware,
and should be using accel=tcg exclusively, so that they have stable and
predictable behaviour for all developers. Only functional / integration
tests should have dependancies on host hardware/OS, so where accel=kvm
would be appropriate.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH 0/2] introduce "accel=best"

[Andrew Jones]

On Tue, Oct 04, 2016 at 01:07:28PM +0100, Daniel P. Berrange wrote:
> On Tue, Oct 04, 2016 at 02:00:46PM +0200, Andrew Jones wrote:
> > On Tue, Oct 04, 2016 at 01:30:46PM +0200, Laurent Vivier wrote:
> > > This series introduces a new parameter to "accel" to
> > > ask to use the best one.
> > > 
> > > This allows to remove ugly "kvm accelerator not found"
> > > from the "make check" logs.
> > > 
> > > Laurent Vivier (2):
> > >   accel: allows to select the "best" accelerator
> > >   tests: use accel=best instead of accel=kvm:tcg
> > > 
> > >  accel.c                  | 58 ++++++++++++++++++++++++++++++++++++++++--------
> > >  include/sysemu/accel.h   |  1 +
> > >  kvm-all.c                |  1 +
> > >  qemu-options.hx          |  4 +++-
> > >  qtest.c                  |  1 +
> > >  tests/bios-tables-test.c |  2 +-
> > >  tests/postcopy-test.c    |  8 +++----
> > >  xen-common.c             |  1 +
> > >  8 files changed, 61 insertions(+), 15 deletions(-)
> > 
> > How about probing the host for KVM in the tests. When it's
> > available, then only emit accel=kvm. When it's not (and
> > it's OK to fallback to tcg), then emit only accel=tcg.
> > That's what we do in kvm-unit-tests.
> 
> Arguably unit tests should not have any dependancy on the host hardware,
> and should be using accel=tcg exclusively, so that they have stable and
> predictable behaviour for all developers. Only functional / integration
> tests should have dependancies on host hardware/OS, so where accel=kvm
> would be appropriate.
>

Agreed. However, if you want to make the test case usable for
both TCG and KVM, then it doesn't hurt to give the option.
kvm-unit-tests has an ACCEL env. When it's not set then the
probe, fallback algorithm described above is used. When it is
set (to either kvm or tcg), then it uses only that selection
or fails.

How about doing that here, but with an unset ACCEL having the
default value of tcg?

Thanks,
drew

Re: [Qemu-devel] [PATCH 1/2] accel: allows to select the "best" accelerator

[Laurent Vivier]

On 04/10/2016 14:00, Daniel P. Berrange wrote:
> On Tue, Oct 04, 2016 at 01:30:47PM +0200, Laurent Vivier wrote:
>> By default, QEMU uses 'tcg' or the one provided with the "accel"
>> property.
>>
>> But sometime, user wants to use a real accelerator without knowing
>> if he really can, with, for instance accel=kvm:tcg.
>> In this case, and if the accelerator is not available we
>> have a noisy "XXX accelerator not found".
> 
> IMHO it would be better to just remove that warning message
> and continue using existing accel=kvm:tcg syntax to specify
> the preference of which accelerators to use.
> 
> Only emit "XXX accelerator not found", if there are not
> further accelerators listed. eg
> 
>   accel=kvm:tcg
> 
> should not print a "KVM accelerator not found" warning
> when it falls back to tcg, but a
> 
>   accel=kvm
> 
> would print a warning, since no fallback is given.


Something like this?

--- a/accel.c
+++ b/accel.c
@@ -99,7 +99,12 @@ void configure_accelerator(MachineState *ms)
         p = get_opt_name(buf, sizeof(buf), p, ':');
         acc = accel_find(buf);
         if (!acc) {
-            fprintf(stderr, "\"%s\" accelerator not found.\n", buf);
+            if (*p != ':') {
+                /* to avoid to pollute logs, display error only
+                 * on the last accelerator of the list
+                 */
+                fprintf(stderr, "\"%s\" accelerator not found.\n", buf);
+            }
             continue;
         }
         if (acc->available && !acc->available()) {

Re: [Qemu-devel] [PATCH 1/2] accel: allows to select the "best" accelerator

[Peter Maydell]

On 4 October 2016 at 13:00, Daniel P. Berrange <berrange@redhat.com> wrote:
> On Tue, Oct 04, 2016 at 01:30:47PM +0200, Laurent Vivier wrote:
>> By default, QEMU uses 'tcg' or the one provided with the "accel"
>> property.
>>
>> But sometime, user wants to use a real accelerator without knowing
>> if he really can, with, for instance accel=kvm:tcg.
>> In this case, and if the accelerator is not available we
>> have a noisy "XXX accelerator not found".
>
> IMHO it would be better to just remove that warning message
> and continue using existing accel=kvm:tcg syntax to specify
> the preference of which accelerators to use.

Oddly we don't give a warning message for what is probably
the most common user error: forgetting to specify -enable-kvm
and thus getting TCG only even though the h/w supports KVM...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] accel: allows to select the "best" accelerator

[Paolo Bonzini]

On 04/10/2016 13:30, Laurent Vivier wrote:
> But sometime, user wants to use a real accelerator without knowing
> if he really can, with, for instance accel=kvm:tcg.
> In this case, and if the accelerator is not available we
> have a noisy "XXX accelerator not found".
> 
> By allowing the user to ask the "best" accelerator for the given
> target, we can avoid this problem.
> 
> This patch introduces a new parameter for the "accel" property, the
> "best" keyword.
> 
> You can ask to use the best accelerator with "-M accel=best",
> or if you want to use your favorite accelerator and if it is not
> available, the best one, you can use, for instance
> "-M accel=kvm:best".

I don't think there's a single definition of a "best" accelerator.  For
example, some "-cpu" features may be available only with TCG.  In that
case, "kvm:tcg" has a clear meaning ("kvm" if it exists, otherwise
"tcg") but "best" doesn't.

I agree with Daniel that unit tests should use "tcg" exclusively, at
least as a default.

Paolo

Re: [Qemu-devel] [PATCH 1/2] accel: allows to select the "best" accelerator

[Thomas Huth]

On 04.10.2016 14:41, Paolo Bonzini wrote:
> 
> 
> On 04/10/2016 13:30, Laurent Vivier wrote:
>> But sometime, user wants to use a real accelerator without knowing
>> if he really can, with, for instance accel=kvm:tcg.
>> In this case, and if the accelerator is not available we
>> have a noisy "XXX accelerator not found".
>>
>> By allowing the user to ask the "best" accelerator for the given
>> target, we can avoid this problem.
>>
>> This patch introduces a new parameter for the "accel" property, the
>> "best" keyword.
>>
>> You can ask to use the best accelerator with "-M accel=best",
>> or if you want to use your favorite accelerator and if it is not
>> available, the best one, you can use, for instance
>> "-M accel=kvm:best".
> 
> I don't think there's a single definition of a "best" accelerator.  For
> example, some "-cpu" features may be available only with TCG.  In that
> case, "kvm:tcg" has a clear meaning ("kvm" if it exists, otherwise
> "tcg") but "best" doesn't.
> 
> I agree with Daniel that unit tests should use "tcg" exclusively, at
> least as a default.

Using only tcg has also some disadvantages: For some tests, it's
interesting to know whether they also work properly with KVM (e.g.
migration tests), and only using tcg by default slows down the "make
check" quite a bit - which might become an issue now that we're adding
more and more tests.

So maybe we need something like a qtest_get_accel() function that
returns "tcg" by default, but if the user set an environment variable
like QTEST_ACCEL, it uses that value instead?

 Thomas

Re: [Qemu-devel] [PATCH 1/2] accel: allows to select the "best" accelerator

[Daniel P. Berrange]

On Tue, Oct 04, 2016 at 02:49:21PM +0200, Thomas Huth wrote:
> On 04.10.2016 14:41, Paolo Bonzini wrote:
> > 
> > 
> > On 04/10/2016 13:30, Laurent Vivier wrote:
> >> But sometime, user wants to use a real accelerator without knowing
> >> if he really can, with, for instance accel=kvm:tcg.
> >> In this case, and if the accelerator is not available we
> >> have a noisy "XXX accelerator not found".
> >>
> >> By allowing the user to ask the "best" accelerator for the given
> >> target, we can avoid this problem.
> >>
> >> This patch introduces a new parameter for the "accel" property, the
> >> "best" keyword.
> >>
> >> You can ask to use the best accelerator with "-M accel=best",
> >> or if you want to use your favorite accelerator and if it is not
> >> available, the best one, you can use, for instance
> >> "-M accel=kvm:best".
> > 
> > I don't think there's a single definition of a "best" accelerator.  For
> > example, some "-cpu" features may be available only with TCG.  In that
> > case, "kvm:tcg" has a clear meaning ("kvm" if it exists, otherwise
> > "tcg") but "best" doesn't.
> > 
> > I agree with Daniel that unit tests should use "tcg" exclusively, at
> > least as a default.
> 
> Using only tcg has also some disadvantages: For some tests, it's
> interesting to know whether they also work properly with KVM (e.g.
> migration tests), and only using tcg by default slows down the "make
> check" quite a bit - which might become an issue now that we're adding
> more and more tests.

Which tests are you seeing a slow-down for ? make check-unit doesn't
show any difference and for 'make check-qtest' the difference was
negligible (1m47 for KVM vs 1m57 for TCG).  We shouldn't be running
extensive guest workloads in unit tests, so I'd be surprised to see
a major hit in unit test time.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH 1/2] accel: allows to select the "best" accelerator

[Thomas Huth]

On 04.10.2016 15:31, Daniel P. Berrange wrote:
> On Tue, Oct 04, 2016 at 02:49:21PM +0200, Thomas Huth wrote:
>> On 04.10.2016 14:41, Paolo Bonzini wrote:
>>>
>>>
>>> On 04/10/2016 13:30, Laurent Vivier wrote:
>>>> But sometime, user wants to use a real accelerator without knowing
>>>> if he really can, with, for instance accel=kvm:tcg.
>>>> In this case, and if the accelerator is not available we
>>>> have a noisy "XXX accelerator not found".
>>>>
>>>> By allowing the user to ask the "best" accelerator for the given
>>>> target, we can avoid this problem.
>>>>
>>>> This patch introduces a new parameter for the "accel" property, the
>>>> "best" keyword.
>>>>
>>>> You can ask to use the best accelerator with "-M accel=best",
>>>> or if you want to use your favorite accelerator and if it is not
>>>> available, the best one, you can use, for instance
>>>> "-M accel=kvm:best".
>>>
>>> I don't think there's a single definition of a "best" accelerator.  For
>>> example, some "-cpu" features may be available only with TCG.  In that
>>> case, "kvm:tcg" has a clear meaning ("kvm" if it exists, otherwise
>>> "tcg") but "best" doesn't.
>>>
>>> I agree with Daniel that unit tests should use "tcg" exclusively, at
>>> least as a default.
>>
>> Using only tcg has also some disadvantages: For some tests, it's
>> interesting to know whether they also work properly with KVM (e.g.
>> migration tests), and only using tcg by default slows down the "make
>> check" quite a bit - which might become an issue now that we're adding
>> more and more tests.
> 
> Which tests are you seeing a slow-down for ?

Well, everything that is using accel=tcg in tests/ could be accelerated.
For example, the new ipv6/ppc64 unit test is quite slow with TCG:

sudo QTEST_QEMU_BINARY=ppc64-softmmu/qemu-system-ppc64 time tests/pxe-test

 48.46user 0.07system 0:48.48elapsed

If I replace the "accel=tcg" with "accel=kvm" in pxe-test.c, the test runs
much faster:

 11.94user 0.38system 0:12.28elapsed

  Thomas

Re: [Qemu-devel] [PATCH 1/2] accel: allows to select the "best" accelerator

[Paolo Bonzini]

On 04/10/2016 16:18, Thomas Huth wrote:
>>> >> Using only tcg has also some disadvantages: For some tests, it's
>>> >> interesting to know whether they also work properly with KVM (e.g.
>>> >> migration tests), and only using tcg by default slows down the "make
>>> >> check" quite a bit - which might become an issue now that we're adding
>>> >> more and more tests.
>> > 
>> > Which tests are you seeing a slow-down for ?
> Well, everything that is using accel=tcg in tests/ could be accelerated.
> For example, the new ipv6/ppc64 unit test is quite slow with TCG:
> 
> sudo QTEST_QEMU_BINARY=ppc64-softmmu/qemu-system-ppc64 time tests/pxe-test
> 
>  48.46user 0.07system 0:48.48elapsed

Could that point to a firmware bug?  10 network-bound seconds for a boot
makes some sense, but 10 CPU-bound seconds don't...

Paolo

Re: [Qemu-devel] [PATCH 1/2] accel: allows to select the "best" accelerator

[Michael S. Tsirkin]

On Tue, Oct 04, 2016 at 07:16:49PM +0200, Paolo Bonzini wrote:
> 
> 
> On 04/10/2016 16:18, Thomas Huth wrote:
> >>> >> Using only tcg has also some disadvantages: For some tests, it's
> >>> >> interesting to know whether they also work properly with KVM (e.g.
> >>> >> migration tests), and only using tcg by default slows down the "make
> >>> >> check" quite a bit - which might become an issue now that we're adding
> >>> >> more and more tests.
> >> > 
> >> > Which tests are you seeing a slow-down for ?
> > Well, everything that is using accel=tcg in tests/ could be accelerated.
> > For example, the new ipv6/ppc64 unit test is quite slow with TCG:
> > 
> > sudo QTEST_QEMU_BINARY=ppc64-softmmu/qemu-system-ppc64 time tests/pxe-test
> > 
> >  48.46user 0.07system 0:48.48elapsed
> 
> Could that point to a firmware bug?  10 network-bound seconds for a boot
> makes some sense, but 10 CPU-bound seconds don't...
> 
> Paolo

AFAIK pxe firmware typically can't handle interrupts
so it polls for packets.

-- 
MST

Re: [Qemu-devel] [PATCH 1/2] accel: allows to select the "best" accelerator

[Thomas Huth]

On 04.10.2016 19:16, Paolo Bonzini wrote:
> 
> 
> On 04/10/2016 16:18, Thomas Huth wrote:
>>>>>> Using only tcg has also some disadvantages: For some tests, it's
>>>>>> interesting to know whether they also work properly with KVM (e.g.
>>>>>> migration tests), and only using tcg by default slows down the "make
>>>>>> check" quite a bit - which might become an issue now that we're adding
>>>>>> more and more tests.
>>>>
>>>> Which tests are you seeing a slow-down for ?
>> Well, everything that is using accel=tcg in tests/ could be accelerated.
>> For example, the new ipv6/ppc64 unit test is quite slow with TCG:
>>
>> sudo QTEST_QEMU_BINARY=ppc64-softmmu/qemu-system-ppc64 time tests/pxe-test
>>
>>  48.46user 0.07system 0:48.48elapsed
> 
> Could that point to a firmware bug?  10 network-bound seconds for a boot
> makes some sense, but 10 CPU-bound seconds don't...

SLOF is incredibly slow with TCG - most parts are written in Forth that
gets interpreted during runtime, and that seems to perform quite badly
with TCG for some reasons. We're in progress of speeding up the boot
process of SLOF a little bit (see
https://github.com/aik/SLOF/commit/b3fde41bc75269df2 for example), but
it will likely always be slower than a firmware that has been written in
C only.

 Thomas

Re: [Qemu-devel] [PATCH 1/2] accel: allows to select the "best" accelerator

[Paolo Bonzini]

On 05/10/2016 03:13, Michael S. Tsirkin wrote:
> On Tue, Oct 04, 2016 at 07:16:49PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 04/10/2016 16:18, Thomas Huth wrote:
>>>>>>> Using only tcg has also some disadvantages: For some tests, it's
>>>>>>> interesting to know whether they also work properly with KVM (e.g.
>>>>>>> migration tests), and only using tcg by default slows down the "make
>>>>>>> check" quite a bit - which might become an issue now that we're adding
>>>>>>> more and more tests.
>>>>>
>>>>> Which tests are you seeing a slow-down for ?
>>> Well, everything that is using accel=tcg in tests/ could be accelerated.
>>> For example, the new ipv6/ppc64 unit test is quite slow with TCG:
>>>
>>> sudo QTEST_QEMU_BINARY=ppc64-softmmu/qemu-system-ppc64 time tests/pxe-test
>>>
>>>  48.46user 0.07system 0:48.48elapsed
>>
>> Could that point to a firmware bug?  10 network-bound seconds for a boot
>> makes some sense, but 10 CPU-bound seconds don't...
> 
> AFAIK pxe firmware typically can't handle interrupts
> so it polls for packets.

Still wouldn't be appreciably slower when emulating.

Paolo

Re: [Qemu-devel] [PATCH 1/2] accel: allows to select the "best" accelerator

[Paolo Bonzini]

On 05/10/2016 09:00, Thomas Huth wrote:
> On 04.10.2016 19:16, Paolo Bonzini wrote:
>>
>>
>> On 04/10/2016 16:18, Thomas Huth wrote:
>>>>>>> Using only tcg has also some disadvantages: For some tests, it's
>>>>>>> interesting to know whether they also work properly with KVM (e.g.
>>>>>>> migration tests), and only using tcg by default slows down the "make
>>>>>>> check" quite a bit - which might become an issue now that we're adding
>>>>>>> more and more tests.
>>>>>
>>>>> Which tests are you seeing a slow-down for ?
>>> Well, everything that is using accel=tcg in tests/ could be accelerated.
>>> For example, the new ipv6/ppc64 unit test is quite slow with TCG:
>>>
>>> sudo QTEST_QEMU_BINARY=ppc64-softmmu/qemu-system-ppc64 time tests/pxe-test
>>>
>>>  48.46user 0.07system 0:48.48elapsed
>>
>> Could that point to a firmware bug?  10 network-bound seconds for a boot
>> makes some sense, but 10 CPU-bound seconds don't...
> 
> SLOF is incredibly slow with TCG - most parts are written in Forth that
> gets interpreted during runtime, and that seems to perform quite badly
> with TCG for some reasons. We're in progress of speeding up the boot
> process of SLOF a little bit (see
> https://github.com/aik/SLOF/commit/b3fde41bc75269df2 for example), but
> it will likely always be slower than a firmware that has been written in
> C only.

For what it's worth, just adding "-nodefaults -serial mon:stdio -net
nic,model=virtio -net user" saves 7 seconds (30%) on the time needed to
reach the OpenFirmware prompt.  SCSI seems to be the slowest device to scan.

Paolo

Re: [Qemu-devel] [PATCH 2/2] tests: use accel=best instead of accel=kvm:tcg

[Michael S. Tsirkin]

On Tue, Oct 04, 2016 at 01:30:48PM +0200, Laurent Vivier wrote:
> This avoids to have logs polluted by "kvm accelerator not found".
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>


Acked-by: Michael S. Tsirkin <mst@redhat.com>


> ---
>  tests/bios-tables-test.c | 2 +-
>  tests/postcopy-test.c    | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 7e27ea9..497dd56 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -716,7 +716,7 @@ static void test_acpi_one(const char *params, test_data *data)
>                             "-net none -display none %s "
>                             "-drive id=hd0,if=none,file=%s,format=raw "
>                             "-device ide-hd,drive=hd0 ",
> -                           data->machine, "kvm:tcg",
> +                           data->machine, "best",
>                             params ? params : "", disk);
>  
>      qtest_start(args);
> diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
> index 41ed1a9..d549d5e 100644
> --- a/tests/postcopy-test.c
> +++ b/tests/postcopy-test.c
> @@ -368,12 +368,12 @@ static void test_migrate(void)
>  
>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>          init_bootfile_x86(bootpath);
> -        cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
> +        cmd_src = g_strdup_printf("-machine accel=best -m 150M"
>                                    " -name pcsource,debug-threads=on"
>                                    " -serial file:%s/src_serial"
>                                    " -drive file=%s,format=raw",
>                                    tmpfs, bootpath);
> -        cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
> +        cmd_dst = g_strdup_printf("-machine accel=best -m 150M"
>                                    " -name pcdest,debug-threads=on"
>                                    " -serial file:%s/dest_serial"
>                                    " -drive file=%s,format=raw"
> @@ -381,12 +381,12 @@ static void test_migrate(void)
>                                    tmpfs, bootpath, uri);
>      } else if (strcmp(arch, "ppc64") == 0) {
>          init_bootfile_ppc(bootpath);
> -        cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
> +        cmd_src = g_strdup_printf("-machine accel=best -m 256M"
>                                    " -name pcsource,debug-threads=on"
>                                    " -serial file:%s/src_serial"
>                                    " -drive file=%s,if=pflash,format=raw",
>                                    tmpfs, bootpath);
> -        cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
> +        cmd_dst = g_strdup_printf("-machine accel=best -m 256M"
>                                    " -name pcdest,debug-threads=on"
>                                    " -serial file:%s/dest_serial"
>                                    " -incoming %s",
> -- 
> 2.7.4