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

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

Ian Jackson wrote:
> > -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 ?

To keep the async user values "alive", we keep them in a set in OCaml for the 
duration of the async call. See the code that deals with "async_users" in 
xenlight.ml.in. The GC will definitely not destroy these values.

I'm not sure if the GC may _move_ the value (I assumed it wouldn't)...

Dave, do you know?

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

The CAMLparam macros indeed need to be called as soon as you enter a stub, but 
that is what we do. A function like stub_libxl_domain_create_new calls 
CAMLparam on async in the first line.

CAMLparam is then called a second time on async, inside aohow_val, even though 
it may not be strictly needed. This is fine, however, as long as CAMLparam and 
CAMLreturn occur in pairs and are properly nested. CAMLreturn does nothing more 
than restore the list of roots to what it was before the preceeding CAMLparam.

I think it is good practise to just use CAMLparam + CAMLreturn in every 
function that deals with ocaml values, even if they might be "optimised away", 
just to avoid possible mistakes.

Rob

> 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