>
> On Fri, Oct 23, 2015 at 09:17:48AM -0400, Frediano Ziglio wrote:
> >
> > >
> > > 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
>
> Looks like it should be fine because the clearing is done only once the
> operation is completed. Hopefully this is fine too on arch with weak
> memory ordering.
>
> Christophe
>
Clear and set are implemented using atomic operations so order should
be assured by these memory barriers
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel