<p>laforge <strong>submitted</strong> this change.</p><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/20732">View Change</a></p><div style="white-space:pre-wrap">Approvals:
laforge: Looks good to me, approved
fixeria: Looks good to me, but someone else must approve
pespin: Looks good to me, but someone else must approve
Jenkins Builder: Verified
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">select: Migrate over to poll()<br><br>select is an ancient interface with weird restrictions, such as<br>the fact that it cannot be used for file descriptor values > 1024.<br><br>This may have been sufficient 40 years ago, but certainly is not in<br>2020. I wanted to migrate to epoll(), but unfortunately it doesn't<br>work well with the fact that existing programs simply set osmo_fd.flags<br>without making any API calls at the time they change those flags.<br><br>So let's do the migration to poll() as a first step, and then consider<br>epoll() as a second step further down the road, after introducing new<br>APIs and porting applications over.<br><br>The poll() code introduced in this patch is not extremely efficient,<br>as it needs to do extensive linked list iterations after poll() returns<br>in order to find the osmo_fd from the fd. Optimization is possible,<br>but let's postpone that to a follow-up patch.<br><br>At compile time, a new --enable-force-io-select argument can be given<br>to configure, forcing the use of the old select() backend instead of the<br>new poll() based backend.<br><br>Change-Id: I9e80da68a144b36926066610d0d3df06abe09bca<br>---<br>M configure.ac<br>M include/osmocom/core/timer.h<br>M src/select.c<br>M src/timer.c<br>4 files changed, 164 insertions(+), 3 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/configure.ac b/configure.ac</span><br><span>index e867197..7de495b 100644</span><br><span>--- a/configure.ac</span><br><span>+++ b/configure.ac</span><br><span>@@ -62,7 +62,7 @@</span><br><span> </span><br><span> dnl checks for header files</span><br><span> AC_HEADER_STDC</span><br><span style="color: hsl(0, 100%, 40%);">-AC_CHECK_HEADERS(execinfo.h sys/select.h sys/socket.h sys/signalfd.h sys/timerfd.h syslog.h ctype.h netinet/tcp.h netinet/in.h)</span><br><span style="color: hsl(120, 100%, 40%);">+AC_CHECK_HEADERS(execinfo.h poll.h sys/select.h sys/socket.h sys/signalfd.h sys/timerfd.h syslog.h ctype.h netinet/tcp.h netinet/in.h)</span><br><span> # for src/conv.c</span><br><span> AC_FUNC_ALLOCA</span><br><span> AC_SEARCH_LIBS([dlopen], [dl dld], [LIBRARY_DLOPEN="$LIBS";LIBS=""])</span><br><span>@@ -252,6 +252,16 @@</span><br><span> AC_DEFINE([OSMO_FD_CHECK],[1],[Instrument the osmo_fd_register])</span><br><span> fi</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+AC_ARG_ENABLE([force_io_select],</span><br><span style="color: hsl(120, 100%, 40%);">+ [AS_HELP_STRING(</span><br><span style="color: hsl(120, 100%, 40%);">+ [--enable-force-io-select],</span><br><span style="color: hsl(120, 100%, 40%);">+ [Build with old select I/O instead of poll]</span><br><span style="color: hsl(120, 100%, 40%);">+ )],</span><br><span style="color: hsl(120, 100%, 40%);">+ [force_io_select=$enableval], [force_io_select="no"])</span><br><span style="color: hsl(120, 100%, 40%);">+AS_IF([test "x$force_io_select" = "xyes"], [</span><br><span style="color: hsl(120, 100%, 40%);">+ AC_DEFINE([FORCE_IO_SELECT], [1], [Force the use of select() instaed of poll()])</span><br><span style="color: hsl(120, 100%, 40%);">+])</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> AC_ARG_ENABLE(msgfile,</span><br><span> [AS_HELP_STRING(</span><br><span> [--disable-msgfile],</span><br><span>diff --git a/include/osmocom/core/timer.h b/include/osmocom/core/timer.h</span><br><span>index 1979766..6ffc3b1 100644</span><br><span>--- a/include/osmocom/core/timer.h</span><br><span>+++ b/include/osmocom/core/timer.h</span><br><span>@@ -84,6 +84,7 @@</span><br><span> * internal timer list management</span><br><span> */</span><br><span> struct timeval *osmo_timers_nearest(void);</span><br><span style="color: hsl(120, 100%, 40%);">+int osmo_timers_nearest_ms(void);</span><br><span> void osmo_timers_prepare(void);</span><br><span> int osmo_timers_update(void);</span><br><span> int osmo_timers_check(void);</span><br><span>diff --git a/src/select.c b/src/select.c</span><br><span>index 1bb354b..71ee7f6 100644</span><br><span>--- a/src/select.c</span><br><span>+++ b/src/select.c</span><br><span>@@ -4,7 +4,7 @@</span><br><span> * userspace logging daemon for the iptables ULOG target</span><br><span> * of the linux 2.4 netfilter subsystem. */</span><br><span> /*</span><br><span style="color: hsl(0, 100%, 40%);">- * (C) 2000-2009 by Harald Welte <laforge@gnumonks.org></span><br><span style="color: hsl(120, 100%, 40%);">+ * (C) 2000-2020 by Harald Welte <laforge@gnumonks.org></span><br><span> * All Rights Reserverd.</span><br><span> *</span><br><span> * SPDX-License-Identifier: GPL-2.0+</span><br><span>@@ -41,8 +41,9 @@</span><br><span> </span><br><span> #include "../config.h"</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-#ifdef HAVE_SYS_SELECT_H</span><br><span style="color: hsl(120, 100%, 40%);">+#if defined(HAVE_SYS_SELECT_H) && defined(HAVE_POLL_H)</span><br><span> #include <sys/select.h></span><br><span style="color: hsl(120, 100%, 40%);">+#include <poll.h></span><br><span> </span><br><span> /*! \addtogroup select</span><br><span> * @{</span><br><span>@@ -56,6 +57,18 @@</span><br><span> static __thread struct llist_head osmo_fds; /* TLS cannot use LLIST_HEAD() */</span><br><span> static __thread int unregistered_count;</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+#ifndef FORCE_IO_SELECT</span><br><span style="color: hsl(120, 100%, 40%);">+struct poll_state {</span><br><span style="color: hsl(120, 100%, 40%);">+ /* array of pollfd */</span><br><span style="color: hsl(120, 100%, 40%);">+ struct pollfd *poll;</span><br><span style="color: hsl(120, 100%, 40%);">+ /* number of entries in pollfd allocated */</span><br><span style="color: hsl(120, 100%, 40%);">+ unsigned int poll_size;</span><br><span style="color: hsl(120, 100%, 40%);">+ /* number of osmo_fd registered */</span><br><span style="color: hsl(120, 100%, 40%);">+ unsigned int num_registered;</span><br><span style="color: hsl(120, 100%, 40%);">+};</span><br><span style="color: hsl(120, 100%, 40%);">+static __thread struct poll_state g_poll;</span><br><span style="color: hsl(120, 100%, 40%);">+#endif /* FORCE_IO_SELECT */</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> /*! Set up an osmo-fd. Will not register it.</span><br><span> * \param[inout] ofd Osmo FD to be set-up</span><br><span> * \param[in] fd OS-level file descriptor number</span><br><span>@@ -136,6 +149,19 @@</span><br><span> return 0;</span><br><span> }</span><br><span> #endif</span><br><span style="color: hsl(120, 100%, 40%);">+#ifndef FORCE_IO_SELECT</span><br><span style="color: hsl(120, 100%, 40%);">+ if (g_poll.num_registered + 1 > g_poll.poll_size) {</span><br><span style="color: hsl(120, 100%, 40%);">+ struct pollfd *p;</span><br><span style="color: hsl(120, 100%, 40%);">+ unsigned int new_size = g_poll.poll_size ? g_poll.poll_size * 2 : 1024;</span><br><span style="color: hsl(120, 100%, 40%);">+ p = talloc_realloc(OTC_GLOBAL, g_poll.poll, struct pollfd, new_size);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!p)</span><br><span style="color: hsl(120, 100%, 40%);">+ return -ENOMEM;</span><br><span style="color: hsl(120, 100%, 40%);">+ memset(p + g_poll.poll_size, 0, new_size - g_poll.poll_size);</span><br><span style="color: hsl(120, 100%, 40%);">+ g_poll.poll = p;</span><br><span style="color: hsl(120, 100%, 40%);">+ g_poll.poll_size = new_size;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+ g_poll.num_registered++;</span><br><span style="color: hsl(120, 100%, 40%);">+#endif /* FORCE_IO_SELECT */</span><br><span> </span><br><span> llist_add_tail(&fd->list, &osmo_fds);</span><br><span> </span><br><span>@@ -152,6 +178,9 @@</span><br><span> * osmo_fd_is_registered() */</span><br><span> unregistered_count++;</span><br><span> llist_del(&fd->list);</span><br><span style="color: hsl(120, 100%, 40%);">+#ifndef FORCE_IO_SELECT</span><br><span style="color: hsl(120, 100%, 40%);">+ g_poll.num_registered--;</span><br><span style="color: hsl(120, 100%, 40%);">+#endif /* FORCE_IO_SELECT */</span><br><span> }</span><br><span> </span><br><span> /*! Close a file descriptor, mark it as closed + unregister from select loop abstraction</span><br><span>@@ -246,6 +275,110 @@</span><br><span> return work;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+#ifndef FORCE_IO_SELECT</span><br><span style="color: hsl(120, 100%, 40%);">+/* fill g_poll.poll and return the number of entries filled */</span><br><span style="color: hsl(120, 100%, 40%);">+static unsigned int poll_fill_fds(void)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ struct osmo_fd *ufd;</span><br><span style="color: hsl(120, 100%, 40%);">+ unsigned int i = 0;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ llist_for_each_entry(ufd, &osmo_fds, list) {</span><br><span style="color: hsl(120, 100%, 40%);">+ struct pollfd *p;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!ufd->when)</span><br><span style="color: hsl(120, 100%, 40%);">+ continue;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ p = &g_poll.poll[i++];</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ p->fd = ufd->fd;</span><br><span style="color: hsl(120, 100%, 40%);">+ p->events = 0;</span><br><span style="color: hsl(120, 100%, 40%);">+ p->revents = 0;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* use the same mapping as the Linux kernel does in fs/select.c */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ufd->when & OSMO_FD_READ)</span><br><span style="color: hsl(120, 100%, 40%);">+ p->events |= POLLIN | POLLHUP | POLLERR;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ufd->when & OSMO_FD_WRITE)</span><br><span style="color: hsl(120, 100%, 40%);">+ p->events |= POLLOUT | POLLERR;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ufd->when & OSMO_FD_EXCEPT)</span><br><span style="color: hsl(120, 100%, 40%);">+ p->events |= POLLPRI;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ return i;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+/* iterate over first n_fd entries of g_poll.poll + dispatch */</span><br><span style="color: hsl(120, 100%, 40%);">+static int poll_disp_fds(int n_fd)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ struct osmo_fd *ufd;</span><br><span style="color: hsl(120, 100%, 40%);">+ unsigned int i;</span><br><span style="color: hsl(120, 100%, 40%);">+ int work = 0;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ for (i = 0; i < n_fd; i++) {</span><br><span style="color: hsl(120, 100%, 40%);">+ struct pollfd *p = &g_poll.poll[i];</span><br><span style="color: hsl(120, 100%, 40%);">+ int flags = 0;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!p->revents)</span><br><span style="color: hsl(120, 100%, 40%);">+ continue;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ ufd = osmo_fd_get_by_fd(p->fd);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!ufd) {</span><br><span style="color: hsl(120, 100%, 40%);">+ /* FD might have been unregistered meanwhile */</span><br><span style="color: hsl(120, 100%, 40%);">+ continue;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+ /* use the same mapping as the Linux kernel does in fs/select.c */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (p->revents & (POLLIN | POLLHUP | POLLERR))</span><br><span style="color: hsl(120, 100%, 40%);">+ flags |= OSMO_FD_READ;</span><br><span style="color: hsl(120, 100%, 40%);">+ if (p->revents & (POLLOUT | POLLERR))</span><br><span style="color: hsl(120, 100%, 40%);">+ flags |= OSMO_FD_WRITE;</span><br><span style="color: hsl(120, 100%, 40%);">+ if (p->revents & POLLPRI)</span><br><span style="color: hsl(120, 100%, 40%);">+ flags |= OSMO_FD_EXCEPT;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* make sure we never report more than the user requested */</span><br><span style="color: hsl(120, 100%, 40%);">+ flags &= ufd->when;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (flags) {</span><br><span style="color: hsl(120, 100%, 40%);">+ work = 1;</span><br><span style="color: hsl(120, 100%, 40%);">+ /* make sure to clear any log context before processing the next incoming message</span><br><span style="color: hsl(120, 100%, 40%);">+ * as part of some file descriptor callback. This effectively prevents "context</span><br><span style="color: hsl(120, 100%, 40%);">+ * leaking" from processing of one message into processing of the next message as part</span><br><span style="color: hsl(120, 100%, 40%);">+ * of one iteration through the list of file descriptors here. See OS#3813 */</span><br><span style="color: hsl(120, 100%, 40%);">+ log_reset_context();</span><br><span style="color: hsl(120, 100%, 40%);">+ ufd->cb(ufd, flags);</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ return work;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+static int _osmo_select_main(int polling)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ unsigned int n_poll;</span><br><span style="color: hsl(120, 100%, 40%);">+ int rc;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* prepare read and write fdsets */</span><br><span style="color: hsl(120, 100%, 40%);">+ n_poll = poll_fill_fds();</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!polling)</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_timers_prepare();</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ rc = poll(g_poll.poll, n_poll, polling ? 0 : osmo_timers_nearest_ms());</span><br><span style="color: hsl(120, 100%, 40%);">+ if (rc < 0)</span><br><span style="color: hsl(120, 100%, 40%);">+ return 0;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* fire timers */</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_timers_update();</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(osmo_ctx->select);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* call registered callback functions */</span><br><span style="color: hsl(120, 100%, 40%);">+ return poll_disp_fds(n_poll);</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+#else /* FORCE_IO_SELECT */</span><br><span style="color: hsl(120, 100%, 40%);">+/* the old implementation based on select, used 2008-2020 */</span><br><span> static int _osmo_select_main(int polling)</span><br><span> {</span><br><span> fd_set readset, writeset, exceptset;</span><br><span>@@ -273,6 +406,7 @@</span><br><span> /* call registered callback functions */</span><br><span> return osmo_fd_disp_fds(&readset, &writeset, &exceptset);</span><br><span> }</span><br><span style="color: hsl(120, 100%, 40%);">+#endif /* FORCE_IO_SELECT */</span><br><span> </span><br><span> /*! select main loop integration</span><br><span> * \param[in] polling should we pollonly (1) or block on select (0)</span><br><span>diff --git a/src/timer.c b/src/timer.c</span><br><span>index d3129a7..bcd9f5b 100644</span><br><span>--- a/src/timer.c</span><br><span>+++ b/src/timer.c</span><br><span>@@ -184,6 +184,22 @@</span><br><span> return nearest_p;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+/*! Determine time between now and the nearest timer in milliseconds</span><br><span style="color: hsl(120, 100%, 40%);">+ * \returns number of milliseconds until nearest timer expires; -1 if no timers pending</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+int osmo_timers_nearest_ms(void)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ int nearest_ms;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!nearest_p)</span><br><span style="color: hsl(120, 100%, 40%);">+ return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ nearest_ms = nearest_p->tv_sec * 1000;</span><br><span style="color: hsl(120, 100%, 40%);">+ nearest_ms += nearest_p->tv_usec / 1000;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ return nearest_ms;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> static void update_nearest(struct timeval *cand, struct timeval *current)</span><br><span> {</span><br><span> if (cand->tv_sec != LONG_MAX) {</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/20732">change 20732</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/libosmocore/+/20732"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I9e80da68a144b36926066610d0d3df06abe09bca </div>
<div style="display:none"> Gerrit-Change-Number: 20732 </div>
<div style="display:none"> Gerrit-PatchSet: 7 </div>
<div style="display:none"> Gerrit-Owner: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>