Re: [Xen-devel] [PATCH 15/15] libxl: New event generation API

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2011-12-12 at 12:30 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 15/15] libxl: New event 
> generation API"):
> > On Mon, 2011-12-12 at 11:40 +0000, Ian Jackson wrote:
> > > Well, we have to have an internal version because we don't want to
> > > call GC_INIT again, obviously.
> > 
> > In this case it would take a ctx not a gc and it's ok and expected for
> > libxl a function calling another libxl function to end up with another
> > gc in the inner function (it happens all the time).
> 
> This is not allowed in functions which might generate events (ie,
> which might call libxl__event_occurred), because the application's
> event callback will be called at an unexpected point in libxl's
> execution.  This has three problems:
> 
>  * There is a reentrancy hazard.  Exactly whether this is a problem in
>    a specific case is difficult to analyse; hence the recording of
>    pending callbacks in the gc, so that they can be delayed until the
>    gc is freed.
> 
>  * The application's callbacks will be called with the libxl ctx
>    locked.  This might result in deadlock.
> 
>  * The  application's callbacks  might be  called in  the  wrong order
>    because the  inner gc (containing  later events) is  unwound before
>    the outer gc (containing earlier events).
> 
> In general this means that functions which lock the ctx must not
> allocate an inner gc.

Hmm, yes that all makes sense :-/

I was a little surprised when I looked when writing my previous reply
that LIBXL_INITGC doesn't actually take nesting into account. I had
expected that LIBXL_INITGC would return (or somehow chain) the existing
GC when libxl is calling itself such that all the freeing work (and now
callbacks) would only happen when exiting the final frame.

It's probably not so important for memory allocation cleanup but it
would be a semantically useful change for the callbacks if we could
arrange for them to happen only on final exit of the library -- exactly
because of the difficulty of reasoning about things otherwise. Alas I
can't think of a good way to do this since the ctx can be shared, thread
local storage would be one or libxl__gc_owner could return a special
"nested ctx" instead of the real outer ctx, but, ick.

If we can't figure a way round this then the restriction about libxl not
calling into itself via its own public interfaces should be documented
(apologies if I've simply missed that bit, I did go and look because I
thought I recalled seeing it, but I didn't find it, I think I was
thinking about the comment associated with the CTX lock). In particular
if the restriction is that you cannot call public API functions which
might generate events from within libxl then those functions need to be
clearly annotated as potentially generating events.

> > IOW if you make beforepoll_internal take the lock as you suggest then
> > you may as well inline it into libxl_osevent_beforepoll (removing the
> > second lock use) and use that internally too.
> 
> beforepoll_unlocked is called in two places: libxl_osevent_beforepoll,
> and libxl_event_wait.

My suggestion was to call libxl_osevent_beforepoll from
libxl_event_wait. The documentation for libxl_event_wait even says this
is how libxl_event_wait is implemented (I know I'm not a liberty to take
that as literally as that).

Of course your explanation above put paid to that idea, unless we can
work around it.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel