On Fri, Jan 16, 2009 at 12:49:06PM +0100, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange@xxxxxxxxxx> 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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list