[ovs-dev] [PATCH] Optimize the poll loop for poll_immediate_wake()

[Ben Pfaff]

I'm impressed by the improvement.  I didn't really expect any.

On Tue, Jul 13, 2021 at 09:19:34AM +0100, Anton Ivanov wrote:
> I ran the revised patch series (v2) which addresses your comments on the ovn-heater benchmark.
> 
> With 9 fake nodes, 50 fake pods per node I get ~ 2% reproducible improvement. IMHO it should be even more noticeable at high scale.
> 
> Best regards,
> 
> A
> 
> On 09/07/2021 19:45, Ben Pfaff wrote:
> > On Fri, Jul 09, 2021 at 06:19:06PM +0100, 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>
> > Thanks for the patch.
> > 
> > I think that this will have some undesirable side effects because it
> > avoids calling time_poll() if the wakeup should happen immediately, and
> > time_poll() does some thing that we always want to happen between one
> > main loop and another.  In particular the following calls from
> > time_poll() are important:
> > 
> >      coverage_clear();
> >      coverage_run();
> >      if (*last_wakeup && !thread_is_pmd()) {
> >          log_poll_interval(*last_wakeup);
> >      }
> > 
> > ...
> > 
> >              if (!time_left) {
> >                  ovsrcu_quiesce();
> >              } else {
> >                  ovsrcu_quiesce_start();
> >              }
> > 
> > ...
> > 
> >          if (deadline <= time_msec()) {
> > #ifndef _WIN32
> >              fatal_signal_handler(SIGALRM);
> > #else
> >              VLOG_ERR("wake up from WaitForMultipleObjects after deadline");
> >              fatal_signal_handler(SIGTERM);
> > #endif
> >              if (retval < 0) {
> >                  retval = 0;
> >              }
> >              break;
> >          }
> > 
> > ...
> > 
> >      *last_wakeup = time_msec();
> >      refresh_rusage();
> > 
> > Instead of this change, I'd suggest something more like the following,
> > along with the changes you made to suppress the file descriptors if the
> > timeout is already zero:
> > 
> > diff --git a/lib/timeval.c b/lib/timeval.c
> > index 193c7bab1781..f080a9999742 100644
> > --- a/lib/timeval.c
> > +++ b/lib/timeval.c
> > @@ -323,7 +323,7 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE *handles OVS_UNUSED,
> >           }
> >   #ifndef _WIN32
> > -        retval = poll(pollfds, n_pollfds, time_left);
> > +        retval = time_left ? poll(pollfds, n_pollfds, time_left) : 0;
> >           if (retval < 0) {
> >               retval = -errno;
> >           }
> > 
> > 
> -- 
> Anton R. Ivanov
> Cambridgegreys Limited. Registered in England. Company Number 10273661
> https://www.cambridgegreys.com/
>