[Spice-devel] [PATCH spice-server 0/4] Refactor RedChannel ClientCbs

[Christophe Fergeau]

On Wed, Aug 30, 2017 at 04:32:48AM -0400, Frediano Ziglio wrote:
> > 
> > > On 30 Aug 2017, at 09:31, Frediano Ziglio <fziglio at redhat.com> wrote:
> > - Why is being able to use the main thread or a separate one more “forced”
> > than (always?) making a channel run in a separate thread? Naively, it seems
> > like there are more choices in the “forced” case (namely, the main thread).
> > So I think you wanted to write something else than what you wrote. But that
> > part I was unable to make sense of by myself, so that’s really my primary
> > question :-)
> > 
> 
> After these patches the CursorChannel and DisplayChannel hardcoded the
> need for a Dispatcher which means that there's a separate thread
> receiving the requests. This as the callbacks/methods have only the
> implementation calling the dispatcher.
> 
> > In any case, if you do another iteration of your patch series, would it be
> > possible
> > to make sure this explanation is part of the envelope or commit message?
> > 
> > 
> > > For instance in my streaming patches I use the CursorChannel
> > > from the main thread, now I would have to:
> > > - write a fake QXL device to make CursorChannel happy;
> > > - create a new Dispatcher to handle requests from CursorChannel;
> > > - create another thread to handle synchronous calls (like
> > >  disconnect)
> > 
> > Is the CursorChannel the only one running from the main thread?
> > If so, what would be the rationale / trade-off for choosing one approach
> > vs. the other? I.e. if I add a new channel, what do I pick?
> > 
> 
> The current (master) design is flexible. It allows to write
> channels that can run in the main thread or a separate one.
> Currently (master) CursorChannel and DisplayChannel run on
> separate threads (the reason is that image compression could
> take time and is not a good idea to suspend the main thread
> that long) but this is not in these channel implementation
> but on the RedQXL/RedWorker implementation (RedQxl and
> RedWorker are close friends basically implementing this
> thread separation, RedQxl implements the marshalling part
> of the dispatcher while RedWorker implements the unmarshalling
> one).
> 
> I think the possibility to have a single implementation
> that could run in either the main thread or in another is
> a good feature. If in the future one channel like SpiceVMC
> get expensive to run inside the main thread we could move
> easily or decide where to run dynamically.

I'd like to note that deciding to move non-trivial code from a helper thread to
the main thread, or from the main thread to a helper thread is in
general not going to be so easy, in the first case you have to make sure
that it's not blocking too much, and in both cases, you have to make
sure that you don't break code which was supposed to run in a
specific  thread, and now is moved to a different one.
Even the code currently in git does not get this right as shown by your
recent threading patches.

Something which probably would work would be to introduce a
RedChannel::dispatcher, and change
red_channel_disconnect_client/red_channel_migrate_client:
if RedChannel::dispatcher is set, then you invoke the vfunc in a
separate thread, if RedChannel::dispatcher is not set, then you invoke
the vfunc directly.

I think this would remove the issue you are having with the cursor
channel changes.

Christophe