| Message ID | 20210712171529.11161-2-anton.ivanov@cambridgegreys.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2,1/2] Optimize the poll loop for poll_immediate_wake() | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot | success | github build: passed |
Hi Anton, Out of curiosity, has this change made a noticeable impact on performance? On 7/12/21 1:15 PM, anton.ivanov@cambridgegreys.com wrote: > From: Anton Ivanov <anton.ivanov@cambridgegreys.com> > > time_poll() makes an excessive number of time_msec() calls > which incur a performance penalty. > > 1. Avoid time_msec() call for timeout calculation when time_poll() > is asked to skip poll() > > 2. Reuse the time_msec() result from deadline calculation for > last_wakeup and timeout calculation. > > Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com> > --- > lib/timeval.c | 36 +++++++++++++++++++++--------------- > 1 file changed, 21 insertions(+), 15 deletions(-) > > diff --git a/lib/timeval.c b/lib/timeval.c > index c6ac87376..64ab22e05 100644 > --- a/lib/timeval.c > +++ b/lib/timeval.c > @@ -287,7 +287,7 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE *handles OVS_UNUSED, > long long int timeout_when, int *elapsed) > { > long long int *last_wakeup = last_wakeup_get(); > - long long int start; > + long long int start, now; > bool quiescent; > int retval = 0; > > @@ -297,28 +297,31 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE *handles OVS_UNUSED, > if (*last_wakeup && !thread_is_pmd()) { > log_poll_interval(*last_wakeup); > } > - start = time_msec(); > + now = start = time_msec(); > > timeout_when = MIN(timeout_when, deadline); > quiescent = ovsrcu_is_quiescent(); > > for (;;) { > - long long int now = time_msec(); > int time_left; > > - if (now >= timeout_when) { > + if (n_pollfds == 0) { > time_left = 0; > - } else if ((unsigned long long int) timeout_when - now > INT_MAX) { > - time_left = INT_MAX; > } else { > - time_left = timeout_when - now; > - } > - > - if (!quiescent) { > - if (!time_left) { > - ovsrcu_quiesce(); > + if (now >= timeout_when) { > + time_left = 0; > + } else if ((unsigned long long int) timeout_when - now > INT_MAX) { > + time_left = INT_MAX; > } else { > - ovsrcu_quiesce_start(); > + time_left = timeout_when - now; > + } > + > + if (!quiescent) { > + if (!time_left) { > + ovsrcu_quiesce(); > + } else { > + ovsrcu_quiesce_start(); > + } > } > } > > @@ -329,6 +332,8 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE *handles OVS_UNUSED, > */ > if (n_pollfds != 0) { > retval = poll(pollfds, n_pollfds, time_left); > + } else { > + retval = 0; > } > if (retval < 0) { > retval = -errno; > @@ -355,7 +360,8 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE *handles OVS_UNUSED, > ovsrcu_quiesce_end(); > } > > - if (deadline <= time_msec()) { > + now = time_msec(); > + if (deadline <= now) { > #ifndef _WIN32 > fatal_signal_handler(SIGALRM); > #else > @@ -372,7 +378,7 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE *handles OVS_UNUSED, > break; > } > } > - *last_wakeup = time_msec(); > + *last_wakeup = now; > refresh_rusage(); > *elapsed = *last_wakeup - start; > return retval; >
On 15/07/2021 20:49, Mark Michelson wrote: > Hi Anton, > > Out of curiosity, has this change made a noticeable impact on > performance? I can't pick it up on OVN heater. That is not unexpected. It is "noise" compared to the compute time and the number of syscalls used in one iteration in northd or ovsdb. I suspect it will be more useful in places like the vswitch itself where the loops are tighter, processing per loop is less and lower latency is the key. There, one syscall less per iteration may show up as a noticeable difference. I do not have a benchmark set up for those. A. > On 7/12/21 1:15 PM, anton.ivanov@cambridgegreys.com wrote: >> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >> >> time_poll() makes an excessive number of time_msec() calls >> which incur a performance penalty. >> >> 1. Avoid time_msec() call for timeout calculation when time_poll() >> is asked to skip poll() >> >> 2. Reuse the time_msec() result from deadline calculation for >> last_wakeup and timeout calculation. >> >> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com> >> --- >> lib/timeval.c | 36 +++++++++++++++++++++--------------- >> 1 file changed, 21 insertions(+), 15 deletions(-) >> >> diff --git a/lib/timeval.c b/lib/timeval.c >> index c6ac87376..64ab22e05 100644 >> --- a/lib/timeval.c >> +++ b/lib/timeval.c >> @@ -287,7 +287,7 @@ time_poll(struct pollfd *pollfds, int n_pollfds, >> HANDLE *handles OVS_UNUSED, >> long long int timeout_when, int *elapsed) >> { >> long long int *last_wakeup = last_wakeup_get(); >> - long long int start; >> + long long int start, now; >> bool quiescent; >> int retval = 0; >> @@ -297,28 +297,31 @@ time_poll(struct pollfd *pollfds, int >> n_pollfds, HANDLE *handles OVS_UNUSED, >> if (*last_wakeup && !thread_is_pmd()) { >> log_poll_interval(*last_wakeup); >> } >> - start = time_msec(); >> + now = start = time_msec(); >> timeout_when = MIN(timeout_when, deadline); >> quiescent = ovsrcu_is_quiescent(); >> for (;;) { >> - long long int now = time_msec(); >> int time_left; >> - if (now >= timeout_when) { >> + if (n_pollfds == 0) { >> time_left = 0; >> - } else if ((unsigned long long int) timeout_when - now > >> INT_MAX) { >> - time_left = INT_MAX; >> } else { >> - time_left = timeout_when - now; >> - } >> - >> - if (!quiescent) { >> - if (!time_left) { >> - ovsrcu_quiesce(); >> + if (now >= timeout_when) { >> + time_left = 0; >> + } else if ((unsigned long long int) timeout_when - now > >> INT_MAX) { >> + time_left = INT_MAX; >> } else { >> - ovsrcu_quiesce_start(); >> + time_left = timeout_when - now; >> + } >> + >> + if (!quiescent) { >> + if (!time_left) { >> + ovsrcu_quiesce(); >> + } else { >> + ovsrcu_quiesce_start(); >> + } >> } >> } >> @@ -329,6 +332,8 @@ time_poll(struct pollfd *pollfds, int >> n_pollfds, HANDLE *handles OVS_UNUSED, >> */ >> if (n_pollfds != 0) { >> retval = poll(pollfds, n_pollfds, time_left); >> + } else { >> + retval = 0; >> } >> if (retval < 0) { >> retval = -errno; >> @@ -355,7 +360,8 @@ time_poll(struct pollfd *pollfds, int n_pollfds, >> HANDLE *handles OVS_UNUSED, >> ovsrcu_quiesce_end(); >> } >> - if (deadline <= time_msec()) { >> + now = time_msec(); >> + if (deadline <= now) { >> #ifndef _WIN32 >> fatal_signal_handler(SIGALRM); >> #else >> @@ -372,7 +378,7 @@ time_poll(struct pollfd *pollfds, int n_pollfds, >> HANDLE *handles OVS_UNUSED, >> break; >> } >> } >> - *last_wakeup = time_msec(); >> + *last_wakeup = now; >> refresh_rusage(); >> *elapsed = *last_wakeup - start; >> return retval; >> > >
On Mon, Jul 12, 2021 at 06:15:29PM +0100, anton.ivanov@cambridgegreys.com wrote: > From: Anton Ivanov <anton.ivanov@cambridgegreys.com> > > time_poll() makes an excessive number of time_msec() calls > which incur a performance penalty. > > 1. Avoid time_msec() call for timeout calculation when time_poll() > is asked to skip poll() > > 2. Reuse the time_msec() result from deadline calculation for > last_wakeup and timeout calculation. > > Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com> I'd like another look at that after patch 1 is revised.
diff --git a/lib/timeval.c b/lib/timeval.c index c6ac87376..64ab22e05 100644 --- a/lib/timeval.c +++ b/lib/timeval.c @@ -287,7 +287,7 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE *handles OVS_UNUSED, long long int timeout_when, int *elapsed) { long long int *last_wakeup = last_wakeup_get(); - long long int start; + long long int start, now; bool quiescent; int retval = 0; @@ -297,28 +297,31 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE *handles OVS_UNUSED, if (*last_wakeup && !thread_is_pmd()) { log_poll_interval(*last_wakeup); } - start = time_msec(); + now = start = time_msec(); timeout_when = MIN(timeout_when, deadline); quiescent = ovsrcu_is_quiescent(); for (;;) { - long long int now = time_msec(); int time_left; - if (now >= timeout_when) { + if (n_pollfds == 0) { time_left = 0; - } else if ((unsigned long long int) timeout_when - now > INT_MAX) { - time_left = INT_MAX; } else { - time_left = timeout_when - now; - } - - if (!quiescent) { - if (!time_left) { - ovsrcu_quiesce(); + if (now >= timeout_when) { + time_left = 0; + } else if ((unsigned long long int) timeout_when - now > INT_MAX) { + time_left = INT_MAX; } else { - ovsrcu_quiesce_start(); + time_left = timeout_when - now; + } + + if (!quiescent) { + if (!time_left) { + ovsrcu_quiesce(); + } else { + ovsrcu_quiesce_start(); + } } } @@ -329,6 +332,8 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE *handles OVS_UNUSED, */ if (n_pollfds != 0) { retval = poll(pollfds, n_pollfds, time_left); + } else { + retval = 0; } if (retval < 0) { retval = -errno; @@ -355,7 +360,8 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE *handles OVS_UNUSED, ovsrcu_quiesce_end(); } - if (deadline <= time_msec()) { + now = time_msec(); + if (deadline <= now) { #ifndef _WIN32 fatal_signal_handler(SIGALRM); #else @@ -372,7 +378,7 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE *handles OVS_UNUSED, break; } } - *last_wakeup = time_msec(); + *last_wakeup = now; refresh_rusage(); *elapsed = *last_wakeup - start; return retval;