[Spice-devel] [PATCH spice-server] docs: Add some documentation on spice threading model

[Frediano Ziglio]

> 
> On Thu, Aug 31, 2017 at 02:07:14PM +0100, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  .gitignore                     |  1 +
> >  docs/Makefile.am               |  6 ++--
> >  docs/spice_threading_model.txt | 73
> >  ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 78 insertions(+), 2 deletions(-)
> >  create mode 100644 docs/spice_threading_model.txt
> > 
> > diff --git a/.gitignore b/.gitignore
> > index 25db761a..bf618932 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -35,6 +35,7 @@ INSTALL
> >  docs/manual/manual.chunked/
> >  docs/manual/manual.html
> >  docs/spice_style.html
> > +docs/spice_threading_model.html
> >  .dirstamp
> >  .deps
> >  .libs
> > diff --git a/docs/Makefile.am b/docs/Makefile.am
> > index 3dba2c3a..45667a60 100644
> > --- a/docs/Makefile.am
> > +++ b/docs/Makefile.am
> > @@ -4,14 +4,16 @@ ASCIIDOC_FLAGS = -a icons -a toc
> >  EXTRA_DIST =					\
> >  	spice_style.html			\
> >  	spice_style.txt				\
> > +	spice_threading_model.html		\
> > +	spice_threading_model.txt		\
> >  	$(NULL)
> >  
> >  if BUILD_MANUAL
> >  SUBDIRS = manual
> >  
> > -all-local: spice_style.html
> > +all-local: spice_style.html spice_threading_model.html
> >  
> > -spice_style.html: spice_style.txt
> > +%.html: %.txt
> >  	$(AM_V_GEN) $(ASCIIDOC) -n $(ASCIIDOC_FLAGS) -o $@ $<
> >  endif
> >  
> > diff --git a/docs/spice_threading_model.txt
> > b/docs/spice_threading_model.txt
> > new file mode 100644
> > index 00000000..f88ead6d
> > --- /dev/null
> > +++ b/docs/spice_threading_model.txt
> > @@ -0,0 +1,73 @@
> > +How does SPICE handle threading
> > +-------------------------------
> > +
> > +Lot of code run in a single thread.
> 
> "Lots of code"
> 
> > +
> > +Qemu usually calls SPICE from its main thread except on call backs to code
> > +already running in different threads.
> 
> I think it's "callbacks", but not sure.
> 
> > +
> > +Channels run mostly on a single thread except on creation and destruction
> > which
> > +MUST be done in the main thread.
> 
> I'd be a bit more explicit about "Channels" at least here:
> "Channels (RedChannel/RedChannelClient code) run mostly in a single
> thread..." (ok, you detail this a bit down the text)
> 
> > +Lots of channels run in the main thread but currently CursorChannel and
> > +DisplayChannel can be run from within a thread created by RedWorker.
> 
> "can be run" -> "are run"?
> 
> > +Note that different CursorChannel/DisplayChannel (they differ by id) run
> > in
> > +separate RedWorker threads (currently a single RedWorker thread run a pair
> 
> "runs"
> 
> > +of CursorChannel and DisplayChannel).
> > +
> > +RedChannelClient runs in the RedChannel thread. Creation is done in the
> > same
> > +thread while destruction can happen in different threads.
> > +
> > +A RedClient instance groups the various RedChannelClient connected to
> > +the same remote SPICE client.
> > +These RedChannelClient instances can run in separate threads:
> > +MainChannelClient and most of the other RedChannelClient will be
> > +running in the main thread, while the CursorChannelClient and the
> > +DisplayChannelClient usually will be running from a different thread.
> > +
> > +Another important aspect of dealing with multiple threads are the
> > dispatchers.
> > +Dispatchers are used to send messages/request from one thread to another.
> > +The main dispatcher is used to send requests to the main thread.
> > +The Qxl uses a dispatcher to send requests to the RedWorker which will
> > forward
> > +to DisplayChannel/CursorChannel.
> 
> For what it's worth, I would not call this an "important aspect", more
> of an uninteresting implementation detail :)
> I think I'd describe this as a way of ensuring a method is called in a
> different thread, either synchronously or asynchronously. It's more
> explicit than "sending requests"
> 

I think is pretty important. Lots of implementations uses mostly mutexes
while in SPICE code there's a lot of requests between threads.

> > +
> > +RedClient may call some RedChannelClient functions using some callbacks
> > +registered inside ClientCbs. Usually these callbacks are functions that do
> > the
> > +job directly if the RedChannel is running in the main thread or they use a
> > +dispatcher to do the job in the right thread. Currently there are 3
> > callbacks:
> > +connect, disconnect and migrate. Connect and migrate are asynchronous (the
> > job
> > +is done while the current thread is doing something else) while disconnect
> > is
> > +synchronous (the main thread will wait for termination).
> > +
> > +Reference counting and ownership
> > +--------------------------------
> > +->  pointer
> > +
> > +=>  pointer with owning (mostly GObject ref counting)
> > +
> > +RedChannelClient::client -> RedClient
> > +
> > +RedClient::channels => RedChannelClient
> > +
> > +RedClient::mcc -> MainChannelClient
> > +
> > +RedChannelClient::channel => RedChannel
> > +
> > +RedChannel::clients -> RedChannelClient
> > +
> > +The RedClient represents a connection from a remote SPICE client,
> > +RedClient::channels corresponding to the connections for the individual
> > +channels. Their lifetime is tied to the TCP connection to this remote
> > client.
> > +when a disconnection is detected, the corresponding RedChannelClient are
> 
> "When"
> 
> > +removed from RedClient::channels and Channel::clients (the main owner of
> 
> I guess should be "RedChannel", but fairly obvious given all the
> surrounding text.
> 
> > +RedChannelClient is RedClient).
> > +When MainChannelClient is disconnected all RedChannelClients connected to
> > the
> > +same RedClient are automatically disconnected.
> 
> "is disconnected, all RedChannelClients"
> 
> > +
> > +Who owns RedClient? RedClient is released when the MainChannelClient
> > attached
> > +to it is disconnected. In this case a request is scheduled in the main
> > thread
> > +(even if MainChannelClient runs on the main thread too) and
> > +red_client_disconnect is called which calls red_client_destroy which uses
> > +g_object_unref to free the object.
> > +
> > +Where is freed the MainChannelClient? On disconnection like other channel
> 
> I believe "Where is the MainChannelClient freed?" sounds better/is more
> correct
> 
> Overall looks good!
> 
> > +clients, currently before RedClient which will have a dangling pointer.

Hope I got all the other comments right.

Frediano