[Spice-devel] Some notes on threading and reference counting on spice-server

[Christophe Fergeau]

Hey,

Great write-up overall, couple of comments, minor clarifications, ...
below.

On Fri, Aug 25, 2017 at 01:07:17PM -0400, Frediano Ziglio wrote:
> How Spice handle threading

"handles" (or "how does Spice handle threading")

> --------------------------
> 
> Lot of code run in a single thread.
> 
> Qemu usually calls Spice from the same thread except on call backs to
> code already run in different threads.

We probably can give them names, "the same thread" (-> a single thread?)
would be "the qemu thread", and the other threads usually are "io
threads" if I'm not mistaken.

> 
> Channel run mostly on a single thread except on creation and

"Channels run mostly in a single thread"

> destroying which MUST be done in the main thread.  Lot of Channels run

hm destruction would work better
s/Lot/lots

> on the main thread but currently CursorChannel and DisplayChannel can

s/on/in I believe

> be run from within a thread created by Worker.  Note that different
> CursorChannel/DisplayChannel (they differ by id) run in separate
> Worker threads.

Not 100% clear here if there is CursorChannel has its own thread, and
DisplayChannel has its own thread different from the previous one. I
think it's a pair of (CursorChannel, DisplayChannel) which are sharing a
thread. The "Note ..." could probably be slightly reworded to avoid this
ambiguity. 
Another note, we could use pthread_setname_np() to "annotate" our
threads.

> 
> ChannelClient runs in the Channel thread. Ever creation is done

s/Ever/Even ? Or this word could be removed.

> in the same thread while destruction can happen in different
> phases.

different phases? from a different thread you mean?

> 
> RedClient is bound to ChannelClients that can be in separate threads.
> Is one of the cases where mutexes are used.

Hmm, not sure I would formulate things this way.
"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 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
> Worker which will forward to DisplayChannel/CursorChannel.
> 
> Client may call some ChannelClient functions using some callbacks
> registered inside ClientCbs. Usually these callbacks are functions that
> does the job directly if the Channel is running in the main thread or

"that do"

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

s/owning/ownership.

> -----------------------------
> ->    pointer 
> --->  pointer with owning (mostly GObject ref counting)
> 
> ChannelClient -> Client
> Client ---> ChannelClient
> Client -> MainChannelClient
> 
> ChannelClient ---> Channel
> Channel -> ChannelClient

Imo we should be more specific.. (I don't feel too strongly about having
the Red namespace or not, but it's easier for me with it)

RedChannelClient::client -> RedClient
RedClient::channels --> GList(RedChannelClient)
RedClient::mcc -> MainChannelClient (RedClient has a MainChannelClient
reference it owns in the 'channels' list I believe?)

RedChannelClient::channel ---> RedChannel
RedChannel::clients -> ChannelClient

> 
> In this case the "real" owner is the TCP connection.

"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 removed from RedClient::channels and
Channel::clients."
Then this can get back to your "(the main owner ...)"



> When a
> disconnection is detected the links Channel -> ChannelClient and
> Client ----> ChannelClient are removed causing possibly ChannelClient
> to be released (the main owner of ChannelClient is Client).  When
> MainChannelClient is disconnected it disconnects all ChannelClients
> connected to the same Client

Hmm, is this in addition to what was said previously? MainChannelClient
being disconnected will trigger the disconnection of all
RedChannelClient regardless of their state (ie regardless of whether
they are disconnecting or not)?

> Who owns Client? Client is released when the MainChannelClient
> attached is disconnected. In this case a request is scheduled in the

"attached to it" I think

> main thread (even if MainChannelClient run on the main thread too) and

"runs"

> red_client_disconnect is called which calls red_client_destroy which
> use g_object_unref to free the object.

"which uses"


> Where is freed the MainChannelClient? On disconnection like other
> channel clients, currently before Client which will have a dandling
> pointer.

"dangling". I'd give method names here so that it's clear where it's
happening.

Thanks for writing this up!

Christophe