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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

If you resent a single patch can you use "git send-email --in-reply-to"
to chain it into the original thread so I don't loose it when I come to
apply. Thanks.

On Tue, 2013-11-26 at 23:15 +0000, Rob Hoes wrote:
> Ocaml has a heap lock which must be held whenever ocaml code is running. Ocaml
> usually drops this lock when it enters a potentially blocking low-level
> function, such as writing to a file. Libxl has its own lock, which it may
> acquire when being called.
> 
> Things get interesting when libxl calls back into ocaml code. There is a risk
> of ending up in a deadlock when a thread holds both locks at the same time,
> then temporarily drop the ocaml lock, while another thread calls another libxl
> function.
> 
> 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.
> 
> 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.
> 
> Signed-off-by: Rob Hoes <rob.hoes@xxxxxxxxxx>
> CC: Ian Campbell <ian.campbell@xxxxxxxxxx>
> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> CC: Dave Scott <dave.scott@xxxxxxxxxxxxx>
> ---
>  tools/ocaml/libs/xentoollog/Makefile           |    3 +
>  tools/ocaml/libs/xentoollog/xentoollog_stubs.c |   13 +-
>  tools/ocaml/libs/xl/Makefile                   |    5 +-
>  tools/ocaml/libs/xl/xenlight_stubs.c           |  265 
> +++++++++++++++++++-----
>  4 files changed, 229 insertions(+), 57 deletions(-)
> 
> diff --git a/tools/ocaml/libs/xentoollog/Makefile 
> b/tools/ocaml/libs/xentoollog/Makefile
> index e535ba5..471b428 100644
> --- a/tools/ocaml/libs/xentoollog/Makefile
> +++ b/tools/ocaml/libs/xentoollog/Makefile
> @@ -2,6 +2,9 @@ TOPLEVEL=$(CURDIR)/../..
>  XEN_ROOT=$(TOPLEVEL)/../..
>  include $(TOPLEVEL)/common.make
>  
> +# allow mixed declarations and code
> +CFLAGS += -Wno-declaration-after-statement
> +
>  CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenguest)
>  OCAMLINCLUDE +=
>  
> diff --git a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c 
> b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
> index 3b2f91b..daf48fe 100644
> --- a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
> +++ b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
> @@ -31,6 +31,11 @@
>  
>  #include "caml_xentoollog.h"
>  
> +/* The following is equal to the CAMLreturn macro, but without the return */
> +#define CAMLdone do{ \
> +caml_local_roots = caml__frame; \
> +}while (0)
> +
>  #define XTL ((xentoollog_logger *) Xtl_val(handle))
>  
>  static char * dup_String_val(value s)
> @@ -81,6 +86,7 @@ static void stub_xtl_ocaml_vmessage(struct 
> xentoollog_logger *logger,
>       const char *format,
>       va_list al)
>  {
> +     caml_leave_blocking_section();
>       CAMLparam0();
>       CAMLlocalN(args, 4);
>       struct caml_xtl *xtl = (struct caml_xtl*)logger;
> @@ -103,7 +109,8 @@ static void stub_xtl_ocaml_vmessage(struct 
> xentoollog_logger *logger,
>       free(msg);
>  
>       caml_callbackN(*func, 4, args);
> -     CAMLreturn0;
> +     CAMLdone;
> +     caml_enter_blocking_section();
>  }

So the concern here is that CAMLreturn0/CAMLparam0 might call into ocaml
heap code, and trigger the gc and therefore need the lock? is that
right?

I might have been tempted to wrap each callback in a macro/function
which ensured the locking was appropriate rather than enabling
-Wno-declaration-after-statement but this works too.

We use no-declaration-after-statement in libxl itself so I have no
objection to it.

The dropping/taking of the lock is a bit repetitive as well. As a future
cleanup you might consider 
        #define LIBXL(call) ({ \
            int libxl_retval;\
            caml_leave...();\
            libxl_retval = call;\
            caml_enter...();\
            libxl_retval;
        })
        
        foo = LIBXL( libxl_do_something(CTX) );

I dunno, maybe that's not cleaner. Or s/LIBXL/CAML_UNLOCKED/ perhaps is
more expressive. Some of those ";" might need to be ",".

> -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);
>  }
>  
>  value stub_libxl_domain_create_new(value ctx, value domain_config, value 
> async, value unit)
> @@ -411,7 +420,7 @@ value stub_libxl_domain_create_new(value ctx, value 
> domain_config, value async,
>       int ret;
>       libxl_domain_config c_dconfig;
>       uint32_t c_domid;
> -     libxl_asyncop_how ao_how;
> +     libxl_asyncop_how *ao_how;

These changes seem to have something (subtle) to do with the lifecycle
of the ao_how rather than any of the locking stuff which is the meat of
this change? Does the ao_how need to be dynamically allocated rather
than stack based for some reason?

If this relates the locking stuff can we get a description in the commit
message please.




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