Skip to contentSkip to conversation

xenstored: allow guests to shutdown all of its watches using XS_RESET_WATCHES

PATCH Applied Olaf Hering 3 participants 6 messages Aug 15, 2011 – Sep 15, 2011 Xen hypervisor interface

lore
Share & tools
lore

Olaf Hering

author
#0 lore raw

PATCH 1/1 xenstored: allow guests to shutdown all of its watches using XS_RESET_WATCHES

# HG changeset patch
# User Olaf Hering <[email protected]>
# 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 <[email protected]>
docs/misc/xenstore.txt +3 -0
diff -r 1f08b380d438 -r 4b483f4fa715 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>|
tools/xenstore/xenstored_core.c +5 -0
diff -r 1f08b380d438 -r 4b483f4fa715 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);
tools/xenstore/xenstored_domain.c +14 -0
diff -r 1f08b380d438 -r 4b483f4fa715 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);
tools/xenstore/xenstored_domain.h +3 -0
diff -r 1f08b380d438 -r 4b483f4fa715 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) */
xen/include/public/io/xs_wire.h +2 -1
diff -r 1f08b380d438 -r 4b483f4fa715 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"
author
#1 lore raw

PATCH 1/1 xen/pv-on-hvm kexec: add xs_reset_watches to shutdown watches from old kernel

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 <[email protected]>
---
 drivers/xen/xenbus/xenbus_xs.c     |   20 ++++++++++++++++++++
 include/xen/interface/io/xs_wire.h |    6 +++++-
 2 files changed, 25 insertions(+), 1 deletion(-)
drivers/xen/xenbus/xenbus_xs.c +20 -0
Index: 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;
 }
include/xen/interface/io/xs_wire.h +5 -1
Index: 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"
#2 lore raw

Re: [PATCH] xen/pv-on-hvm kexec: add xs_reset_watches to shutdown watches from old kernel

Quoted 8 lines · Olaf Hering

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]

Quoted 4 lines

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.

Quoted 28 lines

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 <[email protected]>
---
 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
===================================================================

drivers/xen/xenbus/xenbus_xs.c +4 -0
@@ -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];

drivers/xen/xenbus/xenbus_xs.c static struct xenbus_watch *find_watch(c · line 636

Quoted 10 lines
drivers/xen/xenbus/xenbus_xs.c +10 -0
+	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.

Quoted 37 lines
drivers/xen/xenbus/xenbus_xs.c +6 -0
+}
+
 /* 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
===================================================================

include/xen/interface/io/xs_wire.h +5 -1
@@ -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
[email protected]
http://lists.xensource.com/xen-devel

author
#3 lore raw

Re: [PATCH] xen/pv-on-hvm kexec: add xs_reset_watches to shutdown watches from old kernel

Quoted 10 lines · Konrad Rzeszutek Wilk
Quoted 8 lines · Olaf Hering

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.

Quoted 3 lines

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?

Quoted 5 lines

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

#4 lore raw

Re: [PATCH] xen/pv-on-hvm kexec: add xs_reset_watches to shutdown watches from old kernel

Quoted 16 lines · Olaf Hering
Quoted 10 lines · Konrad Rzeszutek Wilk
Quoted 8 lines · Olaf Hering

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.

Quoted 3 lines

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]

Quoted 7 lines

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.

Quoted 2 lines

Which one did you already apply?

commit 6b71c52e7f848e2c9f804e175215e5965ea90d32

Author: Olaf Hering <[email protected]>
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 <[email protected]>
[v2: Rebased on v3.0]
Quoted 10 lines
Quoted 6 lines

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.

Quoted 2 lines

Olaf

Applied
#5 lore raw
In reply to Olaf Hering

Olaf Hering writes ("[Xen-devel] [PATCH] xenstored: allow guests to shutdown all of its watches using XS_RESET_WATCHES"):

Quoted 7 lines

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.