Nithin, There is a v2 version of this patch series. Still, I will address your comments inline.
Thanks, Sorin -----Original Message----- From: Nithin Raju [mailto:nit...@vmware.com] Sent: Monday, 3 August, 2015 19:19 To: Sorin Vinturis Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH 1/2] datapath-windows: Process tunnel filter requests iteratively > On Jul 15, 2015, at 1:55 AM, Sorin Vinturis > <svintu...@cloudbasesolutions.com> wrote: > > In order to support IRP cancelling mechanism for pending IRPs, all > tunnel filter requests, VXLAN tunnel create/delete, need to be > processed iteratively. > > Signed-off-by: Sorin Vinturis <svintu...@cloudbasesolutions.com> > --- > datapath-windows/ovsext/TunnelFilter.c | 89 > +++++++++++----------------------- > 1 file changed, 27 insertions(+), 62 deletions(-) > > diff --git a/datapath-windows/ovsext/TunnelFilter.c > b/datapath-windows/ovsext/TunnelFilter.c > index 231750e..caaf3f6 100644 > --- a/datapath-windows/ovsext/TunnelFilter.c > +++ b/datapath-windows/ovsext/TunnelFilter.c > @@ -951,38 +951,26 @@ OvsTunnelFilterExecuteAction(HANDLE > engineSession, > > /* > * > ---------------------------------------------------------------------- > ---- > - * This function pops the whole request entries from the queue and > returns the > - * number of entries through the 'count' parameter. The operation is > - * synchronized using request list spinlock. > + * This function pops the head item from the requests list while > + holding > + * the list's spinlock. > * > ---------------------------------------------------------------------- > ---- > */ > +POVS_TUNFLT_REQUEST > +OvsTunnelFilterRequestPop(POVS_TUNFLT_REQUEST_LIST listRequests) > { > + POVS_TUNFLT_REQUEST request = NULL; > + > NdisAcquireSpinLock(&listRequests->spinlock); > > if (!IsListEmpty(&listRequests->head)) { > + request = (POVS_TUNFLT_REQUEST)RemoveHeadList(&listRequests->head); > + InitializeListHead(&request->entry); > + listRequests->numEntries--; > } > > NdisReleaseSpinLock(&listRequests->spinlock); > + > + return request; > } > > /* > @@ -1052,16 +1040,11 @@ VOID > OvsTunnelFilterRequestListProcess(POVS_TUNFLT_THREAD_CONTEXT > threadCtx) { > POVS_TUNFLT_REQUEST request = NULL; > NTSTATUS status = STATUS_SUCCESS; > BOOLEAN inTransaction = FALSE; > + BOOLEAN error = FALSE; > > + do { > if (!InterlockedCompareExchange( > (LONG volatile *)&threadCtx->listRequests.numEntries, 0, 0)) { > OVS_LOG_INFO("Nothing to do... request list is empty."); > @@ -1076,27 +1059,23 @@ > OvsTunnelFilterRequestListProcess(POVS_TUNFLT_THREAD_CONTEXT threadCtx) > } > inTransaction = TRUE; > > + while (NULL != (request = OvsTunnelFilterRequestPop( > + &threadCtx->listRequests))) { minor: indentation can be improved to: while (NULL != (request = OvsTunnelFilterRequestPop(&threadCtx->listRequests))) { SV: OK. > status = OvsTunnelFilterExecuteAction(threadCtx->engineSession, > request); > + > + /* Complete the IRP last operation status. */ > + OvsTunnelFilterCompleteRequest(request->irp, > + request->callback, > + request->context, > + status); > + OvsFreeMemory(request); > + request = NULL; > + > if (!NT_SUCCESS(status)) { > + error = TRUE; > + break; Why break if there’s an error. Latency of processing requests will go up if we don’t drain the queue, when we can. If you are concerned about keeping the transaction open for too long, you can open a new transaction for each request that is being processed. SV: Why continue adding filters if a same previous request failed? I think it is reasonable to stop and try to commit the changes made and, if that fails too, abort the transaction. The latency here comes from the transactions, which are being processed very slow by the BFE. That is why it's not a good idea to perform only one request on a single transaction. If a request fails, the subsequent request will be processed in a new transaction. Also, how do we use ‘error’ anyway? Also, it seems that the do-while loop is not needed anymore, esp. if ‘error’ is not required. SV: 'error' does not exists in v2. thanks, -- Nithin _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
