| Message ID | 20210804083419.26603-2-anton.ivanov@cambridgegreys.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v5,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 |
On Wed, Aug 4, 2021, at 10:34, 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> To conclude our previous thread, I don't disagree that reducing time_msec() calls is interesting. Coherency traffic can take a toll on some architectures, and this code is used by all cores, even fastpath ones for dpif-netdev. That being said, there is still the 'else { retval = 0; }' from the previous version. > --- > lib/timeval.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/lib/timeval.c b/lib/timeval.c > index dbfbcb9e6..45a934513 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,13 +297,12 @@ 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; > retval = 0; > > @@ -331,6 +330,8 @@ time_poll(struct pollfd *pollfds, int n_pollfds, > HANDLE *handles OVS_UNUSED, > */ > if (timeout_when != LLONG_MIN) { > retval = poll(pollfds, n_pollfds, time_left); > + } else { > + retval = 0; > } > if (retval < 0) { > retval = -errno; > @@ -357,7 +358,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 > @@ -374,7 +376,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; > -- > 2.20.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 04/08/2021 15:45, Gaëtan Rivet wrote: > On Wed, Aug 4, 2021, at 10:34, 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> > To conclude our previous thread, I don't disagree that > reducing time_msec() calls is interesting. Coherency traffic can take > a toll on some architectures, and this code is used by all cores, > even fastpath ones for dpif-netdev. > > That being said, there is still the 'else { retval = 0; }' from the previous > version. Sorry, missed that. > >> --- >> lib/timeval.c | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/lib/timeval.c b/lib/timeval.c >> index dbfbcb9e6..45a934513 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,13 +297,12 @@ 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; >> retval = 0; >> >> @@ -331,6 +330,8 @@ time_poll(struct pollfd *pollfds, int n_pollfds, >> HANDLE *handles OVS_UNUSED, >> */ >> if (timeout_when != LLONG_MIN) { >> retval = poll(pollfds, n_pollfds, time_left); >> + } else { >> + retval = 0; >> } >> if (retval < 0) { >> retval = -errno; >> @@ -357,7 +358,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 >> @@ -374,7 +376,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; >> -- >> 2.20.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
diff --git a/lib/timeval.c b/lib/timeval.c index dbfbcb9e6..45a934513 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,13 +297,12 @@ 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; retval = 0; @@ -331,6 +330,8 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE *handles OVS_UNUSED, */ if (timeout_when != LLONG_MIN) { retval = poll(pollfds, n_pollfds, time_left); + } else { + retval = 0; } if (retval < 0) { retval = -errno; @@ -357,7 +358,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 @@ -374,7 +376,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;