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

[Francois Gouget]

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

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.


-- 
Francois Gouget <fgouget at codeweavers.com>