[ovs-dev] datapath-windows: Avoid busy wait in OvsStartIpHelper

Previously, the IP Helper thread would wait for an event
but with a timeout of 0, which resulted in the thread
busy waiting causing high CPU usage by the kernel.
Since the IP Helper thread is only required based on
certain events, it makes sense to wait indefinitely
till we receieve such an event notification to wake up
the thread. This change aims to address this issue.

When OvsEnqueueIpHelperRequest() or OvsInternalAdapterUp()
is called, the ovsNumIpHelperRequests counter is incremented,
but upon consumption of the request, is not decremented.
Since the wakeup logic for the thread is determined by this
counter value, we need to reset the counter back correctly
once the request has been consumed by the IP Helper thread.

Signed-off-by: Shashank Ram <rams@vmware.com>
---
 datapath-windows/ovsext/IpHelper.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

--
2.6.2

Thanks Shashank for the fix! We definitely need to get it as soon as
possible. Otherwise, OVS driver will end up consuming a whole core of CPU
on Windows machines.

However, I have a little doubt about the logic here:

    ovsNumIpHelperRequests++;
    if (ovsNumIpHelperRequests == 1) {
        OvsWakeupIPHelper();
    }

Let's say we have 5 InternalAdapterUp events in a row. We'll end up
incrementing IpHelperRequests by 5, but signalling the event only once
(because in the remaining times ovsNumIpHelperRequests is greater than 1).

Now we decrease the count by only one every time receives a signal. It
means that in such cases, we will end up never calling OvsWakupIPHelper any
more, as the variable will stay >= 4 from then on.

I feel the if clause should be removed. If there is a reason we need it,
then we need to copy implementation of a robust consumer-producer solution
rather than the hack here.

Also, we need to release the spin lock before calling OvsWakeupIPHelper to
avoid unnecessary context switch (Signaling event is mostly likely to
trigger a context switch, but once IPHelper is waked up, it will try to
grab lock which hasn't been released yet, which triggers another context
switch, followed by a third context switch when you release the lock.

On Tue, Nov 22, 2016 at 9:53 AM, Shashank Ram <rams@vmware.com> wrote:

> Previously, the IP Helper thread would wait for an event
> but with a timeout of 0, which resulted in the thread
> busy waiting causing high CPU usage by the kernel.
> Since the IP Helper thread is only required based on
> certain events, it makes sense to wait indefinitely
> till we receieve such an event notification to wake up
> the thread. This change aims to address this issue.
>
> When OvsEnqueueIpHelperRequest() or OvsInternalAdapterUp()
> is called, the ovsNumIpHelperRequests counter is incremented,
> but upon consumption of the request, is not decremented.
> Since the wakeup logic for the thread is determined by this
> counter value, we need to reset the counter back correctly
> once the request has been consumed by the IP Helper thread.
>
> Signed-off-by: Shashank Ram <rams@vmware.com>
> ---
>  datapath-windows/ovsext/IpHelper.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/datapath-windows/ovsext/IpHelper.c b/datapath-windows/ovsext/
> IpHelper.c
> index d747e8c..559ddaa 100644
> --- a/datapath-windows/ovsext/IpHelper.c
> +++ b/datapath-windows/ovsext/IpHelper.c
> @@ -1455,12 +1455,14 @@ OvsStartIpHelper(PVOID data)
>      POVS_IPNEIGH_ENTRY ipn;
>      PLIST_ENTRY link;
>      UINT64   timeVal, timeout;
> +    PLARGE_INTEGER threadSleepTimeout;
>
>      OVS_LOG_INFO("Start the IP Helper Thread, context: %p", context);
>
>      NdisAcquireSpinLock(&ovsIpHelperLock);
>      while (!context->exit) {
>
> +        threadSleepTimeout = NULL;
>          timeout = 0;
>          while (!IsListEmpty(&ovsIpHelperRequestList)) {
>              if (context->exit) {
> @@ -1468,6 +1470,7 @@ OvsStartIpHelper(PVOID data)
>              }
>              link = ovsIpHelperRequestList.Flink;
>              RemoveEntryList(link);
> +            ovsNumIpHelperRequests--;
>              NdisReleaseSpinLock(&ovsIpHelperLock);
>              req = CONTAINING_RECORD(link, OVS_IP_HELPER_REQUEST, link);
>              switch (req->command) {
> @@ -1497,6 +1500,7 @@ OvsStartIpHelper(PVOID data)
>              KeQuerySystemTime((LARGE_INTEGER *)&timeVal);
>              if (ipn->timeout > timeVal) {
>                  timeout = ipn->timeout;
> +                threadSleepTimeout = (PLARGE_INTEGER)&timeout;
>                  break;
>              }
>              ipAddr = ipn->ipAddr;
> @@ -1519,8 +1523,13 @@ ip_helper_wait:
>          KeClearEvent(&context->event);
>          NdisReleaseSpinLock(&ovsIpHelperLock);
>
> +        /*
> +         * Wait indefinitely for the thread to be woken up.
> +         * Passing NULL as the Timeout value in the below
> +         * call to KeWaitForSingleObject achieves this.
> +         */
>          KeWaitForSingleObject(&context->event, Executive, KernelMode,
> -                              FALSE, (LARGE_INTEGER *)&timeout);
> +                              FALSE, threadSleepTimeout);
>          NdisAcquireSpinLock(&ovsIpHelperLock);
>      }
>      NdisReleaseSpinLock(&ovsIpHelperLock);
> --
> 2.6.2
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>