Hi Anton,

From my perspective (i.e. a user of poll_immediate_wake()), this looks fine by me. It results in the same return values and logging, and it removes unnecessary overhead. It's probably best for someone on the OVS team to give the final approval, but for me,

Acked-by: Mark Michelson <[email protected]>

On 7/12/21 1:15 PM, [email protected] wrote:
From: Anton Ivanov <[email protected]>

If we are not obtaining any useful information out of the poll(),
such as is a fd busy or not, we do not need to do a poll() if
an immediate_wake() has been requested.

This cuts out all the pollfd hash additions, forming the poll
arguments and the actual poll() after a call to
poll_immediate_wake()

Signed-off-by: Anton Ivanov <[email protected]>
---
  lib/poll-loop.c | 69 ++++++++++++++++++++++++++++++++-----------------
  lib/timeval.c   | 11 +++++++-
  2 files changed, 56 insertions(+), 24 deletions(-)

diff --git a/lib/poll-loop.c b/lib/poll-loop.c
index 4e751ff2c..09bc4f5c4 100644
--- a/lib/poll-loop.c
+++ b/lib/poll-loop.c
@@ -53,6 +53,7 @@ struct poll_loop {
       * wake up immediately, or LLONG_MAX to wait forever. */
      long long int timeout_when; /* In msecs as returned by time_msec(). */
      const char *timeout_where;  /* Where 'timeout_when' was set. */
+    bool immediate_wake;
  };
static struct poll_loop *poll_loop(void);
@@ -107,6 +108,13 @@ poll_create_node(int fd, HANDLE wevent, short int events, 
const char *where)
COVERAGE_INC(poll_create_node); + if (loop->immediate_wake) {
+        /* We have been asked to bail out of this poll loop.
+         * There is no point to engage in yack shaving a poll hmap.
+         */
+        return;
+    }
+
      /* Both 'fd' and 'wevent' cannot be set. */
      ovs_assert(!fd != !wevent);
@@ -181,8 +189,15 @@ poll_wevent_wait_at(HANDLE wevent, const char *where)
  void
  poll_timer_wait_at(long long int msec, const char *where)
  {
-    long long int now = time_msec();
+    long long int now;
      long long int when;
+    struct poll_loop *loop = poll_loop();
+
+    if (loop->immediate_wake) {
+        return;
+    }
+
+    now = time_msec();
if (msec <= 0) {
          /* Wake up immediately. */
@@ -229,7 +244,9 @@ poll_timer_wait_until_at(long long int when, const char 
*where)
  void
  poll_immediate_wake_at(const char *where)
  {
+    struct poll_loop *loop = poll_loop();
      poll_timer_wait_at(0, where);
+    loop->immediate_wake = true;
  }
/* Logs, if appropriate, that the poll loop was awakened by an event
@@ -320,10 +337,10 @@ poll_block(void)
  {
      struct poll_loop *loop = poll_loop();
      struct poll_node *node;
-    struct pollfd *pollfds;
+    struct pollfd *pollfds = NULL;
      HANDLE *wevents = NULL;
      int elapsed;
-    int retval;
+    int retval = 0;
      int i;
/* Register fatal signal events before actually doing any real work for
@@ -335,34 +352,38 @@ poll_block(void)
      }
timewarp_run();
-    pollfds = xmalloc(hmap_count(&loop->poll_nodes) * sizeof *pollfds);
+    if (!loop->immediate_wake) {
+        pollfds = xmalloc(hmap_count(&loop->poll_nodes) * sizeof *pollfds);
#ifdef _WIN32
-    wevents = xmalloc(hmap_count(&loop->poll_nodes) * sizeof *wevents);
+        wevents = xmalloc(hmap_count(&loop->poll_nodes) * sizeof *wevents);
  #endif
- /* Populate with all the fds and events. */
-    i = 0;
-    HMAP_FOR_EACH (node, hmap_node, &loop->poll_nodes) {
-        pollfds[i] = node->pollfd;
+        /* Populate with all the fds and events. */
+        i = 0;
+        HMAP_FOR_EACH (node, hmap_node, &loop->poll_nodes) {
+            pollfds[i] = node->pollfd;
  #ifdef _WIN32
-        wevents[i] = node->wevent;
-        if (node->pollfd.fd && node->wevent) {
-            short int wsa_events = 0;
-            if (node->pollfd.events & POLLIN) {
-                wsa_events |= FD_READ | FD_ACCEPT | FD_CLOSE;
-            }
-            if (node->pollfd.events & POLLOUT) {
-                wsa_events |= FD_WRITE | FD_CONNECT | FD_CLOSE;
+            wevents[i] = node->wevent;
+            if (node->pollfd.fd && node->wevent) {
+                short int wsa_events = 0;
+                if (node->pollfd.events & POLLIN) {
+                    wsa_events |= FD_READ | FD_ACCEPT | FD_CLOSE;
+                }
+                if (node->pollfd.events & POLLOUT) {
+                    wsa_events |= FD_WRITE | FD_CONNECT | FD_CLOSE;
+                }
+                WSAEventSelect(node->pollfd.fd, node->wevent, wsa_events);
              }
-            WSAEventSelect(node->pollfd.fd, node->wevent, wsa_events);
-        }
  #endif
-        i++;
-    }
+            i++;
+        }
- retval = time_poll(pollfds, hmap_count(&loop->poll_nodes), wevents,
-                       loop->timeout_when, &elapsed);
+        retval = time_poll(pollfds, hmap_count(&loop->poll_nodes), wevents,
+                           loop->timeout_when, &elapsed);
+    } else {
+        retval = time_poll(NULL, 0, NULL, loop->timeout_when, &elapsed);
+    }
      if (retval < 0) {
          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
          VLOG_ERR_RL(&rl, "poll: %s", ovs_strerror(-retval));
@@ -381,6 +402,7 @@ poll_block(void)
      free_poll_nodes(loop);
      loop->timeout_when = LLONG_MAX;
      loop->timeout_where = NULL;
+    loop->immediate_wake = false;
      free(pollfds);
      free(wevents);
@@ -417,6 +439,7 @@ poll_loop(void)
          loop = xzalloc(sizeof *loop);
          loop->timeout_when = LLONG_MAX;
          hmap_init(&loop->poll_nodes);
+        loop->immediate_wake = false;
          xpthread_setspecific(key, loop);
      }
      return loop;
diff --git a/lib/timeval.c b/lib/timeval.c
index 193c7bab1..c6ac87376 100644
--- a/lib/timeval.c
+++ b/lib/timeval.c
@@ -323,7 +323,13 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE 
*handles OVS_UNUSED,
          }
#ifndef _WIN32
-        retval = poll(pollfds, n_pollfds, time_left);
+        /* Pollfds is NULL only if we are doing a immediate_wake
+         * shortcut. Otherwise there is at least one fd in it for
+         * the signals pipe fd.
+         */
+        if (n_pollfds != 0) {
+            retval = poll(pollfds, n_pollfds, time_left);
+        }
          if (retval < 0) {
              retval = -errno;
          }
@@ -331,6 +337,9 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE 
*handles OVS_UNUSED,
          if (n_pollfds > MAXIMUM_WAIT_OBJECTS) {
              VLOG_ERR("Cannot handle more than maximum wait objects\n");
          } else if (n_pollfds != 0) {
+            /* If we are doing an immediate_wake shortcut n_pollfds is
+             * zero, so we skip the actual call.
+             */
              retval = WaitForMultipleObjects(n_pollfds, handles, FALSE,
                                              time_left);
          }


_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to