[Spice-devel] [PATCH spice-server 2/2] docs: Add some notes on event scheduling and threading

[Frediano Ziglio]

> On Thu, Mar 28, 2019 at 10:27:46AM -0400, Frediano Ziglio wrote:
> > > On Thu, Mar 28, 2019 at 04:25:31AM -0400, Frediano Ziglio wrote:
> > > > > 
> > > > > On Mon, Mar 11, 2019 at 02:03:33PM +0000, Frediano Ziglio wrote:
> > > > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > > > ---
> > > > > >  docs/spice_threading_model.txt | 8 ++++++++
> > > > > >  1 file changed, 8 insertions(+)
> > > > > > 
> > > > > > diff --git a/docs/spice_threading_model.txt
> > > > > > b/docs/spice_threading_model.txt
> > > > > > index 9351141c8..25a3a030c 100644
> > > > > > --- a/docs/spice_threading_model.txt
> > > > > > +++ b/docs/spice_threading_model.txt
> > > > > > @@ -39,6 +39,14 @@ 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).
> > > > > >  
> > > > > > +One aspect to take into consideration is the event scheduling.
> > > > > > SPICE
> > > > > > uses
> > > > > > some
> > > > > > +`SpiceCoreInterface` to handle events. As the events will be
> > > > > > handled
> > > > > > from
> > > > > > a
> > > > > > +thread based on the core interface you have to use the correct
> > > > > > core.
> > > > > > Each
> > > > > > +channel has an associated core interface which can be retrieved
> > > > > > using
> > > > > > +`red_channel_get_core_interface`. There's also a main core
> > > > > > interface
> > > > > > you
> > > > > > can get
> > > > > > +using `reds_get_core_interface`. `reds_core_timer_*` and
> > > > > > `reds_core_watch_*`
> > > > > > +functions use the main core interface.
> > > > > 
> > > > > Do we need a few words as to when to use the main core interface?
> > > > > Apart from this, looks good to me.
> > > > > 
> > > > > Christophe
> > > > > 
> > > > 
> > > > It sounds a nice idea.
> > > > 
> > > > But honestly I cannot came with an easy rule beside "If code runs on
> > > > main thread like Qemu character devices or everything not running in
> > > > a channel you can use the main core interface."
> > > 
> > > Yes, something like your rule would work "Code running in the QEMU
> > > thread should use the main core interface. Code running in the cursor or
> > > display channel (through RedWorker) should use xxx interface.. Code
> > > running in other channels should use yyy. Be aware that a channel's
> > 
> > IMHO all code running in channels should use channel core, no matter
> > if they run on main or not (in the past I adjusted this).
> > Note that cursor channel code can run in both main and not main
> > for instance, not all cursor channels runs under RedWorker.
> > I think it's easier to avoid too much exceptions.
> > 
> > > ClientCbs run in a different thread context than the rest of the
> > > channel" (though the last sentence may no longer be accurate with the
> > > work you are doing in that area).
> > 
> > ClientCbs will be removed, one less thing to know, and new callbacks/vfunc
> > will work on the channel thread so not different from other channel code.
> > 
> > > 
> > > Christophe
> > > 
> > 
> > Taking into account that ClientCbs part will be soon (I hope) obsolete
> > and that part of "channel core for channel core" part is partially there
> > I would update to
> > 
> > +One aspect to take into consideration is the event scheduling. SPICE uses
> > some
> > +`SpiceCoreInterface` to handle events. As the events will be handled from
> > a
> > +thread based on the core interface you have to use the correct core. Each
> > +channel has an associated core interface which can be retrieved using
> > +`red_channel_get_core_interface`. There's also a main core interface you
> > can get
> > +using `reds_get_core_interface`. `reds_core_timer_*` and
> > `reds_core_watch_*`
> > +functions use the main core interface.
> 
> 
> > +Even though multiple channel types run in the main thread and so could use
> > +directly the main code interface, for coherence, rule simplicity and to
> > allows
> > +the code to be moved in a separate thread easily if needed, it's advisable
> > to
> > +use the channel core interface (that will be the main core interface in
> > this
> > +case).
> 
> I'm not sure if this paragraph makes things clearer or more confusing
> (because too many details). Maybe remove it?
> 

I added just because you asked. I don't mind removing, if you could
suggest something to replace even better.

> > +Currently character devices and not channel code runs in the main thread.
> 
> Is this here to mean character device code should be using reds_core_*?
> 

Not necessarily, reds_core_* are mainly utilities for the main core,
so if they are useful you can use them, or you can get the main core
and use it instead.

> Overall looks good to me.
> 
> > OT: I though QEMU moved to GLib to handle events (so the main core
> > interface
> > was using GLib) but for performance reasons epoll or other implementations
> > are used (that's the reason for SOCKET limitation for Windows).
> 
> Ah ok, I assumed epoll was there for historical reasons, not because
> of performance.
> 
> Christophe
> 

Historical or performance the point is that we cannot assume there
are GLib ones.

Frediano