[libvirt] PATCH: 8/28: Thread safety for domain events code

[Daniel Veillard]

On Sun, Nov 30, 2008 at 11:47:21PM +0000, Daniel P. Berrange wrote:
> This patch adds thread safety to the domain events processing code of
> the QEMU driver. This entailed rather a large refactoring of the domain
> events code and its quite complicated to explain.
> 
> - A convenient helper is added for creating event queues
> 
>         virDomainEventQueuePtr virDomainEventQueueNew(void);
> 
> - Convenient helpers are added for creating virDomainEventPtr instances
>   from a variety of sources - each driver has different needs
> 
>    virDomainEventPtr virDomainEventNew(int id, const char *name, const unsigned char *uuid, int type, int detail);
>    virDomainEventPtr virDomainEventNewFromDom(virDomainPtr dom, int type, int detail);
>    virDomainEventPtr virDomainEventNewFromObj(virDomainObjPtr obj, int type, int detail);
>    virDomainEventPtr virDomainEventNewFromDef(virDomainDefPtr def, int type, int detail);
> 
> - The virDomainEvent struct held a reference to a virDomainPtr object.
>   This is replaced with a direct triple (id, name, uuid), avoiding the
>   need to instantiate virDomainPtr objects deep inside the QEMU code.
> 
> - The virDomainEventQueuePush() method is changed to take a
>   virDomainEventPtr object, rather than a virDomainPtr object
> 
> These last two changes are important to allow safe re-entrancy of the 
> event dispatch process. The virDomainEventPtr instance can be allocated
> within a critical section lockde on the virDomainObjPtr instance, while
> the event is queued outside the critical section, while only the driver
> lock is held. Without this we'd have to hold the per-driver lock over
> a much larger block of code which hurts concurrancy.
> 
> The QEMU driver cannot directly dispatch events, instead we have to
> follow the remote driver and maintain a queue of pending events, and
> use a timer to flush them. Again this is neccessary to improve
> concurrency & provide safe re-entrancy.
> 
> Since we have two driver maintaining queues of evnts I add more 
> helper functions to allow code sharing
> 
>  - Send a single vent to a list of callbacks:
> 
>   void virDomainEventDispatch(virDomainEventPtr event,
>                               virDomainEventCallbackListPtr cbs,
>                               virDomainEventDispatchFunc dispatch,
>                               void *opaque);
> 
>  - Send a set of queued events to a list of callbacks
> 
>   void virDomainEventQueueDispatch(virDomainEventQueuePtr queue,
>                                    virDomainEventCallbackListPtr cbs,
>                                    virDomainEventDispatchFunc dispatch,
>                                    void *opaque);
> 
> The virDomainEventDispatchFunc is what is invoked to finally dispatch
> a single event, to a single registered callback. The default impl just
> invokes the callback directly. The QEMU driver, however, uses a wrapper
> which first releases its driver lock, invokes the callback, and then
> re-aquires the lock.
> 
> As a further complication it is not safe for virDomainEventUnregister
> to actually remove the callback from the list directly. Instead we
> add a helper that simply marks it as removed, and then actually 
> purge it from a safe point in the code, when its guarenteed that the
> driver isn't in the middle of dispatching.
> 
> As well as making the QEMU driver thread safe, this also takes the
> opportunity to refactor the Xen / remote drivers to use more helpers

  I must admit I'm a bit lost ...

> @@ -192,14 +291,18 @@ virDomainEventQueueFree(virDomainEventQu
>  virDomainEventQueueFree(virDomainEventQueuePtr queue)
>  {
>      int i;
> -    for ( i=0 ; i<queue->count ; i++ ) {
> -        VIR_FREE(queue->events[i]);
> +    if (!queue)
> +        return;
> +
> +    for (i = 0; i < queue->count ; i++) {
> +        virDomainEventFree(queue->events[i]);
>      }
> +    VIR_FREE(queue->events);
>      VIR_FREE(queue);
>  }

  okay we were leaking queue->events here !

[...]
>  struct _virDomainEventCallback {
>      virConnectPtr conn;
>      virConnectDomainEventCallback cb;
>      void *opaque;
>      virFreeCallback freecb;
> -
> +    int deleted;
>  };

  Yup, I'm lost here ... seems we are making something asynchronous here

>  struct _virDomainEvent {
> -    virDomainPtr dom;
> -    int event;
> +    int id;
> +    char *name;
> +    unsigned char uuid[VIR_UUID_BUFLEN];
> +    int type;
>      int detail;
>  };

  Okay we don't want to held references anymore. But the lookups will
require search and locking, I completely fail to build a mental
representation where I can figure out why we don't risk deadlocks there.
There is a risk of lock reentrancy I'm sure but I can't see the model.

[...]
> diff --git a/src/libvirt_sym.version.in b/src/libvirt_sym.version.in
> --- a/src/libvirt_sym.version.in
> +++ b/src/libvirt_sym.version.in
> @@ -382,9 +382,21 @@ LIBVIRT_PRIVATE_ at VERSION@ {
>  	virDomainEventCallbackListFree;
>  	virDomainEventCallbackListRemove;
>  	virDomainEventCallbackListRemoveConn;
> +	virDomainEventCallbackListMarkDelete;
> +	virDomainEventCallbackListPurgeMarked;
> +	virDomainEventQueueNew;
>  	virDomainEventQueueFree;
> -	virDomainEventCallbackQueuePop;
> -	virDomainEventCallbackQueuePush;
> +	virDomainEventQueuePop;
> +	virDomainEventQueuePush;
> +	virDomainEventNew;
> +	virDomainEventNewFromDom;
> +	virDomainEventNewFromObj;
> +	virDomainEventNewFromDef;
> +	virDomainEventFree;
> +	virDomainEventDispatchDefaultFunc;
> +	virDomainEventDispatch;
> +	virDomainEventQueueDispatch;
> +

  reminds me I need to make room in libvirt_sym.version.in for APIs
introduced post 0.5.0, I have the (trivial) patch, will post it asap.

   I'm lost, I failed to figure out most of the remaining of this patch,
but don't consider this a blocker, ...
   Still I'm worried about the increased complexity of the code and
making harder for people to contribute patches or drivers.

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/