Re: /proc/xen/xenbus supports watch?

[Rusty Russell <rusty@rustcorp.com.au>]

On Mon, 2005-09-26 at 20:30 +0100, Keir Fraser wrote:
> On 26 Sep 2005, at 19:51, Christian Limpach wrote:
> 
> >>> Since we
> >>> need to add some kind of transaction identifier to the interface
> >>> to support this, we should make this change now.
> >>
> >> Or, alternately, since we don't need it, we shouldn't.
> >
> > I think we need them since it's the simplest solution to the whole
> > multi-page/multi-connection issue for a saner xenbus_dev 
> > implementation:
> > - lock only held around xs_talkv
> > - transaction ids
> > - single point for demultiplexing watch events
> 
> This is precisely how I expected that xenbus was going to be structured 
> in the first place. It seems the simplest, most natural implementation 
> and happens to avoid a lot of potential unnecessary blocking and 
> serialisation. And not even at much cost in xenstored (how hard can the 
> demultiplex be?).

Well, here's the simple patch that modifies introductions to allow the
domain to introduce new pages, and another one that tests it.  I'm away
for the next week and a half on other stuff, then I'm in Cambridge.

Modifying the xenbus dev to use this should be fairly easy, if you
choose to go this route.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff -r 0e368f851f6a tools/python/xen/lowlevel/xs/xs.c
--- a/tools/python/xen/lowlevel/xs/xs.c	Tue Sep 27 00:55:56 2005
+++ b/tools/python/xen/lowlevel/xs/xs.c	Tue Sep 27 16:36:19 2005
@@ -728,7 +728,7 @@
                                      &dom))
         goto exit;
     Py_BEGIN_ALLOW_THREADS
-    xsval = xs_release_domain(xh, dom);
+    xsval = xs_release_domain(xh, dom, 0);
     Py_END_ALLOW_THREADS
     if (!xsval) {
         PyErr_SetFromErrno(PyExc_RuntimeError);
diff -r 0e368f851f6a tools/xenstore/xenstored_core.c
--- a/tools/xenstore/xenstored_core.c	Tue Sep 27 00:55:56 2005
+++ b/tools/xenstore/xenstored_core.c	Tue Sep 27 16:36:19 2005
@@ -1136,7 +1136,7 @@
 		break;
 
 	case XS_RELEASE:
-		do_release(conn, onearg(in));
+		do_release(conn, in);
 		break;
 
 	case XS_GET_DOMAIN_PATH:
diff -r 0e368f851f6a tools/xenstore/xenstored_domain.c
--- a/tools/xenstore/xenstored_domain.c	Tue Sep 27 00:55:56 2005
+++ b/tools/xenstore/xenstored_domain.c	Tue Sep 27 16:36:19 2005
@@ -50,6 +50,9 @@
 
 	/* Event channel port */
 	u16 port;
+
+	/* Page number. */
+	unsigned long mfn;
 
 	/* Domain path in store. */
 	char *path;
@@ -282,6 +285,7 @@
 	domain->port = 0;
 	domain->shutdown = 0;
 	domain->domid = domid;
+	domain->mfn = mfn;
 	domain->path = talloc_strdup(domain, path);
 	domain->page = xc_map_foreign_range(*xc_handle, domain->domid,
 					    getpagesize(),
@@ -312,15 +316,28 @@
 {
 	struct domain *domain;
 	char *vec[4];
+	domid_t domid;
 
 	if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec)) {
 		send_error(conn, EINVAL);
 		return;
 	}
 
-	if (conn->id != 0 || !conn->can_write) {
+	domid = atoi(vec[0]);
+	if (!conn->can_write || (conn->id != 0 && domid != DOMID_SELF)) {
 		send_error(conn, EACCES);
 		return;
+	}
+
+	/* Domains can introduce more comms pages to store.  FIXME: limit. */
+	if (domid == DOMID_SELF) {
+		if (!conn->domain) {
+			send_error(conn, EINVAL);
+			return;
+		}
+		/* Same domid and path. */
+		domid = conn->domain->domid;
+		vec[3] = conn->domain->path;
 	}
 
 	/* Sanity check args. */
@@ -329,8 +346,7 @@
 		return;
 	}
 	/* Hang domain off "in" until we're finished. */
-	domain = new_domain(in, atoi(vec[0]), atol(vec[1]), atol(vec[2]),
-			    vec[3]);
+	domain = new_domain(in, domid, atol(vec[1]), atol(vec[2]), vec[3]);
 	if (!domain) {
 		send_error(conn, errno);
 		return;
@@ -339,58 +355,69 @@
 	/* Now domain belongs to its connection. */
 	talloc_steal(domain->conn, domain);
 
-	fire_watches(conn, "@introduceDomain", false);
+	if (domid != DOMID_SELF)
+		fire_watches(conn, "@introduceDomain", false);
 
 	send_ack(conn, XS_INTRODUCE);
 }
 
-static struct domain *find_domain_by_domid(domid_t domid)
+static struct domain *find_domain(domid_t domid, unsigned long mfn)
 {
 	struct domain *i;
 
 	list_for_each_entry(i, &domains, list) {
-		if (i->domid == domid)
+		if (i->domid == domid && (!mfn || i->mfn == mfn))
 			return i;
 	}
 	return NULL;
 }
 
-/* domid */
-void do_release(struct connection *conn, const char *domid_str)
+/* domid, mfn */
+void do_release(struct connection *conn, struct buffered_data *in)
 {
 	struct domain *domain;
 	domid_t domid;
-
-	if (!domid_str) {
+	unsigned long mfn;
+	bool released = false;
+	char *vec[2];
+
+	if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec)) {
 		send_error(conn, EINVAL);
 		return;
 	}
-
-	domid = atoi(domid_str);
+	domid = atoi(vec[0]);
+	mfn = atol(vec[1]);
 	if (!domid) {
 		send_error(conn, EINVAL);
 		return;
 	}
 
-	if (conn->id != 0) {
-		send_error(conn, EACCES);
-		return;
-	}
-
-	domain = find_domain_by_domid(domid);
-	if (!domain) {
+	if (domid == DOMID_SELF) {
+		if (!conn->domain || mfn == 0) {
+			send_error(conn, EINVAL);
+			return;
+		}
+		domid = conn->domain->domid;
+	} else {
+		if (conn->id != 0) {
+			send_error(conn, EACCES);
+			return;
+		}
+	}
+
+	/* Can release multiple if mfn == 0 */
+	while ((domain = find_domain(domid, mfn)) != NULL) {
+		talloc_free(domain->conn);
+		released = true;
+	}
+
+	if (!released) {
 		send_error(conn, ENOENT);
 		return;
 	}
 
-	if (!domain->conn) {
-		send_error(conn, EINVAL);
-		return;
-	}
-
-	talloc_free(domain->conn);
-
-	fire_watches(conn, "@releaseDomain", false);
+	if (domid != DOMID_SELF)
+		fire_watches(conn, "@releaseDomain", false);
 
 	send_ack(conn, XS_RELEASE);
 }
@@ -409,7 +436,7 @@
 	if (domid == DOMID_SELF)
 		domain = conn->domain;
 	else
-		domain = find_domain_by_domid(domid);
+		domain = find_domain(domid, 0);
 
 	if (!domain)
 		send_error(conn, ENOENT);
diff -r 0e368f851f6a tools/xenstore/xenstored_domain.h
--- a/tools/xenstore/xenstored_domain.h	Tue Sep 27 00:55:56 2005
+++ b/tools/xenstore/xenstored_domain.h	Tue Sep 27 16:36:19 2005
@@ -25,8 +25,8 @@
 /* domid, mfn, eventchn, path */
 void do_introduce(struct connection *conn, struct buffered_data *in);
 
-/* domid */
-void do_release(struct connection *conn, const char *domid_str);
+/* domid, mfn */
+void do_release(struct connection *conn, struct buffered_data *in);
 
 /* domid */
 void do_get_domain_path(struct connection *conn, const char *domid_str);
diff -r 0e368f851f6a tools/xenstore/xs.c
--- a/tools/xenstore/xs.c	Tue Sep 27 00:55:56 2005
+++ b/tools/xenstore/xs.c	Tue Sep 27 16:36:19 2005
@@ -676,13 +676,21 @@
 	return xs_bool(xs_talkv(h, XS_INTRODUCE, iov, ARRAY_SIZE(iov), NULL));
 }
 
-bool xs_release_domain(struct xs_handle *h, domid_t domid)
+bool xs_release_domain(struct xs_handle *h, domid_t domid, unsigned long mfn)
 {
 	char domid_str[MAX_STRLEN(domid)];
+	char mfn_str[MAX_STRLEN(mfn)];
+	struct iovec iov[2];
 
 	sprintf(domid_str, "%u", domid);
-
-	return xs_bool(xs_single(h, XS_RELEASE, domid_str, NULL));
+	sprintf(mfn_str, "%lu", mfn);
+
+	iov[0].iov_base = domid_str;
+	iov[0].iov_len = strlen(domid_str) + 1;
+	iov[1].iov_base = mfn_str;
+	iov[1].iov_len = strlen(mfn_str) + 1;
+
+	return xs_bool(xs_talkv(h, XS_RELEASE, iov, ARRAY_SIZE(iov), NULL));
 }
 
 char *xs_get_domain_path(struct xs_handle *h, domid_t domid)
diff -r 0e368f851f6a tools/xenstore/xs.h
--- a/tools/xenstore/xs.h	Tue Sep 27 00:55:56 2005
+++ b/tools/xenstore/xs.h	Tue Sep 27 16:36:19 2005
@@ -130,8 +130,9 @@
 
 /* Release a domain.
  * Tells the store domain to release the memory page to the domain.
+ * mfn is 0 to release all of them.
  */
-bool xs_release_domain(struct xs_handle *h, domid_t domid);
+bool xs_release_domain(struct xs_handle *h, domid_t domid, unsigned long mfn);
 
 /* Query the home path of a domain.
  */


Test code for previous patch.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff -r 0e368f851f6a tools/xenstore/fake_libxc.c
--- a/tools/xenstore/fake_libxc.c	Tue Sep 27 00:55:56 2005
+++ b/tools/xenstore/fake_libxc.c	Tue Sep 27 16:36:19 2005
@@ -44,42 +44,38 @@
 	return 0;
 }
 
