[Spice-devel] [spice v10 10/27] server: Make the RedDrawable refcount thread-safe

[Frediano Ziglio]

> 
> On Thu, 3 Mar 2016, Christophe Fergeau wrote:
> [...]
> > Making the refcounting thread safe is one thing, but then the code
> > freeing the drawable needs to be thread-safe too. red_put_drawable might
> > be fine (only looked briefly), but release_resource which is a vfunc
> > provided by QEMU seems much less obvious. I think the drawable refcount
> > is changed in the display channel thread, so I would try to queue an
> > idle on its main_context, and do the unref from there. Hopefully it will
> > not be too messy... (and I'll mention again
> > g_main_context_push_thread_default which could be useful there ;) or
> > some thread-local storage variable).
> > 
> > By "queueing an idle", I mean something like
> > GSource *source = g_idle_source_new();
> > g_source_set_callback(..);
> > g_source_attach(source, worker->core.main_context);
> 
> I have tried this approach but I get a feeling it cannot work and is
> fundamentally flawed :-( There are two sticking points, the dependency
> on the glib main loop and getting access to main_context.
> 
> I have not found a clean way to get the main_context information from
> the RedDrawable:
>  - From the RedDrawable I can get a QXLState pointer with
>    red_drawable->qxl->st.
>  - And given a RedsState pointer I can get the main_context with
>    reds_get_core_interface(reds)->main_context

No, this is the problem. Every RedWorker has a different glib context, if
you pass through the core main_context should be NULL pointing to default
one which is not used.

>  - But while QXLState does have a reds field, it's defined in red-qxl.c
>    so it's not accessible from red-worker.c and I have not found a
>    function that would return this information.

You could use red_qxl_get_server but as said before you would get
the wrong context.

I think here what's missing is a way to get a RedWorker from a QXLInstance.
This could be added to red-qxl.[ch].

> 
> There is a loophole though: reds.h defines a reds global variable so I
> can just use reds_get_core_interface(reds)->main_context! That really
> does not feel right though. Anyway, doing that I got:
> 
>     if (g_atomic_int_dec_and_test(&red_drawable->refs)) {
>         GSource *source = g_idle_source_new();
>         g_source_set_callback(source, red_drawable_unref_cb, red_drawable,
>         NULL);
>         g_source_attach(source, reds_get_core_interface(reds)->main_context);
>     }
> 
> But this resulted in red_drawable_unref_cb never being called. The same
> is true if using g_timeout_source_new() instead wit various timeout
> values. I'll get back to this.
> (there is also the issue of freeing the source which is uglyly fixable)
> 
> 
> I'm also concerned about this use of reds_get_core_interface(). As far
> as I can tell it's only used to call red_cannel_create() /
> red_channel_create_parser(). And access to main_context seems to be
> restricted to initializing RedWorker objects and implementing the timer
> functions in event-loop.c.
> 
> That got me looking at the event-loop.c code and it essentially does all
> the source manipulations we want, except as timers. That makes me think
> that bypassing the reds_core_timer_*() functions is wrong.
> 
> So I implemented the unref through the reds_core_timer_*() functions
> with a 1ms delay (0ms did not work). The callback got called and was
> freeing RedDrawables... for a short time. Then I got a crash in
> TimerSet()!
> 
> TimerSet() is an Xorg function on top of which the reds_core_timer_*()
> functions are implemented in Xspice.
>  - It turns out TimerSet() & co are not thread-safe, hence the crash. So
>    reds_core_timer_*() cannot be used to solve thread-safety issues.
>  - I feel that this confirms the glib approach is wrong as it bypasses
>    the Xorg implementation.
>  - I now wonder if there is really a glib main loop in the Xspice case.
>    If not that would explain why the callback was not called with
>    g_idle_source_new().
> 
> 
> A completely different approach to solving this would be to handle it
> all in the GStreamer encoder code:
>  - Only ref the RedDrawable once but wrap it in a struct with its own
>    refcount. It's that struct which will track the actual refcounting by
>    the GstMemory objects.
>  - When that struct's refcount drops to zero, put the RedDrawable into a
>    GAsyncQueue.
>  - encode_frame() is always called from a safe thread, so unref all the
>    RedDrawables in the async queue whenever we enter / leave
>    encode_frame() or destroy the GStreamer encoder.
> 
> The drawback is that we could get up to a 1 frame delay before a
> RedDrawable is freed.
> 

There is similar code to deal with Glz (see display_channel_free_glz_drawables_to_free
call in red-worker.c).

Frediano