[PATCH v5 04/11] core: add kqueue support
Pekka Paalanen
ppaalanen at gmail.com
Mon Feb 25 12:43:37 UTC 2019
On Wed, 13 Feb 2019 13:39:09 +0200
Leonid Bobrov via wayland-devel <wayland-devel at lists.freedesktop.org> wrote:
> Signed-off-by: Leonid Bobrov <mazocomp at disroot.org>
> ---
> Makefile.am | 3 +-
> configure.ac | 23 +-
> doc/doxygen/Makefile.am | 3 +-
> src/{event-loop.c => event-loop-epoll.c} | 6 +-
> src/event-loop-kqueue.c | 800 +++++++++++++++++++++++
> src/wayland-os.c | 47 +-
> src/wayland-os.h | 2 +-
> tests/os-wrappers-test.c | 55 +-
> 8 files changed, 920 insertions(+), 19 deletions(-)
> rename src/{event-loop.c => event-loop-epoll.c} (99%)
> create mode 100644 src/event-loop-kqueue.c
>
Hi Leonid,
I think the architectural approach here is good, there are some details
below that I have comments on, and I cannot review the kqueue
implementation at all, so someone else needs to give their Reviewed-by
on that (or if the original author was someone else, then your
acceptance of it could count as that review).
This patch is obviously one that I'd like to see the CI work running at
least in a branch somewhere first to make sure that CI will be
integrated as well.
> diff --git a/Makefile.am b/Makefile.am
> index 489f581..6eefc73 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -76,7 +76,8 @@ libwayland_server_la_LDFLAGS = -version-info 1:0:1
> libwayland_server_la_SOURCES = \
> src/wayland-server.c \
> src/wayland-shm.c \
> - src/event-loop.c
> + src/event-loop-epoll.c \
> + src/event-loop-kqueue.c
>
> nodist_libwayland_server_la_SOURCES = \
> protocol/wayland-server-protocol.h \
> diff --git a/configure.ac b/configure.ac
> index f53408a..cf8d82d 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -65,6 +65,15 @@ AC_SUBST(GCC_CFLAGS)
> AC_CHECK_HEADERS([sys/prctl.h])
> AC_CHECK_FUNCS([accept4 mkostemp posix_fallocate prctl])
>
> +AC_CHECK_HEADERS([sys/epoll.h])
> +if test "x$ac_cv_header_sys_epoll_h" != "xyes"; then
> + AC_CHECK_HEADERS([sys/event.h])
> + if test "x$ac_cv_header_sys_event_h" != "xyes"; then
> + AC_MSG_ERROR([Can't find sys/epoll.h or sys/event.h. Please ensure either epoll or kqueue is available.])
> + fi
> +fi
> +
> +
> # *BSD don't have libdl, but they have its functions
> SAVE_LIBS="$LIBS"
> LIBS=
> @@ -116,12 +125,14 @@ AC_SUBST([ICONDIR])
>
> if test "x$enable_libraries" = "xyes"; then
> PKG_CHECK_MODULES(FFI, [libffi])
> - AC_CHECK_DECL(SFD_CLOEXEC,[],
> - [AC_MSG_ERROR("SFD_CLOEXEC is needed to compile wayland libraries")],
> - [[#include <sys/signalfd.h>]])
> - AC_CHECK_DECL(TFD_CLOEXEC,[],
> - [AC_MSG_ERROR("TFD_CLOEXEC is needed to compile wayland libraries")],
> - [[#include <sys/timerfd.h>]])
> + if test "x$ac_cv_header_sys_epoll_h" == "xyes"; then
> + AC_CHECK_DECL(SFD_CLOEXEC,[],
> + [AC_MSG_ERROR("SFD_CLOEXEC is needed to compile wayland libraries")],
> + [[#include <sys/signalfd.h>]])
> + AC_CHECK_DECL(TFD_CLOEXEC,[],
> + [AC_MSG_ERROR("TFD_CLOEXEC is needed to compile wayland libraries")],
> + [[#include <sys/timerfd.h>]])
> + fi
Ok, this seems fine.
> AC_CHECK_DECL(CLOCK_MONOTONIC,[],
> [AC_MSG_ERROR("CLOCK_MONOTONIC is needed to compile wayland libraries")],
> [[#include <time.h>]])
> diff --git a/doc/doxygen/Makefile.am b/doc/doxygen/Makefile.am
> index f8b0b3a..60bed53 100644
> --- a/doc/doxygen/Makefile.am
> +++ b/doc/doxygen/Makefile.am
> @@ -19,7 +19,8 @@ scanned_src_files_Client = \
>
> scanned_src_files_Server = \
> $(scanned_src_files_shared) \
> - $(top_srcdir)/src/event-loop.c \
> + $(top_srcdir)/src/event-loop-epoll.c \
> + $(top_srcdir)/src/event-loop-kqueue.c \
> $(top_srcdir)/src/wayland-server.c \
> $(top_srcdir)/src/wayland-server.h \
> $(top_srcdir)/src/wayland-server-core.h \
> diff --git a/src/event-loop.c b/src/event-loop-epoll.c
> similarity index 99%
> rename from src/event-loop.c
> rename to src/event-loop-epoll.c
> index eb2dce6..4784f64 100644
> --- a/src/event-loop.c
> +++ b/src/event-loop-epoll.c
> @@ -23,6 +23,9 @@
> * SOFTWARE.
> */
>
> +#include "config.h"
> +
> +#ifdef HAVE_SYS_EPOLL_H
Since we have the two .c files for the two different implementations,
rather than building always both with #ifdefs, you could gate in
Makefile.am which one of the two files gets built.
> #include <stddef.h>
> #include <stdio.h>
> #include <errno.h>
> @@ -523,7 +526,7 @@ wl_event_loop_create(void)
> if (loop == NULL)
> return NULL;
>
> - loop->epoll_fd = wl_os_epoll_create_cloexec();
> + loop->epoll_fd = wl_os_event_create_cloexec();
> if (loop->epoll_fd < 0) {
> free(loop);
> return NULL;
> @@ -702,3 +705,4 @@ wl_event_loop_get_destroy_listener(struct wl_event_loop *loop,
> {
> return wl_signal_get(&loop->destroy_signal, notify);
> }
> +#endif
> diff --git a/src/event-loop-kqueue.c b/src/event-loop-kqueue.c
> new file mode 100644
> index 0000000..3149f53
> --- /dev/null
> +++ b/src/event-loop-kqueue.c
> @@ -0,0 +1,800 @@
> +/*
> + * Copyright © 2008 Kristian Høgsberg
There is probably another copyright holder to add, isn't there?
The person who originally wrote the kqueue-based event loop implementation?
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining
> + * a copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sublicense, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
...
> +/** Create an idle task
> + *
> + * \param loop The event loop that will process the new task.
> + * \param func The idle task dispatch function.
> + * \param data User data.
> + * \return A new idle task (an event source).
> + *
> + * Idle tasks are dispatched before wl_event_loop_dispatch() goes to sleep.
> + * See wl_event_loop_dispatch() for more details.
> + *
> + * Idle tasks fire once, and are automatically destroyed right after the
> + * callback function has been called.
> + *
> + * An idle task can be cancelled before the callback has been called by
> + * wl_event_source_remove(). Calling wl_event_source_remove() after or from
> + * within the callback results in undefined behaviour.
> + *
> + * \sa wl_event_loop_idle_func_t
> + * \memberof wl_event_source
> + */
> +WL_EXPORT struct wl_event_source *
> +wl_event_loop_add_idle(struct wl_event_loop *loop,
> + wl_event_loop_idle_func_t func,
> + void *data)
> +{
> + struct wl_event_source_idle *source;
> +
> + source = malloc(sizeof *source);
> + if (source == NULL)
> + return NULL;
> +
> + source->base.interface = &idle_source_interface;
> + source->base.loop = loop;
> + source->base.fd = -1;
> +
> + source->func = func;
> + source->base.data = data;
> +
> + wl_list_insert(loop->idle_list.prev, &source->base.link);
> +
> + return &source->base;
> +}
It's a bit unfortunate to copy all these functions that must be
identical between the epoll and kqueue implementations. We could have a
event-loop-common.c for sharing these and a event-loop-private.h to go
with it. If you want to do that, then that would be a separate patch
before this one.
...
> diff --git a/src/wayland-os.c b/src/wayland-os.c
> index 93b6f5f..7c8a993 100644
> --- a/src/wayland-os.c
> +++ b/src/wayland-os.c
> @@ -23,6 +23,8 @@
> * SOFTWARE.
> */
>
> +#include "config.h"
> +
> #define _GNU_SOURCE
>
> #include <sys/types.h>
> @@ -30,9 +32,13 @@
> #include <unistd.h>
> #include <fcntl.h>
> #include <errno.h>
> +#ifdef HAVE_SYS_EPOLL_H
> #include <sys/epoll.h>
> +#endif
> +#ifdef HAVE_SYS_EVENT_H
> +#include <sys/event.h>
> +#endif
>
> -#include "../config.h"
> #include "wayland-os.h"
>
> static int
> @@ -62,26 +68,50 @@ wl_os_socket_cloexec(int domain, int type, int protocol)
> {
> int fd;
>
> +#ifdef SOCK_CLOEXEC
> fd = socket(domain, type | SOCK_CLOEXEC, protocol);
> if (fd >= 0)
> return fd;
> if (errno != EINVAL)
> return -1;
> +#endif
>
> fd = socket(domain, type, protocol);
> return set_cloexec_or_close(fd);
> }
>
> +int
> +wl_os_socketpair_cloexec(int domain, int type, int protocol, int sv[2])
> +{
> + int retval;
> +
> +#ifdef SOCK_CLOEXEC
> + retval = socketpair(domain, type | SOCK_CLOEXEC, protocol, sv);
> + if (retval >= 0)
> + return retval;
> + if (errno != EINVAL)
> + return -1;
> +#endif
> +
> + retval = socketpair(domain, type, protocol, sv);
> + if (set_cloexec_or_close(sv[0]) < 0 || set_cloexec_or_close(sv[1]) < 0)
> + retval = -1;
> +
> + return retval;
> +}
This stuff does not seem to belong in this patch.
> +
> int
> wl_os_dupfd_cloexec(int fd, long minfd)
> {
> int newfd;
>
> +#ifdef F_DUPFD_CLOEXEC
> newfd = fcntl(fd, F_DUPFD_CLOEXEC, minfd);
> if (newfd >= 0)
> return newfd;
> if (errno != EINVAL)
> return -1;
> +#endif
Neither this.
>
> newfd = fcntl(fd, F_DUPFD, minfd);
> return set_cloexec_or_close(newfd);
> @@ -123,17 +153,20 @@ wl_os_recvmsg_cloexec(int sockfd, struct msghdr *msg, int flags)
> {
> ssize_t len;
>
> +#ifdef MSG_CMSG_CLOEXEC
> len = recvmsg(sockfd, msg, flags | MSG_CMSG_CLOEXEC);
> if (len >= 0)
> return len;
> if (errno != EINVAL)
> return -1;
> +#endif
Nor this.
>
> return recvmsg_cloexec_fallback(sockfd, msg, flags);
> }
>
> +#if defined(HAVE_SYS_EPOLL_H)
> int
> -wl_os_epoll_create_cloexec(void)
> +wl_os_event_create_cloexec(void)
> {
> int fd;
>
> @@ -148,6 +181,16 @@ wl_os_epoll_create_cloexec(void)
> fd = epoll_create(1);
> return set_cloexec_or_close(fd);
> }
> +#elif defined(HAVE_SYS_EVENT_H)
> +int
> +wl_os_event_create_cloexec(void)
> +{
> + int fd;
> +
> + fd = kqueue();
> + return set_cloexec_or_close(fd);
> +}
> +#endif
Is kqueue() really incapable of creating a close-on-exec file
descriptor from the start?
I think it might be better to just keep wl_os_epoll_create_cloexec()
named as is, and have an alternative wl_os_kqueue_create_cloexec().
They will be used from the different .c files which always match which
function is provided.
>
> int
> wl_os_accept_cloexec(int sockfd, struct sockaddr *addr, socklen_t *addrlen)
> diff --git a/src/wayland-os.h b/src/wayland-os.h
> index f51efaa..86685e0 100644
> --- a/src/wayland-os.h
> +++ b/src/wayland-os.h
> @@ -36,7 +36,7 @@ ssize_t
> wl_os_recvmsg_cloexec(int sockfd, struct msghdr *msg, int flags);
>
> int
> -wl_os_epoll_create_cloexec(void);
> +wl_os_event_create_cloexec(void);
>
> int
> wl_os_accept_cloexec(int sockfd, struct sockaddr *addr, socklen_t *addrlen);
> diff --git a/tests/os-wrappers-test.c b/tests/os-wrappers-test.c
> index 102622c..e7bcd6c 100644
> --- a/tests/os-wrappers-test.c
> +++ b/tests/os-wrappers-test.c
> @@ -26,6 +26,7 @@
>
> #define _GNU_SOURCE
>
> +#include <err.h>
> #include <stdlib.h>
> #include <stdint.h>
> #include <assert.h>
> @@ -38,7 +39,11 @@
> #include <stdarg.h>
> #include <fcntl.h>
> #include <stdio.h>
> -#include <sys/epoll.h>
> +#ifdef HAVE_SYS_EPOLL_H
> +# include <sys/epoll.h>
> +#else
> +# include <sys/event.h>
> +#endif
>
> #include "wayland-private.h"
> #include "test-runner.h"
> @@ -55,8 +60,15 @@ static int wrapped_calls_fcntl;
> static ssize_t (*real_recvmsg)(int, struct msghdr *, int);
> static int wrapped_calls_recvmsg;
>
> +#ifdef HAVE_SYS_EPOLL_H
> static int (*real_epoll_create1)(int);
> static int wrapped_calls_epoll_create1;
> +#endif
> +
> +#if HAVE_SYS_EVENT_H
> +static int (*real_kqueue)(void);
> +static int wrapped_calls_kqueue;
> +#endif
>
> static void
> init_fallbacks(int do_fallbacks)
> @@ -65,7 +77,12 @@ init_fallbacks(int do_fallbacks)
> real_socket = dlsym(RTLD_NEXT, "socket");
> real_fcntl = dlsym(RTLD_NEXT, "fcntl");
> real_recvmsg = dlsym(RTLD_NEXT, "recvmsg");
> +#ifdef HAVE_SYS_EPOLL_H
> real_epoll_create1 = dlsym(RTLD_NEXT, "epoll_create1");
> +#endif
> +#ifdef HAVE_SYS_EVENT_H
> + real_kqueue = dlsym(RTLD_NEXT, "kqueue");
> +#endif
> }
This looks ok.
>
> __attribute__ ((visibility("default"))) int
> @@ -81,6 +98,8 @@ socket(int domain, int type, int protocol)
> return real_socket(domain, type, protocol);
> }
>
> +/* won't work on OpenBSD, since real fcntl have to be called early */
What does this mean?
Why can we not wrap fcntl()?
If you did wl_os_kqueue_create_cloexec() as I suggested above, would
that solve this issue as well?
> +#if !defined(__OpenBSD__)
> __attribute__ ((visibility("default"))) int
> fcntl(int fd, int cmd, ...)
> {
> @@ -100,6 +119,7 @@ fcntl(int fd, int cmd, ...)
>
> return real_fcntl(fd, cmd, arg);
> }
> +#endif
>
> __attribute__ ((visibility("default"))) ssize_t
> recvmsg(int sockfd, struct msghdr *msg, int flags)
> @@ -114,6 +134,7 @@ recvmsg(int sockfd, struct msghdr *msg, int flags)
> return real_recvmsg(sockfd, msg, flags);
> }
>
> +#ifdef HAVE_SYS_EPOLL_H
> __attribute__ ((visibility("default"))) int
> epoll_create1(int flags)
> {
> @@ -127,6 +148,23 @@ epoll_create1(int flags)
>
> return real_epoll_create1(flags);
> }
> +#endif
> +
> +#ifdef HAVE_SYS_EVENT_H
> +__attribute__ ((visibility("default"))) int
> +kqueue(void)
> +{
> + wrapped_calls_kqueue++;
> +
> + if (fall_back) {
> + wrapped_calls_kqueue++; /* kqueue() not wrapped */
> + errno = EINVAL;
> + return -1;
> + }
> +
> + return real_kqueue();
> +}
I don't think this is necessary, because if there is no kqueue()
variant that actually could do close-on-exec internally, there is no
need to force it to the fallback path: the fallback path is used always
already.
> +#endif
>
> static void
> do_os_wrappers_socket_cloexec(int n)
> @@ -184,6 +222,7 @@ do_os_wrappers_dupfd_cloexec(int n)
> * Must have 4 calls if falling back, but must also allow
> * falling back without a forced fallback.
> */
> + warnx("wrapped_calls_fcntl is %d, should be larger than %d", wrapped_calls_fcntl, n);
Letfover from debugging?
> assert(wrapped_calls_fcntl > n);
>
> exec_fd_leak_check(nr_fds);
> @@ -335,14 +374,14 @@ TEST(os_wrappers_recvmsg_cloexec_fallback)
> }
>
> static void
> -do_os_wrappers_epoll_create_cloexec(int n)
> +do_os_wrappers_event_create_cloexec(int n)
> {
> int fd;
> int nr_fds;
>
> nr_fds = count_open_fds();
>
> - fd = wl_os_epoll_create_cloexec();
> + fd = wl_os_event_create_cloexec();
> assert(fd >= 0);
>
> #ifdef EPOLL_CLOEXEC
> @@ -354,16 +393,18 @@ do_os_wrappers_epoll_create_cloexec(int n)
> exec_fd_leak_check(nr_fds);
> }
>
> -TEST(os_wrappers_epoll_create_cloexec)
> +TEST(os_wrappers_event_create_cloexec)
> {
> init_fallbacks(0);
> - do_os_wrappers_epoll_create_cloexec(1);
> + do_os_wrappers_event_create_cloexec(1);
> }
>
> -TEST(os_wrappers_epoll_create_cloexec_fallback)
> +TEST(os_wrappers_event_create_cloexec_fallback)
> {
> init_fallbacks(1);
> - do_os_wrappers_epoll_create_cloexec(2);
> + do_os_wrappers_event_create_cloexec(2);
> }
>
> /* FIXME: add tests for wl_os_accept_cloexec() */
> +
> +/* FIXME: add tests for kqueue() */
What could one test of kqueue()? If nothing comes to mind that's not
already covered, no need for a FIXME.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20190225/1ffefe83/attachment.sig>
More information about the wayland-devel
mailing list