Xen devel - Apr 2012 - [PATCH] libxl: default to xenconsoled for pv guests, even if qemu is running

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1335348624 -3600
# Node ID c8486295429011e9a220db1b6ed9f34ba690e729
# Parent  6f740f2f6e3e080e4bba9df59c826947885f6bd7
libxl: default to xenconsoled for pv guests, even if qemu is running.

Currently we prefer to use qemu for the disk backend if we are starting qemu
anyway (e.g. to service a disk).

Unfortunately qemu doesn''t log the console, which xenconsoled can do
via
XENCONSOLED_TRACE=guest. Since xenconsoled is also running anyway it seems like
there is no particular reason to prefer qemu just because it happens to be
running.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---

I''m not sure if this is 4.2 material, perhaps too late to be making
this sort
of change?

diff -r 6f740f2f6e3e -r c84862954290 tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c	Wed Apr 25 11:05:05 2012 +0100
+++ b/tools/libxl/libxl_create.c	Wed Apr 25 11:10:24 2012 +0100
@@ -682,8 +682,7 @@ static int do_domain_create(libxl__gc *g
                 d_config->num_vfbs, d_config->vfbs,
                 d_config->num_disks, &d_config->disks[0]);
 
-        if (need_qemu)
-             console.consback = LIBXL__CONSOLE_BACKEND_IOEMU;
+        console.consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;
 
         libxl__device_console_add(gc, domid, &console, &state);
         libxl__device_console_dispose(&console);
diff -r 6f740f2f6e3e -r c84862954290 tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c	Wed Apr 25 11:05:05 2012 +0100
+++ b/tools/libxl/libxl_dm.c	Wed Apr 25 11:10:24 2012 +0100
@@ -1093,11 +1093,6 @@ int libxl__need_xenpv_qemu(libxl__gc *gc
 {
     int i, ret = 0;
 
-    if (nr_consoles > 1) {
-        ret = 1;
-        goto out;
-    }
-
     for (i = 0; i < nr_consoles; i++) {
         if (consoles[i].consback == LIBXL__CONSOLE_BACKEND_IOEMU) {
             ret = 1;

Ian Campbell writes ("[PATCH] libxl: default to xenconsoled for pv guests,
even if qemu is running"):
> libxl: default to xenconsoled for pv guests, even if qemu is running.
> 
> Currently we prefer to use qemu for the disk backend if we are starting
qemu
> anyway (e.g. to service a disk).
...
> I''m not sure if this is 4.2 material, perhaps too late to be
making this sort
> of change?
I think we can regard this as a bugfix, or make an exception.

But: have you tested this ?  ISTR hearing that there was some
difficulty getting a pv qemu not to try to provide the console.

Ian.

On Wed, 25 Apr 2012, Ian Campbell wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1335348624 -3600
> # Node ID c8486295429011e9a220db1b6ed9f34ba690e729
> # Parent  6f740f2f6e3e080e4bba9df59c826947885f6bd7
> libxl: default to xenconsoled for pv guests, even if qemu is running.
> 
> Currently we prefer to use qemu for the disk backend if we are starting
qemu
> anyway (e.g. to service a disk).
> 
> Unfortunately qemu doesn''t log the console, which xenconsoled can
do via
> XENCONSOLED_TRACE=guest. Since xenconsoled is also running anyway it seems
like
> there is no particular reason to prefer qemu just because it happens to be
> running.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
QEMU''s console backend supports multiple PV consoles while xenconsoled
does not.
I think you should just change the default only in case a single PV
console is configured.

> I''m not sure if this is 4.2 material, perhaps too late to be
making this sort
> of change?
> 
> diff -r 6f740f2f6e3e -r c84862954290 tools/libxl/libxl_create.c
> --- a/tools/libxl/libxl_create.c	Wed Apr 25 11:05:05 2012 +0100
> +++ b/tools/libxl/libxl_create.c	Wed Apr 25 11:10:24 2012 +0100
> @@ -682,8 +682,7 @@ static int do_domain_create(libxl__gc *g
>                  d_config->num_vfbs, d_config->vfbs,
>                  d_config->num_disks, &d_config->disks[0]);
>  
> -        if (need_qemu)
> -             console.consback = LIBXL__CONSOLE_BACKEND_IOEMU;
> +        console.consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;
>  
>          libxl__device_console_add(gc, domid, &console, &state);
>          libxl__device_console_dispose(&console);
I would change the if into:

if (need_qemu and nr_console > 1)

even though I am aware that at the moment nr_console is always 1.

> diff -r 6f740f2f6e3e -r c84862954290 tools/libxl/libxl_dm.c
> --- a/tools/libxl/libxl_dm.c	Wed Apr 25 11:05:05 2012 +0100
> +++ b/tools/libxl/libxl_dm.c	Wed Apr 25 11:10:24 2012 +0100
> @@ -1093,11 +1093,6 @@ int libxl__need_xenpv_qemu(libxl__gc *gc
>  {
>      int i, ret = 0;
>  
> -    if (nr_consoles > 1) {
> -        ret = 1;
> -        goto out;
> -    }
> -
I would get rid of this change

On Wed, 2012-04-25 at 11:36 +0100, Stefano Stabellini
wrote:
> On Wed, 25 Apr 2012, Ian Campbell wrote:
> > # HG changeset patch
> > # User Ian Campbell <ian.campbell@citrix.com>
> > # Date 1335348624 -3600
> > # Node ID c8486295429011e9a220db1b6ed9f34ba690e729
> > # Parent  6f740f2f6e3e080e4bba9df59c826947885f6bd7
> > libxl: default to xenconsoled for pv guests, even if qemu is running.
> > 
> > Currently we prefer to use qemu for the disk backend if we are
starting qemu
> > anyway (e.g. to service a disk).
> > 
> > Unfortunately qemu doesn''t log the console, which xenconsoled
can do via
> > XENCONSOLED_TRACE=guest. Since xenconsoled is also running anyway it
seems like
> > there is no particular reason to prefer qemu just because it happens
to be
> > running.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> QEMU''s console backend supports multiple PV consoles while
xenconsoled
> does not.
> I think you should just change the default only in case a single PV
> console is configured.
> 
> 
> > I''m not sure if this is 4.2 material, perhaps too late to be
making this sort
> > of change?
> > 
> > diff -r 6f740f2f6e3e -r c84862954290 tools/libxl/libxl_create.c
> > --- a/tools/libxl/libxl_create.c	Wed Apr 25 11:05:05 2012 +0100
> > +++ b/tools/libxl/libxl_create.c	Wed Apr 25 11:10:24 2012 +0100
> > @@ -682,8 +682,7 @@ static int do_domain_create(libxl__gc *g
> >                  d_config->num_vfbs, d_config->vfbs,
> >                  d_config->num_disks, &d_config->disks[0]);
> >  
> > -        if (need_qemu)
> > -             console.consback = LIBXL__CONSOLE_BACKEND_IOEMU;
> > +        console.consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;
> >  
> >          libxl__device_console_add(gc, domid, &console,
&state);
> >          libxl__device_console_dispose(&console);
> 
> I would change the if into:
> 
> if (need_qemu and nr_console > 1)
> 
> even though I am aware that at the moment nr_console is always 1.
There isn''t actually a nr_console here, just a hardcoded 1... I could
add a nr_console just for this but perhaps it makes more sense for
libxl__need_xenpv_qemu to enforce this by updating consback for all
consoles iff there are > 1 of them and then returning need_qemu as
appropriate?

I''m a little unhappy with having need_qemu have side-effects like this,
but it seems better than splitting the logic into two places...

Ian.

On Wed, 25 Apr 2012, Ian Campbell wrote:
> On Wed, 2012-04-25 at 11:36 +0100, Stefano Stabellini wrote:
> > On Wed, 25 Apr 2012, Ian Campbell wrote:
> > > # HG changeset patch
> > > # User Ian Campbell <ian.campbell@citrix.com>
> > > # Date 1335348624 -3600
> > > # Node ID c8486295429011e9a220db1b6ed9f34ba690e729
> > > # Parent  6f740f2f6e3e080e4bba9df59c826947885f6bd7
> > > libxl: default to xenconsoled for pv guests, even if qemu is
running.
> > > 
> > > Currently we prefer to use qemu for the disk backend if we are
starting qemu
> > > anyway (e.g. to service a disk).
> > > 
> > > Unfortunately qemu doesn''t log the console, which
xenconsoled can do via
> > > XENCONSOLED_TRACE=guest. Since xenconsoled is also running anyway
it seems like
> > > there is no particular reason to prefer qemu just because it
happens to be
> > > running.
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > QEMU''s console backend supports multiple PV consoles while
xenconsoled
> > does not.
> > I think you should just change the default only in case a single PV
> > console is configured.
> > 
> > 
> > > I''m not sure if this is 4.2 material, perhaps too late
to be making this sort
> > > of change?
> > > 
> > > diff -r 6f740f2f6e3e -r c84862954290 tools/libxl/libxl_create.c
> > > --- a/tools/libxl/libxl_create.c	Wed Apr 25 11:05:05 2012 +0100
> > > +++ b/tools/libxl/libxl_create.c	Wed Apr 25 11:10:24 2012 +0100
> > > @@ -682,8 +682,7 @@ static int do_domain_create(libxl__gc *g
> > >                  d_config->num_vfbs, d_config->vfbs,
> > >                  d_config->num_disks,
&d_config->disks[0]);
> > >  
> > > -        if (need_qemu)
> > > -             console.consback = LIBXL__CONSOLE_BACKEND_IOEMU;
> > > +        console.consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;
> > >  
> > >          libxl__device_console_add(gc, domid, &console,
&state);
> > >          libxl__device_console_dispose(&console);
> > 
> > I would change the if into:
> > 
> > if (need_qemu and nr_console > 1)
> > 
> > even though I am aware that at the moment nr_console is always 1.
> 
> There isn''t actually a nr_console here, just a hardcoded 1... I
could
> add a nr_console just for this but perhaps it makes more sense for
> libxl__need_xenpv_qemu to enforce this by updating consback for all
> consoles iff there are > 1 of them and then returning need_qemu as
> appropriate?
> 
> I''m a little unhappy with having need_qemu have side-effects like
this,
> but it seems better than splitting the logic into two places...
I would be OK with that, maybe it is worth adding a comment on
libxl__need_xenpv_qemu to document the behavior.

On Wed, 2012-04-25 at 11:32 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH] libxl: default to xenconsoled for pv
guests, even if qemu is running"):
> > libxl: default to xenconsoled for pv guests, even if qemu is running.
> > 
> > Currently we prefer to use qemu for the disk backend if we are
starting qemu
> > anyway (e.g. to service a disk).
> ...
> > I''m not sure if this is 4.2 material, perhaps too late to be
making this sort
> > of change?
> 
> I think we can regard this as a bugfix, or make an exception.
> 
> But: have you tested this ?  ISTR hearing that there was some
> difficulty getting a pv qemu not to try to provide the console.
I did run a PV guest with it and it seemed to be doing what I expected
(i.e. my guest logs showed up in /var/log/xen/console).

Ian.

On Wed, 2012-04-25 at 11:49 +0100, Stefano Stabellini wrote:
> I would be OK with that, maybe it is worth adding a comment on
> libxl__need_xenpv_qemu to document the behavior.
8<-------------------------------------

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1335351300 -3600
# Node ID 28f86b71782a099c0373fb237e6b08e0788ff69d
# Parent  02f0161ae6201ded5415e7f0c92214b4783c8d72
libxl: default to xenconsoled for pv guests, even if qemu is running.

Currently we prefer to use qemu for the disk backend if we are starting qemu
anyway (e.g. to service a disk).

Unfortunately qemu doesn''t log the console, which xenconsoled can do
via
XENCONSOLED_TRACE=guest. Since xenconsoled is also running anyway it seems like
there is no particular reason to prefer qemu just because it happens to be
running.

However we must use qemu if thereis more than one console (xenconsoled only
supports a single console).

Therefore push the logic to change the console backend down into
libxl__need_xenpv_qemu so that it can do the right thing.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---

I''m not sure if this is 4.2 material, perhaps too late to be making
this sort
of change?

diff -r 02f0161ae620 -r 28f86b71782a tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c	Wed Apr 25 11:30:58 2012 +0100
+++ b/tools/libxl/libxl_create.c	Wed Apr 25 11:55:00 2012 +0100
@@ -682,9 +682,6 @@ static int do_domain_create(libxl__gc *g
                 d_config->num_vfbs, d_config->vfbs,
                 d_config->num_disks, &d_config->disks[0]);
 
-        if (need_qemu)
-             console.consback = LIBXL__CONSOLE_BACKEND_IOEMU;
-
         libxl__device_console_add(gc, domid, &console, &state);
         libxl__device_console_dispose(&console);
 
diff -r 02f0161ae620 -r 28f86b71782a tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c	Wed Apr 25 11:30:58 2012 +0100
+++ b/tools/libxl/libxl_dm.c	Wed Apr 25 11:55:00 2012 +0100
@@ -1093,7 +1093,13 @@ int libxl__need_xenpv_qemu(libxl__gc *gc
 {
     int i, ret = 0;
 
+    /*
+     * qemu is required in order to support 2 or more consoles. So switch all
+     * backends to qemu if this is the case
+     */
     if (nr_consoles > 1) {
+        for (i = 0; i < nr_consoles; i++)
+            consoles[i].consback = LIBXL__CONSOLE_BACKEND_IOEMU;
         ret = 1;
         goto out;
     }

On Wed, 25 Apr 2012, Ian Campbell wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1335351300 -3600
> # Node ID 28f86b71782a099c0373fb237e6b08e0788ff69d
> # Parent  02f0161ae6201ded5415e7f0c92214b4783c8d72
> libxl: default to xenconsoled for pv guests, even if qemu is running.
> 
> Currently we prefer to use qemu for the disk backend if we are starting
qemu
> anyway (e.g. to service a disk).
> 
> Unfortunately qemu doesn''t log the console, which xenconsoled can
do via
> XENCONSOLED_TRACE=guest. Since xenconsoled is also running anyway it seems
like
> there is no particular reason to prefer qemu just because it happens to be
> running.
> 
> However we must use qemu if thereis more than one console (xenconsoled only
> supports a single console).
> 
> Therefore push the logic to change the console backend down into
> libxl__need_xenpv_qemu so that it can do the right thing.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] libxl: default to
xenconsoled for pv guests, even if qemu is
running"):
> On Wed, 25 Apr 2012, Ian Campbell wrote:
> > libxl: default to xenconsoled for pv guests, even if qemu is running.
...
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>