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

2 Dec 2008

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_@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@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

    