| Message ID | 1479864774-52038-1-git-send-email-rams@vmware.com |
|---|---|
| State | Accepted |
| Delegated to: | Guru Shetty |
| Headers | show |
Acked-by: Yin Lin<linyi@vmware.com> Best regards, Yin On Tue, Nov 22, 2016 at 5:32 PM, 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 | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/datapath-windows/ovsext/IpHelper.c b/datapath-windows/ovsext/ > IpHelper.c > index d747e8c..636cf95 100644 > --- a/datapath-windows/ovsext/IpHelper.c > +++ b/datapath-windows/ovsext/IpHelper.c > @@ -1091,9 +1091,11 @@ OvsInternalAdapterUp(GUID *netCfgInstanceId) > InsertHeadList(&ovsIpHelperRequestList, &request->link); > ovsNumIpHelperRequests++; > if (ovsNumIpHelperRequests == 1) { > + NdisReleaseSpinLock(&ovsIpHelperLock); > OvsWakeupIPHelper(); > + } else { > + NdisReleaseSpinLock(&ovsIpHelperLock); > } > - NdisReleaseSpinLock(&ovsIpHelperLock); > } > > > @@ -1455,12 +1457,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 +1472,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 +1502,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 +1525,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 >
Hi Shashank, Thanks a lot for the patch! My only concern is that this issue has already been addresed in one of the multiple port patches: http://patchwork.ozlabs.org/patch/669598/ I plan to respin the series until the end of the week. Thanks, Alin. > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of Yin Lin > Sent: Wednesday, November 23, 2016 5:58 AM > To: Shashank Ram <rams@vmware.com> > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2] datapath-windows: Avoid busy wait in > OvsStartIpHelper > > Acked-by: Yin Lin<linyi@vmware.com> > > Best regards, > Yin > > On Tue, Nov 22, 2016 at 5:32 PM, 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>
Hi Alin, I have the following concern with your change around the below code:
KeWaitForSingleObject(&context->event, Executive, KernelMode,
FALSE, (LARGE_INTEGER *)&timeout);
When we call into KeWaitForSingleObject, we want to wait indefinitely till we receive a wakeup event. Your patch simply increases the timeout from 0 to OVS_IPNEIGH_TIMEOUT. This will end up waking up the IpHelper thread even if no wakeup event is received once the timeout value expires. We have seen cases where simply looping in this thread unnecessarily increases the CPU consumed. Hence, we should only wake up this thread if an event is received to wake it up.
Please refer to my change and see if you can incorporate this into your change.
Thanks,
Shashank
On 7 December 2016 at 17:16, Shashank Ram <rams@vmware.com> wrote: > Hi Alin, I have the following concern with your change around the below > code: > > > KeWaitForSingleObject(&context->event, Executive, KernelMode, > FALSE, (LARGE_INTEGER *)&timeout); > > > When we call into KeWaitForSingleObject, we want to wait indefinitely till > we receive a wakeup event. Your patch simply increases the timeout from 0 > to OVS_IPNEIGH_TIMEOUT. This will end up waking up the IpHelper thread even > if no wakeup event is received once the timeout value expires. We have seen > cases where simply looping in this thread unnecessarily increases the CPU > consumed. Hence, we should only wake up this thread if an event is received > to wake it up. > > > Please refer to my change and see if you can incorporate this into your > change. > This patch has been there for a while and I has gotten a couple of Acks. So I applied this. > > > Thanks, > > Shashank > > ________________________________ > From: Alin Serdean <aserdean@cloudbasesolutions.com> > Sent: Wednesday, November 23, 2016 1:14:39 PM > To: Yin Lin; Shashank Ram; Nithin Raju; Sairam Venugopal > Cc: dev@openvswitch.org > Subject: RE: [ovs-dev] [PATCH v2] datapath-windows: Avoid busy wait in > OvsStartIpHelper > > Hi Shashank, > > Thanks a lot for the patch! > > My only concern is that this issue has already been addresed in one of the > multiple port patches: > https://urldefense.proofpoint.com/v2/url?u=http-3A__ > patchwork.ozlabs.org_patch_669598_&d=DgIFAg&c=uilaK90D4TOVoH58JNXRgQ&r= > 6OuVHk-mnufSWzkKa74UkQ&m=1I78-nNpLSZav-yhv-wSNAtXMaiyp6lciwose971dBE&s= > Sfy280kZ0O4q2BTI3Aa4MlG4HSKpDVrPH5koGzTLKmQ&e= > > I plan to respin the series until the end of the week. > > Thanks, > Alin. > > > -----Original Message----- > > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > > bounces@openvswitch.org] On Behalf Of Yin Lin > > Sent: Wednesday, November 23, 2016 5:58 AM > > To: Shashank Ram <rams@vmware.com> > > Cc: dev@openvswitch.org > > Subject: Re: [ovs-dev] [PATCH v2] datapath-windows: Avoid busy wait in > > OvsStartIpHelper > > > > Acked-by: Yin Lin<linyi@vmware.com> > > > > Best regards, > > Yin > > > > On Tue, Nov 22, 2016 at 5:32 PM, 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> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/datapath-windows/ovsext/IpHelper.c b/datapath-windows/ovsext/IpHelper.c index d747e8c..636cf95 100644 --- a/datapath-windows/ovsext/IpHelper.c +++ b/datapath-windows/ovsext/IpHelper.c @@ -1091,9 +1091,11 @@ OvsInternalAdapterUp(GUID *netCfgInstanceId) InsertHeadList(&ovsIpHelperRequestList, &request->link); ovsNumIpHelperRequests++; if (ovsNumIpHelperRequests == 1) { + NdisReleaseSpinLock(&ovsIpHelperLock); OvsWakeupIPHelper(); + } else { + NdisReleaseSpinLock(&ovsIpHelperLock); } - NdisReleaseSpinLock(&ovsIpHelperLock); } @@ -1455,12 +1457,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 +1472,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 +1502,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 +1525,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);
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 | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) -- 2.6.2