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

[Christophe Fergeau]

Hey,

Thanks for the detailed analysis, and as usual, very late answer :(

On Fri, Aug 22, 2014 at 01:41:11PM -0500, Jeremy White wrote:
> I'm hoping to ask for some advice on the spice server.
> 
> Background:
> I've hit a bug with Xspice.  The current implementation uses a separate
> thread to grab pcm data from a fifo.  That same thread creates and drives
> the playback channel, and then pushes that data out using
> spice_server_playback_put_samples.
> 
> (That may have been a poor choice; I no longer recall if there was a careful
> understanding of the inter thread issues at that time.  But please bear with
> me).
> 
> If you have adaptive streaming turned on, you will eventually get a
> update_client_playback_latency message on the display channel (which in the
> Xspice case is being driven by the main, Xorg, thread).  Run the stream long
> enough, and the lack of thread safety will eventually bite you.  (It tends
> to expose some lovely behaviors in the snd_worker.c code; snd_send_data will
> spin loop nicely if some basic assumptions don't line up).
> 
> It is interesting (to me, at any rate), that main_dispatcher.c is provided
> as a mechanism to ensure that a message is sent by the right thread.  Of
> course, in this case, it is used to make sure the main thread is called,
> which doesn't help.

I think there actually is the same issue when running within QEMU: sound
is emitted from a QEMU thread:

#0  snd_playback_send (data=0x7f262efc2890) at snd_worker.c:795
#1  0x00007f26235dae41 in spice_server_playback_put_samples (
    sin=0x7f262898b778, samples=0x7f262efcb15c) at snd_worker.c:1119
#2  0x00007f26263dbe89 in line_out_run (hw=0x7f262898b6f0, 
    live=<value optimized out>)
    at /usr/src/debug/qemu-kvm-0.12.1.2/audio/spiceaudio.c:164
#3  0x00007f26263d9ae5 in audio_run_out (msg=<value optimized out>)
    at /usr/src/debug/qemu-kvm-0.12.1.2/audio/audio.c:1396
#4  audio_run (msg=<value optimized out>)
    at /usr/src/debug/qemu-kvm-0.12.1.2/audio/audio.c:1537
#5  0x00007f26263d9e10 in audio_timer (opaque=<value optimized out>)
    at /usr/src/debug/qemu-kvm-0.12.1.2/audio/audio.c:1100
#6  0x00007f262632ec7a in qemu_run_timers (timeout=1000)
    at /usr/src/debug/qemu-kvm-0.12.1.2/vl.c:1341
#7  main_loop_wait (timeout=1000) at
/usr/src/debug/qemu-kvm-0.12.1.2/vl.c:4085
#8  0x00007f262635241a in kvm_main_loop ()
    at /usr/src/debug/qemu-kvm-0.12.1.2/qemu-kvm.c:2258
#9  0x00007f2626333697 in main_loop (argc=<value optimized out>, 
    argv=<value optimized out>, envp=<value optimized out>)
    at /usr/src/debug/qemu-kvm-0.12.1.2/vl.c:4268
#10 main (argc=<value optimized out>, argv=<value optimized out>, 
    envp=<value optimized out>) at
/usr/src/debug/qemu-kvm-0.12.1.2/vl.c:6725

but the display channel seems to be running from different threads
(either another QEMU thread (?)) or from the SPICE main thread depending
where the call is triggered (I did not have time to trigger the latency
code), so I think the same issue could be hit with QEMU


> 
> Questions:
> 
> What's the best way to fix this?  The alternates I'm considering are:
>   1.  Rewrite the spiceqxl_audio code to run out of the main loop.
>       That's a fair bit of work, and risks discarding some potentially hard
> earned wisdom from the past.  (afair, that was the first implementation
> attempt, and it had issues).

Yup, would need to be fixed in QEMU too.

> 
>   2.  Surgically modify the snd_worker.c calls that handle the Playback
> channel to introduce a mutex; lock and release that mutex around operations.
> This is the direction I'm leaning, but I don't see a lot of evidence that
> this is commonly done.

This is indeed not commonly done in the server code, but I'm not sure I
see a better way of doing that short of serializing all of the sound
calls from whatever thread they happen in to the main thread (ie what
RedDispatcher does, but for SndWorker instead of for the QXL commands).


> 
>   3.  Somehow modify or parallel main_dispatcher.c so that the latency
> update call is invoked in the context of the correct thread.

I don't think this is easily done in the QEMU case

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/20140905/144c8a0b/attachment-0001.sig>