[Spice-devel] [PATCH v3 4/4] worker: use glib main loop

[Frediano Ziglio]

Use the glib mainloop instead of writing our own. The glib loop is both
cleaner to use and is more extensible. It is also very mature and
reduces the maintenance burden on the spice server.

Signed-off-by: Marc-André Lureau 
Signed-off-by: Jonathon Jongsma 
Signed-off-by: Frediano Ziglio 
---
 server/Makefile.am |   4 +-
 server/red-worker.c| 253 +-
 server/red-worker.h|   1 +
 server/spice_timer_queue.c | 267 -
 server/spice_timer_queue.h |  44 
 5 files changed, 101 insertions(+), 468 deletions(-)
 delete mode 100644 server/spice_timer_queue.c
 delete mode 100644 server/spice_timer_queue.h

diff --git a/server/Makefile.am b/server/Makefile.am
index 32ab8eb..5fe95c3 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -104,6 +104,8 @@ libspice_server_la_SOURCES =\
red-parse-qxl.h \
red-worker.c\
red-worker.h\
+   event-loop.c\
+   event-loop.h\
display-channel.c   \
display-channel.h   \
cursor-channel.c\
@@ -119,8 +121,6 @@ libspice_server_la_SOURCES =\
sound.h \
stat.h  \
spicevmc.c  \
-   spice_timer_queue.c \
-   spice_timer_queue.h \
zlib-encoder.c  \
zlib-encoder.h  \
image-cache.h   \
diff --git a/server/red-worker.c b/server/red-worker.c
index 7d3cce3..a35e66e 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -49,30 +49,23 @@
 
 #include "spice.h"
 #include "red-worker.h"
-#include "spice_timer_queue.h"
 #include "cursor-channel.h"
 #include "tree.h"
+#include "event-loop.h"
 
 #define CMD_RING_POLL_TIMEOUT 10 //milli
 #define CMD_RING_POLL_RETRIES 200
 
-#define MAX_EVENT_SOURCES 20
 #define INF_EVENT_WAIT ~0
 
-struct SpiceWatch {
-struct RedWorker *worker;
-SpiceWatchFunc watch_func;
-void *watch_func_opaque;
-};
-
 struct RedWorker {
 pthread_t thread;
 QXLInstance *qxl;
 RedDispatcher *red_dispatcher;
 int running;
 SpiceCoreInterfaceInternal core;
-struct pollfd poll_fds[MAX_EVENT_SOURCES];
-struct SpiceWatch watches[MAX_EVENT_SOURCES];
+
+GMainContext *main_context;
 unsigned int event_timeout;
 
 DisplayChannel *display_channel;
@@ -99,6 +92,13 @@ struct RedWorker {
 FILE *record_fd;
 };
 
+GMainContext* red_worker_get_main_context(RedWorker *worker)
+{
+spice_return_val_if_fail(worker, NULL);
+
+return worker->main_context;
+}
+
 QXLInstance* red_worker_get_qxl(RedWorker *worker)
 {
 spice_return_val_if_fail(worker != NULL, NULL);
@@ -510,84 +510,17 @@ static int common_channel_config_socket(RedChannelClient 
*rcc)
 return TRUE;
 }
 
-static void worker_watch_update_mask(SpiceWatch *watch, int event_mask)
-{
-struct RedWorker *worker;
-int i;
-
-if (!watch) {
-return;
-}
-
-worker = watch->worker;
-i = watch - worker->watches;
-
-worker->poll_fds[i].events = 0;
-if (event_mask & SPICE_WATCH_EVENT_READ) {
-worker->poll_fds[i].events |= POLLIN;
-}
-if (event_mask & SPICE_WATCH_EVENT_WRITE) {
-worker->poll_fds[i].events |= POLLOUT;
-}
-}
-
-static SpiceWatch *worker_watch_add(const SpiceCoreInterfaceInternal *iface,
-int fd, int event_mask, SpiceWatchFunc 
func, void *opaque)
+GMainContext *event_loop_context_from_iface(const SpiceCoreInterfaceInternal 
*iface)
 {
 RedWorker *worker = SPICE_CONTAINEROF(iface, RedWorker, core);
-int i;
 
-/* Search for a free slot in our poll_fds & watches arrays */
-for (i = 0; i < MAX_EVENT_SOURCES; i++) {
-if (worker->poll_fds[i].fd == -1) {
-break;
-}
-}
-if (i == MAX_EVENT_SOURCES) {
-/* Since we are a channel core implementation, we always get called 
from
-   red_channel_client_create(), so opaque always is our rcc */
-RedChannelClient *rcc = opaque;
-spice_warning("could not add a watch for channel type %u id %u",
-  rcc->channel->type, rcc->channel->id);
-return NULL;
-}
-
-worker->poll_fds[i].fd = fd;
-worker->watches[i].worker = worker;
-worker->watches[i].watch_func = func;
-worker->watches[i].watch_func_opaque = opaque;
-worker_watch_update_mask(>watches[i], event_mask);
-
-return >watches[i];
+return worker->main_context;
 }
 
-static void worker_watch_remove(SpiceWatch *watch)
+void 

Re: [Spice-devel] [PATCH v3 4/4] worker: use glib main loop

[Marc-André Lureau]

Hi

- Original Message -
> On Wed, Jan 13, 2016 at 11:39:09AM +, Frediano Ziglio wrote:
> > Use the glib mainloop instead of writing our own. The glib loop is both
> > cleaner to use and is more extensible. It is also very mature and
> > reduces the maintenance burden on the spice server.
> 
> Why not take it further and provide the option to do away
> with the spice event loop entirely. Having 2 threads both
> providing event loops (QEMU event loop thread + SPICE event
> loop thread) feels rather pointless. Obviously for backcompat
> you need to be able to run the event loop in SPICE thread,
> but We could provide an API which lets QEMU tell libspice
> that it can use the default GMainContext and not run its
> own.

The spice "worker" thread is where the display/cursor work takes place. It 
would need significant work to refactor it so the events would all be 
dispatched from the main thread while long job would be in one or more seperate 
threads.

For non-display channels, they are dispatched from the main thread already. But 
some relatively slow job take place in the main thread too, such as audio 
encoding, where a job thread would be welcome.

regards
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3 4/4] worker: use glib main loop

[Daniel P. Berrange]

On Wed, Jan 13, 2016 at 12:18:45PM +, Daniel P. Berrange wrote:
> On Wed, Jan 13, 2016 at 11:39:09AM +, Frediano Ziglio wrote:
> > Use the glib mainloop instead of writing our own. The glib loop is both
> > cleaner to use and is more extensible. It is also very mature and
> > reduces the maintenance burden on the spice server.
> 
> Why not take it further and provide the option to do away
> with the spice event loop entirely. Having 2 threads both
> providing event loops (QEMU event loop thread + SPICE event
> loop thread) feels rather pointless. Obviously for backcompat
> you need to be able to run the event loop in SPICE thread,
> but We could provide an API which lets QEMU tell libspice
> that it can use the default GMainContext and not run its
> own.

Ignore this comment. After talking with Marc-Andre I realize that
spice needs its separate event thread because some of the callbacks
take non-negligble wall-clock time to run.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3 4/4] worker: use glib main loop

[Daniel P. Berrange]

On Wed, Jan 13, 2016 at 11:39:09AM +, Frediano Ziglio wrote:
> Use the glib mainloop instead of writing our own. The glib loop is both
> cleaner to use and is more extensible. It is also very mature and
> reduces the maintenance burden on the spice server.

Why not take it further and provide the option to do away
with the spice event loop entirely. Having 2 threads both
providing event loops (QEMU event loop thread + SPICE event
loop thread) feels rather pointless. Obviously for backcompat
you need to be able to run the event loop in SPICE thread,
but We could provide an API which lets QEMU tell libspice
that it can use the default GMainContext and not run its
own.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel