Hi Nithin,

Please see my answers inline.

Thanks,
Sorin

-----Original Message-----
From: Nithin Raju [mailto:nit...@vmware.com] 
Sent: Wednesday, 20 May, 2015 11:15
To: Sorin Vinturis
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH v4 1/4] datapath-windows: Support for custom 
VXLAN tunnel port

hi Sorin,
The patch had some minor merge issues in Datapath.c for me. You might want to 
fix them.

I have a few relatively minor comments. Pls. address them and the patch should 
be good to go.

> +_Use_decl_annotations_
> +VOID
> +OvsTunnelFilterThreadProc(PVOID context) {
> +    NTSTATUS                   status = STATUS_SUCCESS;
[...]
> +
> +        error = FALSE;
> +    } while (error);

While I’m ok with using do-while loop as an alternative to using labels, you 
are using 2 do-while looks here. Does it add more clarity than using labels? :) 
I am ok either way.
SV: I am not in favor of using labels. For me, it makes the code very hard to 
follow and maintain in the long run. Same is the case for using return in the 
middle of the function, where the cleanup code needs to be duplicate.

> +static NTSTATUS
> +OvsTunnelFilterThreadStart(POVS_TUNFLT_THREAD_CONTEXT threadCtx) {
> +    NTSTATUS    status = STATUS_SUCCESS;
> +    HANDLE      threadHandle = NULL;
> +    BOOLEAN     error = TRUE;
> +
> +    do {
> +        status = PsCreateSystemThread(&threadHandle,
> +                                      SYNCHRONIZE,
> +                                      NULL,
> +                                      NULL,
> +                                      NULL,
> +                                      OvsTunnelFilterThreadProc,
> +                                      threadCtx);
> +        if (!NT_SUCCESS(status)) {
> +            OVS_LOG_ERROR("Failed to create tunnel thread, status: %x.",
> +                          status);
> +            break;
> +        }
> +
> +        ObReferenceObjectByHandle(threadHandle,
> +                                  SYNCHRONIZE,
> +                                  NULL,
> +                                  KernelMode,
> +                                  &threadCtx->threadObject,
> +                                  NULL);
> +        ZwClose(threadHandle);
> +        threadHandle = NULL;
> +
> +        error = FALSE;
> +    } while (error);
> +
> +    return status;
> +}

Even in this case, you can just do “return status” instead of break. The 
do-while look is not making the code any simpler.
SV: I think it makes the code cleaner. The return should be placed only at the 
end of the function and using a do-while loop in this way allows to perform the 
cleanup, in just one place, before exiting the function.

> +static NTSTATUS
> +OvsTunnelFilterThreadInit(POVS_TUNFLT_THREAD_CONTEXT threadCtx) {
> +    NTSTATUS status = STATUS_SUCCESS;
> +    BOOLEAN error = TRUE;
> +
> +    do {
> +        /* Create thread's engine session object. */
> +        status = OvsTunnelEngineOpen(&threadCtx->engineSession);
> +        if (!NT_SUCCESS(status)) {
> +            break;
> +        }
> +
> +        NdisAllocateSpinLock(&threadCtx->listRequests.spinlock);
> +
> +        InitializeListHead(&threadCtx->listRequests.head);
> +
> +        KeInitializeEvent(&threadCtx->stopEvent,
> +            NotificationEvent,
> +            FALSE);
> +
> +        KeInitializeEvent(&threadCtx->requestEvent,
> +            SynchronizationEvent,
> +            FALSE);
> +
> +        error = FALSE;
> +    } while (error);
> +
> +    return status;
> +}

Even in this case, you can just do “return status” instead of break. The 
do-while look is not making the code any simpler.

> @@ -2293,22 +2361,30 @@ OvsDeleteVportCmdHandler(POVS_USER_PARAMS_CONTEXT 
> usrParamsCtx,
>         goto Cleanup;
>     }
> 
> -    status = OvsCreateMsgFromVport(vport, msgIn, usrParamsCtx->outputBuffer,
> -                                   usrParamsCtx->outputLength,
> -                                   gOvsSwitchContext->dpNo);
> -
>     /*
>      * Mark the port as deleted from OVS userspace. If the port does not exist
>      * on the Hyper-V switch, it gets deallocated. Otherwise, it stays.
>      */
> -    OvsRemoveAndDeleteVport(gOvsSwitchContext, vport, FALSE, TRUE, NULL);
> +    status = OvsRemoveAndDeleteVport(usrParamsCtx,
> +                                     gOvsSwitchContext,
> +                                     vport,
> +                                     FALSE,
> +                                     TRUE);
> +    if (STATUS_PENDING == status) {
> +        nlError = NL_ERROR_PENDING;
> +        goto Cleanup;
> +    }
> +
> +    status = OvsCreateMsgFromVport(vport, msgIn, usrParamsCtx->outputBuffer,
> +                                   usrParamsCtx->outputLength,
> +                                   gOvsSwitchContext->dpNo);

‘vport’ might have got freed up in OvsRemoveAndDeleteVport() which is why we 
were calling into OvsCreateMsgFromVport() earlier. This can happen if the port 
has been deleted from Hyper-V first, and then if it gets deleted from OVS 
userspace. I understand that the code will be sort of redundant in case the 
status is STATUS_PENDING, but it is a cost we have to pay, and is OK I think.

