hi Sorin, Looks like you acknowledge my comments. I’ll look forward to the v3. Thanks for your patience.
-- Nithin > On Aug 31, 2015, at 5:08 AM, Sorin Vinturis > <svintu...@cloudbasesolutions.com> wrote: > > Hi Nithin, > > Please see my answers inline. > > Thanks, > Sorin > > -----Original Message----- > From: Nithin Raju [mailto:nit...@vmware.com] > Sent: Thursday, 27 August, 2015 23:44 > To: Sorin Vinturis > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2 1/2] datapath-windows: Process tunnel filter > requests iteratively > >> On Jul 20, 2015, at 12:00 PM, Sorin Vinturis >> <svintu...@cloudbasesolutions.com> wrote: >> >> In order to support IRP cancelling mechanism for pending IRPs, all >> tunnel filter requests, VXLAN create/delete tunnel, need to be >> processed iteratively. >> >> Signed-off-by: Sorin Vinturis <svintu...@cloudbasesolutions.com> > > hi Sorin, > I had a question regarding latency of processing the requests. Looks good > otherwise. A v4 should be good to go. > >> - do >> - { >> + do { >> if (!InterlockedCompareExchange( >> (LONG volatile *)&threadCtx->listRequests.numEntries, 0, 0)) { >> OVS_LOG_INFO("Nothing to do... request list is empty."); >> @@ -1076,38 +1056,25 @@ >> OvsTunnelFilterRequestListProcess(POVS_TUNFLT_THREAD_CONTEXT threadCtx) >> } >> inTransaction = TRUE; >> >> - InitializeListHead(&head); >> - OvsTunnelFilterRequestPopList(&threadCtx->listRequests, &head, >> &count); >> - >> - LIST_FORALL_SAFE(&head, link, next) { >> - request = CONTAINING_RECORD(link, OVS_TUNFLT_REQUEST, entry); >> + while (NULL != (request = OvsTunnelFilterRequestPop( >> + &threadCtx->listRequests))) { > > minor: indentation can be improved to: > while (NULL != > (request = OvsTunnelFilterRequestPop(&threadCtx->listRequests))) > { > SV: Sure. > >> >> status = OvsTunnelFilterExecuteAction(threadCtx->engineSession, >> request); >> + >> + /* Complete the IRP with the last operation status. */ >> + OvsTunnelFilterCompleteRequest(request, status); >> + >> + OvsFreeMemory(request); >> + request = NULL; >> + >> if (!NT_SUCCESS(status)) { >> - RemoveEntryList(&request->entry); >> - count--; >> - >> - /* Complete the IRP with the failure status. */ >> - OvsTunnelFilterCompleteRequest(request->irp, >> - request->callback, >> - request->context, >> - status); >> - OvsFreeMemory(request); >> - request = NULL; >> - } else { >> - error = FALSE; >> + break; >> } >> } >> > > What happens if there are 10 requests to process, and the 5th one fails. > You’d bail out of processing commit the 4 and go back into calling > KeWaitForMultipleObjects() in OvsTunnelFilterThreadProc(). What is the > trigger to processing the remaining 5 request, esp. if they are legitimate > requests and there’s a good chance that they’ll succeed. > SV: You are right. If there are more requests queued and an error happened to > one of them, the remaining request would not be processed until another one > will be received and the request event is signaled. I'll change the code to > continue with the processing the remaining requests, in case of error, > instead of breaking. > > Also, if you are worried about the number of requests being committed per > transaction, maybe you should submit them in batches. > SV: No, I am not worried about the number of requests being too large for a > transaction to handle. I've tested by pushing 2000 of requests on 8 threads > and the code behaves fine. The threads number could be raised to handle an > increased number of requests if necessary. > > Does KeSetEvent() queue up wakeup events? > SV: No, the KeSetEvent() does not queue up the events. > > -- Nithin _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
