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

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

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@xxxxxxxxxx>

Reviewed-by: Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>

> ---
> CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: David Scott <dave@xxxxxxxxxx>
> CC: Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>
> 
> 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.)

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