>
> From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel