[Spice-devel] Help with solving a thread safety issue

[Christophe Fergeau]

Hey,

On Fri, Sep 19, 2014 at 10:22:16AM -0500, Jeremy White wrote:
> This thread veered; I'd like to bring it back, if I can.
> 
> I've got a clear case of thread unsafety in XSpice.
> spice_server_playback_put_samples is called from a different thread than the
> main thread.  If the main thread calls
> main_dispatcher_handle_mm_time_latency while we're putting samples, very bad
> things result.  (I get a spin loop in the X server, mostly; there is a
> fairly easy way to get snd_worker.c to infinite loop in snd_send_data() with
> a little help from a malicious thread; if n != 0 at the end of that loop,
> and  spice_marshaller_fill_iovec returns 0).
> 
> This is a bug, and I think it's well understood (even if the best fix is
> not).
> 
> We were hoping that I may have also exposed an issue in the qemu case, but
> the jury is out on that.

[snip]
> 
> But, back to *my* thread <grin>:
> 
> I was hoping that someone who knew qemu better than I could answer the
> question:  does the qemu audio processing loop run in it's own thread? The
> equivalent call in qemu/spiceaudio.c (i.e. the call to
> spice_server_playback_put_samples) happens in qemu/audio/spiceaudio.c. That,
> in turn, appears to be called from run_out/then timer_run in
> qemu/audio/aduio.c.  That timer seems to be established by a call to
> timer_new_ns in audio/audio.c, which devolves into a timer_init call. But,
> afaict, timer_init just sets up the structure, and doesn't actually start
> anything running.
> 
> I'm hoping a qemu expert can tap me with a clue bat and just help me answer
> that question.

I've instrumented qemu a bit with pthread_self calls and spice_warning
(should learn to use systemtap to achieve that...) and here is the
result:

red_worker.c:5173:red_process_commands: 0x7fa988e8b700 (8)
snd_worker.c:1106:spice_server_playback_put_samples: 0x7fa994be69a0 (8)
red_worker.c:3044:red_stream_update_client_playback_latency: 0x7fa988e8b700 (8)
main_dispatcher.c:155:main_dispatcher_set_mm_time_latency: 0x7fa988e8b700 (8)
main_dispatcher.c:116:main_dispatcher_handle_mm_time_latency: 0x7fa994be69a0 (8)

spice_server_playback_put_samples is running from the QEMU main thread
(0x7fa994be69a0) which is different than the main spice thread (0x7fa988e8b700).
However, the main_dispatcher_handle_mm_time_latency() call ends up in
the same QEMU thread as spice_server_playback_put_samples() so things
should be good.

As you mentioned already, main_dispatcher_handle_mm_time_latency() is
run from the QEMU main loop through a timer. When
ed_stream_update_client_playback_latency() is called, it calls
main_dispatcher_set_mm_time_latency() in the spice main thread, which
will send a message on a file descriptor using
dispatcher_send_message(). This file descriptor is added to QEMU
mainloop in main_dispatcher_init() through the call to core->watch_add()
which ends up calling qemu_set_fd_handler() to have QEMU main loop watch
for new data available on this fd. This will then call the code to run
in the right thread.

So we should be good with that code here.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20140924/f8dcd860/attachment.sig>