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

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

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

Thanks. I'll add a summary of this to the commit message.

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

Yes, I have encountered such deadlocks, and got around them by making all calls 
into libxl mutually exclusive. This should mean that there can be only one 
thread at a time holding or waiting for the libxl lock, and I think that avoids 
the deadlock you described. It seems to be running smoothly now in our test 
setup.

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

Dave has already responded to this.

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

Yes, that would be better. I'll update the patch.

Thanks,
Rob

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