-void *xc_map_foreign_range(int xc_handle, u32 dom __attribute__((unused)),
+void *xc_map_foreign_range(int xc_handle __attribute__((unused)),
+			   u32 dom __attribute__((unused)),
 			   int size, int prot,
 			   unsigned long mfn __attribute__((unused)))
 {
 	void *ret;
+	int *extra;
+	int fd;
 
-	ret = mmap(NULL, size, prot, MAP_SHARED, xc_handle, 0);
+	fd = open("/tmp/xcmap", O_RDWR);
+	if (fd < 0)
+		return NULL;
+
+	/* We actually get extra page, for comms with xs_test. */
+	ret = mmap(NULL, size + getpagesize(), prot, MAP_SHARED, fd, 0);
 	if (ret == MAP_FAILED)
 		return NULL;
 
-	/* xs_test tells us pid and port by putting it in buffer, we reply. */
-	xs_test_pid = *(int *)(ret + 32);
-	port = *(int *)(ret + 36);
-	*(int *)(ret + 32) = getpid();
+	extra = ret + size;
+	xs_test_pid = extra[0];
+	port = extra[1];
+	extra[2] = getpid();
 	return ret;
 }
 
 int xc_interface_open(void)
 {
-	int fd;
-	char page[getpagesize()];
-
-	fd = open("/tmp/xcmap", O_RDWR|O_CREAT|O_TRUNC, 0600);
-	if (fd < 0)
-		return fd;
-
-	memset(page, 0, sizeof(page));
-	if (!xs_write_all(fd, page, sizeof(page)))
-		barf_perror("Failed to write /tmp/xcmap page");
-	
-	return fd;
+	return 1;
 }
 
-int xc_interface_close(int xc_handle)
+int xc_interface_close(int xc_handle __attribute__((unused)))
 {
-	close(xc_handle);
 	return 0;
 }
 
diff -r 0e368f851f6a tools/xenstore/testsuite/09domain.test
--- a/tools/xenstore/testsuite/09domain.test	Tue Sep 27 00:55:56 2005
+++ b/tools/xenstore/testsuite/09domain.test	Tue Sep 27 16:36:19 2005
@@ -17,3 +17,24 @@
 expect handle is 2
 introduce 1 100 7 /my/home
 release 1
+
+# Introduce sub-connection
+write /my/home/entry contents
+close
+expect handle is 3
+introduce 1 100 7 /my/home
+expect handle is 4
+3 introduce-self 120 7 /my/home
+
+# Check home is correct
+expect 4:contents
+4 read entry
+
+# Release 4 from 3
+3 release-self 120
+
+# Introduce a new one and release both at once.
+expect handle is 5
+3 introduce-self 120 7 /my/home
+
+release 1
diff -r 0e368f851f6a tools/xenstore/xs_test.c
--- a/tools/xenstore/xs_test.c	Tue Sep 27 00:55:56 2005
+++ b/tools/xenstore/xs_test.c	Tue Sep 27 16:36:19 2005
@@ -59,6 +59,7 @@
 
 static struct ringbuf_head *out, *in;
 static unsigned int ringbuf_datasize;
+static int event_channel;
 static int daemon_pid;
 
 /* FIXME: Mark connection as broken (close it?) when this happens. */
@@ -208,6 +209,9 @@
 	     "  start <node>\n"
 	     "  abort\n"
 	     "  introduce <domid> <mfn> <eventchn> <path>\n"
+	     "  introduce-self <mfn> <eventchn> <path>\n"
+	     "  release <domid>\n"
+	     "  release-self <mfn>\n"
 	     "  commit\n"
 	     "  sleep <milliseconds>\n"
 	     "  expect <pattern>\n"
@@ -575,16 +579,22 @@
 }
 
 static void do_introduce(unsigned int handle,
-			 const char *domid,
+			 int domid,
 			 const char *mfn,
 			 const char *eventchn,
 			 const char *path)
 {
 	unsigned int i;
-	int fd;
+	int fd, *extra;
+	char pages[getpagesize()*2];
+
+	fd = open("/tmp/xcmap", O_RDWR|O_CREAT, 0600);
+	memset(pages, 0, sizeof(pages));
+	write(fd, pages, sizeof(pages));
 
 	/* This mechanism is v. slow w. valgrind running. */
-	timeout_ms = 5000;
+	if (timeout_ms)
+		timeout_ms = 5000;
 
 	/* We poll, so ignore signal */
 	signal(SIGUSR2, SIG_IGN);
@@ -592,22 +602,24 @@
 		if (!handles[i])
 			break;
 
-	fd = open("/tmp/xcmap", O_RDWR);
 	/* Set in and out pointers. */
-	out = mmap(NULL, getpagesize(), PROT_WRITE|PROT_READ, MAP_SHARED,fd,0);
+	out = mmap(NULL, sizeof(pages), PROT_WRITE|PROT_READ, MAP_SHARED,fd,0);
 	if (out == MAP_FAILED)
 		barf_perror("Failed to map /tmp/xcmap page");
 	in = (void *)out + getpagesize() / 2;
 	close(fd);
 
+	event_channel = atoi(eventchn);
+
 	/* Tell them the event channel and our PID. */
-	*(int *)((void *)out + 32) = getpid();
-	*(u16 *)((void *)out + 36) = atoi(eventchn);
-
-	if (!xs_introduce_domain(handles[handle], atoi(domid),
+	extra = (void *)out + getpagesize();
+	extra[0] = getpid();
+	extra[1] = event_channel;
+
+	if (!xs_introduce_domain(handles[handle], domid,
 				 atol(mfn), atoi(eventchn), path)) {
 		failed(handle);
-		munmap(out, getpagesize());
+		munmap(out, getpagesize()*2);
 		return;
 	}
 	output("handle is %i\n", i);
@@ -622,12 +634,18 @@
 	handles[i]->committing = false;
 
 	/* Read in daemon pid. */
-	daemon_pid = *(int *)((void *)out + 32);
+	daemon_pid = extra[2];
 }
 
 static void do_release(unsigned int handle, const char *domid)
 {
-	if (!xs_release_domain(handles[handle], atoi(domid)))
+	if (!xs_release_domain(handles[handle], atoi(domid), 0))
+		failed(handle);
+}
+
+static void do_release_self(unsigned int handle, const char *mfn)
+{
+	if (!xs_release_domain(handles[handle], DOMID_SELF, atol(mfn)))
 		failed(handle);
 }
 
@@ -802,10 +820,15 @@
 	else if (streq(command, "abort"))
 		do_end(handle, true);
 	else if (streq(command, "introduce"))
-		do_introduce(handle, arg(line, 1), arg(line, 2),
+		do_introduce(handle, atoi(arg(line, 1)), arg(line, 2),
 			     arg(line, 3), arg(line, 4));
+	else if (streq(command, "introduce-self"))
+		do_introduce(handle, DOMID_SELF,
+			     arg(line, 1), arg(line, 2), arg(line, 3));
 	else if (streq(command, "release"))
 		do_release(handle, arg(line, 1));
+	else if (streq(command, "release-self"))
+		do_release_self(handle, arg(line, 1));
 	else if (streq(command, "dump"))
 		dump(handle);
 	else if (streq(command, "sleep")) {

-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman