[Spice-devel] [PATCH 02/14] worker: access dispatcher pending field using helper functions

[Frediano Ziglio]

> 
> From: Marc-André Lureau <marcandre.lureau at gmail.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red_dispatcher.c | 28 ++++++++++++++++++++++------
>  server/red_dispatcher.h |  7 +++++++
>  server/red_worker.c     |  6 ++----
>  server/red_worker.h     |  5 -----
>  4 files changed, 31 insertions(+), 15 deletions(-)
> 
> diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
> index 83aace4..fb85a95 100644
> --- a/server/red_dispatcher.c
> +++ b/server/red_dispatcher.c
> @@ -624,14 +624,24 @@ static void qxl_worker_reset_memslots(QXLWorker
> *qxl_worker)
>      red_dispatcher_reset_memslots((RedDispatcher*)qxl_worker);
>  }
>  
> +static bool red_dispatcher_set_pending(RedDispatcher *dispatcher, int
> pending)
> +{
> +    // TODO: this is not atomic, do we need atomic?

I don't think this is an issue. Dispatcher thread only set while worker
clear it. As clear, set and test are atomic (here the problem is that
test+set is not atomic) the race that can happen is

test
clear
set

now if at the time of test the flag was 0 this means that flag was
already cleared and as only a message is sent this means that worker
is clearing twice or the race was not possible.
If flag was 1 the message was surely sent and we return from function
not calling set so the above race does not occur.

A test_and_set_bit possibly can reduce a bit memory operations but
mainly save a read instruction on a not hot path so I would just
remove the TODO comment.

> +    if (test_bit(pending, dispatcher->pending)) {
> +        return TRUE;
> +    }
> +
> +    set_bit(pending, &dispatcher->pending);
> +    return FALSE;
> +}
> +
>  static void red_dispatcher_wakeup(RedDispatcher *dispatcher)
>  {
>      RedWorkerMessageWakeup payload;
>  
> -    if (test_bit(RED_WORKER_PENDING_WAKEUP, dispatcher->pending)) {
> +    if (red_dispatcher_set_pending(dispatcher,
> RED_DISPATCHER_PENDING_WAKEUP))
>          return;
> -    }
> -    set_bit(RED_WORKER_PENDING_WAKEUP, &dispatcher->pending);
> +
>      dispatcher_send_message(&dispatcher->dispatcher,
>                              RED_WORKER_MESSAGE_WAKEUP,
>                              &payload);
> @@ -646,10 +656,9 @@ static void red_dispatcher_oom(RedDispatcher
> *dispatcher)
>  {
>      RedWorkerMessageOom payload;
>  
> -    if (test_bit(RED_WORKER_PENDING_OOM, dispatcher->pending)) {
> +    if (red_dispatcher_set_pending(dispatcher, RED_DISPATCHER_PENDING_OOM))
>          return;
> -    }
> -    set_bit(RED_WORKER_PENDING_OOM, &dispatcher->pending);
> +
>      dispatcher_send_message(&dispatcher->dispatcher,
>                              RED_WORKER_MESSAGE_OOM,
>                              &payload);
> @@ -1177,3 +1186,10 @@ void
> red_dispatcher_set_dispatcher_opaque(RedDispatcher *red_dispatcher,
>  {
>      dispatcher_set_opaque(&red_dispatcher->dispatcher, opaque);
>  }
> +
> +void red_dispatcher_clear_pending(RedDispatcher *red_dispatcher, int
> pending)
> +{
> +    spice_return_if_fail(red_dispatcher != NULL);
> +
> +    clear_bit(pending, &red_dispatcher->pending);
> +}
> diff --git a/server/red_dispatcher.h b/server/red_dispatcher.h
> index 9ee36d7..671c013 100644
> --- a/server/red_dispatcher.h
> +++ b/server/red_dispatcher.h
> @@ -294,4 +294,11 @@ typedef struct RedWorkerMessageMonitorsConfigAsync {
>  typedef struct RedWorkerMessageDriverUnload {
>  } RedWorkerMessageDriverUnload;
>  
> +enum {
> +    RED_DISPATCHER_PENDING_WAKEUP,
> +    RED_DISPATCHER_PENDING_OOM,
> +};
> +
> +void red_dispatcher_clear_pending(RedDispatcher *red_dispatcher, int
> pending);
> +
>  #endif
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 65961db..0104b0f 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -869,7 +869,6 @@ typedef struct RedWorker {
>      int channel;
>      int id;
>      int running;
> -    uint32_t *pending;
>      struct pollfd poll_fds[MAX_EVENT_SOURCES];
>      struct SpiceWatch watches[MAX_EVENT_SOURCES];
>      unsigned int event_timeout;
> @@ -11271,8 +11270,8 @@ void handle_dev_wakeup(void *opaque, void *payload)
>  {
>      RedWorker *worker = opaque;
>  
> -    clear_bit(RED_WORKER_PENDING_WAKEUP, worker->pending);
>      stat_inc_counter(worker->wakeup_counter, 1);
> +    red_dispatcher_clear_pending(worker->red_dispatcher,
> RED_DISPATCHER_PENDING_WAKEUP);
>  }
>  
>  void handle_dev_oom(void *opaque, void *payload)
> @@ -11305,7 +11304,7 @@ void handle_dev_oom(void *opaque, void *payload)
>                  worker->current_size,
>                  worker->display_channel ?
>                  red_channel_sum_pipes_size(display_red_channel) : 0);
> -    clear_bit(RED_WORKER_PENDING_OOM, worker->pending);
> +    red_dispatcher_clear_pending(worker->red_dispatcher,
> RED_DISPATCHER_PENDING_OOM);
>  }
>  
>  void handle_dev_reset_cursor(void *opaque, void *payload)
> @@ -11883,7 +11882,6 @@ RedWorker* red_worker_new(WorkerInitData *init_data)
>      if (worker->record_fd) {
>          dispatcher_register_universal_handler(dispatcher,
>          worker_dispatcher_record);
>      }
> -    worker->pending = init_data->pending;
>      worker->cursor_visible = TRUE;
>      spice_assert(init_data->num_renderers > 0);
>      worker->num_renderers = init_data->num_renderers;
> diff --git a/server/red_worker.h b/server/red_worker.h
> index c935e0a..91eba24 100644
> --- a/server/red_worker.h
> +++ b/server/red_worker.h
> @@ -24,11 +24,6 @@
>  #include "red_dispatcher.h"
>  
>  enum {
> -    RED_WORKER_PENDING_WAKEUP,
> -    RED_WORKER_PENDING_OOM,
> -};
> -
> -enum {
>      RED_RENDERER_INVALID,
>      RED_RENDERER_SW,
>      RED_RENDERER_OGL_PBUF,

If somebody else agree with me on removing the comment (or changing it
stating is not a problem not being atomic) I would ack and merge it

Frediano