Re: [Xen-devel] [PATCH v6 08/12] libxl: ocaml: drop the ocaml heap lock before calling into libxl

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Rob Hoes writes ("[PATCH v6 08/12] libxl: ocaml: drop the ocaml heap lock 
before calling into libxl"):
> To avoid deadlocks, we drop the ocaml heap lock before entering
> libxl, and reacquire it in callbacks to ocaml. This way, the ocaml
> heap lock is never held together with the libxl lock, except in
> osevent registration callbacks, and xentoollog callbacks. If we
> guarantee to not call any libxl functions inside those callbacks, we
> can avoid deadlocks.

I think this is right.

> This patch handle the dropping and reacquiring of the ocaml heap lock by the
> caml_enter_blocking_section and caml_leave_blocking_section functions, and
> related macros. We are also careful to not call any functions that access the
> ocaml heap while the ocaml heap lock is dropped. This often involves copying
> ocaml values to C before dropping the ocaml lock.

Right.

>  void async_callback(libxl_ctx *ctx, int rc, void *for_callback)
>  {
> +     caml_leave_blocking_section();
>       CAMLparam0();
>       CAMLlocal1(error);
>       int *task = (int *) for_callback;
> @@ -390,19 +396,22 @@ void async_callback(libxl_ctx *ctx, int rc, void 
> *for_callback)
>               error = Val_some(Val_error(rc));
>  
>       caml_callback2(*func, error, (value) for_callback);
> +     CAMLdone;
> +     caml_enter_blocking_section();
>  }
>  
> -static libxl_asyncop_how *aohow_val(value async, libxl_asyncop_how *ao_how)
> +static libxl_asyncop_how *aohow_val(value async)
>  {
>       CAMLparam1(async);
> +     libxl_asyncop_how *ao_how = NULL;
>  
>       if (async != Val_none) {
> +             libxl_asyncop_how *ao_how = malloc(sizeof(*ao_how));
>               ao_how->callback = async_callback;
>               ao_how->u.for_callback = (void *) Some_val(async);
> -             CAMLreturnT(libxl_asyncop_how *, ao_how);
>       }
> -     else
> -             CAMLreturnT(libxl_asyncop_how *, NULL);
> +
> +     CAMLreturnT(libxl_asyncop_how *, ao_how);
>  }

I don't see here how "async" is protected from being gc'd (or,
perhaps, moved by the ocaml gc) during the execution of the
long-running libxl operation.  The pointer to it is hidden inside the
(now malloc'd) ao_how.  AIUI the "CAMLparam1...CAMLreturnT" pair
protect it during the aohow_val function ?

I think this problem probably existed beforehand too.

Also, is it really safe to bury these calls to CAMLparam1 etc. on
async inside aohow_val ?  AIUI these CAML gc protection things need to
occur as the very first thing inside the ocaml stub function, before
calling anything which might trigger the ocaml gc.

I haven't gone through all the callers of aohow_val looking for calls
into ocaml before aohow_val but it seems a fragile pattern.

So in summary I think aohow_val ought to: assume that a ref to async
has been taken by its caller for the duration of this entry (so not
use CAMLparam1 on it) but that that will be dropped while the function
completes (so it needs to make a new ref by calling
caml_register_global_root and release it again in
caml_remove_global_root after the callback is done).


> @@ -508,10 +545,17 @@ value stub_libxl_domain_suspend(value ctx, value domid, 
> value fd, value async, v
>  {
>       CAMLparam5(ctx, domid, fd, async, unit);
>       int ret;
> -     libxl_asyncop_how ao_how;
> +     uint32_t c_domid = Int_val(domid);
> +     int c_fd = Int_val(fd);
> +     libxl_asyncop_how *ao_how = aohow_val(async);
> +
> +     caml_enter_blocking_section();
> +     ret = libxl_domain_suspend(CTX, c_domid, c_fd, 0, ao_how);
> +     caml_leave_blocking_section();
> +
> +     if (ao_how)
> +             free(ao_how);
>  
> -     ret = libxl_domain_suspend(CTX, Int_val(domid), Int_val(fd), 0,
> -             aohow_val(async, &ao_how));
>       if (ret != 0)
>               failwith_xl(ret, "domain_suspend");

These functions are getting more and more boilerplatey :-/.

Ian.

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