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

[Frediano Ziglio]

> 
> 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.
> 

Yes, maybe not so easy for SpiceVMC but at least the RedChannelClient
design currently allows it, with this series you have to write 2
classes. With this series you have to write a separate class for every
combination, so one CursorChannel for main thread, another for
separate (actually one for each possible dispatcher) and so on...

> 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.
> 

This move the thread responsibility to RedChannel and does not
allows to change the dispatching implementation.
So no, I won't store a dispatcher inside RedChannel.
I think the definition "RedChannel can run in a separate single
thread except build and destroy" is easy to understand.

About the interface looks still at RedClient, red_client_migrate
and consider the design:

    FOREACH_CHANNEL_CLIENT(client, rcc) {
        if (red_channel_client_is_connected(rcc)) {
            channel = red_channel_client_get_channel(rcc);
            red_channel_migrate_client(channel, rcc);
        }
    }

problems:
- the list is iterated without a lock. I don't know migration
  that much but I assume that during migration a new RCC
  (from the new client) will be created and the old one (from
  old client) will be disconnected so I see a possible problem;
- red_channel_client_is_connected. This is unsafe to call, it
  accesses a list in RedChannel which is not protected by a lock.
  This regression was introduced when we move from ring to GList
  as the ring iterators are more stable then GList ones. This bug
  could have been prevented if RedClient instead of accessing
  directly to RCC would have used a thread safe interface. This
  was basically the idea behind ClientCbs which was lost in all
  the changes done. The idea behind was that RedClient should
  access RCC only using ClientCbs method which were supposed to
  be thread safe. But having this interface just as
  RedChannelClient method make easier to introduce a call to
  an unsafe method. Is red_channel_client_get_channel safe?
  Well, not easy to understand, you have to looks at the source,
  check that fields are not changed (there's so locking!) at the
  end... yes. Is documented that this function can be accesses
  from multiple thread? No.

The interface which RedClient should access to should be
easier and safer. I want to disconnect/migrate this client.
RedClient should have access only to this safe interface.

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

Frediano