[ovs-dev] [PATCH v4 1/4] datapath-windows: Support for custom VXLAN tunnel port
Nithin Raju
nithin at vmware.com
Wed May 20 08:15:29 UTC 2015
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.
> +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.
> +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);
}
> + * 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.
> 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.
thanks,
-- Nithin
More information about the dev
mailing list