[PATCH] cxenstored: correct calculation of data/space in the ring

[PATCH] cxenstored: correct calculation of data/space in the ring

[Wei Liu]

The cxenstored implementation can't handle cross ring boundary read and
write. It gets aways with buggy behaviour because upper layer won't
sleep when short-write or short-read occurs.

It would be nice, however, to make the low level routine correct
regardless of what upper layer is doing. This should at least avoid
latent bug should upper layer logic changes.

This patch ports 8a2c11f8 ("tools/ocaml/xb: Correct calculations of
data/space the ring") for oxenstored to cxenstored.

Two functions -- get_{input,output}_chunks -- become unused. Nuke them.
Preserve the behaviour of always sending notification to the other end
when exiting.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/xenstore/xenstored_domain.c | 76 +++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 34 deletions(-)

diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index dcd6581..4c0b87e 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -82,33 +82,12 @@ static bool check_indexes(XENSTORE_RING_IDX cons, XENSTORE_RING_IDX prod)
 	return ((prod - cons) <= XENSTORE_RING_SIZE);
 }
 
-static void *get_output_chunk(XENSTORE_RING_IDX cons,
-			      XENSTORE_RING_IDX prod,
-			      char *buf, uint32_t *len)
-{
-	*len = XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(prod);
-	if ((XENSTORE_RING_SIZE - (prod - cons)) < *len)
-		*len = XENSTORE_RING_SIZE - (prod - cons);
-	return buf + MASK_XENSTORE_IDX(prod);
-}
-
-static const void *get_input_chunk(XENSTORE_RING_IDX cons,
-				   XENSTORE_RING_IDX prod,
-				   const char *buf, uint32_t *len)
-{
-	*len = XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(cons);
-	if ((prod - cons) < *len)
-		*len = prod - cons;
-	return buf + MASK_XENSTORE_IDX(cons);
-}
-
 static int writechn(struct connection *conn,
-		    const void *data, unsigned int len)
+		    const void *buffer, unsigned int len)
 {
-	uint32_t avail;
-	void *dest;
 	struct xenstore_domain_interface *intf = conn->domain->interface;
 	XENSTORE_RING_IDX cons, prod;
+	int total_space, space;
 
 	/* Must read indexes once, and before anything else, and verified. */
 	cons = intf->rsp_cons;
@@ -120,25 +99,39 @@ static int writechn(struct connection *conn,
 		return -1;
 	}
 
-	dest = get_output_chunk(cons, prod, intf->rsp, &avail);
-	if (avail < len)
-		len = avail;
+	total_space = XENSTORE_RING_SIZE - (prod - cons);
+	if (total_space == 0) {
+		/* No space at all - exit having done nothing. */
+		len = 0;
+		goto exit;
+	} else if (total_space < len)
+		/* Some space - make a partial write */
+		len = total_space;
+
+	space = XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(prod);
+	if (len < space)
+		/* Message fits inside the remaining part of the ring */
+		memcpy(intf->rsp + MASK_XENSTORE_IDX(prod), buffer, len);
+	else {
+		/* Message wraps around the end of ring. Write both halves. */
+		memcpy(intf->rsp + MASK_XENSTORE_IDX(prod), buffer, space);
+		memcpy(intf->rsp, buffer + space, len - space);
+	}
 
-	memcpy(dest, data, len);
 	xen_mb();
 	intf->rsp_prod += len;
 
+exit:
 	xc_evtchn_notify(xce_handle, conn->domain->port);
 
 	return len;
 }
 
-static int readchn(struct connection *conn, void *data, unsigned int len)
+static int readchn(struct connection *conn, void *buffer, unsigned int len)
 {
-	uint32_t avail;
-	const void *src;
 	struct xenstore_domain_interface *intf = conn->domain->interface;
 	XENSTORE_RING_IDX cons, prod;
+	int total_data, data;
 
 	/* Must read indexes once, and before anything else, and verified. */
 	cons = intf->req_cons;
@@ -150,14 +143,29 @@ static int readchn(struct connection *conn, void *data, unsigned int len)
 		return -1;
 	}
 
-	src = get_input_chunk(cons, prod, intf->req, &avail);
-	if (avail < len)
-		len = avail;
+	total_data = prod - cons;
+	if (total_data == 0) {
+		/* No pending data at all. */
+		len = 0;
+		goto exit;
+	} else if (total_data < len)
+		/* Some data - make a partial read. */
+		len = total_data;
+
+	data = XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(cons);
+	if (len < data)
+		/* Data within the remaining part of the ring. */
+		memcpy(buffer, intf->req + MASK_XENSTORE_IDX(cons), len);
+	else {
+		/* Data crosses the ring boundary. Read both halves. */
+		memcpy(buffer, intf->req + MASK_XENSTORE_IDX(cons), data);
+		memcpy(buffer + data, intf->req, len - data);
+	}
 
-	memcpy(data, src, len);
 	xen_mb();
 	intf->req_cons += len;
 
+exit:
 	xc_evtchn_notify(xce_handle, conn->domain->port);
 
 	return len;
-- 
2.1.4

Re: [PATCH] cxenstored: correct calculation of data/space in the ring

[Ian Jackson]

Wei Liu writes ("[PATCH] cxenstored: correct calculation of data/space in the ring"):
> The cxenstored implementation can't handle cross ring boundary read and
> write. It gets aways with buggy behaviour because upper layer won't
> sleep when short-write or short-read occurs.

I don't understand why you think this is a bug.

> It would be nice, however, to make the low level routine correct
> regardless of what upper layer is doing. This should at least avoid
> latent bug should upper layer logic changes.

I don't think this is as hard a layer boundary as you suggest.

Ian.

Re: [PATCH] cxenstored: correct calculation of data/space in the ring

[Andrew Cooper]

On 16/11/15 18:01, Ian Jackson wrote:
> Wei Liu writes ("[PATCH] cxenstored: correct calculation of data/space in the ring"):
>> The cxenstored implementation can't handle cross ring boundary read and
>> write. It gets aways with buggy behaviour because upper layer won't
>> sleep when short-write or short-read occurs.
> I don't understand why you think this is a bug.

It is exactly the same bug as I fixed in c/s 8a2c11f8

The short reads/writes themselves aren't inherently a problem.  They are
genuine signals that the server should wait for the client to
produce/consume more data.

However, the low level functions erroneously return a short read/write
when hitting the ring boundary when there is actually more space/data. 
This causes a protocol stall as the server incorrectly believes that the
client has the next action to perform.

~Andrew

Re: [PATCH] cxenstored: correct calculation of data/space in the ring

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH] cxenstored: correct calculation of data/space in the ring"):
> On 16/11/15 18:01, Ian Jackson wrote:
> > Wei Liu writes ("[PATCH] cxenstored: correct calculation of data/space in the ring"):
> >> The cxenstored implementation can't handle cross ring boundary read and
> >> write. It gets aways with buggy behaviour because upper layer won't
> >> sleep when short-write or short-read occurs.
> > I don't understand why you think this is a bug.
> 
> It is exactly the same bug as I fixed in c/s 8a2c11f8
> 
> The short reads/writes themselves aren't inherently a problem.  They are
> genuine signals that the server should wait for the client to
> produce/consume more data.
> 
> However, the low level functions erroneously return a short read/write
> when hitting the ring boundary when there is actually more space/data. 
> This causes a protocol stall as the server incorrectly believes that the
> client has the next action to perform.

If I understand Wei correctly you are contradicting him.  The `upper
layer' in question is inside the C xenstored so there is no protocol
stall.

(I haven't peered at the code...)

Ian.

Re: [PATCH] cxenstored: correct calculation of data/space in the ring

[Wei Liu]

On Mon, Nov 16, 2015 at 06:09:54PM +0000, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH] cxenstored: correct calculation of data/space in the ring"):
> > On 16/11/15 18:01, Ian Jackson wrote:
> > > Wei Liu writes ("[PATCH] cxenstored: correct calculation of data/space in the ring"):
> > >> The cxenstored implementation can't handle cross ring boundary read and
> > >> write. It gets aways with buggy behaviour because upper layer won't
> > >> sleep when short-write or short-read occurs.
> > > I don't understand why you think this is a bug.
> > 
> > It is exactly the same bug as I fixed in c/s 8a2c11f8
> > 
> > The short reads/writes themselves aren't inherently a problem.  They are
> > genuine signals that the server should wait for the client to
> > produce/consume more data.
> > 
> > However, the low level functions erroneously return a short read/write
> > when hitting the ring boundary when there is actually more space/data. 
> > This causes a protocol stall as the server incorrectly believes that the
> > client has the next action to perform.
> 
> If I understand Wei correctly you are contradicting him.  The `upper
> layer' in question is inside the C xenstored so there is no protocol
> stall.
> 

There is no protocol stall for now. But the code that controls whether
to sleep or not can change (however unlikely). And it would be hard to
debug such bug as the effort for debugging stubdom / oxenstored already
demonstrated.

IMO short-writing and short-reading when there is still space / data is
a bug in its own right. We might as well just fix it before we get hit
again.

Wei.

> (I haven't peered at the code...)
> 
> Ian.

Re: [PATCH] cxenstored: correct calculation of data/space in the ring

[Ian Campbell]

On Mon, 2015-11-16 at 18:36 +0000, Wei Liu wrote:

Ian, were your concerns addressed by this threadlet?

> On Mon, Nov 16, 2015 at 06:09:54PM +0000, Ian Jackson wrote:
> > Andrew Cooper writes ("Re: [PATCH] cxenstored: correct calculation of
> > data/space in the ring"):
> > > On 16/11/15 18:01, Ian Jackson wrote:
> > > > Wei Liu writes ("[PATCH] cxenstored: correct calculation of
> > > > data/space in the ring"):
> > > > > The cxenstored implementation can't handle cross ring boundary
> > > > > read and
> > > > > write. It gets aways with buggy behaviour because upper layer
> > > > > won't
> > > > > sleep when short-write or short-read occurs.
> > > > I don't understand why you think this is a bug.
> > > 
> > > It is exactly the same bug as I fixed in c/s 8a2c11f8
> > > 
> > > The short reads/writes themselves aren't inherently a problem.  They
> > > are
> > > genuine signals that the server should wait for the client to
> > > produce/consume more data.
> > > 
> > > However, the low level functions erroneously return a short
> > > read/write
> > > when hitting the ring boundary when there is actually more
> > > space/data. 
> > > This causes a protocol stall as the server incorrectly believes that
> > > the
> > > client has the next action to perform.
> > 
> > If I understand Wei correctly you are contradicting him.  The `upper
> > layer' in question is inside the C xenstored so there is no protocol
> > stall.
> > 
> 
> There is no protocol stall for now. But the code that controls whether
> to sleep or not can change (however unlikely). And it would be hard to
> debug such bug as the effort for debugging stubdom / oxenstored already
> demonstrated.
> 
> IMO short-writing and short-reading when there is still space / data is
> a bug in its own right. We might as well just fix it before we get hit
> again.
> 
> Wei.
> 
> > (I haven't peered at the code...)
> > 
> > Ian.

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