[PATCH v2] tools/ocaml/xb: Correct calculations of data/space the ring

[PATCH v2] tools/ocaml/xb: Correct calculations of data/space the ring

[Andrew Cooper]

ml_interface_{read,write}() would miscalculate the quantity of
data/space in the ring if it crossed the ring boundary, and incorrectly
return a short read/write.

This causes a protocol stall, as either side of the ring ends up waiting
for what they believe to be the other side needing to take the next
action.

Correct the calculations to cope with crossing the ring boundary.

In addition, correct the error detection.  It is a hard error if the
producer index gets more than a ring size ahead of the consumer, or if
the consumer ever overtakes the producer.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: David Scott <dave@recoil.org>
CC: Samuel Thibault <samuel.thibault@ens-lyon.org>

v2:
 * Correct error handling adjustments
 * More fixes to space calculations
 * Fix whitespace errors
 * No longer RFC - passed XenServer sanity testing
---
 tools/ocaml/libs/xb/xs_ring_stubs.c | 64 +++++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 20 deletions(-)

diff --git a/tools/ocaml/libs/xb/xs_ring_stubs.c b/tools/ocaml/libs/xb/xs_ring_stubs.c
index fd561a2..4737870 100644
--- a/tools/ocaml/libs/xb/xs_ring_stubs.c
+++ b/tools/ocaml/libs/xb/xs_ring_stubs.c
@@ -50,7 +50,7 @@ CAMLprim value ml_interface_read(value ml_interface,
 
 	struct xenstore_domain_interface *intf = interface->addr;
 	XENSTORE_RING_IDX cons, prod; /* offsets only */
-	int to_read;
+	int total_data, data;
 	uint32_t connection;
 
 	cons = *(volatile uint32_t*)&intf->req_cons;
@@ -65,19 +65,28 @@ CAMLprim value ml_interface_read(value ml_interface,
 	if ((prod - cons) > XENSTORE_RING_SIZE)
 		caml_failwith("bad connection");
 
-	if (prod == cons) {
+	/* Check for any pending data at all. */
+	total_data = prod - cons;
+	if (total_data == 0) {
+		/* No pending data at all. */
 		result = 0;
 		goto exit;
 	}
-	cons = MASK_XENSTORE_IDX(cons);
-	prod = MASK_XENSTORE_IDX(prod);
-	if (prod > cons)
-		to_read = prod - cons;
-	else
-		to_read = XENSTORE_RING_SIZE - cons;
-	if (to_read < len)
-		len = to_read;
-	memcpy(buffer, intf->req + cons, len);
+	else if (total_data < len)
+		/* Some data - make a partial read. */
+		len = total_data;
+
+	/* Check whether data crosses the end of the ring. */
+	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);
+	}
+
 	xen_mb();
 	intf->req_cons += len;
 	result = len;
@@ -100,7 +109,7 @@ CAMLprim value ml_interface_write(value ml_interface,
 
 	struct xenstore_domain_interface *intf = interface->addr;
 	XENSTORE_RING_IDX cons, prod;
-	int can_write;
+	int total_space, space;
 	uint32_t connection;
 
 	cons = *(volatile uint32_t*)&intf->rsp_cons;
@@ -111,17 +120,32 @@ CAMLprim value ml_interface_write(value ml_interface,
 		caml_raise_constant(*caml_named_value("Xb.Reconnect"));
 
 	xen_mb();
-	if ( (prod - cons) >= XENSTORE_RING_SIZE ) {
+
+	if ((prod - cons) > XENSTORE_RING_SIZE)
+		caml_failwith("bad connection");
+
+	/* Check for space to write the full message. */
+	total_space = XENSTORE_RING_SIZE - (prod - cons);
+	if (total_space == 0) {
+		/* No space at all - exit having done nothing. */
 		result = 0;
 		goto exit;
 	}
-	if (MASK_XENSTORE_IDX(prod) >= MASK_XENSTORE_IDX(cons))
-		can_write = XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(prod);
-	else 
-		can_write = MASK_XENSTORE_IDX(cons) - MASK_XENSTORE_IDX(prod);
-	if (can_write < len)
-		len = can_write;
-	memcpy(intf->rsp + MASK_XENSTORE_IDX(prod), buffer, len);
+	else if (total_space < len)
+		/* Some space - make a partial write. */
+		len = total_space;
+
+	/* Check for space until the ring wraps. */
+	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 the ring. Write both halves. */
+		memcpy(intf->rsp + MASK_XENSTORE_IDX(prod), buffer, space);
+		memcpy(intf->rsp, buffer + space, len - space);
+	}
+
 	xen_mb();
 	intf->rsp_prod += len;
 	result = len;
-- 
2.1.4

Re: [PATCH v2] tools/ocaml/xb: Correct calculations of data/space the ring

[Wei Liu]

On Tue, Nov 10, 2015 at 10:46:44AM +0000, Andrew Cooper wrote:
> ml_interface_{read,write}() would miscalculate the quantity of
> data/space in the ring if it crossed the ring boundary, and incorrectly
> return a short read/write.
> 
> This causes a protocol stall, as either side of the ring ends up waiting
> for what they believe to be the other side needing to take the next
> action.
> 
> Correct the calculations to cope with crossing the ring boundary.
> 
> In addition, correct the error detection.  It is a hard error if the
> producer index gets more than a ring size ahead of the consumer, or if
> the consumer ever overtakes the producer.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH v2] tools/ocaml/xb: Correct calculations of data/space the ring

[Samuel Thibault]

Andrew Cooper, on Tue 10 Nov 2015 10:46:44 +0000, wrote:
> ml_interface_{read,write}() would miscalculate the quantity of
> data/space in the ring if it crossed the ring boundary, and incorrectly
> return a short read/write.
> 
> This causes a protocol stall, as either side of the ring ends up waiting
> for what they believe to be the other side needing to take the next
> action.
> 
> Correct the calculations to cope with crossing the ring boundary.
> 
> In addition, correct the error detection.  It is a hard error if the
> producer index gets more than a ring size ahead of the consumer, or if
> the consumer ever overtakes the producer.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: David Scott <dave@recoil.org>
> CC: Samuel Thibault <samuel.thibault@ens-lyon.org>
> 
> v2:
>  * Correct error handling adjustments
>  * More fixes to space calculations
>  * Fix whitespace errors
>  * No longer RFC - passed XenServer sanity testing
> ---
>  tools/ocaml/libs/xb/xs_ring_stubs.c | 64 +++++++++++++++++++++++++------------
>  1 file changed, 44 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/ocaml/libs/xb/xs_ring_stubs.c b/tools/ocaml/libs/xb/xs_ring_stubs.c
> index fd561a2..4737870 100644
> --- a/tools/ocaml/libs/xb/xs_ring_stubs.c
> +++ b/tools/ocaml/libs/xb/xs_ring_stubs.c
> @@ -50,7 +50,7 @@ CAMLprim value ml_interface_read(value ml_interface,
>  
>  	struct xenstore_domain_interface *intf = interface->addr;
>  	XENSTORE_RING_IDX cons, prod; /* offsets only */
> -	int to_read;
> +	int total_data, data;
>  	uint32_t connection;
>  
>  	cons = *(volatile uint32_t*)&intf->req_cons;
> @@ -65,19 +65,28 @@ CAMLprim value ml_interface_read(value ml_interface,
>  	if ((prod - cons) > XENSTORE_RING_SIZE)
>  		caml_failwith("bad connection");
>  
> -	if (prod == cons) {
> +	/* Check for any pending data at all. */
> +	total_data = prod - cons;
> +	if (total_data == 0) {
> +		/* No pending data at all. */
>  		result = 0;
>  		goto exit;
>  	}
> -	cons = MASK_XENSTORE_IDX(cons);
> -	prod = MASK_XENSTORE_IDX(prod);
> -	if (prod > cons)
> -		to_read = prod - cons;
> -	else
> -		to_read = XENSTORE_RING_SIZE - cons;
> -	if (to_read < len)
> -		len = to_read;
> -	memcpy(buffer, intf->req + cons, len);
> +	else if (total_data < len)
> +		/* Some data - make a partial read. */
> +		len = total_data;
> +
> +	/* Check whether data crosses the end of the ring. */
> +	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);
> +	}
> +
>  	xen_mb();
>  	intf->req_cons += len;
>  	result = len;
> @@ -100,7 +109,7 @@ CAMLprim value ml_interface_write(value ml_interface,
>  
>  	struct xenstore_domain_interface *intf = interface->addr;
>  	XENSTORE_RING_IDX cons, prod;
> -	int can_write;
> +	int total_space, space;
>  	uint32_t connection;
>  
>  	cons = *(volatile uint32_t*)&intf->rsp_cons;
> @@ -111,17 +120,32 @@ CAMLprim value ml_interface_write(value ml_interface,
>  		caml_raise_constant(*caml_named_value("Xb.Reconnect"));
>  
>  	xen_mb();
> -	if ( (prod - cons) >= XENSTORE_RING_SIZE ) {
> +
> +	if ((prod - cons) > XENSTORE_RING_SIZE)
> +		caml_failwith("bad connection");
> +
> +	/* Check for space to write the full message. */
> +	total_space = XENSTORE_RING_SIZE - (prod - cons);
> +	if (total_space == 0) {
> +		/* No space at all - exit having done nothing. */
>  		result = 0;
>  		goto exit;
>  	}
> -	if (MASK_XENSTORE_IDX(prod) >= MASK_XENSTORE_IDX(cons))
> -		can_write = XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(prod);
> -	else 
> -		can_write = MASK_XENSTORE_IDX(cons) - MASK_XENSTORE_IDX(prod);
> -	if (can_write < len)
> -		len = can_write;
> -	memcpy(intf->rsp + MASK_XENSTORE_IDX(prod), buffer, len);
> +	else if (total_space < len)
> +		/* Some space - make a partial write. */
> +		len = total_space;
> +
> +	/* Check for space until the ring wraps. */
> +	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 the ring. Write both halves. */
> +		memcpy(intf->rsp + MASK_XENSTORE_IDX(prod), buffer, space);
> +		memcpy(intf->rsp, buffer + space, len - space);
> +	}
> +
>  	xen_mb();
>  	intf->rsp_prod += len;
>  	result = len;
> -- 
> 2.1.4
> 

-- 
Samuel
As usual, this being a 1.3.x release, I haven't even compiled this
kernel yet.  So if it works, you should be doubly impressed.
(Linus Torvalds, announcing kernel 1.3.3 on the linux-kernel mailing list.)

Re: [PATCH v2] tools/ocaml/xb: Correct calculations of data/space the ring

[David Scott]

> On 10 Nov 2015, at 11:41, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> 
> Andrew Cooper, on Tue 10 Nov 2015 10:46:44 +0000, wrote:
>> ml_interface_{read,write}() would miscalculate the quantity of
>> data/space in the ring if it crossed the ring boundary, and incorrectly
>> return a short read/write.
>> 
>> This causes a protocol stall, as either side of the ring ends up waiting
>> for what they believe to be the other side needing to take the next
>> action.
>> 
>> Correct the calculations to cope with crossing the ring boundary.
>> 
>> In addition, correct the error detection.  It is a hard error if the
>> producer index gets more than a ring size ahead of the consumer, or if
>> the consumer ever overtakes the producer.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Looks good to me too

Reviewed-by: David Scott <dave@recoil.org>

> 
>> ---
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: David Scott <dave@recoil.org>
>> CC: Samuel Thibault <samuel.thibault@ens-lyon.org>
>> 
>> v2:
>> * Correct error handling adjustments
>> * More fixes to space calculations
>> * Fix whitespace errors
>> * No longer RFC - passed XenServer sanity testing
>> ---
>> tools/ocaml/libs/xb/xs_ring_stubs.c | 64 +++++++++++++++++++++++++------------
>> 1 file changed, 44 insertions(+), 20 deletions(-)
>> 
>> diff --git a/tools/ocaml/libs/xb/xs_ring_stubs.c b/tools/ocaml/libs/xb/xs_ring_stubs.c
>> index fd561a2..4737870 100644
>> --- a/tools/ocaml/libs/xb/xs_ring_stubs.c
>> +++ b/tools/ocaml/libs/xb/xs_ring_stubs.c
>> @@ -50,7 +50,7 @@ CAMLprim value ml_interface_read(value ml_interface,
>> 
>> 	struct xenstore_domain_interface *intf = interface->addr;
>> 	XENSTORE_RING_IDX cons, prod; /* offsets only */
>> -	int to_read;
>> +	int total_data, data;
>> 	uint32_t connection;
>> 
>> 	cons = *(volatile uint32_t*)&intf->req_cons;
>> @@ -65,19 +65,28 @@ CAMLprim value ml_interface_read(value ml_interface,
>> 	if ((prod - cons) > XENSTORE_RING_SIZE)
>> 		caml_failwith("bad connection");
>> 
>> -	if (prod == cons) {
>> +	/* Check for any pending data at all. */
>> +	total_data = prod - cons;
>> +	if (total_data == 0) {
>> +		/* No pending data at all. */
>> 		result = 0;
>> 		goto exit;
>> 	}
>> -	cons = MASK_XENSTORE_IDX(cons);
>> -	prod = MASK_XENSTORE_IDX(prod);
>> -	if (prod > cons)
>> -		to_read = prod - cons;
>> -	else
>> -		to_read = XENSTORE_RING_SIZE - cons;
>> -	if (to_read < len)
>> -		len = to_read;
>> -	memcpy(buffer, intf->req + cons, len);
>> +	else if (total_data < len)
>> +		/* Some data - make a partial read. */
>> +		len = total_data;
>> +
>> +	/* Check whether data crosses the end of the ring. */
>> +	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);
>> +	}
>> +
>> 	xen_mb();
>> 	intf->req_cons += len;
>> 	result = len;
>> @@ -100,7 +109,7 @@ CAMLprim value ml_interface_write(value ml_interface,
>> 
>> 	struct xenstore_domain_interface *intf = interface->addr;
>> 	XENSTORE_RING_IDX cons, prod;
>> -	int can_write;
>> +	int total_space, space;
>> 	uint32_t connection;
>> 
>> 	cons = *(volatile uint32_t*)&intf->rsp_cons;
>> @@ -111,17 +120,32 @@ CAMLprim value ml_interface_write(value ml_interface,
>> 		caml_raise_constant(*caml_named_value("Xb.Reconnect"));
>> 
>> 	xen_mb();
>> -	if ( (prod - cons) >= XENSTORE_RING_SIZE ) {
>> +
>> +	if ((prod - cons) > XENSTORE_RING_SIZE)
>> +		caml_failwith("bad connection");
>> +
>> +	/* Check for space to write the full message. */
>> +	total_space = XENSTORE_RING_SIZE - (prod - cons);
>> +	if (total_space == 0) {
>> +		/* No space at all - exit having done nothing. */
>> 		result = 0;
>> 		goto exit;
>> 	}
>> -	if (MASK_XENSTORE_IDX(prod) >= MASK_XENSTORE_IDX(cons))
>> -		can_write = XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(prod);
>> -	else 
>> -		can_write = MASK_XENSTORE_IDX(cons) - MASK_XENSTORE_IDX(prod);
>> -	if (can_write < len)
>> -		len = can_write;
>> -	memcpy(intf->rsp + MASK_XENSTORE_IDX(prod), buffer, len);
>> +	else if (total_space < len)
>> +		/* Some space - make a partial write. */
>> +		len = total_space;
>> +
>> +	/* Check for space until the ring wraps. */
>> +	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 the ring. Write both halves. */
>> +		memcpy(intf->rsp + MASK_XENSTORE_IDX(prod), buffer, space);
>> +		memcpy(intf->rsp, buffer + space, len - space);
>> +	}
>> +
>> 	xen_mb();
>> 	intf->rsp_prod += len;
>> 	result = len;
>> -- 
>> 2.1.4
>> 
> 
> -- 
> Samuel
> As usual, this being a 1.3.x release, I haven't even compiled this
> kernel yet.  So if it works, you should be doubly impressed.
> (Linus Torvalds, announcing kernel 1.3.3 on the linux-kernel mailing list.)

Re: [PATCH v2] tools/ocaml/xb: Correct calculations of data/space the ring

[Wei Liu]

On Tue, Nov 10, 2015 at 10:46:44AM +0000, Andrew Cooper wrote:
> ml_interface_{read,write}() would miscalculate the quantity of
> data/space in the ring if it crossed the ring boundary, and incorrectly
> return a short read/write.
> 
> This causes a protocol stall, as either side of the ring ends up waiting
> for what they believe to be the other side needing to take the next
> action.
> 
> Correct the calculations to cope with crossing the ring boundary.
> 
> In addition, correct the error detection.  It is a hard error if the
> producer index gets more than a ring size ahead of the consumer, or if
> the consumer ever overtakes the producer.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I think I will port this patch to cxenstored at some point. As far as I
can tell cxenstored's data / space calculation is bogus in the same way.

Wei.

Re: [PATCH v2] tools/ocaml/xb: Correct calculations of data/space the ring

[Samuel Thibault]

Wei Liu, on Tue 10 Nov 2015 14:59:17 +0000, wrote:
> I think I will port this patch to cxenstored at some point. As far as I
> can tell cxenstored's data / space calculation is bogus in the same way.

The low-level function return short reads and writes, yes, but that is
handled at a higher level: initialize_fds sets timeout to 0 when there
is still room (domain_can_read() or domain_can_write()). So it will
improve performance a little bit, but not fix actual bugs.

Samuel

Re: [PATCH v2] tools/ocaml/xb: Correct calculations of data/space the ring

[Wei Liu]

On Tue, Nov 10, 2015 at 04:06:13PM +0100, Samuel Thibault wrote:
> Wei Liu, on Tue 10 Nov 2015 14:59:17 +0000, wrote:
> > I think I will port this patch to cxenstored at some point. As far as I
> > can tell cxenstored's data / space calculation is bogus in the same way.
> 
> The low-level function return short reads and writes, yes, but that is
> handled at a higher level: initialize_fds sets timeout to 0 when there
> is still room (domain_can_read() or domain_can_write()). So it will
> improve performance a little bit, but not fix actual bugs.
> 

Yeah, that's true. Just IMHO it would be better if we can actually make
low level routine correct. That's orthogonal to what the upper layer is
doing and should prevent latent bug if upper layer logic changes.

Wei.

> Samuel

-- 
Wei.

Re: [PATCH v2] tools/ocaml/xb: Correct calculations of data/space the ring

[Samuel Thibault]

Wei Liu, on Tue 10 Nov 2015 15:09:33 +0000, wrote:
> On Tue, Nov 10, 2015 at 04:06:13PM +0100, Samuel Thibault wrote:
> > Wei Liu, on Tue 10 Nov 2015 14:59:17 +0000, wrote:
> > > I think I will port this patch to cxenstored at some point. As far as I
> > > can tell cxenstored's data / space calculation is bogus in the same way.
> > 
> > The low-level function return short reads and writes, yes, but that is
> > handled at a higher level: initialize_fds sets timeout to 0 when there
> > is still room (domain_can_read() or domain_can_write()). So it will
> > improve performance a little bit, but not fix actual bugs.
> > 
> 
> Yeah, that's true. Just IMHO it would be better if we can actually make
> low level routine correct. That's orthogonal to what the upper layer is
> doing and should prevent latent bug if upper layer logic changes.

Agreed.

Samuel