[ovs-dev] [PATCH 1/2] datapath-windows: Process tunnel filter requests iteratively

[Nithin Raju]

> On Jul 15, 2015, at 1:55 AM, Sorin Vinturis <svinturis at 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 <svinturis at 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))) {


>             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.

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.

thanks,
-- Nithin