[libvirt] PATCH: 25/28: Thread safety for libvirtd event loop

[Daniel P. Berrange]

On Wed, Dec 03, 2008 at 03:48:05PM +0100, Daniel Veillard wrote:
> On Mon, Dec 01, 2008 at 12:18:24AM +0000, Daniel P. Berrange wrote:
> > This patch makes the event loop in the libvirtd daemon thread safe, and
> > re-entrant. This allows one thread to add/remove/update timers/file handle
> > watches while other thread is doing the poll. This sometimes requires that
> > we wakeup the main thread to make it see changes to the poll FD list. We
> > use the traditional self-pipe trick for this task.
> [...]
> > -static int virEventDispatchHandles(struct pollfd *fds) {
> > +static int virEventDispatchHandles(int nfds, struct pollfd *fds) {
> >      int i;
> > -    virEventHandleType hEvents;
> > -    /* Save this now - it may be changed during dispatch */
> > -    int nhandles = eventLoop.handlesCount;
> >  
> > -    for (i = 0 ; i < nhandles ; i++) {
> > +    for (i = 0 ; i < nfds ; i++) {
> >          if (eventLoop.handles[i].deleted) {
> >              EVENT_DEBUG("Skip deleted %d", eventLoop.handles[i].fd);
> >              continue;
> >          }
> >  
> >          if (fds[i].revents) {
> > -            hEvents = virPollEventToEventHandleType(fds[i].revents);
> > -            EVENT_DEBUG("Dispatch %d %d %d %p",
> > -                        eventLoop.handles[i].watch,
> > -                        fds[i].fd, fds[i].revents,
> > -                        eventLoop.handles[i].opaque);
> > -            (eventLoop.handles[i].cb)(eventLoop.handles[i].watch,
> > -                                      fds[i].fd,
> > -                                      hEvents,
> > -                                      eventLoop.handles[i].opaque);
> > +            virEventHandleCallback cb = eventLoop.handles[i].cb;
> > +            void *opaque = eventLoop.handles[i].opaque;
> > +            int hEvents = virPollEventToEventHandleType(fds[i].revents);
> > +            EVENT_DEBUG("Dispatch %d %d %p", fds[i].fd,
> > +                        fds[i].revents, eventLoop.handles[i].opaque);
> > +            virEventUnlock();
> > +            (cb)(eventLoop.handles[i].watch,
> > +                 fds[i].fd, hEvents, opaque);
> > +            virEventLock();
> >          }
> >      }
> 
>   Hum, if this routine assume we enter it with the event lock and keep
> it though its lifetime except when dispatching, then I guess it's worth
> a comment, right ? Is there only one thread which can access or
> unregister fds, or do we have another lock in place ?

The virEventRemoveHandle / RemoveTimeout functions will take the lock,
but then merely set the flag
 
  eventLoop.handles[i].deleted = 1;
 
This ensures that if another thread is currently in virEventDispatchHandles
doing the callback, it won't have the list of handles modified behind its
back. 

Later on, the virEventCleanupHandles() will actually free the memory
for all handles with the 'deleted' flag set to 1 when it knows no
callbacks are being processed.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|