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

"Daniel P. Berrange" <berrange(a)redhat.com&gt; 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.

> + /* 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.

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