[PATCH v1 00/12] libxl: add driver domain backend daemon

[PATCH v1 00/12] libxl: add driver domain backend daemon

[Roger Pau Monne]

This patch series introduces support to launch device backends from
driver domains using libxl, removing the need to setup udev on driver
domains.

The daemon currently supports vbds, vifs and qdisks as backends.

Patches from 1 to 11 focus on getting libxl to work on driver domains,
and add several helper functions that will be used to build the
infraestructure needed for libxl to work on domains different than
Dom0.

The last patch (12) adds a new xl command, "devd", that can be used to
launch a daemon that will listen to changes in the xenstore "backend"
directory for the domain and perform the necessary actions to attach
the devices.

Changes since RFC:
 * Added documentation for the new xenstore paths created on path 4.
 * Synchronize the removal of devices from the driver domain with the 
   toolstack domain.

* Acked
*[PATCH v1 01/12] libxl/hotplug: add support for getting domid
*[PATCH v1 02/12] libxl: remove unneeded libxl_domain_info in
*[PATCH v1 03/12] libxl: make hotplug execution conditional on
 [PATCH v1 04/12] libxl: create a local xenstore libxl and
 [PATCH v1 05/12] libxl: don't remove device frontend path from
 [PATCH v1 06/12] libxl: synchronize device removal when using driver
 [PATCH v1 07/12] libxl: remove the Qemu bodge for driver domain
 [PATCH v1 08/12] libxl: don't launch Qemu on Dom0 for Qdisk devices
 [PATCH v1 09/12] libxl: add Qdisk backend launch helper
 [PATCH v1 10/12] xl: put daemonize code in it's own function
 [PATCH v1 11/12] libxl: revert 326a7b74
 [PATCH v1 12/12] libxl: add device backend listener in order to

[PATCH v1 01/12] libxl/hotplug: add support for getting domid

[Roger Pau Monne]

This patch writes Dom0 domid on xenstore (like it's done for other
guests), and adds a libxl helper function to fetch that domid from
xenstore.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/hotplug/Linux/init.d/xencommons |    1 +
 tools/libxl/libxl.c                   |   17 +++++++++++++++++
 tools/libxl/libxl_internal.h          |    3 +++
 3 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/tools/hotplug/Linux/init.d/xencommons b/tools/hotplug/Linux/init.d/xencommons
index a2e633b..43e09bc 100644
--- a/tools/hotplug/Linux/init.d/xencommons
+++ b/tools/hotplug/Linux/init.d/xencommons
@@ -110,6 +110,7 @@ do_start () {
 
 		echo Setting domain 0 name...
 		${BINDIR}/xenstore-write "/local/domain/0/name" "Domain-0"
+		${BINDIR}/xenstore-write "/local/domain/0/domid" "0"
 	fi
 
 	echo Starting xenconsoled...
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 1bce4bb..5417b48 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1688,6 +1688,23 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
     return ERROR_FAIL;
 }
 
+int libxl__get_domid(libxl__gc *gc, uint32_t *domid)
+{
+    int rc;
+    const char *xs_domid;
+
+    rc = libxl__xs_read_checked(gc, XBT_NULL, DOMID_XS_PATH, &xs_domid);
+    if (rc || !xs_domid) {
+        rc = rc ? rc : ERROR_FAIL;
+        goto out;
+    }
+
+    *domid = atoi(xs_domid);
+
+out:
+    return rc;
+}
+
 /******************************************************************************/
 
 /* generic callback for devices that only need to set ao_complete */
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index f051d91..3b74726 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -101,6 +101,7 @@
 #define STUBDOM_SPECIAL_CONSOLES 3
 #define TAP_DEVICE_SUFFIX "-emu"
 #define DISABLE_UDEV_PATH "libxl/disable_udev"
+#define DOMID_XS_PATH "domid"
 
 #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
 
@@ -978,6 +979,8 @@ _hidden const char *libxl__device_nic_devname(libxl__gc *gc,
                                               uint32_t devid,
                                               libxl_nic_type type);
 
+_hidden int libxl__get_domid(libxl__gc *gc, uint32_t *domid);
+
 /*
  * libxl__ev_devstate - waits a given time for a device to
  * reach a given state.  Follows the libxl_ev_* conventions.
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v1 01/12] libxl/hotplug: add support for getting domid

[Andrew Cooper]

On 02/10/13 10:24, Roger Pau Monne wrote:
> This patch writes Dom0 domid on xenstore (like it's done for other
> guests), and adds a libxl helper function to fetch that domid from
> xenstore.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  tools/hotplug/Linux/init.d/xencommons |    1 +
>  tools/libxl/libxl.c                   |   17 +++++++++++++++++
>  tools/libxl/libxl_internal.h          |    3 +++
>  3 files changed, 21 insertions(+), 0 deletions(-)
>
> diff --git a/tools/hotplug/Linux/init.d/xencommons b/tools/hotplug/Linux/init.d/xencommons
> index a2e633b..43e09bc 100644
> --- a/tools/hotplug/Linux/init.d/xencommons
> +++ b/tools/hotplug/Linux/init.d/xencommons
> @@ -110,6 +110,7 @@ do_start () {
>  
>  		echo Setting domain 0 name...
>  		${BINDIR}/xenstore-write "/local/domain/0/name" "Domain-0"
> +		${BINDIR}/xenstore-write "/local/domain/0/domid" "0"
>  	fi
>  
>  	echo Starting xenconsoled...
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 1bce4bb..5417b48 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1688,6 +1688,23 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
>      return ERROR_FAIL;
>  }
>  
> +int libxl__get_domid(libxl__gc *gc, uint32_t *domid)
> +{
> +    int rc;
> +    const char *xs_domid;
> +
> +    rc = libxl__xs_read_checked(gc, XBT_NULL, DOMID_XS_PATH, &xs_domid);
> +    if (rc || !xs_domid) {

NULL domid check should be done before xenstore read, as an xs read is
quite a long operation.

> +        rc = rc ? rc : ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    *domid = atoi(xs_domid);

atoi() lacks any error reporting.  Use strtoul() and perhaps even a
sanity check on the return value against the valid bounds for a domid.

~Andrew

> +
> +out:
> +    return rc;
> +}
> +
>  /******************************************************************************/
>  
>  /* generic callback for devices that only need to set ao_complete */
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index f051d91..3b74726 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -101,6 +101,7 @@
>  #define STUBDOM_SPECIAL_CONSOLES 3
>  #define TAP_DEVICE_SUFFIX "-emu"
>  #define DISABLE_UDEV_PATH "libxl/disable_udev"
> +#define DOMID_XS_PATH "domid"
>  
>  #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
>  
> @@ -978,6 +979,8 @@ _hidden const char *libxl__device_nic_devname(libxl__gc *gc,
>                                                uint32_t devid,
>                                                libxl_nic_type type);
>  
> +_hidden int libxl__get_domid(libxl__gc *gc, uint32_t *domid);
> +
>  /*
>   * libxl__ev_devstate - waits a given time for a device to
>   * reach a given state.  Follows the libxl_ev_* conventions.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v1 01/12] libxl/hotplug: add support for getting domid

[Ian Campbell]

On Wed, 2013-10-02 at 10:36 +0100, Andrew Cooper wrote:
> On 02/10/13 10:24, Roger Pau Monne wrote:
> > This patch writes Dom0 domid on xenstore (like it's done for other
> > guests), and adds a libxl helper function to fetch that domid from
> > xenstore.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > ---
> >  tools/hotplug/Linux/init.d/xencommons |    1 +
> >  tools/libxl/libxl.c                   |   17 +++++++++++++++++
> >  tools/libxl/libxl_internal.h          |    3 +++
> >  3 files changed, 21 insertions(+), 0 deletions(-)
> >
> > diff --git a/tools/hotplug/Linux/init.d/xencommons b/tools/hotplug/Linux/init.d/xencommons
> > index a2e633b..43e09bc 100644
> > --- a/tools/hotplug/Linux/init.d/xencommons
> > +++ b/tools/hotplug/Linux/init.d/xencommons
> > @@ -110,6 +110,7 @@ do_start () {
> >  
> >  		echo Setting domain 0 name...
> >  		${BINDIR}/xenstore-write "/local/domain/0/name" "Domain-0"
> > +		${BINDIR}/xenstore-write "/local/domain/0/domid" "0"
> >  	fi
> >  
> >  	echo Starting xenconsoled...
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 1bce4bb..5417b48 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -1688,6 +1688,23 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
> >      return ERROR_FAIL;
> >  }
> >  
> > +int libxl__get_domid(libxl__gc *gc, uint32_t *domid)
> > +{
> > +    int rc;
> > +    const char *xs_domid;
> > +
> > +    rc = libxl__xs_read_checked(gc, XBT_NULL, DOMID_XS_PATH, &xs_domid);
> > +    if (rc || !xs_domid) {
> 
> NULL domid check should be done before xenstore read, as an xs read is
> quite a long operation.

xs_domid is the result of the xs_read, not a precursor.

> > +        rc = rc ? rc : ERROR_FAIL;
> > +        goto out;
> > +    }
> > +
> > +    *domid = atoi(xs_domid);
> 
> atoi() lacks any error reporting.  Use strtoul() and perhaps even a
> sanity check on the return value against the valid bounds for a domid.

We use atoi quite a lot and this is reading a toolstack controlled value
I think, so does it matter that much?
> 
> ~Andrew
> 
> > +
> > +out:
> > +    return rc;
> > +}
> > +
> >  /******************************************************************************/
> >  
> >  /* generic callback for devices that only need to set ao_complete */
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index f051d91..3b74726 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -101,6 +101,7 @@
> >  #define STUBDOM_SPECIAL_CONSOLES 3
> >  #define TAP_DEVICE_SUFFIX "-emu"
> >  #define DISABLE_UDEV_PATH "libxl/disable_udev"
> > +#define DOMID_XS_PATH "domid"
> >  
> >  #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
> >  
> > @@ -978,6 +979,8 @@ _hidden const char *libxl__device_nic_devname(libxl__gc *gc,
> >                                                uint32_t devid,
> >                                                libxl_nic_type type);
> >  
> > +_hidden int libxl__get_domid(libxl__gc *gc, uint32_t *domid);
> > +
> >  /*
> >   * libxl__ev_devstate - waits a given time for a device to
> >   * reach a given state.  Follows the libxl_ev_* conventions.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v1 01/12] libxl/hotplug: add support for getting domid

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v1 01/12] libxl/hotplug: add support for getting domid"):
> This patch writes Dom0 domid on xenstore (like it's done for other
> guests), and adds a libxl helper function to fetch that domid from
> xenstore.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH v1 01/12] libxl/hotplug: add support for getting domid

[Ian Campbell]

On Fri, 2013-11-01 at 18:42 +0000, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v1 01/12] libxl/hotplug: add support for getting domid"):
> > This patch writes Dom0 domid on xenstore (like it's done for other
> > guests), and adds a libxl helper function to fetch that domid from
> > xenstore.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

I applied this, dropping the xencommons change which already happened.
Here is what went in:

commit d6bc83a531e4cfdd29da95afa992b95be8316080
Author: Roger Pau Monne <roger.pau@citrix.com>
Date:   Wed Oct 2 11:24:23 2013 +0200

    libxl/hotplug: add support for getting domid
    
    This patch writes Dom0 domid on xenstore (like it's done for other
    guests), and adds a libxl helper function to fetch that domid from
    xenstore.
    
    Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
    Acked-by: Ian Campbell <ian.campbell@citrix.com>
    Acked-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
    [ ijc -- dropped xencommons hunk, same change was made independently
      in 02ebea7768fe ]

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index bede011..0f0f56c 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1692,6 +1692,23 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
     return ERROR_FAIL;
 }
 
+int libxl__get_domid(libxl__gc *gc, uint32_t *domid)
+{
+    int rc;
+    const char *xs_domid;
+
+    rc = libxl__xs_read_checked(gc, XBT_NULL, DOMID_XS_PATH, &xs_domid);
+    if (rc || !xs_domid) {
+        rc = rc ? rc : ERROR_FAIL;
+        goto out;
+    }
+
+    *domid = atoi(xs_domid);
+
+out:
+    return rc;
+}
+
 /******************************************************************************/
 
 /* generic callback for devices that only need to set ao_complete */
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 4f92522..4729566 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -101,6 +101,7 @@
 #define STUBDOM_SPECIAL_CONSOLES 3
 #define TAP_DEVICE_SUFFIX "-emu"
 #define DISABLE_UDEV_PATH "libxl/disable_udev"
+#define DOMID_XS_PATH "domid"
 
 #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
 
@@ -978,6 +979,8 @@ _hidden const char *libxl__device_nic_devname(libxl__gc *gc,
                                               uint32_t devid,
                                               libxl_nic_type type);
 
+_hidden int libxl__get_domid(libxl__gc *gc, uint32_t *domid);
+
 /*
  * libxl__ev_devstate - waits a given time for a device to
  * reach a given state.  Follows the libxl_ev_* conventions.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v1 02/12] libxl: remove unneeded libxl_domain_info in wait_device_connection

[Roger Pau Monne]

The info fetched by libxl_domain_info is not used anywere in the
function.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl_device.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 16a92a4..2971dd3 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -715,16 +715,8 @@ void libxl__wait_device_connection(libxl__egc *egc, libxl__ao_device *aodev)
     STATE_AO_GC(aodev->ao);
     char *be_path = libxl__device_backend_path(gc, aodev->dev);
     char *state_path = libxl__sprintf(gc, "%s/state", be_path);
-    libxl_dominfo info;
-    uint32_t domid = aodev->dev->domid;
     int rc = 0;
 
-    libxl_dominfo_init(&info);
-    rc = libxl_domain_info(CTX, &info, domid);
-    if (rc) {
-        LOG(ERROR, "unable to get info for domain %d", domid);
-        goto out;
-    }
     if (QEMU_BACKEND(aodev->dev)) {
         /*
          * If Qemu is not running, there's no point in waiting for
@@ -746,12 +738,10 @@ void libxl__wait_device_connection(libxl__egc *egc, libxl__ao_device *aodev)
         goto out;
     }
 
-    libxl_dominfo_dispose(&info);
     return;
 
 out:
     aodev->rc = rc;
-    libxl_dominfo_dispose(&info);
     device_hotplug_done(egc, aodev);
     return;
 }
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v1 02/12] libxl: remove unneeded libxl_domain_info in wait_device_connection

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v1 02/12] libxl: remove unneeded libxl_domain_info in wait_device_connection"):
> The info fetched by libxl_domain_info is not used anywere in the
> function.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH v1 03/12] libxl: make hotplug execution conditional on backend_domid == domid

[Roger Pau Monne]

libxl currently refuses to execute hotplug scripts if the backend
domid of a device is different than LIBXL_TOOLSTACK_DOMID. This will
prevent libxl from executing hotplug scripts when running on a domain
different than LIBXL_TOOLSTACK_DOMID, we should instead check if
backend_domid is different than current domid.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl_device.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 2971dd3..48acc92 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -908,12 +908,15 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
     int rc = 0;
     int hotplug;
     pid_t pid;
+    uint32_t domid;
 
     /*
      * If device is attached from a driver domain don't try to execute
      * hotplug scripts
      */
-    if (aodev->dev->backend_domid != LIBXL_TOOLSTACK_DOMID)
+    rc = libxl__get_domid(gc, &domid);
+    if (rc) goto out;
+    if (aodev->dev->backend_domid != domid)
         goto out;
 
     /* Check if we have to execute hotplug scripts for this device
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v1 03/12] libxl: make hotplug execution conditional on backend_domid == domid

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v1 03/12] libxl: make hotplug execution conditional on backend_domid == domid"):
> libxl currently refuses to execute hotplug scripts if the backend
> domid of a device is different than LIBXL_TOOLSTACK_DOMID. This will
> prevent libxl from executing hotplug scripts when running on a domain
> different than LIBXL_TOOLSTACK_DOMID, we should instead check if
> backend_domid is different than current domid.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH v1 03/12] libxl: make hotplug execution conditional on backend_domid == domid

[Ian Campbell]

On Fri, 2013-11-01 at 18:43 +0000, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v1 03/12] libxl: make hotplug execution conditional on backend_domid == domid"):
> > libxl currently refuses to execute hotplug scripts if the backend
> > domid of a device is different than LIBXL_TOOLSTACK_DOMID. This will
> > prevent libxl from executing hotplug scripts when running on a domain
> > different than LIBXL_TOOLSTACK_DOMID, we should instead check if
> > backend_domid is different than current domid.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Applied along with #2.

I stopped at this point because #4 had comments. I considered applying
#5 but I wasn't positive it didn't need #4.

[4] libxl: create a local xenstore libxl and device-model dir for guests
[5] libxl: don't remove device frontend path from driver domains


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v1 04/12] libxl: create a local xenstore libxl and device-model dir for guests

[Roger Pau Monne]

If libxl is executed inside a guest domain it needs write access to
the local libxl xenstore dir (/local/<domid>/libxl) to store internal
data. This also applies to Qemu which needs a
/local/<domid>/device-model xenstore directory.

This patch creates the mentioned directories for each guest launched
from libxl.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
Changes since RFC:
 * Add documentation for the new paths.
---
 docs/misc/xenstore-paths.markdown |   10 ++++++++++
 tools/libxl/libxl_create.c        |   12 ++++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index 1c634b5..a8c4c58 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -318,6 +318,16 @@ protocol definition.
 
 A domain writable path. Available for arbitrary domain use.
 
+### Driver domain specific paths
+
+#### ~/device-model/$DOMID/state [w]
+
+Contains the status of the device models running on the domain.
+
+#### ~/libxl/$DOMID/qdisk-backend-pid [w]
+
+Contains the PIDs of the device models running on the domain.
+
 ## Virtual Machine Paths
 
 The /vm/$UUID namespace is used by toolstacks to store various
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 0c32d0b..aac19b5 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -498,6 +498,18 @@ retry_transaction:
     libxl__xs_mkdir(gc, t,
                     libxl__sprintf(gc, "%s/data", dom_path),
                     rwperm, ARRAY_SIZE(rwperm));
+    /*
+     * Create a local "libxl" directory for each guest, since we might want
+     * to use libxl from inside the guest
+     */
+    libxl__xs_mkdir(gc, t, GCSPRINTF("%s/libxl", dom_path), rwperm,
+                    ARRAY_SIZE(rwperm));
+    /*
+     * Create a local "device-model" directory for each guest, since we
+     * might want to use Qemu from inside the guest
+     */
+    libxl__xs_mkdir(gc, t, GCSPRINTF("%s/device-model", dom_path), rwperm,
+                    ARRAY_SIZE(rwperm));
     if (info->type == LIBXL_DOMAIN_TYPE_HVM)
         libxl__xs_mkdir(gc, t,
             libxl__sprintf(gc, "%s/hvmloader/generation-id-address", dom_path),
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v1 04/12] libxl: create a local xenstore libxl and device-model dir for guests

[Ian Campbell]

On Wed, 2013-10-02 at 11:24 +0200, Roger Pau Monne wrote:
> If libxl is executed inside a guest domain it needs write access to
> the local libxl xenstore dir (/local/<domid>/libxl) to store internal
> data. This also applies to Qemu which needs a
> /local/<domid>/device-model xenstore directory.
> 
> This patch creates the mentioned directories for each guest launched
> from libxl.

Didn't we decide to make this conditional on building a driver domain?

> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> ---
> Changes since RFC:
>  * Add documentation for the new paths.
> ---
>  docs/misc/xenstore-paths.markdown |   10 ++++++++++
>  tools/libxl/libxl_create.c        |   12 ++++++++++++
>  2 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
> index 1c634b5..a8c4c58 100644
> --- a/docs/misc/xenstore-paths.markdown
> +++ b/docs/misc/xenstore-paths.markdown
> @@ -318,6 +318,16 @@ protocol definition.
>  
>  A domain writable path. Available for arbitrary domain use.
>  
> +### Driver domain specific paths
> +
> +#### ~/device-model/$DOMID/state [w]
> +
> +Contains the status of the device models running on the domain.
> +
> +#### ~/libxl/$DOMID/qdisk-backend-pid [w]

Should both of the have BACKEND as a tag too?

And perhaps some tag to indicate that normal guest functions shouldn't
use this, similar perhaps to INTERNAL?

> +
> +Contains the PIDs of the device models running on the domain.
> +
>  ## Virtual Machine Paths
>  
>  The /vm/$UUID namespace is used by toolstacks to store various
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 0c32d0b..aac19b5 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -498,6 +498,18 @@ retry_transaction:
>      libxl__xs_mkdir(gc, t,
>                      libxl__sprintf(gc, "%s/data", dom_path),
>                      rwperm, ARRAY_SIZE(rwperm));
> +    /*
> +     * Create a local "libxl" directory for each guest, since we might want
> +     * to use libxl from inside the guest
> +     */
> +    libxl__xs_mkdir(gc, t, GCSPRINTF("%s/libxl", dom_path), rwperm,
> +                    ARRAY_SIZE(rwperm));
> +    /*
> +     * Create a local "device-model" directory for each guest, since we
> +     * might want to use Qemu from inside the guest
> +     */
> +    libxl__xs_mkdir(gc, t, GCSPRINTF("%s/device-model", dom_path), rwperm,
> +                    ARRAY_SIZE(rwperm));
>      if (info->type == LIBXL_DOMAIN_TYPE_HVM)
>          libxl__xs_mkdir(gc, t,
>              libxl__sprintf(gc, "%s/hvmloader/generation-id-address", dom_path),



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v1 04/12] libxl: create a local xenstore libxl and device-model dir for guests

[Roger Pau Monné]

On 10/10/13 13:37, Ian Campbell wrote:
> On Wed, 2013-10-02 at 11:24 +0200, Roger Pau Monne wrote:
>> If libxl is executed inside a guest domain it needs write access to
>> the local libxl xenstore dir (/local/<domid>/libxl) to store internal
>> data. This also applies to Qemu which needs a
>> /local/<domid>/device-model xenstore directory.
>>
>> This patch creates the mentioned directories for each guest launched
>> from libxl.
> 
> Didn't we decide to make this conditional on building a driver domain?

I wasn't really sure about it, so I didn't make that change. I will add it.

>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> ---
>> Changes since RFC:
>>  * Add documentation for the new paths.
>> ---
>>  docs/misc/xenstore-paths.markdown |   10 ++++++++++
>>  tools/libxl/libxl_create.c        |   12 ++++++++++++
>>  2 files changed, 22 insertions(+), 0 deletions(-)
>>
>> diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
>> index 1c634b5..a8c4c58 100644
>> --- a/docs/misc/xenstore-paths.markdown
>> +++ b/docs/misc/xenstore-paths.markdown
>> @@ -318,6 +318,16 @@ protocol definition.
>>  
>>  A domain writable path. Available for arbitrary domain use.
>>  
>> +### Driver domain specific paths
>> +
>> +#### ~/device-model/$DOMID/state [w]
>> +
>> +Contains the status of the device models running on the domain.
>> +
>> +#### ~/libxl/$DOMID/qdisk-backend-pid [w]
> 
> Should both of the have BACKEND as a tag too?
> 
> And perhaps some tag to indicate that normal guest functions shouldn't
> use this, similar perhaps to INTERNAL?

Ack, this is inside the "Domain Controlled Paths" section, do you think
it would be best to place it on the "Backend Device Paths" section? Or
maybe create a new "Backend Device Paths" inside of "Domain Controlled
Paths".


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v1 04/12] libxl: create a local xenstore libxl and device-model dir for guests

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH v1 04/12] libxl: create a local xenstore libxl and device-model dir for guests"):
> On 10/10/13 13:37, Ian Campbell wrote:
> >> --- a/docs/misc/xenstore-paths.markdown
...
> >> +#### ~/libxl/$DOMID/qdisk-backend-pid [w]
> > 
> > Should both of the have BACKEND as a tag too?
> > 
> > And perhaps some tag to indicate that normal guest functions shouldn't
> > use this, similar perhaps to INTERNAL?
> 
> Ack, this is inside the "Domain Controlled Paths" section, do you think
> it would be best to place it on the "Backend Device Paths" section? Or
> maybe create a new "Backend Device Paths" inside of "Domain Controlled
> Paths".

I think perhaps a new section for "Paths private to the toolstack" or
something ?

I don't think classifying it under "backend device paths" is really
helpful - those are much more public, being used both by the backend
and the corresponding frontend.

Ian.

Re: [PATCH v1 04/12] libxl: create a local xenstore libxl and device-model dir for guests

[Ian Campbell]

On Wed, 2013-10-30 at 11:53 +0000, Ian Jackson wrote:
> Roger Pau Monné writes ("Re: [PATCH v1 04/12] libxl: create a local xenstore libxl and device-model dir for guests"):
> > On 10/10/13 13:37, Ian Campbell wrote:
> > >> --- a/docs/misc/xenstore-paths.markdown
> ...
> > >> +#### ~/libxl/$DOMID/qdisk-backend-pid [w]
> > > 
> > > Should both of the have BACKEND as a tag too?
> > > 
> > > And perhaps some tag to indicate that normal guest functions shouldn't
> > > use this, similar perhaps to INTERNAL?
> > 
> > Ack, this is inside the "Domain Controlled Paths" section, do you think
> > it would be best to place it on the "Backend Device Paths" section? Or
> > maybe create a new "Backend Device Paths" inside of "Domain Controlled
> > Paths".
> 
> I think perhaps a new section for "Paths private to the toolstack" or
> something ?

Sounds good to me.

> I don't think classifying it under "backend device paths" is really
> helpful - those are much more public, being used both by the backend
> and the corresponding frontend.

Ack.

Ian.

[PATCH v1 05/12] libxl: don't remove device frontend path from driver domains

[Roger Pau Monne]

A domain different than LIBXL_TOOLSTACK_DOMID should not try to remove
the frontend paths of a device.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
Changes since RFC:
 * Toolstack domain is in charge of cleaning both the frontend and the
   backend paths, driver domain only removes the first directory of
   the backend path.
 * I have decided to not take IanC Ack on this patch because it has
   changed since RFC.
---
 tools/libxl/libxl_device.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 48acc92..d726f0b 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -562,6 +562,10 @@ int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
     const char *tapdisk_params;
     xs_transaction_t t = 0;
     int rc;
+    uint32_t domid;
+
+    rc = libxl__get_domid(gc, &domid);
+    if (rc) goto out;
 
     for (;;) {
         rc = libxl__xs_transaction_start(gc, &t);
@@ -571,8 +575,20 @@ int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
         rc = libxl__xs_read_checked(gc, t, tapdisk_path, &tapdisk_params);
         if (rc) goto out;
 
-        libxl__xs_path_cleanup(gc, t, fe_path);
-        libxl__xs_path_cleanup(gc, t, be_path);
+        if (domid == LIBXL_TOOLSTACK_DOMID) {
+            /*
+             * The toolstack domain is in charge for removing both the
+             * frontend and the backend path
+             */
+            libxl__xs_path_cleanup(gc, t, fe_path);
+            libxl__xs_path_cleanup(gc, t, be_path);
+        } else if (dev->backend_domid == domid) {
+            /*
+             * The driver domain is in charge for removing what it can
+             * from the backend path
+             */
+            libxl__xs_path_cleanup(gc, t, be_path);
+        }
 
         rc = libxl__xs_transaction_commit(gc, &t);
         if (!rc) break;
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v1 05/12] libxl: don't remove device frontend path from driver domains

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v1 05/12] libxl: don't remove device frontend path from driver domains"):
> A domain different than LIBXL_TOOLSTACK_DOMID should not try to remove
> the frontend paths of a device.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> ---
> Changes since RFC:
>  * Toolstack domain is in charge of cleaning both the frontend and the
>    backend paths, driver domain only removes the first directory of
>    the backend path.
>  * I have decided to not take IanC Ack on this patch because it has
>    changed since RFC.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH v1 06/12] libxl: synchronize device removal when using driver domains

[Roger Pau Monne]

Synchronize the clean up of the backend from the toolstack domain when
the driver domain has actually finished closing the backend for the
device.

This is accomplished by waiting for the driver domain to  remove the
directory containing the backend keys, then the toolstack domain will
finish the cleanup by removing the empty folders on the backend path.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
Changes since RFC:
 * This patch has been reworked to synchronize the toolstack and the
   driver domain, by making the driver domain only remove the first
   directory of the backend path, and the toolstack domain remove the
   rest.
---
 tools/libxl/libxl_device.c   |   82 ++++++++++++++++++++++++++++++++++++++++-
 tools/libxl/libxl_internal.h |    2 +
 2 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index d726f0b..fbab0d5 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -722,6 +722,14 @@ static void device_hotplug_child_death_cb(libxl__egc *egc,
                                           libxl__ev_child *child,
                                           pid_t pid, int status);
 
+static void device_destroy_be_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
+                                         const struct timeval *requested_abs);
+
+static void device_destroy_be_watch_cb(libxl__egc *egc,
+                                       libxl__ev_xswatch *watch,
+                                       const char *watch_path,
+                                       const char *event_path);
+
 static void device_hotplug_done(libxl__egc *egc, libxl__ao_device *aodev);
 
 static void device_hotplug_clean(libxl__gc *gc, libxl__ao_device *aodev);
@@ -932,8 +940,32 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
      */
     rc = libxl__get_domid(gc, &domid);
     if (rc) goto out;
-    if (aodev->dev->backend_domid != domid)
-        goto out;
+    if (aodev->dev->backend_domid != domid) {
+        if (aodev->action != LIBXL__DEVICE_ACTION_REMOVE)
+            goto out;
+
+        /* Wait for the driver domain to remove the backend path */
+        libxl__ev_time_init(&aodev->timeout);
+        libxl__ev_xswatch_init(&aodev->xs_watch);
+
+        rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
+                                         device_destroy_be_timeout_cb,
+                                         LIBXL_DESTROY_TIMEOUT * 1000);
+        if (rc) {
+            LOG(ERROR, "setup of xs watch timeout failed");
+            goto out;
+        }
+
+        rc = libxl__ev_xswatch_register(gc, &aodev->xs_watch,
+                                        device_destroy_be_watch_cb,
+                                        be_path);
+        if (rc) {
+            LOG(ERROR, "setup of xs watch for %s failed", be_path);
+            libxl__ev_time_deregister(gc, &aodev->timeout);
+            goto out;
+        }
+        return;
+    }
 
     /* Check if we have to execute hotplug scripts for this device
      * and return the necessary args/env vars for execution */
@@ -1051,6 +1083,52 @@ error:
     device_hotplug_done(egc, aodev);
 }
 
+static void device_destroy_be_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
+                                         const struct timeval *requested_abs)
+{
+    libxl__ao_device *aodev = CONTAINER_OF(ev, *aodev, timeout);
+    STATE_AO_GC(aodev->ao);
+
+    libxl__ev_time_deregister(gc, &aodev->timeout);
+    libxl__ev_xswatch_deregister(gc, &aodev->xs_watch);
+
+    LOG(ERROR, "timed out while waiting for %s to be removed",
+               libxl__device_backend_path(gc, aodev->dev));
+
+    aodev->rc = ERROR_TIMEDOUT;
+
+    device_hotplug_done(egc, aodev);
+    return;
+}
+
+static void device_destroy_be_watch_cb(libxl__egc *egc,
+                                       libxl__ev_xswatch *watch,
+                                       const char *watch_path,
+                                       const char *event_path)
+{
+    libxl__ao_device *aodev = CONTAINER_OF(watch, *aodev, xs_watch);
+    STATE_AO_GC(aodev->ao);
+    const char *dir;
+    int rc;
+
+    rc = libxl__xs_read_checked(gc, XBT_NULL, watch_path, &dir);
+    if (rc) {
+        LOG(ERROR, "unable to read backend path: %s", watch_path);
+        aodev->rc = rc;
+        goto out;
+    }
+    if (dir) {
+        /* backend path still exists, wait a little longer... */
+        return;
+    }
+
+out:
+    /* We are done, backend path no longer exists */
+    libxl__ev_time_deregister(gc, &aodev->timeout);
+    libxl__ev_xswatch_deregister(gc, &aodev->xs_watch);
+    device_hotplug_done(egc, aodev);
+}
+
 static void device_hotplug_done(libxl__egc *egc, libxl__ao_device *aodev)
 {
     STATE_AO_GC(aodev->ao);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 3b74726..7b86fb4 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1882,6 +1882,8 @@ struct libxl__ao_device {
     libxl__ev_devstate backend_ds;
     /* Bodge for Qemu devices, also used for timeout of hotplug execution */
     libxl__ev_time timeout;
+    /* xenstore watch for backend path of driver domains */
+    libxl__ev_xswatch xs_watch;
     /* device hotplug execution */
     const char *what;
     int num_exec;
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v1 06/12] libxl: synchronize device removal when using driver domains

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v1 06/12] libxl: synchronize device removal when using driver domains"):
> Synchronize the clean up of the backend from the toolstack domain when
> the driver domain has actually finished closing the backend for the
> device.
> 
> This is accomplished by waiting for the driver domain to  remove the
> directory containing the backend keys, then the toolstack domain will
> finish the cleanup by removing the empty folders on the backend path.
...

This is broadly speaking good.  I have one suggestion:

> +static void device_destroy_be_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
> +                                         const struct timeval *requested_abs)
> +{
...
> +    libxl__ev_time_deregister(gc, &aodev->timeout);
> +    libxl__ev_xswatch_deregister(gc, &aodev->xs_watch);
...
> +    device_hotplug_done(egc, aodev);

You might consider moving those two idemopotent cleanup calls into
device_hotplug_done, which is on all the outward paths from here.
That reduces the number of times you have to write them from two to
one.

Ian.

[PATCH v1 07/12] libxl: remove the Qemu bodge for driver domain devices

[Roger Pau Monne]

When Qemu is launched from a driver domain to act as a PV disk
backend we can make sure that Qemu is running before detaching
devices, so there's no need for the bodge there.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_device.c |   71 ++++++++++++++++++++++++++++++++-----------
 1 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index fbab0d5..0404f8d 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -780,30 +780,38 @@ void libxl__initiate_device_remove(libxl__egc *egc,
     char *online_path = GCSPRINTF("%s/online", be_path);
     const char *state;
     libxl_dominfo info;
-    uint32_t domid = aodev->dev->domid;
+    uint32_t my_domid, domid = aodev->dev->domid;
     int rc = 0;
 
-    libxl_dominfo_init(&info);
-    rc = libxl_domain_info(CTX, &info, domid);
+    rc = libxl__get_domid(gc, &my_domid);
     if (rc) {
-        LOG(ERROR, "unable to get info for domain %d", domid);
+        LOG(ERROR, "unable to get my domid");
         goto out;
     }
-    if (QEMU_BACKEND(aodev->dev) &&
-        (info.paused || info.dying || info.shutdown)) {
-        /*
-         * TODO: 4.2 Bodge due to QEMU, see comment on top of
-         * libxl__initiate_device_remove in libxl_internal.h
-         */
-        rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
-                                         device_qemu_timeout,
-                                         LIBXL_QEMU_BODGE_TIMEOUT * 1000);
+
+    if (my_domid == LIBXL_TOOLSTACK_DOMID) {
+        libxl_dominfo_init(&info);
+        rc = libxl_domain_info(CTX, &info, domid);
         if (rc) {
-            LOG(ERROR, "unable to register timeout for Qemu device %s",
-                       be_path);
+            LOG(ERROR, "unable to get info for domain %d", domid);
             goto out;
         }
-        return;
+        if (QEMU_BACKEND(aodev->dev) &&
+            (info.paused || info.dying || info.shutdown)) {
+            /*
+             * TODO: 4.2 Bodge due to QEMU, see comment on top of
+             * libxl__initiate_device_remove in libxl_internal.h
+             */
+            rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
+                                             device_qemu_timeout,
+                                             LIBXL_QEMU_BODGE_TIMEOUT * 1000);
+            if (rc) {
+                LOG(ERROR, "unable to register timeout for Qemu device %s",
+                           be_path);
+                goto out;
+            }
+            return;
+        }
     }
 
     for (;;) {
@@ -872,19 +880,46 @@ static void device_qemu_timeout(libxl__egc *egc, libxl__ev_time *ev,
     STATE_AO_GC(aodev->ao);
     char *be_path = libxl__device_backend_path(gc, aodev->dev);
     char *state_path = GCSPRINTF("%s/state", be_path);
+    const char *xs_state;
+    xs_transaction_t t = 0;
     int rc = 0;
 
     libxl__ev_time_deregister(gc, &aodev->timeout);
 
-    rc = libxl__xs_write_checked(gc, XBT_NULL, state_path, "6");
-    if (rc) goto out;
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &t);
+        if (rc) {
+            LOG(ERROR, "unable to start transaction");
+            goto out;
+        }
+
+        /*
+         * Check that the state path exists and is actually different than
+         * 6 before unconditionally setting it. If Qemu runs on a driver
+         * domain it is possible that the driver domain has already cleaned
+         * the backend path if the device has reached state 6.
+         */
+        rc = libxl__xs_read_checked(gc, XBT_NULL, state_path, &xs_state);
+        if (rc) goto out;
+
+        if (xs_state && atoi(xs_state) != XenbusStateClosed) {
+            rc = libxl__xs_write_checked(gc, XBT_NULL, state_path, "6");
+            if (rc) goto out;
+        }
+
+        rc = libxl__xs_transaction_commit(gc, &t);
+        if (!rc) break;
+        if (rc < 0) goto out;
+    }
 
     device_hotplug(egc, aodev);
     return;
 
 out:
+    libxl__xs_transaction_abort(gc, &t);
     aodev->rc = rc;
     device_hotplug_done(egc, aodev);
+    return;
 }
 
 static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v1 07/12] libxl: remove the Qemu bodge for driver domain devices

[Andrew Cooper]

On 02/10/13 10:24, Roger Pau Monne wrote:
> When Qemu is launched from a driver domain to act as a PV disk
> backend we can make sure that Qemu is running before detaching
> devices, so there's no need for the bodge there.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxl/libxl_device.c |   71 ++++++++++++++++++++++++++++++++-----------
>  1 files changed, 53 insertions(+), 18 deletions(-)
>
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index fbab0d5..0404f8d 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -780,30 +780,38 @@ void libxl__initiate_device_remove(libxl__egc *egc,
>      char *online_path = GCSPRINTF("%s/online", be_path);
>      const char *state;
>      libxl_dominfo info;
> -    uint32_t domid = aodev->dev->domid;
> +    uint32_t my_domid, domid = aodev->dev->domid;
>      int rc = 0;
>  
> -    libxl_dominfo_init(&info);
> -    rc = libxl_domain_info(CTX, &info, domid);
> +    rc = libxl__get_domid(gc, &my_domid);
>      if (rc) {
> -        LOG(ERROR, "unable to get info for domain %d", domid);
> +        LOG(ERROR, "unable to get my domid");
>          goto out;
>      }
> -    if (QEMU_BACKEND(aodev->dev) &&
> -        (info.paused || info.dying || info.shutdown)) {
> -        /*
> -         * TODO: 4.2 Bodge due to QEMU, see comment on top of
> -         * libxl__initiate_device_remove in libxl_internal.h
> -         */
> -        rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
> -                                         device_qemu_timeout,
> -                                         LIBXL_QEMU_BODGE_TIMEOUT * 1000);
> +
> +    if (my_domid == LIBXL_TOOLSTACK_DOMID) {
> +        libxl_dominfo_init(&info);
> +        rc = libxl_domain_info(CTX, &info, domid);
>          if (rc) {
> -            LOG(ERROR, "unable to register timeout for Qemu device %s",
> -                       be_path);
> +            LOG(ERROR, "unable to get info for domain %d", domid);
>              goto out;
>          }
> -        return;
> +        if (QEMU_BACKEND(aodev->dev) &&
> +            (info.paused || info.dying || info.shutdown)) {
> +            /*
> +             * TODO: 4.2 Bodge due to QEMU, see comment on top of
> +             * libxl__initiate_device_remove in libxl_internal.h
> +             */
> +            rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
> +                                             device_qemu_timeout,
> +                                             LIBXL_QEMU_BODGE_TIMEOUT * 1000);
> +            if (rc) {
> +                LOG(ERROR, "unable to register timeout for Qemu device %s",
> +                           be_path);
> +                goto out;
> +            }
> +            return;
> +        }
>      }
>  
>      for (;;) {
> @@ -872,19 +880,46 @@ static void device_qemu_timeout(libxl__egc *egc, libxl__ev_time *ev,
>      STATE_AO_GC(aodev->ao);
>      char *be_path = libxl__device_backend_path(gc, aodev->dev);
>      char *state_path = GCSPRINTF("%s/state", be_path);
> +    const char *xs_state;
> +    xs_transaction_t t = 0;
>      int rc = 0;
>  
>      libxl__ev_time_deregister(gc, &aodev->timeout);
>  
> -    rc = libxl__xs_write_checked(gc, XBT_NULL, state_path, "6");
> -    if (rc) goto out;
> +    for (;;) {
> +        rc = libxl__xs_transaction_start(gc, &t);
> +        if (rc) {
> +            LOG(ERROR, "unable to start transaction");
> +            goto out;
> +        }
> +
> +        /*
> +         * Check that the state path exists and is actually different than
> +         * 6 before unconditionally setting it. If Qemu runs on a driver
> +         * domain it is possible that the driver domain has already cleaned
> +         * the backend path if the device has reached state 6.
> +         */
> +        rc = libxl__xs_read_checked(gc, XBT_NULL, state_path, &xs_state);
> +        if (rc) goto out;
> +
> +        if (xs_state && atoi(xs_state) != XenbusStateClosed) {
> +            rc = libxl__xs_write_checked(gc, XBT_NULL, state_path, "6");
> +            if (rc) goto out;
> +        }
> +
> +        rc = libxl__xs_transaction_commit(gc, &t);
> +        if (!rc) break;
> +        if (rc < 0) goto out;
> +    }
>  
>      device_hotplug(egc, aodev);
>      return;
>  
>  out:
> +    libxl__xs_transaction_abort(gc, &t);
>      aodev->rc = rc;
>      device_hotplug_done(egc, aodev);
> +    return;

This is a void function.  The return here is completely superfluous.

~Andrew

>  }
>  
>  static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v1 08/12] libxl: don't launch Qemu on Dom0 for Qdisk devices on driver domains

[Roger Pau Monne]

In libxl__need_xenpv_qemu check that the backend domain of the Qdisk
device is Dom0 before launching a Qemu instance in the toolstack
domain.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
Changes since RFC:
 * Fetch and use domid instead of LIBXL_TOOLSTACK_DOMID (altough the
   function is only called on Dom0 right now).
---
 tools/libxl/libxl_dm.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 4035b6d..7c0e3ce 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1342,6 +1342,7 @@ int libxl__need_xenpv_qemu(libxl__gc *gc,
         int nr_disks, libxl_device_disk *disks)
 {
     int i, ret = 0;
+    uint32_t domid;
 
     /*
      * qemu is required in order to support 2 or more consoles. So switch all
@@ -1367,8 +1368,11 @@ int libxl__need_xenpv_qemu(libxl__gc *gc,
     }
 
     if (nr_disks > 0) {
+        ret = libxl__get_domid(gc, &domid);
+        if (ret) goto out;
         for (i = 0; i < nr_disks; i++) {
-            if (disks[i].backend == LIBXL_DISK_BACKEND_QDISK) {
+            if (disks[i].backend == LIBXL_DISK_BACKEND_QDISK &&
+                disks[i].backend_domid == domid) {
                 ret = 1;
                 goto out;
             }
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v1 09/12] libxl: add Qdisk backend launch helper

[Roger Pau Monne]

Current Qemu launch functions in libxl require the usage of data
structures only avaialbe on domain creation. All this information is
not need in order to launch a Qemu instance to serve Qdisk backends,
so introduce a new simplified helper that can be used to launch
Qemu/Qdisk, that will be used to launch Qdisk in driver domains.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl_dm.c       |  149 +++++++++++++++++++++++++++++++++++++++---
 tools/libxl/libxl_internal.h |   17 +++++
 2 files changed, 157 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 7c0e3ce..04e1ed7 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1308,18 +1308,123 @@ static void device_model_spawn_outcome(libxl__egc *egc,
     dmss->callback(egc, dmss, rc);
 }
 
-int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
+/* callbacks passed in libxl__spawn_spawn for Qdisk attach */
+static void qdisk_confirm(libxl__egc *egc, libxl__spawn_state *spawn,
+                          const char *xsdata);
+static void qdisk_startup_failed(libxl__egc *egc,
+                                 libxl__spawn_state *spawn);
+static void qdisk_detached(libxl__egc *egc,
+                           libxl__spawn_state *spawn);
+
+void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__spawn_qdisk_state *sqs)
 {
-    char *pid;
-    int ret;
+    STATE_AO_GC(sqs->spawn.ao);
+    flexarray_t *dm_args;
+    char **args;
+    char *logfile = NULL;
+    const char *dm;
+    int logfile_w, null, rc;
+    uint32_t domid = sqs->guest_domid;
+
+    /* Always use qemu-xen as device model */
+    dm = qemu_xen_path(gc);
+
+    dm_args = flexarray_make(gc, 15, 1);
+    flexarray_vappend(dm_args, dm, "-xen-domid",
+                      GCSPRINTF("%d", domid), NULL);
+    flexarray_append(dm_args, "-xen-attach");
+    flexarray_vappend(dm_args, "-name",
+                      GCSPRINTF("domain-%u", domid), NULL);
+    flexarray_append(dm_args, "-nographic");
+    flexarray_vappend(dm_args, "-M", "xenpv", NULL);
+    flexarray_vappend(dm_args, "-monitor", "/dev/null", NULL);
+    flexarray_vappend(dm_args, "-serial", "/dev/null", NULL);
+    flexarray_vappend(dm_args, "-parallel", "/dev/null", NULL);
+    flexarray_append(dm_args, NULL);
+    args = (char **) flexarray_contents(dm_args);
 
-    pid = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "/local/domain/%d/image/device-model-pid", domid));
-    if (!pid || !atoi(pid)) {
-        LOG(ERROR, "could not find device-model's pid for dom %u", domid);
-        ret = ERROR_FAIL;
+    libxl_create_logfile(CTX, GCSPRINTF("qdisk-%u", domid), &logfile);
+    logfile_w = open(logfile, O_WRONLY|O_CREAT|O_APPEND, 0644);
+    if (logfile_w < 0) {
+        free(logfile);
+        rc = logfile_w;
+        goto error;
+    }
+    free(logfile);
+    null = open("/dev/null", O_RDONLY);
+
+    sqs->spawn.what = GCSPRINTF("domain %u Qdisk backend", domid);
+    sqs->spawn.xspath = GCSPRINTF("device-model/%u/state", domid);
+    sqs->spawn.timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000;
+    /*
+     * We cannot save Qemu pid anywhere in the xenstore guest dir,
+     * because we will call this from unprivileged driver domains,
+     * so save it in the current domain libxl private dir.
+     */
+    sqs->spawn.pidpath = GCSPRINTF("libxl/%u/qdisk-backend-pid", domid);
+    sqs->spawn.midproc_cb = libxl__spawn_record_pid;
+    sqs->spawn.confirm_cb = qdisk_confirm;
+    sqs->spawn.failure_cb = qdisk_startup_failed;
+    sqs->spawn.detached_cb = qdisk_detached;
+    rc = libxl__spawn_spawn(egc, &sqs->spawn);
+    if (rc < 0)
+        goto error;
+    if (!rc) { /* inner child */
+        setsid();
+        libxl__exec(gc, null, logfile_w, logfile_w, dm, args, NULL);
+    }
+
+    return;
+
+error:
+    assert(rc);
+    sqs->callback(egc, sqs, rc);
+    return;
+}
+
+static void qdisk_confirm(libxl__egc *egc, libxl__spawn_state *spawn,
+                          const char *xsdata)
+{
+    STATE_AO_GC(spawn->ao);
+
+    if (!xsdata)
+        return;
+
+    if (strcmp(xsdata, "running"))
+        return;
+
+    libxl__spawn_initiate_detach(gc, spawn);
+}
+
+static void qdisk_startup_failed(libxl__egc *egc,
+                                 libxl__spawn_state *spawn)
+{
+    libxl__spawn_qdisk_state *sqs = CONTAINER_OF(spawn, *sqs, spawn);
+    sqs->callback(egc, sqs, ERROR_FAIL);
+}
+
+static void qdisk_detached(libxl__egc *egc,
+                           libxl__spawn_state *spawn)
+{
+    libxl__spawn_qdisk_state *sqs = CONTAINER_OF(spawn, *sqs, spawn);
+    sqs->callback(egc, sqs, 0);
+}
+
+/* Generic function to signal a Qemu instance to exit */
+static int kill_device_model(libxl__gc *gc, const char *xs_path_pid)
+{
+    const char *xs_pid;
+    int ret, pid;
+
+    ret = libxl__xs_read_checked(gc, XBT_NULL, xs_path_pid, &xs_pid);
+    if (ret || !xs_pid) {
+        LOG(ERROR, "unable to find device model pid in %s", xs_path_pid);
+        ret = ret ? : ERROR_FAIL;
         goto out;
     }
-    ret = kill(atoi(pid), SIGHUP);
+    pid = atoi(xs_pid);
+
+    ret = kill(pid, SIGHUP);
     if (ret < 0 && errno == ESRCH) {
         LOG(ERROR, "Device Model already exited");
         ret = 0;
@@ -1327,7 +1432,7 @@ int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
         LOG(DEBUG, "Device Model signaled");
         ret = 0;
     } else {
-        LOGE(ERROR, "failed to kill Device Model [%d]", atoi(pid));
+        LOGE(ERROR, "failed to kill Device Model [%d]", pid);
         ret = ERROR_FAIL;
         goto out;
     }
@@ -1336,6 +1441,32 @@ out:
     return ret;
 }
 
+/* Helper to destroy a Qdisk backend */
+int libxl__destroy_qdisk_backend(libxl__gc *gc, uint32_t domid)
+{
+    char *pid_path;
+    int rc;
+
+    pid_path = GCSPRINTF("libxl/%u/qdisk-backend-pid", domid);
+
+    rc = kill_device_model(gc, pid_path);
+    if (rc)
+        goto out;
+
+    libxl__xs_rm_checked(gc, XBT_NULL, pid_path);
+    libxl__xs_rm_checked(gc, XBT_NULL,
+                         GCSPRINTF("device-model/%u", domid));
+
+out:
+    return rc;
+}
+
+int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
+{
+    return kill_device_model(gc,
+                GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
+}
+
 int libxl__need_xenpv_qemu(libxl__gc *gc,
         int nr_consoles, libxl__device_console *consoles,
         int nr_vfbs, libxl_device_vfb *vfbs,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 7b86fb4..c1f4e30 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2502,6 +2502,23 @@ _hidden void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state*);
 
 _hidden char *libxl__stub_dm_name(libxl__gc *gc, const char * guest_name);
 
+/* Qdisk backend launch helpers */
+
+typedef struct libxl__spawn_qdisk_state libxl__spawn_qdisk_state;
+
+typedef void libxl__qdisk_spawn_cb(libxl__egc *egc, libxl__spawn_qdisk_state*,
+                                   int rc /* if !0, error was logged */);
+
+struct libxl__spawn_qdisk_state {
+    uint32_t guest_domid;
+    libxl__spawn_state spawn;
+    libxl__qdisk_spawn_cb *callback;
+};
+
+_hidden void libxl__spawn_qdisk_backend(libxl__egc *egc,
+                                        libxl__spawn_qdisk_state *sqs);
+_hidden int libxl__destroy_qdisk_backend(libxl__gc *gc, uint32_t domid);
+
 /*----- Domain creation -----*/
 
 typedef struct libxl__domain_create_state libxl__domain_create_state;
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v1 10/12] xl: put daemonize code in it's own function

[Roger Pau Monne]

Move the daemonizer code from create_domain into it's own function
that can be called from other places different than create_domain.
This will be used to daemonize the driver domain backend handler.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |  102 ++++++++++++++++++++++++++--------------------
 1 files changed, 58 insertions(+), 44 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 884f050..553ba70 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -370,6 +370,58 @@ out:
     if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-1); }
 }
 
+static int do_daemonize(char *name)
+{
+    char *fullname;
+    pid_t child1, got_child;
+    int nullfd, ret = 0;
+    int status = 0;
+
+    child1 = xl_fork(child_waitdaemon);
+    if (child1) {
+        printf("Daemon running with PID %d\n", child1);
+
+        for (;;) {
+            got_child = xl_waitpid(child_waitdaemon, &status, 0);
+            if (got_child == child1) break;
+            assert(got_child == -1);
+            perror("failed to wait for daemonizing child");
+            ret = ERROR_FAIL;
+            goto out;
+        }
+        if (status) {
+            libxl_report_child_exitstatus(ctx, XTL_ERROR,
+                       "daemonizing child", child1, status);
+            ret = ERROR_FAIL;
+            goto out;
+        }
+        ret = 1;
+        goto out;
+    }
+
+    postfork();
+
+    ret = libxl_create_logfile(ctx, name, &fullname);
+    if (ret) {
+        LOG("failed to open logfile %s: %s",fullname,strerror(errno));
+        exit(-1);
+    }
+
+    CHK_ERRNO(( logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND,
+                               0644) )<0);
+    free(fullname);
+
+    CHK_ERRNO(( nullfd = open("/dev/null", O_RDONLY) )<0);
+    dup2(nullfd, 0);
+    dup2(logfile, 1);
+    dup2(logfile, 2);
+
+    CHK_ERRNO(daemon(0, 1) < 0);
+
+out:
+    return ret;
+}
+
 static int parse_action_on_shutdown(const char *buf, libxl_action_on_shutdown *a)
 {
     int i;
@@ -1859,7 +1911,6 @@ static uint32_t create_domain(struct domain_create *dom_info)
     void *config_data = 0;
     int config_len = 0;
     int restore_fd = -1;
-    int status = 0;
     const libxl_asyncprogress_how *autoconnect_console_how;
     struct save_file_header hdr;
 
@@ -2091,55 +2142,18 @@ start:
         vncviewer(domid, vncautopass);
 
     if (need_daemon) {
-        char *fullname, *name;
-        pid_t child1, got_child;
-        int nullfd;
-
-        child1 = xl_fork(child_waitdaemon);
-        if (child1) {
-            printf("Daemon running with PID %d\n", child1);
-
-            for (;;) {
-                got_child = xl_waitpid(child_waitdaemon, &status, 0);
-                if (got_child == child1) break;
-                assert(got_child == -1);
-                perror("failed to wait for daemonizing child");
-                ret = ERROR_FAIL;
-                goto out;
-            }
-            if (status) {
-                libxl_report_child_exitstatus(ctx, XTL_ERROR,
-                           "daemonizing child", child1, status);
-                ret = ERROR_FAIL;
-                goto out;
-            }
-            ret = domid;
-            goto out;
-        }
-
-        postfork();
+        char *name;
 
         if (asprintf(&name, "xl-%s", d_config.c_info.name) < 0) {
             LOG("Failed to allocate memory in asprintf");
             exit(1);
         }
-        rc = libxl_create_logfile(ctx, name, &fullname);
-        if (rc) {
-            LOG("failed to open logfile %s: %s",fullname,strerror(errno));
-            exit(-1);
-        }
-
-        CHK_ERRNO(( logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND,
-                                   0644) )<0);
-        free(fullname);
+        ret = do_daemonize(name);
         free(name);
-
-        CHK_ERRNO(( nullfd = open("/dev/null", O_RDONLY) )<0);
-        dup2(nullfd, 0);
-        dup2(logfile, 1);
-        dup2(logfile, 2);
-
-        CHK_ERRNO(daemon(0, 1) < 0);
+        if (ret) {
+            ret = (ret == 1) ? domid : ret;
+            goto out;
+        }
         need_daemon = 0;
     }
     LOG("Waiting for domain %s (domid %d) to die [pid %ld]",
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v1 11/12] libxl: revert 326a7b74

