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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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@citrix.com>
>>
>> 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)
>