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

[David Scott <dave@xxxxxxxxxx>]

> On 10 Nov 2015, at 11:41, Samuel Thibault <samuel.thibault@xxxxxxxxxxxx> 
> 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@xxxxxxxxxx>
> 
> Reviewed-by: Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>

Looks good to me too

Reviewed-by: David Scott <dave@xxxxxxxxxx>

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