Re: [Xen-devel] [PATCH 1 of 2] tools/ocaml: libxc bindings: Fix stub_xc_readconsolering()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/02/13 14:26, Ian Campbell wrote:
> On Thu, 2013-02-14 at 19:29 +0000, Andrew Cooper wrote:
>> There are two problems with this function:
>>   * The caml_{enter,leave}_blocking_section() calls mean that two threads can
>>     compete for use of the static ring[] buffer.
>>   * It fails to retrieve the entire buffer if the hypervisor has used 32K or
>>     more of its console ring.
>>
>> Unfortunately, fixing the code in a sensible is rather difficult.  The Ocaml
>> caller expects to get the entire console ring in a single buffer.  There are
>> no mechanisms to ask Xen for the size of the console ring (which is fixed
>> after boot).
> It would be easy to add a sysctl to get this

Yes.  I didn't because of a combination of the reasons below, and
because I needed a fix for our automated testing, but I think I have had
a cunning idea.

>
>>   Even if there was a way to get the size of the console ring, the
>> XEN_SYSCTL_readconsole hypercall itself races with printk(), leading to
>> possibly discontinuities if the ring is full.
> Right, that one is tricky, but as you say we could accept the
> inefficiencies here.
>
>>   The requirement for a NULL
>> terminating string means that a full console ring will appear to fail when 
>> use
>> the correct power of two, forcing us to double up again.
> Double? Or just allocate buffersize + 1? We could also consider fixing
> the XEN_SYSCTL_readconsole interface to remove this issue.

Requires buffersize + 1 + nr_of_chars_from_a_raced_printk to get any
indication back from the hypercall that we have indeed read the full ring.

Choosing buffersize rounded up to the next PAGE_SIZE will suffice except
in extreme circumstances.

>
>> Furthermore, libxc will bounce the allocated buffer, as will 
>> caml_copy_string().
> You could avoid the first of these by using xc_hypercall_buffer_alloc or
> xc_hypercall_buffer_alloc_pages in the caller, these are exposed from
> libxc exactly to allow us to avoid bouncing of larger objects.

I will investigate using these.

~Andrew

>
>
>> On the other hand, this is very definitely for debugging and testing, so lets
>> do the best we can to get the entire buffer, and accept the inefficiencies.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>
>> diff -r 63594ce1708f -r 1daa30509f1c tools/ocaml/libs/xc/xenctrl_stubs.c
>> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
>> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
>> @@ -523,27 +523,55 @@ CAMLprim value stub_xc_evtchn_reset(valu
>>      CAMLreturn(Val_unit);
>>  }
>>  
>> -
>> -#define RING_SIZE 32768
>> -static char ring[RING_SIZE];
>> -
>> +/* Maximum size of console ring we are prepared to read, 16MB */
>> +#define MAX_CONSOLE_RING_SIZE (1U << 24)
>>  CAMLprim value stub_xc_readconsolering(value xch)
>>  {
>> -    unsigned int size = RING_SIZE - 1;
>> -    char *ring_ptr = ring;
>> -    int retval;
>> +    /* 32K starting size (adj for loop doubling at start) */
>> +    unsigned int ring_size = 1U << 14;
>> +    unsigned int nr_chars = ring_size;
>> +    char * ring = NULL;
>> +    int r;
>>  
>>      CAMLparam1(xch);
>> +    CAMLlocal1(conring);
>>  
>> -    caml_enter_blocking_section();
>> -    retval = xc_readconsolering(_H(xch), ring_ptr, &size, 0, 0, NULL);
>> -    caml_leave_blocking_section();
>> +    do
>> +    {
>> +            ring_size *= 2;
>> +            if ( ring_size > MAX_CONSOLE_RING_SIZE )
>> +            {
>> +                    caml_failwith("Console ring too large");
>> +                    CAMLreturn(Val_unit);
>> +            }
>> +            nr_chars = ring_size;
>>  
>> -    if (retval)
>> -            failwith_xc(_H(xch));
>> +            free(ring);
>> +            if ( ! ( ring = malloc(ring_size) ) )
>> +            {
>> +                    caml_failwith("Out of memory");
>> +                    CAMLreturn(Val_unit);
>> +            }
>>  
>> -    ring[size] = '\0';
>> -    CAMLreturn(caml_copy_string(ring));
>> +            caml_enter_blocking_section();
>> +            r = xc_readconsolering(_H(xch), ring, &nr_chars, 0, 0, NULL);
>> +            caml_leave_blocking_section();
>> +
>> +            if ( r )
>> +            {
>> +                    failwith_xc(_H(xch));
>> +                    free(ring);
>> +                    CAMLreturn(Val_unit);
>> +            }
>> +
>> +            /* If nr_chars == ring_size, we have not read the entire ring.
>> +             * Try again with a larger buffer. */
>> +    } while ( nr_chars >= ring_size );
>> +
>> +    ring[nr_chars] = '\0';
>> +    conring = caml_copy_string(ring);
>> +    free(ring);
>> +    CAMLreturn(conring);
>>  }
>>  
>>  CAMLprim value stub_xc_send_debug_keys(value xch, value keys)
>


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