> 
>     *replyLen = msgOut->nlMsg.nlmsgLen;
> 
> Cleanup:
>     NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState);
> 
> -    if (nlError != NL_ERROR_SUCCESS) {
> +    if ((nlError != NL_ERROR_SUCCESS) && (nlError != 
> + NL_ERROR_PENDING)) {
>         POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
>             usrParamsCtx->outputBuffer;
> 
> @@ -2316,5 +2392,168 @@ Cleanup:
>         *replyLen = msgError->nlMsg.nlmsgLen;
>     }
> 
> -    return STATUS_SUCCESS;
> +    return (status == STATUS_PENDING) ? STATUS_PENDING : 
> +STATUS_SUCCESS; }
> +
> +static VOID
> +OvsTunnelVportPendingUninit(PVOID context,
> +                            NTSTATUS status,
> +                            UINT32 *replyLen) {
> +    POVS_TUNFLT_INIT_CONTEXT tunnelContext =
> +        (POVS_TUNFLT_INIT_CONTEXT) context;
> +    POVS_VPORT_ENTRY vport = tunnelContext->vport;
> +    POVS_MESSAGE msgIn = (POVS_MESSAGE)tunnelContext->inputBuffer;
> +    POVS_MESSAGE msgOut = (POVS_MESSAGE)tunnelContext->outputBuffer;
> +    NL_ERROR nlError = NL_ERROR_SUCCESS;
> +    BOOLEAN error = TRUE;
> +
> +    do {
> +        if (!NT_SUCCESS(status)) {
> +            nlError = NlMapStatusToNlErr(status);
> +            break;
> +        }
> +
> +        OvsCreateMsgFromVport(vport,
> +                              msgIn,
> +                              msgOut,
> +                              tunnelContext->outputLength,
> +                              gOvsSwitchContext->dpNo);
> +
> +        *replyLen = msgOut->nlMsg.nlmsgLen;
> +
> +        error = FALSE;
> +    } while (error);
> +
> +    if (error) {
> +        POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) msgOut;
> +
> +        NlBuildErrorMsg(msgIn, msgError, nlError);
> +        *replyLen = msgError->nlMsg.nlmsgLen;
> +    }
> +}

Wouldn’t we have freed up the vxlan ‘vport’ in  OvsDeleteVportCmdHandler() -> 
OvsRemoveAndDeleteVport(). How is it safe to access it again in 
OvsTunnelVportPendingUninit()? We need to delink it from the hash tables, but 
free it up in OvsTunnelVportPendingUninit() for this code to work. This is the 
code that frees up the vport:

OvsRemoveAndDeleteVport():
    /*                                                                         
     * Deallocate the port if it has been deleted on the Hyper-V switch as well
     * as OVS userspace.                                                       
     */                                                                        
    if (deletedOnHv && deletedOnOvs) {                                         
        if (hvSwitchPort) {                                                    
            switchContext->numHvVports--;                                      
        } else {                                                               
            switchContext->numNonHvVports--;                                   
        }                                                                      
        OvsFreeMemoryWithTag(vport, OVS_VPORT_POOL_TAG);                       
    }                                                                          

SV: I will address this issue in the next version of this patch.

> + * This function allocates and initializes the OVS_VXLAN_VPORT. The 
> + function
> + * also creates a WFP tunnel filter for the necessary destination 
> + port. The
> + * tunnel filter create request is passed to the tunnel filter 
> + threads that
> + * will complete the request at a later time when IRQL is lowered to
> + * PASSIVE_LEVEL.
> + *
>  * udpDestPort: the vxlan is set as payload to a udp frame. If the 
> destination
>  * port of an udp frame is udpDestPort, we understand it to be vxlan.
>  */

Pls. add a " * 
--------------------------------------------------------------------------“. It 
makes it look a nice function description.
SV: OK.

> NTSTATUS
> -OvsInitVxlanTunnel(POVS_VPORT_ENTRY vport,
> -                   UINT16 udpDestPort)
> +OvsInitVxlanTunnel(PIRP irp,
> +                   POVS_VPORT_ENTRY vport,
> +                   UINT16 udpDestPort,
> +                   PFNTunnelVportPendingOp callback,
> +                   PVOID tunnelContext)
> {
> -    POVS_VXLAN_VPORT vxlanPort;
> +    NTSTATUS status = STATUS_SUCCESS;
> +    POVS_VXLAN_VPORT vxlanPort = NULL;
> 
>     vxlanPort = OvsAllocateMemoryWithTag(sizeof (*vxlanPort),
>                                          OVS_VXLAN_POOL_TAG); @@ 
> -67,28 +104,52 @@ OvsInitVxlanTunnel(POVS_VPORT_ENTRY vport,
> 
>     RtlZeroMemory(vxlanPort, sizeof(*vxlanPort));
>     vxlanPort->dstPort = udpDestPort;
> -    /*
> -     * since we are installing the WFP filter before the port is created
> -     * We need to check if it is the same number
> -     * XXX should be removed later
> -     */
> -    ASSERT(vxlanPort->dstPort == VXLAN_UDP_PORT);
>     vport->priv = (PVOID)vxlanPort;
> 
> -    return STATUS_SUCCESS;
> -}
> +    if (!OvsIsTunnelFilterCreated(gOvsSwitchContext, udpDestPort)) {
> +        status = OvsTunelFilterCreate(irp,
> +                                      udpDestPort,
> +                                      &vxlanPort->filterID,
> +                                      callback,
> +                                      tunnelContext);
> +    }
> 
> +    return status;

Ok, if OvsIsTunnelFilterCreated() returns TRUE, you want to return 
‘NL_ERROR_EXIST’. You might have taken care of this in later patches.
SV: I will address this issue in the next version of this patch.

thanks,
-- Nithin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to