[libvirt] PATCH: 8/25: Concurrent dispatch of RPC methods

[Daniel P. Berrange]

On Fri, Jan 16, 2009 at 12:49:06PM +0100, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange at redhat.com> wrote:
> > This patch re-writes the code for dispatching RPC calls in the
> > remote driver to allow use from multiple threads. Only one thread
> > is allowed to send/recv on the socket at a time though. If another
> > thread comes along it will put itself on a queue and go to sleep.
> > The first thread may actually get around to transmitting the 2nd
> > thread's request while it is waiting for its own reply. It may
> > even get the 2nd threads reply, if its own RPC call is being really
> > slow. So when a thread wakes up from sleeping, it has to check
> > whether its own RPC call has already been processed. Likewise when
> > a thread owning the socket finishes with its own wor, it may have
> > to pass the buck to another thread. The upshot of this, is that
> > we have mutliple RPC calls executing in parallel, and requests+reply
> > are no longer guarenteed to be FIFO on the wire if talking to a new
> > enough server.
> >
> > This refactoring required use of a self-pipe/poll trick for sync
> > between threads, but fortunately gnulib now provides this on Windows
> > too, so there's no compatability problem there.
> 
> Quick summary: dense ;-)
> though lots of moved code.
> 
> I haven't finished, but did find at least one problem, below.
> 
> > diff --git a/src/remote_internal.c b/src/remote_internal.c
> ...
> > @@ -114,6 +164,11 @@ struct private_data {
> >      virDomainEventQueuePtr domainEvents;
> >      /* Timer for flushing domainEvents queue */
> >      int eventFlushTimer;
> > +
> > +    /* List of threads currently doing dispatch */
> > +    int wakeupSend;
> > +    int wakeupRead;
> 
> How about appending "FD" to indicate these are file descriptors.
> The names combined with the comment (which must apply to waitDispatch)
> made me wonder what they represented.  Only when I saw them used
> in safewrite /saferead calls did I get it.

Yes, good idea - and its not really a list of threads either,
so the comment is a little misleading :-)

> > +    /* Serialise header followed by args. */
> > +    xdrmem_create (&xdr, rv->buffer+4, REMOTE_MESSAGE_MAX, XDR_ENCODE);
> > +    if (!xdr_remote_message_header (&xdr, &hdr)) {
> > +        error (flags & REMOTE_CALL_IN_OPEN ? NULL : conn,
> > +               VIR_ERR_RPC, _("xdr_remote_message_header failed"));
> > +        goto error;
> > +    }
> > +
> > +    if (!(*args_filter) (&xdr, args)) {
> > +        error (flags & REMOTE_CALL_IN_OPEN ? NULL : conn, VIR_ERR_RPC,
> > +               _("marshalling args"));
> > +        goto error;
> > +    }
> > +
> > +    /* Get the length stored in buffer. */
> > +    rv->bufferLength = xdr_getpos (&xdr);
> > +    xdr_destroy (&xdr);
> > +
> > +    /* Length must include the length word itself (always encoded in
> > +     * 4 bytes as per RFC 4506).
> > +     */
> > +    rv->bufferLength += 4;
> > +
> > +    /* Encode the length word. */
> > +    xdrmem_create (&xdr, rv->buffer, 4, XDR_ENCODE);
> > +    if (!xdr_int (&xdr, (int *)&rv->bufferLength)) {
> > +        error (flags & REMOTE_CALL_IN_OPEN ? NULL : conn, VIR_ERR_RPC,
> > +               _("xdr_int (length word)"));
> 
> I haven't done enough xdr* work to know, and man pages
> didn't provide an immediate answer:
> Is there no need to call xdr_destroy on this error path?
> I'd expect xdrmem_create to do any allocation, not xdr_int.
> There are many like this.

Yes, the 'error:' codepath should be calling 'xdr_destroy(&xdr)'
to ensure free'ing of memory.

> 
> > +        goto error;
> > +    }
> > +    xdr_destroy (&xdr);
> > +
> > +    return rv;
> > +
> > +error:
> > +    VIR_FREE(ret);
> > +    return NULL;
> 
> The above should free rv, not ret:
> 
>        VIR_FREE(rv);

Doh, bad naming convention for arguments - we always use 'ret' for return
values. I should rename the argument to 'reply' since its what contains
the RPC reply, so we can use the normal convention of 'ret' for return
value.

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 :|