[Spice-devel] [PATCH spice] Provide thread safety between spice_server_playback_put_samples and snd_set_playback_latency.

[Marc-André Lureau]

On Wed, Sep 24, 2014 at 8:06 PM, Jeremy White <jwhite at codeweavers.com>
wrote:

> A MsgMainMultiMediaTime message on the main channel will be relayed
> through main_dispatcher so as to be fired in the context of the
> main (not worker) thread.
>
> In qemu, that thread happens to also be the thread that drives the
> audio channel, so it works.
>
> In XSpice, it is a different thread, which leads to unpleasant
> side effects.  The predominant side effect I noticed was an infinite loop
> in snd_send_data.
>

Is this difficult to solve? It seems the thread is reading from a fd and
pushing to spice server. Couldn't this be done from X main loop?


> This patch uses a mutex to prevent such collisions.
>

Is the mutex supposed to protect the whole SndChannel struct?It seems there
are various code path that could be called with or without the lock held
(snd_playback_send callback, for example). I would rather move the
protected bits into a substructure, and make sure only access is made with
the lock.

But I think we are going in a dangerous place if we start making spice
server MT-safe api


> Signed-off-by: Jeremy White <jwhite at codeweavers.com>
> ---
>  server/snd_worker.c |   30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/server/snd_worker.c b/server/snd_worker.c
> index 70148b7..b977bed 100644
> --- a/server/snd_worker.c
> +++ b/server/snd_worker.c
> @@ -115,6 +115,8 @@ struct SndChannel {
>      snd_channel_handle_message_proc handle_message;
>      snd_channel_on_message_done_proc on_message_done;
>      snd_channel_cleanup_channel_proc cleanup;
> +
> +    pthread_mutex_t lock;
>  };
>
>  typedef struct PlaybackChannel PlaybackChannel;
> @@ -220,8 +222,21 @@ static void snd_disconnect_channel(SndChannel
> *channel)
>      spice_marshaller_destroy(channel->send_data.marshaller);
>      snd_channel_put(channel);
>      worker->connection = NULL;
> +
> +    pthread_mutex_destroy(&channel->lock);
> +}
> +
> +static int snd_lock(SndChannel *channel)
> +{
> +    return pthread_mutex_lock(&channel->lock);
> +}
> +
> +static void snd_unlock(SndChannel *channel)
> +{
> +    pthread_mutex_unlock(&channel->lock);
>  }
>
> +
>  static void snd_playback_free_frame(PlaybackChannel *playback_channel,
> AudioFrame *frame)
>  {
>      frame->channel = playback_channel;
> @@ -967,6 +982,8 @@ static SndChannel *__new_channel(SndWorker *worker,
> int size, uint32_t channel_i
>      if (!channel->channel_client) {
>          goto error2;
>      }
> +
> +    pthread_mutex_init(&channel->lock, NULL);
>      return channel;
>
>  error2:
> @@ -1105,10 +1122,15 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_put_samples(SpicePlaybackInstance
>      frame = SPICE_CONTAINEROF(samples, AudioFrame, samples);
>      playback_channel = frame->channel;
>      spice_assert(playback_channel);
> +
> +    if (snd_lock(&playback_channel->base) != 0)
> +        return;
> +
>      if (!snd_channel_put(&playback_channel->base) ||
>          sin->st->worker.connection != &playback_channel->base) {
>          /* lost last reference, channel has been destroyed previously */
>          spice_info("audio samples belong to a disconnected channel");
> +        snd_unlock(&playback_channel->base);
>          return;
>      }
>      spice_assert(playback_channel->base.active);
> @@ -1121,6 +1143,7 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_put_samples(SpicePlaybackInstance
>      playback_channel->pending_frame = frame;
>      snd_set_command(&playback_channel->base, SND_PLAYBACK_PCM_MASK);
>      snd_playback_send(&playback_channel->base);
> +    snd_unlock(&playback_channel->base);
>  }
>
>  void snd_set_playback_latency(RedClient *client, uint32_t latency)
> @@ -1136,8 +1159,11 @@ void snd_set_playback_latency(RedClient *client,
> uint32_t latency)
>                  PlaybackChannel* playback =
> (PlaybackChannel*)now->connection;
>
>                  playback->latency = latency;
> -                snd_set_command(now->connection,
> SND_PLAYBACK_LATENCY_MASK);
> -                snd_playback_send(now->connection);
> +                if (snd_lock(now->connection) == 0) {
> +                    snd_set_command(now->connection,
> SND_PLAYBACK_LATENCY_MASK);
> +                    snd_playback_send(now->connection);
> +                    snd_unlock(now->connection);
> +                }
>              } else {
>                  spice_debug("client doesn't not support
> SPICE_PLAYBACK_CAP_LATENCY");
>              }
> --
> 1.7.10.4
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>



-- 
Marc-André Lureau
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20141008/82633e15/attachment-0001.html>