On Fri, Jan 16, 2009 at 12:49:06PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@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 :|