Xen devel - Aug 2011 - [PATCH] xenstored: allow guests to shutdown all of its watches using XS_RESET_WATCHES

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1313396962 -7200
# Node ID 4b483f4fa715566847b57f4c20908e5c3d38b5a2
# Parent  1f08b380d4386cdd6714786a9163e5f51aecab5d
xenstored: allow guests to shutdown all of its watches using XS_RESET_WATCHES

During kexec all old watches have to be removed, otherwise the new
kernel will receive unexpected events. Allow a guest to reset itself and
cleanup all of its watches.

Add a new XS_RESET_WATCHES command to do the reset on behalf of the guest.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r 1f08b380d438 -r 4b483f4fa715 docs/misc/xenstore.txt
--- a/docs/misc/xenstore.txt
+++ b/docs/misc/xenstore.txt
@@ -217,6 +217,9 @@ WATCH_EVENT					<epath>|<token>|
 
 UNWATCH			<wpath>|<token>|?
 
+RESET_WATCHES		<dummy>
+	Reset all watches and transactions of the caller.
+
 ---------- Transactions ----------
 
 TRANSACTION_START	|			<transid>|
diff -r 1f08b380d438 -r 4b483f4fa715 tools/xenstore/xenstored_core.c
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -120,6 +120,7 @@ static char *sockmsg_string(enum xsd_soc
 	case XS_IS_DOMAIN_INTRODUCED: return "XS_IS_DOMAIN_INTRODUCED";
 	case XS_RESUME: return "RESUME";
 	case XS_SET_TARGET: return "SET_TARGET";
+	case XS_RESET_WATCHES: return "RESET_WATCHES";
 	default:
 		return "**UNKNOWN**";
 	}
@@ -1242,6 +1243,10 @@ static void process_message(struct conne
 		do_set_target(conn, in);
 		break;
 
+	case XS_RESET_WATCHES:
+		do_reset_watches(conn);
+		break;
+
 	default:
 		eprintf("Client unknown operation %i", in->hdr.msg.type);
 		send_error(conn, ENOSYS);
diff -r 1f08b380d438 -r 4b483f4fa715 tools/xenstore/xenstored_domain.c
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -537,6 +537,20 @@ void do_is_domain_introduced(struct conn
 	send_reply(conn, XS_IS_DOMAIN_INTRODUCED, result ? "T" :
"F", 2);
 }
 
+/* Allow guest to reset all watches */
+void do_reset_watches(struct connection *conn)
+{
+	if (!conn->can_write) {
+		send_error(conn, EACCES);
+		return;
+	}
+
+	conn_delete_all_watches(conn);
+	conn_delete_all_transactions(conn);
+
+	send_ack(conn, XS_RESET_WATCHES);
+}
+
 static int close_xc_handle(void *_handle)
 {
 	xc_interface_close(*(xc_interface**)_handle);
diff -r 1f08b380d438 -r 4b483f4fa715 tools/xenstore/xenstored_domain.h
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -40,6 +40,9 @@ void do_set_target(struct connection *co
 /* domid */
 void do_get_domain_path(struct connection *conn, const char *domid_str);
 
+/* Allow guest to reset all watches */
+void do_reset_watches(struct connection *conn);
+
 void domain_init(void);
 
 /* Returns the implicit path of a connection (only domains have this) */
diff -r 1f08b380d438 -r 4b483f4fa715 xen/include/public/io/xs_wire.h
--- a/xen/include/public/io/xs_wire.h
+++ b/xen/include/public/io/xs_wire.h
@@ -48,7 +48,8 @@ enum xsd_sockmsg_type
     XS_IS_DOMAIN_INTRODUCED,
     XS_RESUME,
     XS_SET_TARGET,
-    XS_RESTRICT
+    XS_RESTRICT,
+    XS_RESET_WATCHES
 };
 
 #define XS_WRITE_NONE "NONE"

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

Add new xs_reset_watches function to shutdown watches from old kernel after
kexec boot.  The old kernel does not unregister all watches in the
shutdown path.  They are still active, the double registration can not
be detected by the new kernel.  When the watches fire, unexpected events
will arrive and the xenwatch thread will crash (jumps to NULL).  An
orderly reboot of a hvm guest will destroy the entire guest with all its
resources (including the watches) before it is rebuilt from scratch, so
the missing unregister is not an issue in that case.

With this change the xenstored is instructed to wipe all active watches
for the guest.  However, a patch for xenstored is required so that it
accepts the XS_RESET_WATCHES request from a guest.

v3:
  - use XS_RESET_WATCHES instead of XS_INTRODUCE

v2:
  - move all code which deals with XS_INTRODUCE into xs_introduce()
    (based on feedback from Ian Campbell)
  - remove casts from kvec assignment

Signed-off-by: Olaf Hering <olaf@aepfle.de>

---
 drivers/xen/xenbus/xenbus_xs.c     |   20 ++++++++++++++++++++
 include/xen/interface/io/xs_wire.h |    6 +++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

Index: linux-3.0/drivers/xen/xenbus/xenbus_xs.c
==================================================================---
linux-3.0.orig/drivers/xen/xenbus/xenbus_xs.c
+++ linux-3.0/drivers/xen/xenbus/xenbus_xs.c
@@ -620,6 +620,22 @@ static struct xenbus_watch *find_watch(c
 	return NULL;
 }
 
+static void xs_reset_watches(void)
+{
+	/* dummy, empty iov not handled by xenstored */
+	struct kvec iov[1];
+	int err;
+
+
+	iov[0].iov_base = "dummy";
+	iov[0].iov_len = strlen(iov[0].iov_base) + 1;
+
+	err = xs_error(xs_talkv(XBT_NIL, XS_RESET_WATCHES, iov,
+				 ARRAY_SIZE(iov), NULL));
+	if (err)
+		printk(KERN_WARNING "xs_reset_watches failed: %d\n", err);
+}
+
 /* Register callback to watch this node. */
 int register_xenbus_watch(struct xenbus_watch *watch)
 {
@@ -897,5 +913,9 @@ int xs_init(void)
 	if (IS_ERR(task))
 		return PTR_ERR(task);
 
+	/* shutdown watches for kexec boot */
+	if (xen_hvm_domain())
+		xs_reset_watches();
+
 	return 0;
 }
Index: linux-3.0/include/xen/interface/io/xs_wire.h
==================================================================---
linux-3.0.orig/include/xen/interface/io/xs_wire.h
+++ linux-3.0/include/xen/interface/io/xs_wire.h
@@ -26,7 +26,11 @@ enum xsd_sockmsg_type
     XS_SET_PERMS,
     XS_WATCH_EVENT,
     XS_ERROR,
-    XS_IS_DOMAIN_INTRODUCED
+    XS_IS_DOMAIN_INTRODUCED,
+    XS_RESUME,
+    XS_SET_TARGET,
+    XS_RESTRICT,
+    XS_RESET_WATCHES
 };
 
 #define XS_WRITE_NONE "NONE"

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

On Mon, Aug 15, 2011 at 11:25:30AM +0200, Olaf Hering
wrote:
> Add new xs_reset_watches function to shutdown watches from old kernel after
> kexec boot.  The old kernel does not unregister all watches in the
> shutdown path.  They are still active, the double registration can not
> be detected by the new kernel.  When the watches fire, unexpected events
> will arrive and the xenwatch thread will crash (jumps to NULL).  An
> orderly reboot of a hvm guest will destroy the entire guest with all its
> resources (including the watches) before it is rebuilt from scratch, so
> the missing unregister is not an issue in that case.
So this patch replaces the big patch series you sent some while ago?
[I''ve one of your patches in my tree, but I wasn''t sure about
the other
ones]
> 
> With this change the xenstored is instructed to wipe all active watches
> for the guest.  However, a patch for xenstored is required so that it
> accepts the XS_RESET_WATCHES request from a guest.
Ok, make sure to note the c/s when that is accepted.
> 
> v3:
>   - use XS_RESET_WATCHES instead of XS_INTRODUCE
> 
> v2:
>   - move all code which deals with XS_INTRODUCE into xs_introduce()
>     (based on feedback from Ian Campbell)
>   - remove casts from kvec assignment
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> ---
>  drivers/xen/xenbus/xenbus_xs.c     |   20 ++++++++++++++++++++
>  include/xen/interface/io/xs_wire.h |    6 +++++-
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> Index: linux-3.0/drivers/xen/xenbus/xenbus_xs.c
> ==================================================================> ---
linux-3.0.orig/drivers/xen/xenbus/xenbus_xs.c
> +++ linux-3.0/drivers/xen/xenbus/xenbus_xs.c
> @@ -620,6 +620,22 @@ static struct xenbus_watch *find_watch(c
>  	return NULL;
>  }
>  
> +static void xs_reset_watches(void)
> +{
> +	/* dummy, empty iov not handled by xenstored */
> +	struct kvec iov[1];
> +	int err;
> +
> +
> +	iov[0].iov_base = "dummy";
> +	iov[0].iov_len = strlen(iov[0].iov_base) + 1;
> +
> +	err = xs_error(xs_talkv(XBT_NIL, XS_RESET_WATCHES, iov,
> +				 ARRAY_SIZE(iov), NULL));
> +	if (err)
> +		printk(KERN_WARNING "xs_reset_watches failed: %d\n", err);
If the xenstore does not have the patch for this, what is the
error code? Is it ENOSYS? If we get that can we not print this message?
Or perhaps print:
"Yikes! We can''t reset the watches. Potential crash
immienient"
or something similar.
> +}
> +
>  /* Register callback to watch this node. */
>  int register_xenbus_watch(struct xenbus_watch *watch)
>  {
> @@ -897,5 +913,9 @@ int xs_init(void)
>  	if (IS_ERR(task))
>  		return PTR_ERR(task);
>  
> +	/* shutdown watches for kexec boot */
> +	if (xen_hvm_domain())
> +		xs_reset_watches();
> +
>  	return 0;
>  }
> Index: linux-3.0/include/xen/interface/io/xs_wire.h
> ==================================================================> ---
linux-3.0.orig/include/xen/interface/io/xs_wire.h
> +++ linux-3.0/include/xen/interface/io/xs_wire.h
> @@ -26,7 +26,11 @@ enum xsd_sockmsg_type
>      XS_SET_PERMS,
>      XS_WATCH_EVENT,
>      XS_ERROR,
> -    XS_IS_DOMAIN_INTRODUCED
> +    XS_IS_DOMAIN_INTRODUCED,
> +    XS_RESUME,
> +    XS_SET_TARGET,
> +    XS_RESTRICT,
> +    XS_RESET_WATCHES
>  };
>  
>  #define XS_WRITE_NONE "NONE"
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, Aug 15, Konrad Rzeszutek Wilk wrote:
> On Mon, Aug 15, 2011 at 11:25:30AM +0200, Olaf Hering wrote:
> > Add new xs_reset_watches function to shutdown watches from old kernel
after
> > kexec boot.  The old kernel does not unregister all watches in the
> > shutdown path.  They are still active, the double registration can not
> > be detected by the new kernel.  When the watches fire, unexpected
events
> > will arrive and the xenwatch thread will crash (jumps to NULL).  An
> > orderly reboot of a hvm guest will destroy the entire guest with all
its
> > resources (including the watches) before it is rebuilt from scratch,
so
> > the missing unregister is not an issue in that case.
> 
> So this patch replaces the big patch series you sent some while ago?
> [I''ve one of your patches in my tree, but I wasn''t sure
about the other
> ones]
No, there are 3 other patches required:

xen/pv-on-hvm kexec+kdump: reset PV devices in kexec or crash kernel
xen/pv-on-hvm kexec: rebind virqs to existing eventchannel ports
xen/pv-on-hvm kexec: prevent crash in xenwatch_thread() when stale watch events
arrive

I will send them once we settled on a way to reset the watches.

Which one did you already apply?
> If the xenstore does not have the patch for this, what is the
> error code? Is it ENOSYS? If we get that can we not print this message?
> Or perhaps print:
> "Yikes! We can''t reset the watches. Potential crash
immienient"
> or something similar.
Now that you mention it, the return code should be checked and ENOSYS
should be filtered to not print the warning on a host without the
updated xenstored.  I will update that part of the patch.

Olaf

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

On Mon, Aug 15, 2011 at 02:59:58PM +0200, Olaf Hering
wrote:
> On Mon, Aug 15, Konrad Rzeszutek Wilk wrote:
> 
> > On Mon, Aug 15, 2011 at 11:25:30AM +0200, Olaf Hering wrote:
> > > Add new xs_reset_watches function to shutdown watches from old
kernel after
> > > kexec boot.  The old kernel does not unregister all watches in
the
> > > shutdown path.  They are still active, the double registration
can not
> > > be detected by the new kernel.  When the watches fire, unexpected
events
> > > will arrive and the xenwatch thread will crash (jumps to NULL). 
An
> > > orderly reboot of a hvm guest will destroy the entire guest with
all its
> > > resources (including the watches) before it is rebuilt from
scratch, so
> > > the missing unregister is not an issue in that case.
> > 
> > So this patch replaces the big patch series you sent some while ago?
> > [I''ve one of your patches in my tree, but I wasn''t
sure about the other
> > ones]
> 
> No, there are 3 other patches required:
> 
> xen/pv-on-hvm kexec+kdump: reset PV devices in kexec or crash kernel
> xen/pv-on-hvm kexec: rebind virqs to existing eventchannel ports
> xen/pv-on-hvm kexec: prevent crash in xenwatch_thread() when stale watch
events arrive
> 
> I will send them once we settled on a way to reset the watches.
Ok.
> 
> Which one did you already apply?
commit 6b71c52e7f848e2c9f804e175215e5965ea90d32
Author: Olaf Hering <olaf@aepfle.de>
Date:   Thu Jul 28 15:23:03 2011 +0200

    xen: use static initializers in xen-balloon.c
    
    There is no need to use dynamic initializaion, it just confuses the reader.
    Switch to static initializers like its used in other files.
    
    Signed-off-by: Olaf Hering <olaf@aepfle.de>
    [v2: Rebased on v3.0]
> 
> > If the xenstore does not have the patch for this, what is the
> > error code? Is it ENOSYS? If we get that can we not print this
message?
> > Or perhaps print:
> > "Yikes! We can''t reset the watches. Potential crash
immienient"
> > or something similar.
> 
> Now that you mention it, the return code should be checked and ENOSYS
> should be filtered to not print the warning on a host without the
> updated xenstored.  I will update that part of the patch.
ok.
> 
> Olaf
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering writes ("[Xen-devel] [PATCH] xenstored: allow guests to
shutdown all of its watches using
XS_RESET_WATCHES"):
> xenstored: allow guests to shutdown all of its watches using
XS_RESET_WATCHES
> 
> During kexec all old watches have to be removed, otherwise the new
> kernel will receive unexpected events. Allow a guest to reset itself and
> cleanup all of its watches.
> 
> Add a new XS_RESET_WATCHES command to do the reset on behalf of the guest.
Applied, thanks, with the two changes we discussed.

Thanks,
Ian.

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