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

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

Ian Campbell wrote:
> 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.

Ok, will do.

[...]
> > 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?

Exactly.

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

That was another option indeed. It's just that the return types vary a bit, 
which makes this slightly more complicated.

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

Yes, I copied it from libxl :)

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

Yes, I think that makes sense. I'll keep it in mind.

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

This fall into the "copying ocaml values to C before dropping the ocaml lock" 
category: it avoids calling the aohow_val function inside the argument list of 
the libxl functions, which are called without the ocaml lock.

I reorganised the aohow_val function a little, just to make it a little nicer 
to use. The dynamic allocation is not for the locking stuff.

Cheers,
Rob

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