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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list