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