[Roger Pau Monne]

When running libxl from a driver domain there's no xenstore pid file
(because xenstore is not running on the driver domain). Also, at that
point in libxl initialization there's no way to know wether libxl is
running on a domain different than Dom0, so just revert the change in
order to allow libxl to work on driver domains.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.c          |    7 -------
 tools/libxl/libxl_internal.h |    1 -
 2 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 5417b48..3337518 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -25,7 +25,6 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
                     unsigned flags, xentoollog_logger * lg)
 {
     libxl_ctx *ctx = NULL;
-    struct stat stat_buf;
     int rc;
 
     if (version != LIBXL_VERSION) { rc = ERROR_VERSION; goto out; }
@@ -82,12 +81,6 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
     rc = libxl__poller_init(ctx, &ctx->poller_app);
     if (rc) goto out;
 
-    if ( stat(XENSTORE_PID_FILE, &stat_buf) != 0 ) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Is xenstore daemon running?\n"
-                     "failed to stat %s", XENSTORE_PID_FILE);
-        rc = ERROR_FAIL; goto out;
-    }
-
     ctx->xch = xc_interface_open(lg,lg,0);
     if (!ctx->xch) {
         LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, errno,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c1f4e30..462cebd 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -378,7 +378,6 @@ typedef struct {
 #define PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
 #define PCI_FUNC(devfn)         ((devfn) & 0x07)
 #define AUTO_PHP_SLOT          0x100
-#define XENSTORE_PID_FILE      "/var/run/xenstored.pid"
 
 #define PROC_PCI_NUM_RESOURCES 7
 #define PCI_BAR_IO             0x01
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Roger Pau Monne]

Add the necessary logic in libxl to allow it to act as a listener for
launching backends in a driver domain, replacing udev (like we already
do on Dom0). This new functionality is acomplished by watching the
domain backend path (/local/domain/<domid>/backend) and reacting to
device creation/destruction.

The way to launch this listener daemon is from xl, using the newly
introduced "devd" command. The command will daemonize by default,
using "xldevd.log" as it's logfile. Optionally the user can force the
execution of the listener in the foreground by passing the "-F"
option to the devd command.

Current backends handled by this daemon include Qdisk, vbd and vif
device types.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
In order to use Qdisk on a driver domain the following patch has to be
applied to Qemu:

http://marc.info/?l=qemu-devel&m=137953390608727&w=2
---
I'm quite sure memory management is not done correctly, after each
call to backend_watch_callback the garbage collector should free all
the references, but I think this is not happening, and I would
like someone with experience on libxl memory management (IanJ) to
comment on possible ways to deal with that.
---
 tools/libxl/libxl.c       |  309 +++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl.h       |    8 ++
 tools/libxl/xl.h          |    1 +
 tools/libxl/xl_cmdimpl.c  |   24 ++++
 tools/libxl/xl_cmdtable.c |    6 +
 5 files changed, 348 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 3337518..dd9cab6 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3517,6 +3517,315 @@ DEFINE_DEVICE_ADD(vtpm)
 
 /******************************************************************************/
 
+/*
+ * Data structures used to track devices handled by driver domains
+ */
+
+/*
+ * Structure that describes a device handled by a driver domain
+ */
+typedef struct libxl__ddomain_device {
+    libxl__device *dev;
+    LIBXL_SLIST_ENTRY(struct libxl__ddomain_device) next;
+} libxl__ddomain_device;
+
+/*
+ * Structure that describes a domain and it's associated devices
+ */
+typedef struct libxl__ddomain_guest {
+    uint32_t domid;
+    int num_vifs, num_vbds, num_qdisks;
+    LIBXL_SLIST_HEAD(, struct libxl__ddomain_device) devices;
+    LIBXL_SLIST_ENTRY(struct libxl__ddomain_guest) next;
+} libxl__ddomain_guest;
+
+/*
+ * Main structure used by a driver domain to keep track of devices
+ * currently in use
+ */
+typedef struct {
+    libxl__ao *ao;
+    libxl__ev_xswatch watch;
+    LIBXL_SLIST_HEAD(, struct libxl__ddomain_guest) guests;
+} libxl__ddomain;
+
+static libxl__ddomain_guest *search_for_guest(libxl__ddomain *ddomain,
+                                               uint32_t domid)
+{
+    libxl__ddomain_guest *dguest;
+
+    LIBXL_SLIST_FOREACH(dguest, &ddomain->guests, next) {
+        if (dguest->domid == domid)
+            return dguest;
+    }
+    return NULL;
+}
+
+static libxl__ddomain_device *search_for_device(libxl__ddomain_guest *dguest,
+                                                libxl__device *dev)
+{
+    libxl__ddomain_device *ddev;
+
+    LIBXL_SLIST_FOREACH(ddev, &dguest->devices, next) {
+        if (memcmp(ddev->dev, dev, sizeof(*dev)) == 0)
+            return ddev;
+    }
+    return NULL;
+}
+
+static void device_complete(libxl__egc *egc, libxl__ao_device *aodev)
+{
+    STATE_AO_GC(aodev->ao);
+
+    if (aodev->action == LIBXL__DEVICE_ACTION_REMOVE)
+        free(aodev->dev);
+
+    LOG(DEBUG, "device %s %s %s",
+               libxl__device_backend_path(gc, aodev->dev),
+               libxl__device_action_to_string(aodev->action),
+               aodev->rc ? "failed" : "succeed");
+
+    return;
+}
+
+static void qdisk_spawn_outcome(libxl__egc *egc, libxl__spawn_qdisk_state *sqs,
+                                int rc)
+{
+    STATE_AO_GC(sqs->spawn.ao);
+
+    LOG(DEBUG, "qdisk backend spawn for domain %u %s", sqs->guest_domid,
+               rc ? "failed" : "succeed");
+}
+
+static int add_device(libxl__egc *egc, libxl__ddomain *ddomain,
+                      libxl__ddomain_guest *dguest,
+                      libxl__ddomain_device *ddev)
+{
+    STATE_AO_GC(ddomain->ao);
+    libxl__device *dev = ddev->dev;
+    libxl__ao_device *aodev;
+    libxl__spawn_qdisk_state *sqs;
+
+    switch(dev->backend_kind) {
+    case LIBXL__DEVICE_KIND_VBD:
+    case LIBXL__DEVICE_KIND_VIF:
+        if (dev->backend_kind == LIBXL__DEVICE_KIND_VBD) dguest->num_vbds++;
+        if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) dguest->num_vifs++;
+
+        GCNEW(aodev);
+        libxl__prepare_ao_device(ao, aodev);
+        aodev->dev = dev;
+        aodev->action = LIBXL__DEVICE_ACTION_ADD;
+        aodev->callback = device_complete;
+        libxl__wait_device_connection(egc, aodev);
+
+        break;
+    case LIBXL__DEVICE_KIND_QDISK:
+        if (dguest->num_qdisks == 0) {
+            GCNEW(sqs);
+            sqs->guest_domid = dev->domid;
+            sqs->spawn.ao = ao;
+            sqs->callback = qdisk_spawn_outcome;
+
+            libxl__spawn_qdisk_backend(egc, sqs);
+        }
+        dguest->num_qdisks++;
+
+        break;
+    default:
+        break;
+    }
+
+    return 0;
+}
+
+static int remove_device(libxl__egc *egc, libxl__ddomain *ddomain,
+                         libxl__ddomain_guest *dguest,
+                         libxl__ddomain_device *ddev)
+{
+    STATE_AO_GC(ddomain->ao);
+    libxl__device *dev = ddev->dev;
+    libxl__ao_device *aodev;
+    int rc;
+
+    switch(ddev->dev->backend_kind) {
+    case LIBXL__DEVICE_KIND_VBD:
+    case LIBXL__DEVICE_KIND_VIF:
+        if (dev->backend_kind == LIBXL__DEVICE_KIND_VBD) dguest->num_vbds--;
+        if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) dguest->num_vifs--;
+
+        GCNEW(aodev);
+        libxl__prepare_ao_device(ao, aodev);
+        aodev->dev = dev;
+        aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
+        aodev->callback = device_complete;
+        libxl__initiate_device_remove(egc, aodev);
+        break;
+    case LIBXL__DEVICE_KIND_QDISK:
+        if (--dguest->num_qdisks == 0) {
+            rc = libxl__destroy_qdisk_backend(gc, dev->domid);
+            if (rc)
+                goto out;
+        }
+        libxl__device_destroy(gc, dev);
+        free(dev);
+        break;
+    default:
+        break;
+    }
+
+out:
+    return rc;
+}
+
+static void backend_watch_callback(libxl__egc *egc, libxl__ev_xswatch *watch,
+                                   const char *watch_path,
+                                   const char *event_path)
+{
+    libxl__ddomain *ddomain = CONTAINER_OF(watch, *ddomain, watch);
+    STATE_AO_GC(ddomain->ao);
+    char *p, *path;
+    const char *sstate;
+    int state, rc, num_devs;
+    libxl__device *dev = NULL;
+    libxl__ddomain_device *ddev = NULL;
+    libxl__ddomain_guest *dguest = NULL;
+
+    /* Check if event_path ends with "state" and truncate it */
+    if (strlen(event_path) < strlen("state"))
+        goto skip;
+
+    path = libxl__strdup(gc, event_path);
+    p = path + strlen(path) - strlen("state") - 1;
+    if (*p != '/')
+        goto skip;
+    *p = '\0';
+    p++;
+    if (strcmp(p, "state") != 0)
+        goto skip;
+
+    /* Check if the state is 1 (XenbusStateInitialising) or greater */
+    rc = libxl__xs_read_checked(gc, XBT_NULL, event_path, &sstate);
+    if (rc || !sstate)
+        goto skip;
+    state = atoi(sstate);
+
+    dev = libxl__zalloc(NOGC, sizeof(*dev));
+    rc = libxl__parse_backend_path(gc, path, dev);
+    if (rc)
+        goto skip;
+
+    dguest = search_for_guest(ddomain, dev->domid);
+    if (dguest == NULL && state == XenbusStateClosed) {
+        /*
+         * Spurious state change, device has already been disconnected
+         * or never attached.
+         */
+        goto skip;
+    }
+    if (dguest == NULL) {
+        /* Create a new guest struct and initialize it */
+        dguest = libxl__zalloc(NOGC, sizeof(*dguest));
+        dguest->domid = dev->domid;
+        LIBXL_SLIST_INIT(&dguest->devices);
+        LIBXL_SLIST_INSERT_HEAD(&ddomain->guests, dguest, next);
+        LOG(DEBUG, "added domain %u to the list of active guests",
+                   dguest->domid);
+    }
+    ddev = search_for_device(dguest, dev);
+    if (ddev == NULL && state == XenbusStateClosed) {
+        /*
+         * Spurious state change, device has already been disconnected
+         * or never attached.
+         */
+        goto skip;
+    } else if (ddev == NULL) {
+        /*
+         * New device addition, allocate a struct to hold it and add it
+         * to the list of active devices for a given guest.
+         */
+        ddev = libxl__zalloc(NOGC, sizeof(*ddev));
+        ddev->dev = dev;
+        LIBXL_SLIST_INSERT_HEAD(&dguest->devices, ddev, next);
+        LOG(DEBUG, "added device %s to the list of active devices", path);
+        add_device(egc, ddomain, dguest, ddev);
+    } else if (state == XenbusStateClosed) {
+        /*
+         * Removal of an active device, remove it from the list and
+         * free it's data structures if they are no longer needed.
+         *
+         * The free of the associated libxl__device is left to the
+         * helper remove_device function.
+         */
+        LIBXL_SLIST_REMOVE(&dguest->devices, ddev, libxl__ddomain_device,
+                           next);
+        LOG(DEBUG, "removed device %s from the list of active devices", path);
+        remove_device(egc, ddomain, dguest, ddev);
+
+        free(ddev);
+        /* If this was the last device in the domain, remove it from the list */
+        num_devs = dguest->num_vifs + dguest->num_vbds + dguest->num_qdisks;
+        if (num_devs == 0) {
+            LIBXL_SLIST_REMOVE(&ddomain->guests, dguest, libxl__ddomain_guest,
+                               next);
+            LOG(DEBUG, "removed domain %u from the list of active guests",
+                       dguest->domid);
+            /* Clear any leftovers in libxl/<domid> */
+            libxl__xs_rm_checked(gc, XBT_NULL,
+                                 GCSPRINTF("libxl/%u", dguest->domid));
+            free(dguest);
+        }
+    }
+
+    return;
+
+skip:
+    free(dev);
+    free(ddev);
+    free(dguest);
+    return;
+}
+
+/* Handler of events for device driver domains */
+int libxl_device_events_handler(libxl_ctx *ctx,
+                                const libxl_asyncop_how *ao_how)
+{
+    AO_CREATE(ctx, 0, ao_how);
+    int rc;
+    uint32_t domid;
+    libxl__ddomain ddomain;
+    char *be_path;
+
+    ddomain.ao = ao;
+    LIBXL_SLIST_INIT(&ddomain.guests);
+
+    rc = libxl__get_domid(gc, &domid);
+    if (rc) {
+        LOG(ERROR, "unable to get domain id");
+        goto out;
+    }
+
+    rc = libxl__xs_write_checked(gc, XBT_NULL, DISABLE_UDEV_PATH, "1");
+    if (rc) {
+        LOGE(ERROR, "unable to write %s = 1", DISABLE_UDEV_PATH);
+        goto out;
+    }
+
+    /*
+     * We use absolute paths because we want xswatch to also return
+     * absolute paths that can be parsed by libxl__parse_backend_path.
+     */
+    be_path = GCSPRINTF("/local/domain/%u/backend", domid);
+    rc = libxl__ev_xswatch_register(gc, &ddomain.watch, backend_watch_callback,
+                                    be_path);
+
+out:
+    GC_FREE;
+    return rc ? : AO_INPROGRESS;
+}
+
+/******************************************************************************/
+
 int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t max_memkb)
 {
     GC_INIT(ctx);
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index be19bf5..933bb94 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -867,6 +867,14 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid,
                                         int *num);
 
 /*
+ * Function that drives the main loop that checks for device actions and
+ * launches the callbacks passed by the user.
+ */
+int libxl_device_events_handler(libxl_ctx *ctx,
+                                const libxl_asyncop_how *ao_how)
+                                LIBXL_EXTERNAL_CALLERS_ONLY;
+
+/*
  * Functions related to making devices assignable -- that is, bound to
  * the pciback driver, ready to be given to a guest via
  * libxl_pci_device_add.
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index e72a7d2..2dfcf19 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -105,6 +105,7 @@ int main_getenforce(int argc, char **argv);
 int main_setenforce(int argc, char **argv);
 int main_loadpolicy(int argc, char **argv);
 int main_remus(int argc, char **argv);
+int main_devd(int argc, char **argv);
 
 void help(const char *command);
 
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 553ba70..cebfe3a 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7158,6 +7158,30 @@ int main_remus(int argc, char **argv)
     return -ERROR_FAIL;
 }
 
+int main_devd(int argc, char **argv)
+{
+    int ret = 0, opt = 0, daemonize = 1;
+
+    SWITCH_FOREACH_OPT(opt, "F", NULL, "devd", 0) {
+    case 'F':
+        daemonize = 0;
+        break;
+    }
+
+    if (daemonize) {
+        ret = do_daemonize("xldevd");
+        if (ret) {
+            ret = (ret == 1) ? 0 : ret;
+            goto out;
+        }
+    }
+
+    libxl_device_events_handler(ctx, 0);
+
+out:
+    return ret;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 326a660..7709206 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -488,6 +488,12 @@ struct cmd_spec cmd_table[] = {
       "                        of the domain."
 
     },
+    { "devd",
+      &main_devd, 0, 1,
+      "Daemon that listens for devices and launches backends",
+      "[options]",
+      "-F                      Run in the foreground",
+    },
 };
 
 int cmdtable_len = sizeof(cmd_table)/sizeof(struct cmd_spec);
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v1 12/12] libxl: add device backend listener in order to launch backends"):
> Add the necessary logic in libxl to allow it to act as a listener for
> launching backends in a driver domain, replacing udev (like we already
> do on Dom0). This new functionality is acomplished by watching the
> domain backend path (/local/domain/<domid>/backend) and reacting to
> device creation/destruction.
> 
> The way to launch this listener daemon is from xl, using the newly
> introduced "devd" command. The command will daemonize by default,
> using "xldevd.log" as it's logfile. Optionally the user can force the
> execution of the listener in the foreground by passing the "-F"
> option to the devd command.
> 
> Current backends handled by this daemon include Qdisk, vbd and vif
> device types.
...
> I'm quite sure memory management is not done correctly, after each
> call to backend_watch_callback the garbage collector should free all
> the references, but I think this is not happening, and I would
> like someone with experience on libxl memory management (IanJ) to
> comment on possible ways to deal with that.

We discussed this on IRC:

11:57 <Diziet> You seem to be using the ao gc everywhere.  I think if you're 
               going to make a never-ending ao you need to do something more 
               complicated.  The ao gc's allocations aren't freed until the ao 
               completes, which of course will never happen here.
11:58 <Diziet> I would create a fresh gc out of whole cloth and free it 
               explicitly when you have finished processing the event.
11:59 <Diziet> Well when I say whole cloth, I mean using LIBXL_INIT_GC or 
               something.
11:59 <Diziet> I think you may need to make a kind of fake sub-ao.
12:00 <Diziet> For the benefit of libxl_blah_device_blah(ao) etc.
12:00 <Diziet> Does this make any kind of sense ?  If not I can sketch it out 
               or something.
12:01 <royger> Diziet: can I actually have two different GC? I mean, ideally I 
               would like to allocate a GC at the start of each iteration, and 
               free it when we have finished processing the loop
12:12 <Diziet> Yes, you can do that.
12:12 <Diziet> Of course it'll get a bit more complex because there will be a 
               function (your watch event function) where you have two gcs and 
               have to use the right one each time.
12:13 <Diziet> Thinking about it I think we should have something like    
               libxl__nested_ao_create   and   libxl_nested_ao_free   which 
               take an existing ao* and give you a new ao* with its own gc.

If you want me to write libxl__nested_ao_create for you I could do
that.


> +    LIBXL_SLIST_FOREACH(ddev, &dguest->devices, next) {
> +        if (memcmp(ddev->dev, dev, sizeof(*dev)) == 0)
> +            return ddev;

I'm afraid that you can't memcmp a struct like this.  structs are
allowed to have padding which may contain junk.

> +static void device_complete(libxl__egc *egc, libxl__ao_device *aodev)
> +{
> +    STATE_AO_GC(aodev->ao);
> +
> +    if (aodev->action == LIBXL__DEVICE_ACTION_REMOVE)
> +        free(aodev->dev);
> +
> +    LOG(DEBUG, "device %s %s %s",
> +               libxl__device_backend_path(gc, aodev->dev),
> +               libxl__device_action_to_string(aodev->action),
> +               aodev->rc ? "failed" : "succeed");

AFAICT there is nothing here reporting success or failure to the
initiator in the toolstack domain.  So you'll say "xl block-attach"
and it will appear to work but in fact there's an error in a logfile
in the driver domain.

At the very least this deserves something in the documentation.

> +    case LIBXL__DEVICE_KIND_VBD:
> +    case LIBXL__DEVICE_KIND_VIF:
> +        if (dev->backend_kind == LIBXL__DEVICE_KIND_VBD) dguest->num_vbds--;
> +        if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) dguest->num_vifs--;

Is it really safe to decrement these already ?  What if something else
comes along in the meantime and makes num_devs 0 (below) and removes
everything while this operation is still running and liable to be
reentered on completion ?

>  /*
> + * Function that drives the main loop that checks for device actions and
> + * launches the callbacks passed by the user.
> + */

