Re: [PATCH spice-server] RFC: Remove DISPATCHER_ASYNC type

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



And apparently I didn't look at mail for a while and Frediano decided
to do the same thing after our little discussion...


On Wed, 2017-09-06 at 14:03 -0500, Jonathon Jongsma wrote:
> ---
> 
> From a previous conversation:
> On Wed, 2017-09-06 at 13:14 -0400, Frediano Ziglio wrote:
> > > [OT: it's a little bit unclear to me why the async_done handler
> > > is
> > > necessary at all. It is called immediately after the message-
> > > specific
> > > handler is called, and it is called from the same thread. So
> > > anything
> > > that is done in the async_done callback could just as easily be
> > > done in
> > > the message-specific handler, no? I suppose it allows you to
> > > reduce
> > > code duplication slightly, but that seems to be the only
> > > advantage.
> > > If
> > > this async_done feature did not exist, we could simply use a
> > > boolean
> > > argument for whether to send an ACK or not, and the enum would be
> > > unnecessary. But maybe I'm missing something.]
> > > 
> > 
> > Maybe you really got a nice idea! Remove the ASYNC and have
> > ACK/NONE
> > !
> > Would avoid the switch on the async call and also the code would
> > run
> > in the worker thread (that is code should be in a RedWorker or
> > {Display,Cursor}Channel but actually is in RedQXL which should not
> > have code that runs in the worker).
> > 
> > Not checked how easy is...
> > 
> > Frediano
> 
> I guess an initial implementation would look something like this. I
> didn't
> really move any red-qxl code into red-worker, etc.
> 
>  server/dispatcher.c | 11 -----------
>  server/dispatcher.h | 32 +-------------------------------
>  server/red-worker.c | 41 ++++++++++++++++++-----------------------
>  3 files changed, 19 insertions(+), 65 deletions(-)
> 
> diff --git a/server/dispatcher.c b/server/dispatcher.c
> index 76f5eeaf4..931c7efa0 100644
> --- a/server/dispatcher.c
> +++ b/server/dispatcher.c
> @@ -60,7 +60,6 @@ struct DispatcherPrivate {
>      void *payload; /* allocated as max of message sizes */
>      size_t payload_size; /* used to track realloc calls */
>      void *opaque;
> -    dispatcher_handle_async_done handle_async_done;
>      dispatcher_handle_any_message any_handler;
>  };
>  
> @@ -309,8 +308,6 @@ static int
> dispatcher_handle_single_read(Dispatcher *dispatcher)
>              spice_printerr("error writing ack for message %d",
> type);
>              /* TODO: close socketpair? */
>          }
> -    } else if (msg->flag == DISPATCHER_FLAG_ASYNC && dispatcher-
> >priv->handle_async_done) {
> -        dispatcher->priv->handle_async_done(dispatcher->priv-
> >opaque, type, payload);
>      }
>      return 1;
>  }
> @@ -359,14 +356,6 @@ unlock:
>      pthread_mutex_unlock(&dispatcher->priv->lock);
>  }
>  
> -void dispatcher_register_async_done_callback(
> -                                        Dispatcher *dispatcher,
> -                                        dispatcher_handle_async_done
> handler)
> -{
> -    assert(dispatcher->priv->handle_async_done == NULL);
> -    dispatcher->priv->handle_async_done = handler;
> -}
> -
>  void dispatcher_register_handler(Dispatcher *dispatcher, uint32_t
> message_type,
>                                   dispatcher_handle_message handler,
>                                   size_t size, DispatcherFlag flag)
> diff --git a/server/dispatcher.h b/server/dispatcher.h
> index 28d78479e..837522243 100644
> --- a/server/dispatcher.h
> +++ b/server/dispatcher.h
> @@ -85,15 +85,6 @@ typedef void (*dispatcher_handle_any_message)(void
> *opaque,
>                                                uint32_t message_type,
>                                                void *payload);
>  
> -/* The signature of a function that handles async done notifcations
> for all
> - * messages that are registered with DISPATCHER_FLAG_ASYNC flag. See
> - * dispatcher_register_async_done_callback() and
> dispatcher_register_handler()
> - * for more information. */
> -typedef void (*dispatcher_handle_async_done)(void *opaque,
> -                                             uint32_t message_type,
> -                                             void *payload);
> -
> -
>  /* dispatcher_send_message
>   *
>   * Sends a message to the receiving thread. The message type must
> have been
> @@ -124,15 +115,10 @@ void dispatcher_send_message(Dispatcher
> *dispatcher, uint32_t message_type,
>   * since the sender will block until it receives the ACK. This is
> useful when
>   * the sender wants to ensure that the receiver has handled the
> message before
>   * proceeding.
> - *
> - * DISPATCHER_FLAG_ASYNC - When a message is received, the message
> handler is
> - * called and then the dispatcher will call the registered
> async_done handler.
> - * See dispatcher_register_async_done_callback() for more
> information
>   */
>  typedef enum {
>      DISPATCHER_FLAG_NONE = 0,
> -    DISPATCHER_FLAG_ACK,
> -    DISPATCHER_FLAG_ASYNC
> +    DISPATCHER_FLAG_ACK
>  } DispatcherFlag;
>  
>  /* dispatcher_register_handler
> @@ -155,22 +141,6 @@ void dispatcher_register_handler(Dispatcher
> *dispatcher, uint32_t message_type,
>                                   dispatcher_handle_message handler,
> size_t size,
>                                   DispatcherFlag flag);
>  
> -/* dispatcher_register_async_done_callback
> - *
> - * register an async_done callback for this dispatcher. For all
> message types
> - * that were registered with a DISPATCHER_FLAG_ASYNC flag, this
> function will
> - * be called after the normal message handler, and the message will
> be passed
> - * to this function. Note that this function will execute within the
> receiving
> - * thread.
> - *
> - * @dispatcher:     dispatcher
> - * @handler:        callback on the receiver side called *after* the
> - *                  message callback in case flag ==
> DISPATCHER_FLAG_ASYNC.
> - */
> -void dispatcher_register_async_done_callback(
> -                                    Dispatcher *dispatcher,
> -                                    dispatcher_handle_async_done
> handler);
> -
>  /* dispatcher_register_universal_handler
>   *
>   * Register a universal handler that will be called when *any*
> message is
> diff --git a/server/red-worker.c b/server/red-worker.c
> index c3f2dbfa0..cfeba7b35 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -431,6 +431,7 @@ static void handle_dev_update_async(void *opaque,
> void *payload)
>      red_qxl_update_area_complete(worker->qxl, msg->surface_id,
>                                   qxl_dirty_rects, num_dirty_rects);
>      free(qxl_dirty_rects);
> +    red_qxl_async_complete(worker->qxl, msg->base.cmd);
>  }
>  
>  static void handle_dev_update(void *opaque, void *payload)
> @@ -577,14 +578,17 @@ static void
> handle_dev_destroy_primary_surface_async(void *opaque, void *payload
>      uint32_t surface_id = msg->surface_id;
>  
>      destroy_primary_surface(worker, surface_id);
> +    red_qxl_async_complete(worker->qxl, msg->base.cmd);
>  }
>  
>  static void handle_dev_flush_surfaces_async(void *opaque, void
> *payload)
>  {
>      RedWorker *worker = opaque;
> +    RedWorkerMessageFlushSurfacesAsync *msg = payload;
>  
>      flush_all_qxl_commands(worker);
>      display_channel_flush_all_surfaces(worker->display_channel);
> +    red_qxl_async_complete(worker->qxl, msg->base.cmd);
>  }
>  
>  static void handle_dev_stop(void *opaque, void *payload)
> @@ -684,15 +688,18 @@ static void
> handle_dev_destroy_surface_wait_async(void *opaque, void *payload)
>      RedWorker *worker = opaque;
>  
>      display_channel_destroy_surface_wait(worker->display_channel,
> msg->surface_id);
> +    red_qxl_async_complete(worker->qxl, msg->base.cmd);
>  }
>  
>  static void handle_dev_destroy_surfaces_async(void *opaque, void
> *payload)
>  {
>      RedWorker *worker = opaque;
> +    RedWorkerMessageDestroySurfacesAsync *msg = payload;
>  
>      flush_all_qxl_commands(worker);
>      display_channel_destroy_surfaces(worker->display_channel);
>      cursor_channel_reset(worker->cursor_channel);
> +    red_qxl_async_complete(worker->qxl, msg->base.cmd);
>  }
>  
>  static void handle_dev_create_primary_surface_async(void *opaque,
> void *payload)
> @@ -701,6 +708,7 @@ static void
> handle_dev_create_primary_surface_async(void *opaque, void *payload)
>      RedWorker *worker = opaque;
>  
>      dev_create_primary_surface(worker, msg->surface_id, msg-
> >surface);
> +    red_qxl_async_complete(worker->qxl, msg->base.cmd);
>  }
>  
>  static void handle_dev_display_connect(void *opaque, void *payload)
> @@ -797,6 +805,7 @@ static void handle_dev_monitors_config_async(void
> *opaque, void *payload)
>      display_channel_update_monitors_config(worker->display_channel,
> dev_monitors_config,
>                                             MIN(count, msg-
> >max_monitors),
>                                             MIN(max_allowed, msg-
> >max_monitors));
> +    red_qxl_async_complete(worker->qxl, msg->base.cmd);
>  }
>  
>  /* TODO: special, perhaps use another dispatcher? */
> @@ -923,6 +932,7 @@ static void handle_dev_add_memslot_async(void
> *opaque, void *payload)
>      RedWorker *worker = opaque;
>  
>      dev_add_memslot(worker, msg->mem_slot);
> +    red_qxl_async_complete(worker->qxl, msg->base.cmd);
>  }
>  
>  static void handle_dev_reset_memslots(void *opaque, void *payload)
> @@ -995,17 +1005,6 @@ static void handle_dev_loadvm_commands(void
> *opaque, void *payload)
>      }
>  }
>  
> -static void worker_handle_dispatcher_async_done(void *opaque,
> -                                                uint32_t
> message_type,
> -                                                void *payload)
> -{
> -    RedWorker *worker = opaque;
> -    RedWorkerMessageAsync *msg_async = payload;
> -
> -    spice_debug("trace");
> -    red_qxl_async_complete(worker->qxl, msg_async->cmd);
> -}
> -
>  static void worker_dispatcher_record(void *opaque, uint32_t
> message_type, void *payload)
>  {
>      RedWorker *worker = opaque;
> @@ -1015,10 +1014,6 @@ static void worker_dispatcher_record(void
> *opaque, uint32_t message_type, void *
>  
>  static void register_callbacks(Dispatcher *dispatcher)
>  {
> -    dispatcher_register_async_done_callback(
> -                                    dispatcher,
> -                                    worker_handle_dispatcher_async_d
> one);
> -
>      /* TODO: register cursor & display specific msg in respective
> channel files */
>      dispatcher_register_handler(dispatcher,
>                                  RED_WORKER_MESSAGE_DISPLAY_CONNECT,
> @@ -1059,7 +1054,7 @@ static void register_callbacks(Dispatcher
> *dispatcher)
>                                  RED_WORKER_MESSAGE_UPDATE_ASYNC,
>                                  handle_dev_update_async,
>                                  sizeof(RedWorkerMessageUpdateAsync),
> -                                DISPATCHER_FLAG_ASYNC);
> +                                DISPATCHER_FLAG_NONE);
>      dispatcher_register_handler(dispatcher,
>                                  RED_WORKER_MESSAGE_ADD_MEMSLOT,
>                                  handle_dev_add_memslot,
> @@ -1069,7 +1064,7 @@ static void register_callbacks(Dispatcher
> *dispatcher)
>                                  RED_WORKER_MESSAGE_ADD_MEMSLOT_ASYNC
> ,
>                                  handle_dev_add_memslot_async,
>                                  sizeof(RedWorkerMessageAddMemslotAsy
> nc),
> -                                DISPATCHER_FLAG_ASYNC);
> +                                DISPATCHER_FLAG_NONE);
>      dispatcher_register_handler(dispatcher,
>                                  RED_WORKER_MESSAGE_DEL_MEMSLOT,
>                                  handle_dev_del_memslot,
> @@ -1084,7 +1079,7 @@ static void register_callbacks(Dispatcher
> *dispatcher)
>                                  RED_WORKER_MESSAGE_DESTROY_SURFACES_
> ASYNC,
>                                  handle_dev_destroy_surfaces_async,
>                                  sizeof(RedWorkerMessageDestroySurfac
> esAsync),
> -                                DISPATCHER_FLAG_ASYNC);
> +                                DISPATCHER_FLAG_NONE);
>      dispatcher_register_handler(dispatcher,
>                                  RED_WORKER_MESSAGE_DESTROY_PRIMARY_S
> URFACE,
>                                  handle_dev_destroy_primary_surface,
> @@ -1094,12 +1089,12 @@ static void register_callbacks(Dispatcher
> *dispatcher)
>                                  RED_WORKER_MESSAGE_DESTROY_PRIMARY_S
> URFACE_ASYNC,
>                                  handle_dev_destroy_primary_surface_a
> sync,
>                                  sizeof(RedWorkerMessageDestroyPrimar
> ySurfaceAsync),
> -                                DISPATCHER_FLAG_ASYNC);
> +                                DISPATCHER_FLAG_NONE);
>      dispatcher_register_handler(dispatcher,
>                                  RED_WORKER_MESSAGE_CREATE_PRIMARY_SU
> RFACE_ASYNC,
>                                  handle_dev_create_primary_surface_as
> ync,
>                                  sizeof(RedWorkerMessageCreatePrimary
> SurfaceAsync),
> -                                DISPATCHER_FLAG_ASYNC);
> +                                DISPATCHER_FLAG_NONE);
>      dispatcher_register_handler(dispatcher,
>                                  RED_WORKER_MESSAGE_CREATE_PRIMARY_SU
> RFACE,
>                                  handle_dev_create_primary_surface,
> @@ -1134,7 +1129,7 @@ static void register_callbacks(Dispatcher
> *dispatcher)
>                                  RED_WORKER_MESSAGE_FLUSH_SURFACES_AS
> YNC,
>                                  handle_dev_flush_surfaces_async,
>                                  sizeof(RedWorkerMessageFlushSurfaces
> Async),
> -                                DISPATCHER_FLAG_ASYNC);
> +                                DISPATCHER_FLAG_NONE);
>      dispatcher_register_handler(dispatcher,
>                                  RED_WORKER_MESSAGE_STOP,
>                                  handle_dev_stop,
> @@ -1174,7 +1169,7 @@ static void register_callbacks(Dispatcher
> *dispatcher)
>                                  RED_WORKER_MESSAGE_DESTROY_SURFACE_W
> AIT_ASYNC,
>                                  handle_dev_destroy_surface_wait_asyn
> c,
>                                  sizeof(RedWorkerMessageDestroySurfac
> eWaitAsync),
> -                                DISPATCHER_FLAG_ASYNC);
> +                                DISPATCHER_FLAG_NONE);
>      dispatcher_register_handler(dispatcher,
>                                  RED_WORKER_MESSAGE_RESET_MEMSLOTS,
>                                  handle_dev_reset_memslots,
> @@ -1184,7 +1179,7 @@ static void register_callbacks(Dispatcher
> *dispatcher)
>                                  RED_WORKER_MESSAGE_MONITORS_CONFIG_A
> SYNC,
>                                  handle_dev_monitors_config_async,
>                                  sizeof(RedWorkerMessageMonitorsConfi
> gAsync),
> -                                DISPATCHER_FLAG_ASYNC);
> +                                DISPATCHER_FLAG_NONE);
>      dispatcher_register_handler(dispatcher,
>                                  RED_WORKER_MESSAGE_DRIVER_UNLOAD,
>                                  handle_dev_driver_unload,
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]