[Spice-devel] [PATCH 02/14] worker: access dispatcher pending field using helper functions
Christophe Fergeau
cfergeau at redhat.com
Fri Oct 23 08:44:08 PDT 2015
On Fri, Oct 23, 2015 at 09:17:48AM -0400, Frediano Ziglio wrote:
>
> >
> > 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
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20151023/59ddb081/attachment.sig>
More information about the Spice-devel
mailing list