I think this comment is confusing.  I would say:

   /*
    * Turns the current process into a backend device service daemon
    * for a driver domain.
    *
    * From a libxl API point of view, this starts a long-running
    * operation.  That operation consists of "being a driver domain"
    * and never completes.
    */

or something.  I might rename it to have "driver domain" or "backend"
in it somewhere.


Thanks,
Ian.

Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Roger Pau Monné]

On 30/10/13 18:00, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v1 12/12] libxl: add device backend listener in order to launch backends"):
>> Add the necessary logic in libxl to allow it to act as a listener for
>> launching backends in a driver domain, replacing udev (like we already
>> do on Dom0). This new functionality is acomplished by watching the
>> domain backend path (/local/domain/<domid>/backend) and reacting to
>> device creation/destruction.
>>
>> The way to launch this listener daemon is from xl, using the newly
>> introduced "devd" command. The command will daemonize by default,
>> using "xldevd.log" as it's logfile. Optionally the user can force the
>> execution of the listener in the foreground by passing the "-F"
>> option to the devd command.
>>
>> Current backends handled by this daemon include Qdisk, vbd and vif
>> device types.
> ...
>> I'm quite sure memory management is not done correctly, after each
>> call to backend_watch_callback the garbage collector should free all
>> the references, but I think this is not happening, and I would
>> like someone with experience on libxl memory management (IanJ) to
>> comment on possible ways to deal with that.
> 
> We discussed this on IRC:
> 
> 11:57 <Diziet> You seem to be using the ao gc everywhere.  I think if you're 
>                going to make a never-ending ao you need to do something more 
>                complicated.  The ao gc's allocations aren't freed until the ao 
>                completes, which of course will never happen here.
> 11:58 <Diziet> I would create a fresh gc out of whole cloth and free it 
>                explicitly when you have finished processing the event.
> 11:59 <Diziet> Well when I say whole cloth, I mean using LIBXL_INIT_GC or 
>                something.
> 11:59 <Diziet> I think you may need to make a kind of fake sub-ao.
> 12:00 <Diziet> For the benefit of libxl_blah_device_blah(ao) etc.
> 12:00 <Diziet> Does this make any kind of sense ?  If not I can sketch it out 
>                or something.
> 12:01 <royger> Diziet: can I actually have two different GC? I mean, ideally I 
>                would like to allocate a GC at the start of each iteration, and 
>                free it when we have finished processing the loop
> 12:12 <Diziet> Yes, you can do that.
> 12:12 <Diziet> Of course it'll get a bit more complex because there will be a 
>                function (your watch event function) where you have two gcs and 
>                have to use the right one each time.
> 12:13 <Diziet> Thinking about it I think we should have something like    
>                libxl__nested_ao_create   and   libxl_nested_ao_free   which 
>                take an existing ao* and give you a new ao* with its own gc.
> 
> If you want me to write libxl__nested_ao_create for you I could do
> that.

So if I got it right, this new libxl__nested_ao_create will return a new
ao (with a new gc), that I could use in conjunction with the
long-running ao that I use in the main xs_watch loop, right?

That sounds like a good solution to my problem, I wouldn't mind if you
write that :)

I'm wondering if there are also other memory problems, even when using
this approach, for example I register a xswatch callback, and the
callback gets called with a watch_path and an event_path arguments, does
the internal libxl event handler machinery reuse those (or allocate and
free them after each loop)?

>> +    LIBXL_SLIST_FOREACH(ddev, &dguest->devices, next) {
>> +        if (memcmp(ddev->dev, dev, sizeof(*dev)) == 0)
>> +            return ddev;
> 
> I'm afraid that you can't memcmp a struct like this.  structs are
> allowed to have padding which may contain junk.

Yes, will replace this in next version.

>> +static void device_complete(libxl__egc *egc, libxl__ao_device *aodev)
>> +{
>> +    STATE_AO_GC(aodev->ao);
>> +
>> +    if (aodev->action == LIBXL__DEVICE_ACTION_REMOVE)
>> +        free(aodev->dev);
>> +
>> +    LOG(DEBUG, "device %s %s %s",
>> +               libxl__device_backend_path(gc, aodev->dev),
>> +               libxl__device_action_to_string(aodev->action),
>> +               aodev->rc ? "failed" : "succeed");
> 
> AFAICT there is nothing here reporting success or failure to the
> initiator in the toolstack domain.  So you'll say "xl block-attach"
> and it will appear to work but in fact there's an error in a logfile
> in the driver domain.
> 
> At the very least this deserves something in the documentation.

Ack.

> 
>> +    case LIBXL__DEVICE_KIND_VBD:
>> +    case LIBXL__DEVICE_KIND_VIF:
>> +        if (dev->backend_kind == LIBXL__DEVICE_KIND_VBD) dguest->num_vbds--;
>> +        if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) dguest->num_vifs--;
> 
> Is it really safe to decrement these already ?  What if something else
> comes along in the meantime and makes num_devs 0 (below) and removes
> everything while this operation is still running and liable to be
> reentered on completion ?

That's the point of decrementing it here, so that we get to 0 (if this
is the last device), and remove the libxl__ddomain_guest and
libxl__ddomain_device. Then, when the remove AO finishes, the AO
callback will take care of removing the associated libxl__device.

I thought backend_watch_callback could not be called concurrently, but
maybe that's not true? (and if that's the case ignore everything above
because it's completely wrong)

> 
>>  /*
>> + * Function that drives the main loop that checks for device actions and
>> + * launches the callbacks passed by the user.
>> + */
> 
> I think this comment is confusing.  I would say:
> 
>    /*
>     * Turns the current process into a backend device service daemon
>     * for a driver domain.
>     *
>     * From a libxl API point of view, this starts a long-running
>     * operation.  That operation consists of "being a driver domain"
>     * and never completes.
>     */
> 
> or something.  I might rename it to have "driver domain" or "backend"
> in it somewhere.

