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

[Anton Ivanov]

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/