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

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

On 26 Nov 2013, at 18:27, David Scott <dave.scott@xxxxxxxxxxxxx> wrote:
> On 26/11/13 17:52, 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.
> 
> I think the approach sounds good. One or two questions inline: (I think 
> only the last comment about Poll_events_val is significant)
> 
>> [...]
>>  #include "caml_xentoollog.h"
>> 
>> +#define CAMLdone do{ \
>> +caml_local_roots = caml__frame; \
>> +}while (0)
> 
> Might be worth a comment explaining that this is intended to be 
> "CAMLreturn" without the return.

Ok, I’ll add that.

>> […]
>> @@ -651,9 +715,20 @@ value stub_xl_device_disk_of_vdev(value ctx, value 
>> domid, value vdev)
>>      CAMLparam3(ctx, domid, vdev);
>>      CAMLlocal1(disk);
>>      libxl_device_disk c_disk;
>> -    libxl_vdev_to_device_disk(CTX, Int_val(domid), String_val(vdev), 
>> &c_disk);
>> +    char *c_vdev;
>> +    uint32_t c_domid = Int_val(domid);
>> +
>> +    c_vdev = malloc((caml_string_length(vdev) + 1) * sizeof(char *));
>> +    strcpy(c_vdev, String_val(vdev));
> 
> Perhaps this would be clearer as strdup(String_val(vdev))?

I guess so! I just copied the example from the OCaml manual, but strdup looks 
easier indeed.

>> […]
>> @@ -1197,15 +1335,23 @@ value stub_libxl_osevent_occurred_fd(value ctx, 
>> value for_libxl, value fd,
>>      value events, value revents)
>>  {
>>      CAMLparam5(ctx, for_libxl, fd, events, revents);
>> +
>> +    caml_enter_blocking_section();
>>      libxl_osevent_occurred_fd(CTX, (void *) for_libxl, Int_val(fd),
>>              Poll_events_val(events), Poll_events_val(revents));
> 
> Is the "events" OCaml value being accessed here without the heap lock?

Yep, I missed that one!

I’ll send an updated patch in a minute…

Cheers,
Rob


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