Ack, will try to come up with a more "representative" name.

Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends"):
> So if I got it right, this new libxl__nested_ao_create will return a new
> ao (with a new gc), that I could use in conjunction with the
> long-running ao that I use in the main xs_watch loop, right?

Yes.  It would give you a new psuedo-ao, which you can use for
per-event memory allocation.  It's a psuedo-ao in the sense that you
mustn't call libxl__ao_abort or libxl__ao_complete on it, but it would
have the right type and in particular you could stuff it in
sub-operations' ao fields, call STATE_AO_GC on it and so on.  I could
make it possible to call libxl__ao_inprogress and have that reflected
to the underlyhing real ao.

> That sounds like a good solution to my problem, I wouldn't mind if you
> write that :)

OK, watch this space.

> I'm wondering if there are also other memory problems, even when using
> this approach, for example I register a xswatch callback, and the
> callback gets called with a watch_path and an event_path arguments, does
> the internal libxl event handler machinery reuse those (or allocate and
> free them after each loop)?

The event machinery gets those from a different gc which is
per-system-event, so that's not a problem.  (Otherwise waiting for a a
particular thing in xenstore would involve memory growing endlessly
with calls to read from xenstore, ec.)

> >> +    case LIBXL__DEVICE_KIND_VBD:
> >> +    case LIBXL__DEVICE_KIND_VIF:
> >> +        if (dev->backend_kind == LIBXL__DEVICE_KIND_VBD) dguest->num_vbds--;
> >> +        if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) dguest->num_vifs--;
> > 
> > Is it really safe to decrement these already ?  What if something else
> > comes along in the meantime and makes num_devs 0 (below) and removes
> > everything while this operation is still running and liable to be
> > reentered on completion ?
> 
> That's the point of decrementing it here, so that we get to 0 (if this
> is the last device), and remove the libxl__ddomain_guest and
> libxl__ddomain_device. Then, when the remove AO finishes, the AO
> callback will take care of removing the associated libxl__device.
> 
> I thought backend_watch_callback could not be called concurrently, but
> maybe that's not true? (and if that's the case ignore everything above
> because it's completely wrong)

While you are _actually in this function_, you hold the Big Lock.  So
nothing else can come along find the wrong value of num_*.

But what you actually do is call initiate_device_remove and then
return - ie, you return to the event loop.  That gives up the lock,
obviously.  So while the device removal is proceeding, other events
can occur.

If backend_watch_callback happens then, I think you may find that it
seems num_*==0 and decides to tear down the state for that domain.
That would at the very least involve messing about in xenstore with
the device which is still being removed.

Then, later, the device removal will complete and device_complete will
be called.

I think you need to do the decrement in device_complete, and that
means you need a kind of "perhaps tidy up domain" function which you
can call from both there and backend_watch_callback.  And you probably
need to provide some more useful pointers to device_complete.

Thanks,
Ian.

Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends"):
>  It would give you a new psuedo-ao, which you can use for
> per-event memory allocation.  It's a psuedo-ao in the sense that you
> mustn't call libxl__ao_abort or libxl__ao_complete on it, but it would
> have the right type and in particular you could stuff it in
> sub-operations' ao fields, call STATE_AO_GC on it and so on.  I could
> make it possible to call libxl__ao_inprogress and have that reflected
> to the underlyhing real ao.
> 
> > That sounds like a good solution to my problem, I wouldn't mind if you
> > write that :)
> 
> OK, watch this space.

Something like this perhaps.  I've compiled it but I don't have a
caller to test it with.

Ian.

>From be117b96d0072a995ffa77ee1a763a3bdc98b982 Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Mon, 4 Nov 2013 17:56:15 +0000
Subject: [PATCH] libxl: Introduce nested async operations (nested ao)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This allows a long-running ao to avoid accumulating memory.  Each
nested ao has its own gc.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_event.c    |   35 +++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |   24 +++++++++++++++++++++++-
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 0fe4428..54d15db 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1572,6 +1572,7 @@ void libxl__ao_complete(libxl__egc *egc, libxl__ao *ao, int rc)
     LOG(DEBUG,"ao %p: complete, rc=%d",ao,rc);
     assert(ao->magic == LIBXL__AO_MAGIC);
     assert(!ao->complete);
+    assert(!ao->nested);
     ao->complete = 1;
     ao->rc = rc;
 
@@ -1736,6 +1737,7 @@ void libxl__ao_progress_report(libxl__egc *egc, libxl__ao *ao,
         const libxl_asyncprogress_how *how, libxl_event *ev)
 {
     AO_GC;
+    assert(!ao->nested);
     if (how->callback == dummy_asyncprogress_callback_ignore) {
         LOG(DEBUG,"ao %p: progress report: ignored",ao);
         libxl_event_free(CTX,ev);
@@ -1756,6 +1758,39 @@ void libxl__ao_progress_report(libxl__egc *egc, libxl__ao *ao,
 }
 
 
+/* nested ao */
+
+_hidden libxl__ao *libxl__nested_ao_create(libxl__ao *parent)
+{
+    /* We only use the parent to get the ctx.  However, we require the
+     * caller to provide us with an ao, not just a ctx, to prove that
+     * they are already in an asynchronous operation.  That will avoid
+     * people using this to (for example) make an ao in a non-ao_how
+     * function somewhere in the middle of libxl. */
+    libxl__ao *child = NULL;
+    libxl_ctx *ctx = libxl__gc_owner(&parent->gc);
+
+    assert(parent->magic == LIBXL__AO_MAGIC);
+
+    child = libxl__zalloc(&ctx->nogc_gc, sizeof(*child));
+    child->magic = LIBXL__AO_MAGIC;
+    child->nested = 1;
+    LIBXL_INIT_GC(child->gc, ctx);
+    libxl__gc *gc = &child->gc;
+
+    LOG(DEBUG,"ao %p: nested ao, parent %p", child, parent);
+    return child;
+}
+
+_hidden void libxl__nested_ao_free(libxl__ao *child)
+{
+    assert(child->magic == LIBXL__AO_MAGIC);
+    assert(child->nested);
+    libxl_ctx *ctx = libxl__gc_owner(&child->gc);
+    libxl__ao__destroy(ctx, child);
+}
+
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 4f92522..e6a19ec 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -426,7 +426,7 @@ struct libxl__ao {
      * only in libxl__ao_complete.)
      */
     uint32_t magic;
-    unsigned constructing:1, in_initiator:1, complete:1, notified:1;
+    unsigned constructing:1, in_initiator:1, complete:1, notified:1, nested:1;
     int progress_reports_outstanding;
     int rc;
     libxl__gc gc;
@@ -1777,6 +1777,28 @@ _hidden void libxl__ao_complete_check_progress_reports(libxl__egc*, libxl__ao*);
 
 
 /*
+ * Short-lived sub-ao, aka "nested ao".
+ *
+ * Some asynchronous operations are very long-running.  Generally,
+ * since an ao has a gc, any allocations made in that ao will live
+ * until the ao is completed.  When this is not desirable, these
+ * functions may be used to manage a "sub-ao".
+ *
+ * The returned sub-ao is suitable for passing to gc-related functions
+ * and macros such as libxl__ao_inprogress_gc, AO_GC, and STATE_AO_GC.
+ *
+ * It MUST NOT be used with AO_INPROGRESS, AO_ABORT,
+ * libxl__ao_complete, libxl__ao_progress_report, and so on.
+ *
+ * The caller must ensure that all of the sub-ao's are freed before
+ * the parent is.
+ */
+
+_hidden libxl__ao *libxl__nested_ao_create(libxl__ao *parent); /* cannot fail */
+_hidden void libxl__nested_ao_free(libxl__ao *child);
+
+
+/*
  * File descriptors and CLOEXEC
  */
 
-- 
1.7.10.4

Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Roger Pau Monné]

On 04/11/13 18:59, Ian Jackson wrote:
> Ian Jackson writes ("Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends"):
>>  It would give you a new psuedo-ao, which you can use for
>> per-event memory allocation.  It's a psuedo-ao in the sense that you
>> mustn't call libxl__ao_abort or libxl__ao_complete on it, but it would
>> have the right type and in particular you could stuff it in
>> sub-operations' ao fields, call STATE_AO_GC on it and so on.  I could
>> make it possible to call libxl__ao_inprogress and have that reflected
>> to the underlyhing real ao.
>>
>>> That sounds like a good solution to my problem, I wouldn't mind if you
>>> write that :)
>>
>> OK, watch this space.
> 
> Something like this perhaps.  I've compiled it but I don't have a
> caller to test it with.
> 
> Ian.
> 
> From be117b96d0072a995ffa77ee1a763a3bdc98b982 Mon Sep 17 00:00:00 2001
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> Date: Mon, 4 Nov 2013 17:56:15 +0000
> Subject: [PATCH] libxl: Introduce nested async operations (nested ao)
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> This allows a long-running ao to avoid accumulating memory.  Each
> nested ao has its own gc.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Tested-by: Roger Pau Monné <roger.pau@citrix.com>

The interface looks fine to me, maybe other potential users of this
interface have comments, but since it's an internal interface we can
always expand it. I've added this patch to my series, I'm not sure if
you prefer to commit it separately.

Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends"):
> On 04/11/13 18:59, Ian Jackson wrote:
> > Subject: [PATCH] libxl: Introduce nested async operations (nested ao)
...
> > This allows a long-running ao to avoid accumulating memory.  Each
> > nested ao has its own gc.
> > 
> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > Cc: Roger Pau Monné <roger.pau@citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> Tested-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> The interface looks fine to me, maybe other potential users of this
> interface have comments, but since it's an internal interface we can
> always expand it. I've added this patch to my series, I'm not sure if
> you prefer to commit it separately.

I think this might help Shriram's work on Remus too.  I'll wait for a
common from Shriram or Ian C before committing it.

Ian.

Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Ian Campbell]

On Mon, 2013-11-04 at 17:59 +0000, Ian Jackson wrote:
> Ian Jackson writes ("Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends"):
> >  It would give you a new psuedo-ao, which you can use for
> > per-event memory allocation.  It's a psuedo-ao in the sense that you
> > mustn't call libxl__ao_abort or libxl__ao_complete on it, but it would
> > have the right type and in particular you could stuff it in
> > sub-operations' ao fields, call STATE_AO_GC on it and so on.  I could
> > make it possible to call libxl__ao_inprogress and have that reflected
> > to the underlyhing real ao.
> > 
> > > That sounds like a good solution to my problem, I wouldn't mind if you
> > > write that :)
> > 
> > OK, watch this space.
> 
> Something like this perhaps.  I've compiled it but I don't have a
> caller to test it with.
> 
> Ian.
> 
> From be117b96d0072a995ffa77ee1a763a3bdc98b982 Mon Sep 17 00:00:00 2001
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> Date: Mon, 4 Nov 2013 17:56:15 +0000
> Subject: [PATCH] libxl: Introduce nested async operations (nested ao)
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> This allows a long-running ao to avoid accumulating memory.  Each
> nested ao has its own gc.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxl/libxl_event.c    |   35 +++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_internal.h |   24 +++++++++++++++++++++++-
>  2 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> index 0fe4428..54d15db 100644
> --- a/tools/libxl/libxl_event.c
> +++ b/tools/libxl/libxl_event.c
> @@ -1572,6 +1572,7 @@ void libxl__ao_complete(libxl__egc *egc, libxl__ao *ao, int rc)
>      LOG(DEBUG,"ao %p: complete, rc=%d",ao,rc);
>      assert(ao->magic == LIBXL__AO_MAGIC);
>      assert(!ao->complete);
> +    assert(!ao->nested);
>      ao->complete = 1;
>      ao->rc = rc;
>  
> @@ -1736,6 +1737,7 @@ void libxl__ao_progress_report(libxl__egc *egc, libxl__ao *ao,
>          const libxl_asyncprogress_how *how, libxl_event *ev)
>  {
>      AO_GC;
> +    assert(!ao->nested);
>      if (how->callback == dummy_asyncprogress_callback_ignore) {
>          LOG(DEBUG,"ao %p: progress report: ignored",ao);
>          libxl_event_free(CTX,ev);
> @@ -1756,6 +1758,39 @@ void libxl__ao_progress_report(libxl__egc *egc, libxl__ao *ao,
>  }
>  
> 
> +/* nested ao */
> +
> +_hidden libxl__ao *libxl__nested_ao_create(libxl__ao *parent)
> +{
> +    /* We only use the parent to get the ctx.  However, we require the
> +     * caller to provide us with an ao, not just a ctx, to prove that
> +     * they are already in an asynchronous operation.  That will avoid
> +     * people using this to (for example) make an ao in a non-ao_how
> +     * function somewhere in the middle of libxl. */
> +    libxl__ao *child = NULL;
> +    libxl_ctx *ctx = libxl__gc_owner(&parent->gc);
> +
> +    assert(parent->magic == LIBXL__AO_MAGIC);

Is the intention to allow multiple levels of nesting or would it be a
good idea to have an assert(!parent->nested) here?

In either case it would be good to be explicit in the comment, either
here or in the header.

> +
> +    child = libxl__zalloc(&ctx->nogc_gc, sizeof(*child));
> +    child->magic = LIBXL__AO_MAGIC;
> +    child->nested = 1;
> +    LIBXL_INIT_GC(child->gc, ctx);
> +    libxl__gc *gc = &child->gc;
> +
> +    LOG(DEBUG,"ao %p: nested ao, parent %p", child, parent);
> +    return child;
> +}
> +
> +_hidden void libxl__nested_ao_free(libxl__ao *child)
> +{
> +    assert(child->magic == LIBXL__AO_MAGIC);
> +    assert(child->nested);
> +    libxl_ctx *ctx = libxl__gc_owner(&child->gc);
> +    libxl__ao__destroy(ctx, child);
> +}
> +
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 4f92522..e6a19ec 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -426,7 +426,7 @@ struct libxl__ao {
>       * only in libxl__ao_complete.)
>       */
>      uint32_t magic;
> -    unsigned constructing:1, in_initiator:1, complete:1, notified:1;
> +    unsigned constructing:1, in_initiator:1, complete:1, notified:1, nested:1;
>      int progress_reports_outstanding;
>      int rc;
>      libxl__gc gc;
> @@ -1777,6 +1777,28 @@ _hidden void libxl__ao_complete_check_progress_reports(libxl__egc*, libxl__ao*);
>  
> 
>  /*
> + * Short-lived sub-ao, aka "nested ao".
> + *
> + * Some asynchronous operations are very long-running.  Generally,
> + * since an ao has a gc, any allocations made in that ao will live
> + * until the ao is completed.  When this is not desirable, these
> + * functions may be used to manage a "sub-ao".
> + *
> + * The returned sub-ao is suitable for passing to gc-related functions
> + * and macros such as libxl__ao_inprogress_gc, AO_GC, and STATE_AO_GC.
> + *
> + * It MUST NOT be used with AO_INPROGRESS, AO_ABORT,
> + * libxl__ao_complete, libxl__ao_progress_report, and so on.
> + *
> + * The caller must ensure that all of the sub-ao's are freed before
> + * the parent is.

OOI what causes this requirement?

> + */
> +
> +_hidden libxl__ao *libxl__nested_ao_create(libxl__ao *parent); /* cannot fail */
> +_hidden void libxl__nested_ao_free(libxl__ao *child);
> +
> +
> +/*
>   * File descriptors and CLOEXEC
>   */
>  



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends"):
> On Mon, 2013-11-04 at 17:59 +0000, Ian Jackson wrote:
> > +    assert(parent->magic == LIBXL__AO_MAGIC);
> 
> Is the intention to allow multiple levels of nesting or would it be a
> good idea to have an assert(!parent->nested) here?

I don't think there is anything wrong with multiple levels of nesting
although I'm hoping no-one will find a use for it!

> In either case it would be good to be explicit in the comment, either
> here or in the header.

Sure.

> >  /*
> > + * Short-lived sub-ao, aka "nested ao".
> > + *
> > + * Some asynchronous operations are very long-running.  Generally,
> > + * since an ao has a gc, any allocations made in that ao will live
> > + * until the ao is completed.  When this is not desirable, these
> > + * functions may be used to manage a "sub-ao".
> > + *
> > + * The returned sub-ao is suitable for passing to gc-related functions
> > + * and macros such as libxl__ao_inprogress_gc, AO_GC, and STATE_AO_GC.
> > + *
> > + * It MUST NOT be used with AO_INPROGRESS, AO_ABORT,
> > + * libxl__ao_complete, libxl__ao_progress_report, and so on.
> > + *
> > + * The caller must ensure that all of the sub-ao's are freed before
> > + * the parent is.
> 
> OOI what causes this requirement?

If there is no first-class ao running, then nothing might be running
the libxl event loop and the "escaped" sub-ao might never make
progress.

So the requirement is there to stop people writing a broken daemonic
sub-ao (ie, one which outlives its parent).  It's slightly stricter
than the actual requirement for correctness, which is that _some_ ao
must still continue.  But I'm hoping that no-one wants to have some
more complicated semantic relationship between a sub-ao and the
system's real ao's than parenthood.

Ian.

Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Shriram Rajagopalan]

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

>
>
> > >  /*
> > > + * Short-lived sub-ao, aka "nested ao".
> > > + *
> > > + * Some asynchronous operations are very long-running.  Generally,
> > > + * since an ao has a gc, any allocations made in that ao will live
> > > + * until the ao is completed.  When this is not desirable, these
> > > + * functions may be used to manage a "sub-ao".
> > > + *
>

Just to make sure I understand this whole thing fully:

This is targeted against long running AOs that have a lot of intermittent
"events",
each of which may cause more memory allocations. By creating throwaway-ao's
for these events, one can service these events asynchronously (if they are
long
running by themselves) while quickly reclaiming the memory after event
completion.

And this can be used in suspend/resume/restore code to create these
throwaway nested-aos
during each iteration, so as to avoid the possibility of accumulating
memory on the main gc.

[-- Attachment #1.2: Type: text/html, Size: 1453 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Ian Jackson]

Shriram Rajagopalan writes ("Re: [Xen-devel] [PATCH v1 12/12] libxl: add device backend listener in order to launch backends"):
>     > >  /*
>     > > + * Short-lived sub-ao, aka "nested ao".
>     > > + *
>     > > + * Some asynchronous operations are very long-running.  Generally,
>     > > + * since an ao has a gc, any allocations made in that ao will live
>     > > + * until the ao is completed.  When this is not desirable, these
>     > > + * functions may be used to manage a "sub-ao".
>     > > + *
> 
> Just to make sure I understand this whole thing fully:
> 
> This is targeted against long running AOs that have a lot of
> intermittent "events", each of which may cause more memory
> allocations. By creating throwaway-ao's for these events, one can
> service these events asynchronously (if they are long running by
> themselves) while quickly reclaiming the memory after event
> completion.

Yes.

> And this can be used in suspend/resume/restore code to create these
> throwaway nested-aos during each iteration, so as to avoid the
> possibility of accumulating memory on the main gc.

Yes.

Ian.

Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Shriram Rajagopalan]

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

On Fri, Nov 8, 2013 at 5:41 AM, Ian Jackson <Ian.Jackson@eu.citrix.com>wrote:

> Shriram Rajagopalan writes ("Re: [Xen-devel] [PATCH v1 12/12] libxl: add
> device backend listener in order to launch backends"):
> >     > >  /*
> >     > > + * Short-lived sub-ao, aka "nested ao".
> >     > > + *
> >     > > + * Some asynchronous operations are very long-running.
>  Generally,
> >     > > + * since an ao has a gc, any allocations made in that ao will
> live
> >     > > + * until the ao is completed.  When this is not desirable, these
> >     > > + * functions may be used to manage a "sub-ao".
> >     > > + *
> >
> > Just to make sure I understand this whole thing fully:
> >
> > This is targeted against long running AOs that have a lot of
> > intermittent "events", each of which may cause more memory
> > allocations. By creating throwaway-ao's for these events, one can
> > service these events asynchronously (if they are long running by
> > themselves) while quickly reclaiming the memory after event
> > completion.
>
> Yes.
>
> > And this can be used in suspend/resume/restore code to create these
> > throwaway nested-aos during each iteration, so as to avoid the
> > possibility of accumulating memory on the main gc.
>
> Yes.
>
> Ian.
>
>

Are these nested-ao patches in the tree yet ?

[-- Attachment #1.2: Type: text/html, Size: 1977 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Ian Campbell]

On Thu, 2013-11-07 at 16:35 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends"):
> > On Mon, 2013-11-04 at 17:59 +0000, Ian Jackson wrote:
> > > +    assert(parent->magic == LIBXL__AO_MAGIC);
> > 
> > Is the intention to allow multiple levels of nesting or would it be a
> > good idea to have an assert(!parent->nested) here?
> 
> I don't think there is anything wrong with multiple levels of nesting
> although I'm hoping no-one will find a use for it!

Indeed!

> > In either case it would be good to be explicit in the comment, either
> > here or in the header.
> 
> Sure.

OK. I trust you will write something sensible, so here is a preemptive:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Use it wisely ;-)

> [...]
> If there is no first-class ao running, then nothing might be running
> the libxl event loop and the "escaped" sub-ao might never make
> progress.
> 
> So the requirement is there to stop people writing a broken daemonic
> sub-ao (ie, one which outlives its parent).  It's slightly stricter
> than the actual requirement for correctness, which is that _some_ ao
> must still continue.  But I'm hoping that no-one wants to have some
> more complicated semantic relationship between a sub-ao and the
> system's real ao's than parenthood.

Makes sense.

Ian

Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Roger Pau Monné]

On 04/11/13 18:20, Ian Jackson wrote:
> Roger Pau Monné writes ("Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends"):
>> So if I got it right, this new libxl__nested_ao_create will return a new
>> ao (with a new gc), that I could use in conjunction with the
>> long-running ao that I use in the main xs_watch loop, right?
> 
> Yes.  It would give you a new psuedo-ao, which you can use for
> per-event memory allocation.  It's a psuedo-ao in the sense that you
> mustn't call libxl__ao_abort or libxl__ao_complete on it, but it would
> have the right type and in particular you could stuff it in
> sub-operations' ao fields, call STATE_AO_GC on it and so on.  I could
> make it possible to call libxl__ao_inprogress and have that reflected
> to the underlyhing real ao.
> 
>> That sounds like a good solution to my problem, I wouldn't mind if you
>> write that :)
> 
> OK, watch this space.
> 
>> I'm wondering if there are also other memory problems, even when using
>> this approach, for example I register a xswatch callback, and the
>> callback gets called with a watch_path and an event_path arguments, does
>> the internal libxl event handler machinery reuse those (or allocate and
>> free them after each loop)?
> 
> The event machinery gets those from a different gc which is
> per-system-event, so that's not a problem.  (Otherwise waiting for a a
> particular thing in xenstore would involve memory growing endlessly
> with calls to read from xenstore, ec.)
> 
>>>> +    case LIBXL__DEVICE_KIND_VBD:
>>>> +    case LIBXL__DEVICE_KIND_VIF:
>>>> +        if (dev->backend_kind == LIBXL__DEVICE_KIND_VBD) dguest->num_vbds--;
>>>> +        if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) dguest->num_vifs--;
>>>
>>> Is it really safe to decrement these already ?  What if something else
>>> comes along in the meantime and makes num_devs 0 (below) and removes
>>> everything while this operation is still running and liable to be
>>> reentered on completion ?
>>
>> That's the point of decrementing it here, so that we get to 0 (if this
>> is the last device), and remove the libxl__ddomain_guest and
>> libxl__ddomain_device. Then, when the remove AO finishes, the AO
>> callback will take care of removing the associated libxl__device.
>>
>> I thought backend_watch_callback could not be called concurrently, but
>> maybe that's not true? (and if that's the case ignore everything above
>> because it's completely wrong)
> 
> While you are _actually in this function_, you hold the Big Lock.  So
> nothing else can come along find the wrong value of num_*.
> 
> But what you actually do is call initiate_device_remove and then
> return - ie, you return to the event loop.  That gives up the lock,
> obviously.  So while the device removal is proceeding, other events
> can occur.
> 
> If backend_watch_callback happens then, I think you may find that it
> seems num_*==0 and decides to tear down the state for that domain.

The cleanup for the domain already happened, after we decrement num_* we
return to the main backend_watch_callback (all holding the Big Lock),
and libxl proceeds with the removal of the libxl__ddomain_guest if
sum(num_*) == 0.

What happens in backend_watch_callback (with the Big Lock hold) is
basically:

- Decrement num_*
- Check if sum(num_*) == 0 -> cleanup all data for the domain
- END

If another device is added to the domain after the domain has been
removed from the list (because sum(num_*) == 0), a new
libxl__ddomain_guest will be created, just like when a device for a new
domain is added.

> That would at the very least involve messing about in xenstore with
> the device which is still being removed.

Tearing down the domain just involves removing it's associated data
structures, nothing is written to xenstore.

> Then, later, the device removal will complete and device_complete will
> be called.

device_complete doesn't make use of either libxl__ddomain_device or
libxl__ddomain_guest, during normal program flow device_complete will be
called with both of the above data structures already freed.

> I think you need to do the decrement in device_complete, and that
> means you need a kind of "perhaps tidy up domain" function which you
> can call from both there and backend_watch_callback.  And you probably
> need to provide some more useful pointers to device_complete.

It's quite possible that I'm completely wrong, but I don't see a race
with the current program flow.

Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends"):
> On 04/11/13 18:20, Ian Jackson wrote:
> > But what you actually do is call initiate_device_remove and then
> > return - ie, you return to the event loop.  That gives up the lock,
> > obviously.  So while the device removal is proceeding, other events
> > can occur.
> > 
> > If backend_watch_callback happens then, I think you may find that it
> > seems num_*==0 and decides to tear down the state for that domain.
> 
> The cleanup for the domain already happened, after we decrement num_* we
> return to the main backend_watch_callback (all holding the Big Lock),
> and libxl proceeds with the removal of the libxl__ddomain_guest if
> sum(num_*) == 0.

Ah, yes.  So the teardown of the domain tracking structure happens
right away.  The device removal callback happens after libxl has
forgotten about the domain.

> device_complete doesn't make use of either libxl__ddomain_device or
> libxl__ddomain_guest, during normal program flow device_complete will be
> called with both of the above data structures already freed.

Yes.  Mind you, in the future, if we get better error handling, this
is going to have to be redone.

But you've convinced me it's OK for now.

Ian.

Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends

[Roger Pau Monné]

On 30/10/13 18:00, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v1 12/12] libxl: add device backend listener in order to launch backends"):
>> +    LIBXL_SLIST_FOREACH(ddev, &dguest->devices, next) {
>> +        if (memcmp(ddev->dev, dev, sizeof(*dev)) == 0)
>> +            return ddev;
> 
> I'm afraid that you can't memcmp a struct like this.  structs are
> allowed to have padding which may contain junk.

All the libxl__device structures have been allocated with libxl__zalloc,
which should make the padding be 0 in all cases, but I agree it would be
better to do it in a more formal way.