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

[Christophe Fergeau]

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"

> +
> +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.
> -- 
> 2.13.5
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel