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

Tue Dec 15 14:48:01 PST 2015

On Tue, 2015-12-15 at 10:42 -0500, Frediano Ziglio wrote:
> > 
> > > 
> > > Hi
> > > 
> > > ----- Original Message -----
> > > > Same comment as
> > > > http://lists.freedesktop.org/archives/spice-devel/2015-December/024682.h
> > > > tml
> > > > 
> > > > "I don't really understand the reason for this patch. Why do we need to
> > > > wakeup
> > > > the worker immediately after a display pipe item has been released, but
> > > > not
> > > > a
> > > > cursor item? Why can't we wait for the next worker poll timeout to
> > > > expire?
> > > 
> > > The question for me is rather, why would you wait for a timeout when you
> > > know
> > > you can process it now?
> > > 
> > 
> > Process what actually? If you already deleted the item you already processed
> > it.
> > If the item is going out of worker you will get a reply, possibly a new
> > item.
> > If the item was going in it's already processed.
> > Or am I missing something?
> > 
> 
> Oh... now I start getting all these problems!
> All came from Glib integration!
> Basically before there was a loop were is was clear steps:
> 1- select
> 2- timers
> 3- fd events
> 4- guest events
> 5- push data to channel.
> Now point 5 is just a GSource so you don't know if is the last and
> you force an iteration to make sure you don't wait too much.
> 
> Well.. now I have another reason to prove that waiting to merge Glib loop code
> was a good choice.
> 
> About how to solve this issue (GSources ordering)... I don't know!
> Maybe using priorities. I'll have a look at Glib code.

So, let me explain what I think I understand so far. The commit log says "make
sure we dispatch". I assume this is referring to the GSource dispatch function
(worker_source_dispatch()) rather than something related to the spice-server
Dispatcher object. 

The worker_source_dispatch() function basically just processes the cursor and
display commands from the guest, adds them to a queue, and afterwards pushes the
queue of pipe items out to the client.

The patch under discussion attempts to trigger this dispatch function again
after we 'release' a pipe item. Generally the pipe item will be freed from
within the worker_source_dispatch() function itself (e.g. after sending the item
in red_push()). So this patch appears to be a mechanism to re-dispatch the
source as soon as possible after the source was dispatched (but only if we
actually sent a item to the client and released it). 

But why do we need to do this again right away after a pipe item has been
released? Is it because the send queue may have been full and so we want to try
to send some more after the items in the queue have been sent and released? And
we don't want to wait for a timeout to send any future items? It seems we're
actually getting close to an 'idle' source by setting the timeout to 0 whenever
we release a pipe item.

However: the old loop only did display/cursor-handling once in the loop. After
it was finished processing the display/cursor, it looped back and did a poll
with timeout. So I still don't understand why we would need to try to avoid the
poll timeout when we're using the glib loop. I understand that the old loop had
a deterministic order and the new loop doesn't. But I still don't understand the
problem we're trying to solve here. What am I missing?

Jonathon

> 
> Frediano
> 
> > 
> > > > Updating the worker timeout seems like an implementation detail, so
> > > > adding
> > > > an
> > > > API for other classes to call seems a little weird to me."
> > > > 
> > > > Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
> > > > 
> > > > 
> > > > On Wed, 2015-12-09 at 12:17 +0000, Frediano Ziglio wrote:
> > > > > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > > > > 
> > > > > ---
> > > > >  server/display-channel.c | 2 ++
> > > > >  server/red-worker.c      | 8 ++++++++
> > > > >  server/red-worker.h      | 1 +
> > > > >  3 files changed, 11 insertions(+)
> > > > > 
> > > > > diff --git a/server/display-channel.c b/server/display-channel.c
> > > > > index 32d87af..60170cb 100644
> > > > > --- a/server/display-channel.c
> > > > > +++ b/server/display-channel.c
> > > > > @@ -1987,9 +1987,11 @@ static void hold_item(RedChannelClient *rcc,
> > > > > PipeItem
> > > > > *item)
> > > > >  static void release_item(RedChannelClient *rcc, PipeItem *item, int
> > > > > item_pushed)
> > > > >  {
> > > > >      DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
> > > > > +    RedWorker *worker = DCC_TO_WORKER(dcc);
> > > > >  
> > > > >      spice_return_if_fail(item != NULL);
> > > > >      dcc_release_item(dcc, item, item_pushed);
> > > > > +    red_worker_update_timeout(worker, 0);
> > > > >  }
> > > > >  
> > > > >  static int handle_migrate_flush_mark(RedChannelClient *rcc)
> > > > > diff --git a/server/red-worker.c b/server/red-worker.c
> > > > > index b59a5a1..ae4ac27 100644
> > > > > --- a/server/red-worker.c
> > > > > +++ b/server/red-worker.c
> > > > > @@ -112,6 +112,14 @@ RedMemSlotInfo* red_worker_get_memslot(RedWorker
> > > > > *worker)
> > > > >      return &worker->mem_slots;
> > > > >  }
> > > > >  
> > > > > +void red_worker_update_timeout(RedWorker *worker, gint timeout)
> > > > > +{
> > > > > +    spice_return_if_fail(worker != NULL);
> > > > > +    spice_return_if_fail(timeout >= 0);
> > > > > +
> > > > > +    worker->event_timeout = MIN(worker->event_timeout, timeout);
> > > > > +}
> > > > > +
> > > > >  static int display_is_connected(RedWorker *worker)
> > > > >  {
> > > > >      return (worker->display_channel && red_channel_is_connected(
> > > > > diff --git a/server/red-worker.h b/server/red-worker.h
> > > > > index b55a45c..3a9dc19 100644
> > > > > --- a/server/red-worker.h
> > > > > +++ b/server/red-worker.h
> > > > > @@ -94,6 +94,7 @@ static inline void red_pipes_add_verb(RedChannel
> > > > > *channel,
> > > > > uint16_t verb)
> > > > >  
> > > > >  RedWorker* red_worker_new(QXLInstance *qxl, RedDispatcher
> > > > >  *red_dispatcher);
> > > > >  bool       red_worker_run(RedWorker *worker);
> > > > > +void       red_worker_update_timeout(RedWorker *worker, gint
> > > > > timeout);
> > > > >  QXLInstance* red_worker_get_qxl(RedWorker *worker);
> > > > >  RedChannel* red_worker_get_cursor_channel(RedWorker *worker);
> > > > >  RedChannel* red_worker_get_display_channel(RedWorker *worker);
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel