[RFC,weston,1/2] libweston: Add event loop abstraction API

Submitted by Quentin Glidic on Feb. 19, 2018, 7:19 p.m.

Details

Message ID 20180219191905.23769-2-sardemff7+wayland@sardemff7.net
State New
Headers show
Series "Event loop abstraction API" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Quentin Glidic Feb. 19, 2018, 7:19 p.m.
From: Quentin Glidic <sardemff7+git@sardemff7.net>

This will allow compositors to pick any event loop implementation they
want, and integrating the libweston components with it.
Specifically, idle and timeout sources can now have proper priorities to
avoid being ghosted by client event processing.

Signed-off-by: Quentin Glidic <sardemff7+git@sardemff7.net>
---
 compositor/main.c      | 182 +++++++++++++++++++++++++++++++++++++++++++++++--
 libweston/compositor.c |  86 +++++++++++++++++++++++
 libweston/compositor.h |  67 ++++++++++++++++++
 3 files changed, 329 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/compositor/main.c b/compositor/main.c
index 18810f288..c0a5ba506 100644
--- a/compositor/main.c
+++ b/compositor/main.c
@@ -76,6 +76,180 @@  struct wet_compositor {
 	bool drm_use_current_mode;
 };
 
+static struct wet_compositor *
+to_wet_compositor(struct weston_compositor *compositor)
+{
+	return weston_compositor_get_user_data(compositor);
+}
+
+struct wet_event_source {
+	struct wl_event_loop *loop;
+	struct wl_event_source *source;
+	weston_event_source_callback cb;
+	weston_event_source_fd_callback fd_cb;
+	void *user_data;
+};
+
+static void
+wet_event_source_idle_cb(void *data)
+{
+	struct wet_event_source *wsource = data;
+	enum weston_event_source_status ret;
+
+	ret = wsource->cb(wsource->user_data);
+	if (ret == WESTON_EVENT_SOURCE_REMOVE)
+		return;
+
+	wsource->source = wl_event_loop_add_idle(wsource->loop, wet_event_source_idle_cb, wsource);
+	if (wsource->source == NULL)
+		free(wsource);
+}
+
+static void *
+wet_event_source_add_idle(struct weston_compositor *compositor,
+			  enum weston_event_source_priority priority,
+			  weston_event_source_callback cb, void *user_data)
+{
+	struct wet_event_source *wsource = malloc(sizeof(struct wet_event_source));
+	if (wsource == NULL)
+		return NULL;
+
+	wsource->loop = wl_display_get_event_loop(compositor->wl_display);
+	wsource->cb = cb;
+	wsource->user_data = user_data;
+
+	wsource->source = wl_event_loop_add_idle(wsource->loop, wet_event_source_idle_cb, wsource);
+	if (wsource->source == NULL) {
+		free(wsource);
+		return NULL;
+	}
+
+	return wsource;
+}
+
+static int
+wet_event_source_timeout_cb(void *data)
+{
+	struct wet_event_source *wsource = data;
+	enum weston_event_source_status ret;
+
+	ret = wsource->cb(wsource->user_data);
+	if (ret == WESTON_EVENT_SOURCE_CONTINUE)
+		return 0;
+
+	wl_event_source_remove(wsource->source);
+	free(wsource);
+	return 0;
+}
+
+static void *
+wet_event_source_add_timeout(struct weston_compositor *compositor,
+			     enum weston_event_source_priority priority,
+			     uint32_t milliseconds,
+			     weston_event_source_callback cb, void *user_data)
+{
+	struct wet_event_source *wsource = malloc(sizeof(struct wet_event_source));
+	if (wsource == NULL)
+		return NULL;
+
+	wsource->loop = wl_display_get_event_loop(compositor->wl_display);
+	wsource->cb = cb;
+	wsource->user_data = user_data;
+
+	wsource->source = wl_event_loop_add_timer(wsource->loop, wet_event_source_timeout_cb, wsource);
+	if (wsource->source == NULL) {
+		free(wsource);
+		return NULL;
+	}
+
+	if (milliseconds > 0)
+		wl_event_source_timer_update(wsource->source, milliseconds);
+
+	return wsource;
+}
+
+static void
+wet_event_source_update_timeout(struct weston_compositor *compositor,
+				void *source, uint32_t milliseconds)
+{
+	struct wet_event_source *wsource = source;
+
+	wl_event_source_timer_update(wsource->source, milliseconds);
+}
+
+static int
+wet_event_source_fd_cb(int fd, uint32_t mask, void *data)
+{
+	struct wet_event_source *wsource = data;
+	enum weston_event_source_status ret;
+
+	ret = wsource->fd_cb(fd, mask, wsource->user_data);
+	if (ret == WESTON_EVENT_SOURCE_CONTINUE)
+		return 0;
+
+	wl_event_source_remove(wsource->source);
+	free(wsource);
+	return 0;
+}
+
+static void *
+wet_event_source_add_fd(struct weston_compositor *compositor,
+			enum weston_event_source_priority priority,
+			int fd, enum weston_event_source_fd_events events,
+			weston_event_source_fd_callback cb, void *user_data)
+{
+	struct wet_event_source *wsource = malloc(sizeof(struct wet_event_source));
+	if (wsource == NULL)
+		return NULL;
+
+	wsource->loop = wl_display_get_event_loop(compositor->wl_display);
+	wsource->fd_cb = cb;
+	wsource->user_data = user_data;
+
+	wsource->source = wl_event_loop_add_fd(wsource->loop, fd, events, wet_event_source_fd_cb, wsource);
+	if (wsource->source == NULL) {
+		free(wsource);
+		return NULL;
+	}
+
+	return wsource;
+}
+
+static void
+wet_event_source_update_fd(struct weston_compositor *compositor, void *source,
+			   enum weston_event_source_fd_events events)
+{
+	struct wet_event_source *wsource = source;
+
+	wl_event_source_fd_update(wsource->source, events);
+}
+
+static bool
+wet_event_source_is_valid(struct weston_compositor *compositor, void *source)
+{
+	return (source != NULL);
+}
+
+static void
+wet_event_source_remove(struct weston_compositor *compositor, void *source)
+{
+	struct wet_event_source *wsource = source;
+
+	wl_event_source_remove(wsource->source);
+	free(wsource);
+}
+
+
+static const struct weston_event_loop wet_event_loop = {
+	.add_idle = wet_event_source_add_idle,
+	.add_timeout = wet_event_source_add_timeout,
+	.update_timeout = wet_event_source_update_timeout,
+	.add_fd = wet_event_source_add_fd,
+	.update_fd = wet_event_source_update_fd,
+	.is_valid = wet_event_source_is_valid,
+	.remove = wet_event_source_remove,
+};
+
 static FILE *weston_logfile = NULL;
 
 static int cached_tm_mday = -1;
@@ -346,12 +520,6 @@  log_uname(void)
 						usys.version, usys.machine);
 }
 
-static struct wet_compositor *
-to_wet_compositor(struct weston_compositor *compositor)
-{
-	return weston_compositor_get_user_data(compositor);
-}
-
 static void
 wet_set_pending_output_handler(struct weston_compositor *ec,
 			       wl_notify_func_t handler)
@@ -1773,6 +1941,8 @@  int main(int argc, char *argv[])
 	}
 	segv_compositor = ec;
 
+	ec->event_loop = &wet_event_loop;
+
 	if (weston_compositor_init_config(ec, config) < 0)
 		goto out;
 
diff --git a/libweston/compositor.c b/libweston/compositor.c
index aec937bb8..2f129da38 100644
--- a/libweston/compositor.c
+++ b/libweston/compositor.c
@@ -5732,3 +5732,89 @@  weston_compositor_load_xwayland(struct weston_compositor *compositor)
 		return -1;
 	return 0;
 }
+
+
+WL_EXPORT void
+*weston_compositor_event_source_add_idle(struct weston_compositor *compositor, enum weston_event_source_priority priority, weston_event_source_callback cb, void *user_data)
+{
+	assert(compositor->event_loop != NULL);
+	assert(compositor->event_loop->add_idle != NULL);
+
+	return compositor->event_loop->add_idle(compositor, priority, cb, user_data);
+}
+
+WL_EXPORT void
+*weston_compositor_event_source_add_timeout(struct weston_compositor *compositor, enum weston_event_source_priority priority, uint32_t milliseconds, weston_event_source_callback cb, void *user_data)
+{
+	assert(compositor->event_loop != NULL);
+	assert(compositor->event_loop->add_timeout != NULL);
+
+	return compositor->event_loop->add_timeout(compositor, priority, milliseconds, cb, user_data);
+}
+
+WL_EXPORT void
+weston_compositor_event_source_update_timeout(struct weston_compositor *compositor, void *source, uint32_t milliseconds)
+{
+	assert(compositor->event_loop != NULL);
+	assert(compositor->event_loop->update_timeout != NULL);
+
+	compositor->event_loop->update_timeout(compositor, source, milliseconds);
+}
+
+WL_EXPORT void
+*weston_compositor_event_source_add_timeout_seconds(struct weston_compositor *compositor, enum weston_event_source_priority priority, uint32_t seconds, weston_event_source_callback cb, void *user_data)
+{
+	assert(compositor->event_loop != NULL);
+
+	if (compositor->event_loop->add_timeout_seconds == NULL)
+		return compositor->event_loop->add_timeout(compositor, priority, seconds * 1000, cb, user_data);
+	else
+		return compositor->event_loop->add_timeout_seconds(compositor, priority, seconds, cb, user_data);
+}
+
+WL_EXPORT void
+weston_compositor_event_source_update_timeout_seconds(struct weston_compositor *compositor, void *source, uint32_t seconds)
+{
+	assert(compositor->event_loop != NULL);
+
+	if (compositor->event_loop->update_timeout_seconds == NULL)
+		return compositor->event_loop->update_timeout(compositor, source, seconds * 1000);
+	else
+		return compositor->event_loop->update_timeout_seconds(compositor, source, seconds);
+}
+
+WL_EXPORT void
+*weston_compositor_event_source_add_fd(struct weston_compositor *compositor, enum weston_event_source_priority priority, int fd, enum weston_event_source_fd_events events, weston_event_source_fd_callback cb, void *user_data)
+{
+	assert(compositor->event_loop != NULL);
+	assert(compositor->event_loop->add_fd != NULL);
+
+	return compositor->event_loop->add_fd(compositor, priority, fd, events, cb, user_data);
+}
+
+WL_EXPORT void
+weston_compositor_event_source_update_fd(struct weston_compositor *compositor, void *source, enum weston_event_source_fd_events events)
+{
+	assert(compositor->event_loop != NULL);
+	assert(compositor->event_loop->update_fd != NULL);
+
+	return compositor->event_loop->update_fd(compositor, source, events);
+}
+
+WL_EXPORT bool
+weston_compositor_event_source_is_valid(struct weston_compositor *compositor, void *source)
+{
+	assert(compositor->event_loop != NULL);
+	assert(compositor->event_loop->is_valid != NULL);
+
+	return compositor->event_loop->is_valid(compositor, source);
+}
+
+WL_EXPORT void
+weston_compositor_event_source_remove(struct weston_compositor *compositor, void *source)
+{
+	assert(compositor->event_loop != NULL);
+	assert(compositor->event_loop->remove != NULL);
+
+	return compositor->event_loop->remove(compositor, source);
+}
diff --git a/libweston/compositor.h b/libweston/compositor.h
index ca1acc605..9d662b9d5 100644
--- a/libweston/compositor.h
+++ b/libweston/compositor.h
@@ -857,11 +857,13 @@  struct weston_backend {
 
 struct weston_desktop_xwayland;
 struct weston_desktop_xwayland_interface;
+struct weston_event_loop;
 
 struct weston_compositor {
 	struct wl_signal destroy_signal;
 
 	struct wl_display *wl_display;
+	const struct weston_event_loop *event_loop;
 	struct weston_desktop_xwayland *xwayland;
 	const struct weston_desktop_xwayland_interface *xwayland_interface;
 
@@ -1959,6 +1961,71 @@  weston_pending_output_coldplug(struct weston_compositor *compositor);
 struct weston_output *
 weston_output_from_resource(struct wl_resource *resource);
 
+enum weston_event_source_priority {
+	WESTON_EVENT_SOURCE_PRIORITY_HIGH = -100,
+	WESTON_EVENT_SOURCE_PRIORITY_DEFAULT = 0,
+	WESTON_EVENT_SOURCE_PRIORITY_TIMEOUT = 0,
+	WESTON_EVENT_SOURCE_PRIORITY_FD = 0,
+	WESTON_EVENT_SOURCE_PRIORITY_IDLE = 200,
+	WESTON_EVENT_SOURCE_PRIORITY_LOW = 300,
+};
+enum weston_event_source_status {
+	WESTON_EVENT_SOURCE_REMOVE,
+	WESTON_EVENT_SOURCE_CONTINUE,
+};
+enum weston_event_source_fd_events {
+	WESTON_EVENT_SOURCE_FD_IN  = (1 << 0),
+	WESTON_EVENT_SOURCE_FD_OUT = (1 << 1),
+	WESTON_EVENT_SOURCE_FD_HUP = (1 << 2),
+	WESTON_EVENT_SOURCE_FD_ERR = (1 << 3),
+};
+typedef enum weston_event_source_status (*weston_event_source_callback)(void *user_data);
+typedef enum weston_event_source_status (*weston_event_source_fd_callback)(int fd, enum weston_event_source_fd_events events, void *user_data);
+struct weston_event_loop {
+	void *(*add_idle)
+		(struct weston_compositor *compositor,
+		 enum weston_event_source_priority priority,
+		 weston_event_source_callback cb, void *user_data);
+	void *(*add_timeout)
+		(struct weston_compositor *compositor,
+		 enum weston_event_source_priority priority,
+		 uint32_t milliseconds,
+		 weston_event_source_callback cb, void *user_data);
+	void (*update_timeout)
+		(struct weston_compositor *compositor, void *source,
+		 uint32_t milliseconds);
+	void *(*add_timeout_seconds)
+		(struct weston_compositor *compositor,
+		 enum weston_event_source_priority priority,
+		 uint32_t seconds,
+		 weston_event_source_callback cb, void *user_data);
+	void (*update_timeout_seconds)
+		(struct weston_compositor *compositor, void *source,
+		 uint32_t seconds);
+	void *(*add_fd)
+		(struct weston_compositor *compositor,
+		 enum weston_event_source_priority priority,
+		 int fd, enum weston_event_source_fd_events events,
+		 weston_event_source_fd_callback cb, void *user_data);
+	void (*update_fd)
+		(struct weston_compositor *compositor, void *source,
+		 enum weston_event_source_fd_events events);
+	bool (*is_valid)
+		(struct weston_compositor *compositor, void *source);
+	void (*remove)
+		(struct weston_compositor *compositor, void *source);
+};
+
+void *weston_compositor_event_source_add_idle(struct weston_compositor *compositor, enum weston_event_source_priority priority, weston_event_source_callback cb, void *user_data);
+void *weston_compositor_event_source_add_timeout(struct weston_compositor *compositor, enum weston_event_source_priority priority, uint32_t milliseconds, weston_event_source_callback cb, void *user_data);
+void weston_compositor_event_source_update_timeout(struct weston_compositor *compositor, void *source, uint32_t milliseconds);
+void *weston_compositor_event_source_add_timeout_seconds(struct weston_compositor *compositor, enum weston_event_source_priority priority, uint32_t seconds, weston_event_source_callback cb, void *user_data);
+void weston_compositor_event_source_update_timeout_seconds(struct weston_compositor *compositor, void *source, uint32_t seconds);
+void *weston_compositor_event_source_add_fd(struct weston_compositor *compositor, enum weston_event_source_priority priority, int fd, enum weston_event_source_fd_events events, weston_event_source_fd_callback cb, void *user_data);
+void weston_compositor_event_source_update_fd(struct weston_compositor *compositor, void *source, enum weston_event_source_fd_events events);
+bool weston_compositor_event_source_is_valid(struct weston_compositor *compositor, void *source);
+void weston_compositor_event_source_remove(struct weston_compositor *compositor, void *source);
+
 #ifdef  __cplusplus
 }
 #endif

Comments

On Mon, 19 Feb 2018 20:19:04 +0100
Quentin Glidic <sardemff7+wayland@sardemff7.net> wrote:

> From: Quentin Glidic <sardemff7+git@sardemff7.net>
> 
> This will allow compositors to pick any event loop implementation they
> want, and integrating the libweston components with it.
> Specifically, idle and timeout sources can now have proper priorities to
> avoid being ghosted by client event processing.
> 
> Signed-off-by: Quentin Glidic <sardemff7+git@sardemff7.net>
> ---
>  compositor/main.c      | 182 +++++++++++++++++++++++++++++++++++++++++++++++--
>  libweston/compositor.c |  86 +++++++++++++++++++++++
>  libweston/compositor.h |  67 ++++++++++++++++++
>  3 files changed, 329 insertions(+), 6 deletions(-)
> 

Hi,

in principle, the idea of making an interface for this is ok by me, but
I would like to hear others' opinions as well.

The comments below are about the API details.

> diff --git a/compositor/main.c b/compositor/main.c
> index 18810f288..c0a5ba506 100644
> --- a/compositor/main.c
> +++ b/compositor/main.c
> @@ -76,6 +76,180 @@ struct wet_compositor {
>  	bool drm_use_current_mode;
>  };
>  
> +static struct wet_compositor *
> +to_wet_compositor(struct weston_compositor *compositor)
> +{
> +	return weston_compositor_get_user_data(compositor);
> +}
> +
> +struct wet_event_source {
> +	struct wl_event_loop *loop;
> +	struct wl_event_source *source;
> +	weston_event_source_callback cb;
> +	weston_event_source_fd_callback fd_cb;
> +	void *user_data;
> +};
> +
> +static void
> +wet_event_source_idle_cb(void *data)
> +{
> +	struct wet_event_source *wsource = data;
> +	enum weston_event_source_status ret;
> +
> +	ret = wsource->cb(wsource->user_data);
> +	if (ret == WESTON_EVENT_SOURCE_REMOVE)
> +		return;
> +
> +	wsource->source = wl_event_loop_add_idle(wsource->loop, wet_event_source_idle_cb, wsource);
> +	if (wsource->source == NULL)
> +		free(wsource);

Do we actually want an automatic re-add mode? I don't remember
libweston having any need for it. This implementation is also silent
about failures, it has no way to signal them.

If you modelled this after the "must check" return value in
libwayland-server, I don't think it means this.

> +}
> +
> +static void *
> +wet_event_source_add_idle(struct weston_compositor *compositor,
> +			  enum weston_event_source_priority priority,
> +			  weston_event_source_callback cb, void *user_data)
> +{
> +	struct wet_event_source *wsource = malloc(sizeof(struct wet_event_source));
> +	if (wsource == NULL)
> +		return NULL;
> +
> +	wsource->loop = wl_display_get_event_loop(compositor->wl_display);
> +	wsource->cb = cb;
> +	wsource->user_data = user_data;
> +
> +	wsource->source = wl_event_loop_add_idle(wsource->loop, wet_event_source_idle_cb, wsource);
> +	if (wsource->source == NULL) {
> +		free(wsource);
> +		return NULL;
> +	}
> +
> +	return wsource;
> +}
> +
> +static int
> +wet_event_source_timeout_cb(void *data)
> +{
> +	struct wet_event_source *wsource = data;
> +	enum weston_event_source_status ret;
> +
> +	ret = wsource->cb(wsource->user_data);
> +	if (ret == WESTON_EVENT_SOURCE_CONTINUE)

libwayland-server timers are always single-shot, there is no repeat
mode.

> +		return 0;
> +
> +	wl_event_source_remove(wsource->source);
> +	free(wsource);
> +	return 0;
> +}
> +
> +static void *
> +wet_event_source_add_timeout(struct weston_compositor *compositor,
> +			     enum weston_event_source_priority priority,
> +			     uint32_t milliseconds,
> +			     weston_event_source_callback cb, void *user_data)

I know the libwayland-server timers use an integer milliseconds delay,
but often we actually compute an absolute time when we want be woken up
and convert it to a delay. How about making the API use an absolute
timestamp instead? That means it would also need to specify the clock
to use, but I think it would be more robust against system load when
setting up a timer, if the event loop actually supports absolute times.

> +{
> +	struct wet_event_source *wsource = malloc(sizeof(struct wet_event_source));
> +	if (wsource == NULL)
> +		return NULL;
> +
> +	wsource->loop = wl_display_get_event_loop(compositor->wl_display);
> +	wsource->cb = cb;
> +	wsource->user_data = user_data;
> +
> +	wsource->source = wl_event_loop_add_timer(wsource->loop, wet_event_source_timeout_cb, wsource);
> +	if (wsource->source == NULL) {
> +		free(wsource);
> +		return NULL;
> +	}
> +
> +	if (milliseconds > 0)
> +		wl_event_source_timer_update(wsource->source, milliseconds);
> +
> +	return wsource;
> +}
> +
> +static void
> +wet_event_source_update_timeout(struct weston_compositor *compositor,
> +				void *source, uint32_t milliseconds)
> +{
> +	struct wet_event_source *wsource = source;
> +
> +	wl_event_source_timer_update(wsource->source, milliseconds);
> +}
> +
> +static int
> +wet_event_source_fd_cb(int fd, uint32_t mask, void *data)
> +{
> +	struct wet_event_source *wsource = data;
> +	enum weston_event_source_status ret;
> +
> +	ret = wsource->fd_cb(fd, mask, wsource->user_data);
> +	if (ret == WESTON_EVENT_SOURCE_CONTINUE)
> +		return 0;
> +
> +	wl_event_source_remove(wsource->source);
> +	free(wsource);

What's with this automatic removal? Where do we use it?

For fds, it would be common to change the events to watch for (polling
for writable is usually temporary, polling for readable is practically
always on).

> +	return 0;
> +}
> +
> +static void *
> +wet_event_source_add_fd(struct weston_compositor *compositor,
> +			enum weston_event_source_priority priority,
> +			int fd, enum weston_event_source_fd_events events,
> +			weston_event_source_fd_callback cb, void *user_data)
> +{
> +	struct wet_event_source *wsource = malloc(sizeof(struct wet_event_source));
> +	if (wsource == NULL)
> +		return NULL;
> +
> +	wsource->loop = wl_display_get_event_loop(compositor->wl_display);
> +	wsource->fd_cb = cb;
> +	wsource->user_data = user_data;
> +
> +	wsource->source = wl_event_loop_add_fd(wsource->loop, fd, events, wet_event_source_fd_cb, wsource);
> +	if (wsource->source == NULL) {
> +		free(wsource);
> +		return NULL;
> +	}
> +
> +	return wsource;
> +}
> +
> +static void
> +wet_event_source_update_fd(struct weston_compositor *compositor, void *source,
> +			   enum weston_event_source_fd_events events)
> +{
> +	struct wet_event_source *wsource = source;
> +
> +	wl_event_source_fd_update(wsource->source, events);
> +}
> +
> +static bool
> +wet_event_source_is_valid(struct weston_compositor *compositor, void *source)
> +{
> +	return (source != NULL);

In isolation, this looks a bit... unnecessary?

> +}
> +
> +static void
> +wet_event_source_remove(struct weston_compositor *compositor, void *source)
> +{
> +	struct wet_event_source *wsource = source;
> +
> +	wl_event_source_remove(wsource->source);
> +	free(wsource);
> +}
> +
> +
> +static const struct weston_event_loop wet_event_loop = {
> +	.add_idle = wet_event_source_add_idle,
> +	.add_timeout = wet_event_source_add_timeout,
> +	.update_timeout = wet_event_source_update_timeout,
> +	.add_fd = wet_event_source_add_fd,
> +	.update_fd = wet_event_source_update_fd,
> +	.is_valid = wet_event_source_is_valid,
> +	.remove = wet_event_source_remove,
> +};
> +
>  static FILE *weston_logfile = NULL;
>  
>  static int cached_tm_mday = -1;
> @@ -346,12 +520,6 @@ log_uname(void)
>  						usys.version, usys.machine);
>  }
>  
> -static struct wet_compositor *
> -to_wet_compositor(struct weston_compositor *compositor)
> -{
> -	return weston_compositor_get_user_data(compositor);
> -}
> -
>  static void
>  wet_set_pending_output_handler(struct weston_compositor *ec,
>  			       wl_notify_func_t handler)
> @@ -1773,6 +1941,8 @@ int main(int argc, char *argv[])
>  	}
>  	segv_compositor = ec;
>  
> +	ec->event_loop = &wet_event_loop;
> +
>  	if (weston_compositor_init_config(ec, config) < 0)
>  		goto out;
>  
> diff --git a/libweston/compositor.c b/libweston/compositor.c
> index aec937bb8..2f129da38 100644
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c
> @@ -5732,3 +5732,89 @@ weston_compositor_load_xwayland(struct weston_compositor *compositor)
>  		return -1;
>  	return 0;
>  }
> +

All this stuff could use a new file, IMO.

> +
> +WL_EXPORT void
> +*weston_compositor_event_source_add_idle(struct weston_compositor *compositor, enum weston_event_source_priority priority, weston_event_source_callback cb, void *user_data)
> +{
> +	assert(compositor->event_loop != NULL);
> +	assert(compositor->event_loop->add_idle != NULL);
> +
> +	return compositor->event_loop->add_idle(compositor, priority, cb, user_data);
> +}
> +
> +WL_EXPORT void
> +*weston_compositor_event_source_add_timeout(struct weston_compositor *compositor, enum weston_event_source_priority priority, uint32_t milliseconds, weston_event_source_callback cb, void *user_data)
> +{
> +	assert(compositor->event_loop != NULL);
> +	assert(compositor->event_loop->add_timeout != NULL);
> +
> +	return compositor->event_loop->add_timeout(compositor, priority, milliseconds, cb, user_data);
> +}
> +
> +WL_EXPORT void
> +weston_compositor_event_source_update_timeout(struct weston_compositor *compositor, void *source, uint32_t milliseconds)
> +{
> +	assert(compositor->event_loop != NULL);
> +	assert(compositor->event_loop->update_timeout != NULL);
> +
> +	compositor->event_loop->update_timeout(compositor, source, milliseconds);
> +}
> +
> +WL_EXPORT void
> +*weston_compositor_event_source_add_timeout_seconds(struct weston_compositor *compositor, enum weston_event_source_priority priority, uint32_t seconds, weston_event_source_callback cb, void *user_data)
> +{

Why the _seconds variant?

> +	assert(compositor->event_loop != NULL);
> +
> +	if (compositor->event_loop->add_timeout_seconds == NULL)
> +		return compositor->event_loop->add_timeout(compositor, priority, seconds * 1000, cb, user_data);
> +	else
> +		return compositor->event_loop->add_timeout_seconds(compositor, priority, seconds, cb, user_data);
> +}
> +
> +WL_EXPORT void
> +weston_compositor_event_source_update_timeout_seconds(struct weston_compositor *compositor, void *source, uint32_t seconds)
> +{
> +	assert(compositor->event_loop != NULL);
> +
> +	if (compositor->event_loop->update_timeout_seconds == NULL)
> +		return compositor->event_loop->update_timeout(compositor, source, seconds * 1000);
> +	else
> +		return compositor->event_loop->update_timeout_seconds(compositor, source, seconds);
> +}
> +
> +WL_EXPORT void
> +*weston_compositor_event_source_add_fd(struct weston_compositor *compositor, enum weston_event_source_priority priority, int fd, enum weston_event_source_fd_events events, weston_event_source_fd_callback cb, void *user_data)
> +{
> +	assert(compositor->event_loop != NULL);
> +	assert(compositor->event_loop->add_fd != NULL);
> +
> +	return compositor->event_loop->add_fd(compositor, priority, fd, events, cb, user_data);
> +}
> +
> +WL_EXPORT void
> +weston_compositor_event_source_update_fd(struct weston_compositor *compositor, void *source, enum weston_event_source_fd_events events)
> +{
> +	assert(compositor->event_loop != NULL);
> +	assert(compositor->event_loop->update_fd != NULL);
> +
> +	return compositor->event_loop->update_fd(compositor, source, events);
> +}
> +
> +WL_EXPORT bool
> +weston_compositor_event_source_is_valid(struct weston_compositor *compositor, void *source)
> +{
> +	assert(compositor->event_loop != NULL);
> +	assert(compositor->event_loop->is_valid != NULL);
> +
> +	return compositor->event_loop->is_valid(compositor, source);
> +}
> +
> +WL_EXPORT void
> +weston_compositor_event_source_remove(struct weston_compositor *compositor, void *source)
> +{
> +	assert(compositor->event_loop != NULL);
> +	assert(compositor->event_loop->remove != NULL);
> +
> +	return compositor->event_loop->remove(compositor, source);
> +}
> diff --git a/libweston/compositor.h b/libweston/compositor.h
> index ca1acc605..9d662b9d5 100644
> --- a/libweston/compositor.h
> +++ b/libweston/compositor.h
> @@ -857,11 +857,13 @@ struct weston_backend {
>  
>  struct weston_desktop_xwayland;
>  struct weston_desktop_xwayland_interface;
> +struct weston_event_loop;
>  
>  struct weston_compositor {
>  	struct wl_signal destroy_signal;
>  
>  	struct wl_display *wl_display;
> +	const struct weston_event_loop *event_loop;
>  	struct weston_desktop_xwayland *xwayland;
>  	const struct weston_desktop_xwayland_interface *xwayland_interface;
>  
> @@ -1959,6 +1961,71 @@ weston_pending_output_coldplug(struct weston_compositor *compositor);
>  struct weston_output *
>  weston_output_from_resource(struct wl_resource *resource);
>  
> +enum weston_event_source_priority {
> +	WESTON_EVENT_SOURCE_PRIORITY_HIGH = -100,
> +	WESTON_EVENT_SOURCE_PRIORITY_DEFAULT = 0,
> +	WESTON_EVENT_SOURCE_PRIORITY_TIMEOUT = 0,
> +	WESTON_EVENT_SOURCE_PRIORITY_FD = 0,
> +	WESTON_EVENT_SOURCE_PRIORITY_IDLE = 200,
> +	WESTON_EVENT_SOURCE_PRIORITY_LOW = 300,
> +};

There can be several kinds of fd sources etc., maybe the API functions
should take an int instead of enum as argument? This list will probably
change in any case to list not high/default/low, but
input-device/output-device/repaint-timing/shell-timeout/...

> +enum weston_event_source_status {
> +	WESTON_EVENT_SOURCE_REMOVE,
> +	WESTON_EVENT_SOURCE_CONTINUE,
> +};

This was the thing I didn't understand the need of.

> +enum weston_event_source_fd_events {
> +	WESTON_EVENT_SOURCE_FD_IN  = (1 << 0),
> +	WESTON_EVENT_SOURCE_FD_OUT = (1 << 1),
> +	WESTON_EVENT_SOURCE_FD_HUP = (1 << 2),
> +	WESTON_EVENT_SOURCE_FD_ERR = (1 << 3),
> +};

I think ERR and HUP should be delivered always, but I suppose you need
them here for the callbacks more than the API funcs.

> +typedef enum weston_event_source_status (*weston_event_source_callback)(void *user_data);
> +typedef enum weston_event_source_status (*weston_event_source_fd_callback)(int fd, enum weston_event_source_fd_events events, void *user_data);
> +struct weston_event_loop {
> +	void *(*add_idle)
> +		(struct weston_compositor *compositor,
> +		 enum weston_event_source_priority priority,
> +		 weston_event_source_callback cb, void *user_data);
> +	void *(*add_timeout)
> +		(struct weston_compositor *compositor,
> +		 enum weston_event_source_priority priority,
> +		 uint32_t milliseconds,
> +		 weston_event_source_callback cb, void *user_data);
> +	void (*update_timeout)
> +		(struct weston_compositor *compositor, void *source,
> +		 uint32_t milliseconds);
> +	void *(*add_timeout_seconds)
> +		(struct weston_compositor *compositor,
> +		 enum weston_event_source_priority priority,
> +		 uint32_t seconds,
> +		 weston_event_source_callback cb, void *user_data);
> +	void (*update_timeout_seconds)
> +		(struct weston_compositor *compositor, void *source,
> +		 uint32_t seconds);
> +	void *(*add_fd)
> +		(struct weston_compositor *compositor,
> +		 enum weston_event_source_priority priority,
> +		 int fd, enum weston_event_source_fd_events events,
> +		 weston_event_source_fd_callback cb, void *user_data);
> +	void (*update_fd)
> +		(struct weston_compositor *compositor, void *source,
> +		 enum weston_event_source_fd_events events);
> +	bool (*is_valid)
> +		(struct weston_compositor *compositor, void *source);
> +	void (*remove)
> +		(struct weston_compositor *compositor, void *source);
> +};
> +
> +void *weston_compositor_event_source_add_idle(struct weston_compositor *compositor, enum weston_event_source_priority priority, weston_event_source_callback cb, void *user_data);
> +void *weston_compositor_event_source_add_timeout(struct weston_compositor *compositor, enum weston_event_source_priority priority, uint32_t milliseconds, weston_event_source_callback cb, void *user_data);
> +void weston_compositor_event_source_update_timeout(struct weston_compositor *compositor, void *source, uint32_t milliseconds);
> +void *weston_compositor_event_source_add_timeout_seconds(struct weston_compositor *compositor, enum weston_event_source_priority priority, uint32_t seconds, weston_event_source_callback cb, void *user_data);
> +void weston_compositor_event_source_update_timeout_seconds(struct weston_compositor *compositor, void *source, uint32_t seconds);
> +void *weston_compositor_event_source_add_fd(struct weston_compositor *compositor, enum weston_event_source_priority priority, int fd, enum weston_event_source_fd_events events, weston_event_source_fd_callback cb, void *user_data);
> +void weston_compositor_event_source_update_fd(struct weston_compositor *compositor, void *source, enum weston_event_source_fd_events events);
> +bool weston_compositor_event_source_is_valid(struct weston_compositor *compositor, void *source);
> +void weston_compositor_event_source_remove(struct weston_compositor *compositor, void *source);
> +
>  #ifdef  __cplusplus
>  }
>  #endif

Thanks,
pq
On Wed, 21 Feb 2018 11:54:43 +0200
Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Mon, 19 Feb 2018 20:19:04 +0100
> Quentin Glidic <sardemff7+wayland@sardemff7.net> wrote:
> 
> > From: Quentin Glidic <sardemff7+git@sardemff7.net>
> > 
> > This will allow compositors to pick any event loop implementation they
> > want, and integrating the libweston components with it.
> > Specifically, idle and timeout sources can now have proper priorities to
> > avoid being ghosted by client event processing.
> > 
> > Signed-off-by: Quentin Glidic <sardemff7+git@sardemff7.net>
> > ---
> >  compositor/main.c      | 182 +++++++++++++++++++++++++++++++++++++++++++++++--
> >  libweston/compositor.c |  86 +++++++++++++++++++++++
> >  libweston/compositor.h |  67 ++++++++++++++++++
> >  3 files changed, 329 insertions(+), 6 deletions(-)
> >   
> 
> Hi,
> 
> in principle, the idea of making an interface for this is ok by me, but
> I would like to hear others' opinions as well.
> 
> The comments below are about the API details.


> > @@ -1959,6 +1961,71 @@ weston_pending_output_coldplug(struct weston_compositor *compositor);
> >  struct weston_output *
> >  weston_output_from_resource(struct wl_resource *resource);
> >  
> > +enum weston_event_source_priority {
> > +	WESTON_EVENT_SOURCE_PRIORITY_HIGH = -100,
> > +	WESTON_EVENT_SOURCE_PRIORITY_DEFAULT = 0,
> > +	WESTON_EVENT_SOURCE_PRIORITY_TIMEOUT = 0,
> > +	WESTON_EVENT_SOURCE_PRIORITY_FD = 0,
> > +	WESTON_EVENT_SOURCE_PRIORITY_IDLE = 200,
> > +	WESTON_EVENT_SOURCE_PRIORITY_LOW = 300,
> > +};  
> 
> There can be several kinds of fd sources etc., maybe the API functions
> should take an int instead of enum as argument? This list will probably
> change in any case to list not high/default/low, but
> input-device/output-device/repaint-timing/shell-timeout/...

It seems I have forgotten how libwayland-server works. For each
readable client socket, it does one read up to a max number of bytes
and processes all the fully received messages in the buffer. It does
this at most once for each connection on one event loop cycle.

The above means that a single client flooding the socket cannot starve
other clients or any other processing either.

Furthermore, all Wayland protocol requests are "fast", meaning that
they shouldn't take very long, which makes it less likely for a
flooding client to be able to hog an unfair portion of the server
processing time. However, this is just a speculation. One would have to
audit all requests and do some testing to verify. One thing coming to
mind would be to create and destroy a huge wl_shm_pool, in case the
mmap()/munmap() overhead is significant.

I stand corrected, maybe we don't need the priorities in the event loop
after all.


Thanks,
pq