[Spice-devel] [PATCH 8/9] worker: make sure we dispatch after releasing items

[Jonathon Jongsma]

On Wed, 2015-12-16 at 18:16 -0500, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > 
> > 
> > It seems that "watch" is related to watches on non-Dispatcher fds, and
> > "dispatch" is related to the watch on the Dispatcher fd.
> > 
> > Items can be released in the "display" stage when we try to queue pipe items
> > when the client is not connected (see red_channel_client_pipe_add(), which
> > calls
> > validate_pipe_add() which releases the pipe item if the client is not
> > connected). They can also be released immediately after connection as part
> > of
> > dcc_start() -> dcc_push_surface_image(). In either of these cases, I don't
> > think
> > it's useful to trigger another source dispatch.
> > 
> > Those released in the "push" stage have just been sent to the client, so the
> > only reason I can think to trigger another source dispatch here is perhaps
> > to
> > re
> > -run the display/cursor handler immediately in the case that the client pipe
> > item queue was full. But I have no idea whether this is a regular occurrence
> > or
> > not. I suspect this was not really the goal of this patch.
> 
> I actually think that was it. The worker thread may currently block because
> the client is too slow. But then, when we release item, the pipe is going to
> be smaller hopefully, and thus we can try to dispatch again without waiting
> for the timeout. (waiting for timers can really degrade performances in some
> cases, like you see today with the qemu virgl timer, but that's another
> story where I can show you figures when we merge gl scanounts in spice ;)

Then I'm still confused. In the old loop, we had a very explicit sequence of
behavior. For each loop:

 1 calculate timeout for poll() function
 2 poll() fds with timeout from step 1
 3 service expired timers
 4 handle fd watch events
 5 process cursor commands from guest
 6 process display commands from guest
 7 push queued messages to clients
 8 GOTO 1

After pushing the queued messages to the client, we did not go back and handle
cursor and display commands again. We went back to the top of the loop and did a
poll with timeout again. So I fail to see why the glib loop needs to short
-circuit the timeout and return to step 5 when the old design didn't. 

Or is this change unrelated to the new glib loop and the worker has simply
always been too slow even with the old loop implementation?

Jonathon


> 
> > 
> > The "dispatch" stage happens when we receive a message from the Dispatcher
> > object. These messages from the Dispatcher can cause us to send new pipe
> > items
> > to the client, and they can also queue new items without sending them. For
> > example, handle_dev_destroy_primary_surface() will flush all queued display
> > and
> > cursor commands (which results in a red_channel_push() to send those to the
> > client, which will in turn release those pipe items), and then it does a
> > bunch
> > of stuff with dependent drawables (which may result in pipe items being
> > released
> > without being sent) and then it queues a DESTROY_SURFACE pipe item. It
> > doesn't
> > seem to me that the act of releasing any of those pipe items that I
> > described
> > above should result in an immediate source dispatch.
> > 
> > The "watch" stage is perhaps more interesting since it's the second highest
> > total in your results. The watch handler is generally triggered when we
> > receive
> > data on the client socket. Different channels have different handlers, and
> > different behaviors related to Pipe Items. Some handlers don't result in any
> > messages sent back to the client (and thus, no pipe items are created or
> > released). Some handlers result in a new pipe item being queued to be sent
> > to
> > the client (e.g. INPUTS_MOUSE_POSITION queues a MOUSE_MOTION_ACK item to
> > send
> > to
> > the client), but do not send it directly. And some handlers send a message
> > to
> > the client immediately (SPICE_MSGC_ACK calls red_channel_client_push() and
> > thus
> > results in all of the sent items being released). Of these three different
> > situations, only the third one (immediately sending a message to the client)
> > would cause us to call release_item() and thus set the timeout to 0 and
> > trigger
> > another source dispatch. But I still don't understand why that would be
> > useful.
> > 
> > So I'm left with a big question: What problem are we trying to solve here? I
> > can't think of a problem that would be solved by this patch.
> > 
> > Jonathon
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >