Re: [Xen-devel] [PATCH v2-resend 22/30] libxl: ocaml: event management [and 1 more messages]

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Rob Hoes writes ("RE: [Xen-devel] [PATCH v2-resend 22/30] libxl: ocaml: event 
management [and 1 more messages]"):
> > But I haven't had an answer.
> 
> Sorry, I had missed this... :(

NP.

> > I think that whether this is the right approach depends on how
> > event loops are traditionally done in ocaml.
> 
> Having bindings to the low-level functions
> libxl_osevent_register_hooks and related, allows us to run an event
> loop in OCaml; either one we write ourselves, or one that is
> available elsewhere.

Right.

> We are currently running a straightforward, custom event loop in
> xenopsd. It simply maintains a list of fds and timeouts, and runs
> poll in a loop, as you would expect (see
> https://github.com/xapi-project/xenopsd/blob/master/xl/xenlight_events.ml
> for the full code).

OK, that makes sense.

> Exposing the low-level event hooks in OCaml gives us the choice to
> implement either of the above options.

Right.  Good.

I have some questions about the details:


Firstly, locking.  Is all of this code running inside a single big
lock which is never released ?  If so I think everything is fine.  If
something more complicated might be happening then there are possible
deadlock problems.

See the comment on "Lock hierarchy" in libxl_event.h.  In particular,
I worry about the following scenario:

AIUI in your setup the callback might, in principle, call any ocaml
function.  That ocaml function might call back into some long-running
C function which temporarily gives up the ocaml lock.  If another
thread then takes the ocaml lock, and enters libxl, it will block
waiting for the libxl lock.  The original thread will presumably at
some point come back from the long-running operation and try to
acquire the ocaml lock.  Deadlock.

Have you considered this problem ?


Rob Hoes writes ("[Xen-devel] [PATCH v2-resend 22/30] libxl: ocaml: event 
management"):
> +/* Event handling */
...
> +short Poll_val(value event)
...
> +     switch (Int_val(event)) {
> +             case 0: res = POLLIN; break;
> +             case 1: res = POLLPRI; break;
...
> +short Poll_events_val(value event_list)
...
> +     while (event_list != Val_emptylist) {
> +             events |= Poll_val(Field(event_list, 0));
> +             event_list = Field(event_list, 1);

This is quite striking.  You're converting a bitfield into a linked
list of consed enums.  Does ocaml really not have something more
resembling a set-of-small-enum type, represeted as a bitfield ?

The result is going to be a lot of consing every time libxl scratches
its nose.  In some cases very frequently.  For example, if we're
running the bootloader and copying input and output back and forth,
we're using the datacopier machinery in libxl_aoutils.c.  That
involves enabling the fd writeability callback on each output fd,
every time data is read from the input fd, and then disabling the
writeability callback every time the data has been written.  So one
fd register/deregister pair for every lump of console output.  There
are probably other examples.


...
> +value stub_libxl_event_register_callbacks(value ctx, value user)
...
> +     c_user = malloc(sizeof(*c_user));
> +     c_user->user = (void *) user;

Shouldn't you be using some kind of error-handling wrapper for
malloc ?  Having the program dereference null when malloc fails is
rather an unfortunate failure mode.  At the very least printing
something to stderr would be useful.


Thanks,
Ian.

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