[ovs-dev] [PATCH v2 1/2] Optimize the poll loop for poll_immediate_wake()
Mark Michelson
mmichels at redhat.com
Thu Jul 15 18:37:08 UTC 2021
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 <mmichels at redhat.com>
On 7/12/21 1:15 PM, anton.ivanov at cambridgegreys.com wrote:
> From: Anton Ivanov <anton.ivanov at cambridgegreys.com>
>
> 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 <anton.ivanov at cambridgegreys.com>
> ---
> 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);
> }
>
More information about the dev
mailing list