[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 08/12] libxl: ocaml: drop the ocaml heap lock before calling into libxl
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |