Hi Yin, looking at the following code, there is a lock that needs to be acquired prior to incrementing the 'ovsNumIpHelperRequests' counter. So consider the following:
1. At the start, ovsNumIpHelperRequests = 0 & IPHelper thread is sleeping 2. OvsInternalAdapterUp() is invoked, IPHelper lock is acquired, ovsNumIpHelperRequests is incremented to 1, IP Helper thread is woken up, ovsNumIpHelperRequests is decremented to 0, IPHelper lock is released. 3. If OvsInternalAdapterUp() is called during step 2., then it will wait for the lock to be released, which is after the ovsNumIpHelperRequests is set to 0. So I don't see how the ovsNumIpHelperRequests can ever be > 1. Thanks, Shashank ________________________________ From: Yin Lin <[email protected]> Sent: Tuesday, November 22, 2016 2:42:26 PM To: Shashank Ram Cc: [email protected] Subject: Re: [ovs-dev] [PATCH] datapath-windows: Avoid busy wait in OvsStartIpHelper 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 <[email protected]<mailto:[email protected]>> 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 <[email protected]<mailto:[email protected]>> --- 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 [email protected]<mailto:[email protected]> https://mail.openvswitch.org/mailman/listinfo/ovs-dev<https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DgMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=6OuVHk-mnufSWzkKa74UkQ&m=7HqBD-WMBuiemoaXFgc_PAFk4mlOqPkdrUQFfuoNZbg&s=7ZVRdCUUdv2AVxYeozXWBMB-DPUQ6DQpIPBD0fAao9U&e=> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
