On Wed, Mar 07, 2018 at 06:48:25PM -0500, John Ferlan wrote:
>
>
> On 03/06/2018 12:58 PM, Daniel P. Berrangé wrote:
> > Currently if the virNetServer instance is created with max_workers==0 to
> > request a non-threaded dispatch process, we deadlock during dispatch
> >
> > #0 0x00007fb845f6f42d in __lll_lock_wait () from /lib64/libpthread.so.0
> > #1 0x00007fb845f681d3 in pthread_mutex_lock () from /lib64/libpthread.so.0
> > #2 0x000055a6628bb305 in virMutexLock (m=<optimized out>) at util/virthread.c:89
> > #3 0x000055a6628a984b in virObjectLock (anyobj=<optimized out>) at util/virobject.c:435
> > #4 0x000055a66286fcde in virNetServerClientIsAuthenticated (client=client@entry=0x55a663a7b960)
> > at rpc/virnetserverclient.c:1565
> > #5 0x000055a66286cc17 in virNetServerProgramDispatchCall (msg=0x55a663a7bc50, client=0x55a663a7b960,
> > server=0x55a663a77550, prog=0x55a663a78020) at rpc/virnetserverprogram.c:407
> > #6 virNetServerProgramDispatch (prog=prog@entry=0x55a663a78020, server=server@entry=0x55a663a77550,
> > client=client@entry=0x55a663a7b960, msg=msg@entry=0x55a663a7bc50) at rpc/virnetserverprogram.c:307
> > #7 0x000055a662871d56 in virNetServerProcessMsg (msg=0x55a663a7bc50, prog=0x55a663a78020, client=0x55a663a7b960,
> > srv=0x55a663a77550) at rpc/virnetserver.c:148
> > #8 virNetServerDispatchNewMessage (client=0x55a663a7b960, msg=0x55a663a7bc50, opaque=0x55a663a77550)
> > at rpc/virnetserver.c:227
> > #9 0x000055a66286e4c0 in virNetServerClientDispatchRead (client=client@entry=0x55a663a7b960)
> > at rpc/virnetserverclient.c:1322
> > #10 0x000055a66286e813 in virNetServerClientDispatchEvent (sock=<optimized out>, events=1, opaque=0x55a663a7b960)
> > at rpc/virnetserverclient.c:1507
> > #11 0x000055a662899be0 in virEventPollDispatchHandles (fds=0x55a663a7bdc0, nfds=<optimized out>)
> > at util/vireventpoll.c:508
> > #12 virEventPollRunOnce () at util/vireventpoll.c:657
> > #13 0x000055a6628982f1 in virEventRunDefaultImpl () at util/virevent.c:327
> > #14 0x000055a6628716d5 in virNetDaemonRun (dmn=0x55a663a771b0) at rpc/virnetdaemon.c:858
> > #15 0x000055a662864c1d in main (argc=<optimized out>, argv=0x7ffd105b4838) at logging/log_daemon.c:1235
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> > src/rpc/virnetserverclient.c | 79 ++++++++++++++++++++++++++++++--------------
> > 1 file changed, 55 insertions(+), 24 deletions(-)
> >
>
> I had the same syntax-check notes that Jim had.
>
> Beyond that since both callers Unlock prior to calling
> virNetServerClientDispatchMessage and the code relocks right away, would
> calling with client lock'd still be prudent/possible? Callers would
> then be able to
>
> if (msg)
> virNetServerClientDispatchMessage(...)
> else
> Unlock(client)
>
> Once we get to virNetServerProgramDispatch it expects an unlocked
> client. Maybe we should comment some of the other callers in the path to
> indicate whether server/client need to be locked. Not required though -
> only helpful for future spelunkers of this code.
I generally like to avoid situations where one method locks the object and
then calls another method which unlocks it. IMHO, it leads to harder to
understand code where the lock/unlock calls are spread out.
Since we own the reference on client, I think unlocking & locking again
is harmless here.
> > @@ -1313,16 +1347,6 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
> > }
> > }
> >
> > - /* Send off to for normal dispatch to workers */
> > - if (msg) {
> > - if (!client->dispatchFunc) {
> > - virNetMessageFree(msg);
> > - client->wantClose = true;
> > - } else {
> > - client->dispatchFunc(client, msg, client->dispatchOpaque);
> > - }
> > - }
> > -
>
> I thought about the order change here w/r/t the following code being run
> before the dispatch options above; however, that would perhaps more of a
> concern for the path where we have no workers. If we have a worker, then
> it gets queued and the following would then occur first anyway, right?
> So, IOW, I think the reordering here is fine. It would happen before the
> worker got around to handling the client and moving it around to
> virNetServerClientDispatchMessage could get ugly w/r/t locks.
Yeah, that was my rationale too - since worker threads are asynchronous
we're effectively running the second block first regardless, so the
ordering should be harmless.
>
> Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
>
> John
>
>
> > /* Possibly need to create another receive buffer */
> > if (client->nrequests < client->nrequests_max) {
> > if (!(client->rx = virNetMessageNew(true))) {
> > @@ -1338,6 +1362,8 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
> > }
> > }
> > virNetServerClientUpdateEvent(client);
> > +
> > + return msg;
> > }
> > }
> >
>
> [...]
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
