[Spice-devel] [RFC] server: add main_dispatcher
Alon Levy
alevy at redhat.com
Thu Sep 8 04:25:56 PDT 2011
On Thu, Sep 08, 2011 at 02:24:32AM +0200, Marc-André Lureau wrote:
> Hi Alon,
>
> On Thu, Sep 8, 2011 at 1:17 AM, Alon Levy <alevy at redhat.com> wrote:
> > ---
> > server/Makefile.am | 2 +
> > server/main_dispatcher.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++
> > server/main_dispatcher.h | 9 +++
> > server/reds.c | 5 ++-
> > 4 files changed, 147 insertions(+), 1 deletions(-)
> > create mode 100644 server/main_dispatcher.c
> > create mode 100644 server/main_dispatcher.h
> >
> > diff --git a/server/Makefile.am b/server/Makefile.am
> > index a7cdd84..f1d4052 100644
> > --- a/server/Makefile.am
> > +++ b/server/Makefile.am
> > @@ -80,6 +80,8 @@ libspice_server_la_SOURCES = \
> > red_common.h \
> > red_dispatcher.c \
> > red_dispatcher.h \
> > + main_dispatcher.c \
> > + main_dispatcher.h \
> > red_memslots.c \
> > red_memslots.h \
> > red_parse_qxl.c \
> > diff --git a/server/main_dispatcher.c b/server/main_dispatcher.c
> > new file mode 100644
> > index 0000000..8cdb2e0
> > --- /dev/null
> > +++ b/server/main_dispatcher.c
> > @@ -0,0 +1,132 @@
> > +#include <unistd.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +#include <pthread.h>
> > +
> > +#include "common/spice_common.h"
> > +#include "main_dispatcher.h"
> > +
> > +/*
> > + * Main Dispatcher
> > + * ===============
> > + *
> > + * Communication channel between any non main thread and the main thread.
> > + *
> > + * The main thread is that from which spice_server_init is called.
> > + *
> > + * Messages are single sized, sent from the non-main thread to the main-thread.
> > + * No acknowledge is sent back. This prevents a possible deadlock with the main
> > + * thread already waiting on a response for the existing red_dispatcher used
> > + * by the worker thread.
> > + *
>
> How often do you expect it to be used? What will be the overhead of
> those messages, size & rate? From the description, it looks a lot like
> the async message queues in Pulseaudio (see src/pulsecore/asyncq.h).
Ok, having looked at that stuff I think we don't need that complexity. It is
trying to solve a more complicated problem, sending from a single or multiple writers
to a real time thread. We don't have that requirement, since this is a strictly
secondary information channel. I think just adding a mutex to ensure multiple writer
support despite currently using a single write, and that's it. What do you think?
> PA uses eventfd for notification/semaphore & lock-free queues (and a
> simple object system, not unions). Just an idea, could be changed
> later perhaps. socketpair is probably fine.
>
> Lennart, I wonder what today you think about it, if there is any
> drawback or improvements. ie would you use it again?
>
> cheers
>
> > + * All events have three functions:
> > + * main_dispatcher_<event_name> - non static, public function
> > + * main_dispatcher_self_<event_name> - handler for main thread
> > + * main_dispatcher_handle_<event_name> - handler for callback from main thread
> > + * seperate from self because it may send an ack or do other work in the future.
> > + */
> > +
> > +typedef struct {
> > + SpiceCoreInterface *core;
> > + int main_fd;
> > + int other_fd;
> > + pthread_t self;
> > +} MainDispatcher;
> > +
> > +MainDispatcher main_dispatcher;
> > +
> > +enum {
> > + MAIN_DISPATCHER_CHANNEL_EVENT = 0,
> > +
> > + MAIN_DISPATCHER_NUM_MESSAGES
> > +};
> > +
> > +typedef struct MainDispatcherMessage {
> > + uint32_t type;
> > + union {
> > + struct {
> > + int event;
> > + SpiceChannelEventInfo info;
> > + } channel_event;
> > + } data;
> > +} MainDispatcherMessage;
> > +
> > +
> > +/* channel_event - calls core->channel_event, must be done in main thread */
> > +static void main_dispatcher_self_handle_channel_event(int event,
> > + SpiceChannelEventInfo *info)
> > +{
> > + main_dispatcher.core->channel_event(event, info);
> > +}
> > +
> > +static void main_dispatcher_handle_channel_event(MainDispatcherMessage *msg)
> > +{
> > + main_dispatcher_self_handle_channel_event(msg->data.channel_event.event,
> > + &msg->data.channel_event.info);
> > +}
> > +
> > +void main_dispatcher_channel_event(int event, SpiceChannelEventInfo *info)
> > +{
> > + MainDispatcherMessage msg;
> > +
> > + if (pthread_self() == main_dispatcher.self) {
> > + main_dispatcher_self_handle_channel_event(event, info);
> > + return;
> > + }
> > + msg.type = MAIN_DISPATCHER_CHANNEL_EVENT;
> > + msg.data.channel_event.event = event;
> > + msg.data.channel_event.info = *info;
> > + write(main_dispatcher.other_fd, &msg, sizeof(msg));
> > +}
> > +
> > +
> > +static void main_dispatcher_handle_read(int fd, int event, void *opaque)
> > +{
> > + int ret;
> > + MainDispatcher *md = opaque;
> > + MainDispatcherMessage msg;
> > +
> > + ret = read(md->main_fd, &msg, sizeof(msg));
> > + if (ret == -1) {
> > + if (errno == EAGAIN) {
> > + return;
> > + } else {
> > + red_printf("error reading from main dispatcher: %d\n", errno);
> > + /* TODO: close channel? */
> > + return;
> > + }
> > + }
> > + if (ret != sizeof(msg)) {
> > + red_printf("short read in main dispatcher: %d < %zd\n", ret, sizeof(msg));
> > + /* TODO: close channel? */
> > + return;
> > + }
> > +
> > + switch (msg.type) {
> > + case MAIN_DISPATCHER_CHANNEL_EVENT:
> > + main_dispatcher_handle_channel_event(&msg);
> > + break;
> > + default:
> > + red_printf("error: unhandled main dispatcher message type %d\n",
> > + msg.type);
> > + }
> > +}
> > +
> > +void main_dispatcher_init(SpiceCoreInterface *core)
> > +{
> > + int channels[2];
> > +
> > + memset(&main_dispatcher, 0, sizeof(main_dispatcher));
> > + main_dispatcher.core = core;
> > +
> > + if (socketpair(AF_LOCAL, SOCK_STREAM, 0, channels) == -1) {
> > + red_error("socketpair failed %s", strerror(errno));
> > + return;
> > + }
> > + main_dispatcher.main_fd = channels[0];
> > + main_dispatcher.other_fd = channels[1];
> > + main_dispatcher.self = pthread_self();
> > +
> > + core->watch_add(main_dispatcher.main_fd, SPICE_WATCH_EVENT_READ,
> > + main_dispatcher_handle_read, &main_dispatcher);
> > +}
> > diff --git a/server/main_dispatcher.h b/server/main_dispatcher.h
> > new file mode 100644
> > index 0000000..2c201c7
> > --- /dev/null
> > +++ b/server/main_dispatcher.h
> > @@ -0,0 +1,9 @@
> > +#ifndef MAIN_DISPATCHER_H
> > +#define MAIN_DISPATCHER_H
> > +
> > +#include <spice.h>
> > +
> > +void main_dispatcher_channel_event(int event, SpiceChannelEventInfo *info);
> > +void main_dispatcher_init(SpiceCoreInterface *core);
> > +
> > +#endif //MAIN_DISPATCHER_H
> > diff --git a/server/reds.c b/server/reds.c
> > index 90779ff..ef9da4f 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -56,6 +56,7 @@
> > #include "main_channel.h"
> > #include "red_common.h"
> > #include "red_dispatcher.h"
> > +#include "main_dispatcher.h"
> > #include "snd_worker.h"
> > #include <spice/stats.h>
> > #include "stat.h"
> > @@ -322,7 +323,8 @@ static void reds_stream_channel_event(RedsStream *s, int event)
> > {
> > if (core->base.minor_version < 3 || core->channel_event == NULL)
> > return;
> > - core->channel_event(event, &s->info);
> > +
> > + main_dispatcher_channel_event(event, &s->info);
> > }
> >
> > static ssize_t stream_write_cb(RedsStream *s, const void *buf, size_t size)
> > @@ -3519,6 +3521,7 @@ static int do_spice_init(SpiceCoreInterface *core_interface)
> > init_vd_agent_resources();
> > ring_init(&reds->clients);
> > reds->num_clients = 0;
> > + main_dispatcher_init(core);
> >
> > if (!(reds->mig_timer = core->timer_add(migrate_timout, NULL))) {
> > red_error("migration timer create failed");
> > --
> > 1.7.6.1
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
>
>
>
> --
> Marc-André Lureau
More information about the Spice-devel
mailing list