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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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

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

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


> 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