Re: [libvirt] [PATCH] events: Propose a separate lock for event queue
On Mon, Oct 10, 2011 at 01:45:50PM +0200, Michal Privoznik wrote:
Currently, push & pop from event queue (both server & client
side)
rely on lock from higher levels, e.g. on driver lock (qemu),
private_data (remote), ...; This alone is not sufficient as not
every function that interacts with this queue can/does lock,
esp. in client where we have a different approach, "passing
the buck".
Therefore we need a separate lock just to protect event queue.
+static void
+virDomainEventStateLock(virDomainEventStatePtr state)
+{
+ if (!state)
+ return;
+
+ virMutexLock(&state->lock);
+}
+
+static void
+virDomainEventStateUnlock(virDomainEventStatePtr state)
+{
+ if (!state)
+ return;
+
+ virMutexUnlock(&state->lock);
+}
Normal coding policy is to *not* check for NULL in the mutex
functions. It is safer to crash on NULL, then to return without
locking.
virDomainEventStateQueue(virDomainEventStatePtr state,
virDomainEventPtr event)
{
+ int count;
+
if (state->timer < 0) {
virDomainEventFree(event);
return;
}
+ virDomainEventStateLock(state);
+
if (virDomainEventQueuePush(state->queue, event) < 0) {
VIR_DEBUG("Error adding event to queue");
virDomainEventFree(event);
}
- if (state->queue->count == 1)
+ count = state->queue->count;
+ virDomainEventStateUnlock(state);
+
+ if (count == 1)
virEventUpdateTimeout(state->timer, 0);
+
}
Updating the timer outside the lock is unsafe.
void
@@ -1182,6 +1217,7 @@ virDomainEventStateFlush(virDomainEventStatePtr state,
{
virDomainEventQueue tempQueue;
+ virDomainEventStateLock(state);
state->isDispatching = true;
/* Copy the queue, so we're reentrant safe when dispatchFunc drops the
@@ -1190,6 +1226,7 @@ virDomainEventStateFlush(virDomainEventStatePtr state,
tempQueue.events = state->queue->events;
state->queue->count = 0;
state->queue->events = NULL;
+ virDomainEventStateUnlock(state);
virEventUpdateTimeout(state->timer, -1);
virDomainEventQueueDispatch(&tempQueue,
This is unsafe
@@ -1198,9 +1235,11 @@
virDomainEventStateFlush(virDomainEventStatePtr state,
opaque);
/* Purge any deleted callbacks */
+ virDomainEventStateLock(state);
virDomainEventCallbackListPurgeMarked(state->callbacks);
state->isDispatching = false;
+ virDomainEventStateUnlock(state);
}
With these two timer updates you have a race condition with 2 threads:
T1: virDomainEventStateFlush T2: virDomainEventQueue
1. Lock
2. Set count = 0;
3. Unlock
4. lock
5. Save current count value
6. Queue new event
7. unlock
8. virEventUpdateTimer(1);
9. virEventUpdateTimer(-1)
Now the timer is disabled, but we have an event pending
You need to make sure all timer updates are *inside* the critical
section.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|