Great, thanks.
On Fri, Feb 13, 2015 at 09:18:38PM +0000, Eitan Eliahu wrote: > Hi Ben, > Sorin plans to address some issues once he is back. > Thanks, > Eitan > > > -----Original Message----- > From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Eitan Eliahu > Sent: Wednesday, January 07, 2015 1:16 PM > To: Sorin Vinturis; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH] datapath-windows: Support for custom VXLAN > tunnel port > > > > Hi Sorin, > > On the default port, it is also created through the user mode using the NL > interface so if we create all other ports dynamically I would prefer to > create the default port using the same method. (unless you see an issue > here). > > > > On the concurrency issue: The spec reads as follows: > > " Each client session can have only one transaction in progress at a time" > > If we don't serialize transactions we cannot guaranty that transactions are > not overlapped . You are right the second transaction will return with > IN_PROGRESS but is should have really completeed successfully if we were > serializing the transactions. > > I realize that it involves a significant work to serialize the transactions > and to initiate them from another thread which runs in a lower IRQL. Let me > get back to you on that ( iad this code before but removed it). Perhaps we > could retry if it fails on with FWP_E_TXN_IN_PROGRESS. > > Thank you, > > Eitan > > > > -----Original Message----- > > From: Sorin Vinturis [mailto:svintu...@cloudbasesolutions.com] > > Sent: Wednesday, January 07, 2015 11:59 AM > > To: Eitan Eliahu; dev@openvswitch.org > > Subject: RE: [ovs-dev] [PATCH] datapath-windows: Support for custom VXLAN > tunnel port > > > > Hi Eitan, > > > > I think we should keep the default port filter creation at the initialization > phase because most users are using the default port. > > > > Regarding the new filter creation for other tunnel port, the > OvsTunnelAddFilterEx function does not open the session to the filtering > engine. The session is already opened when the OvsTunnelAddFilterEx function > is called. It is opened during tunnel initialization phase at filter attach > and is closed at filter detach. > > > > The FwpmTransactionBegin0 function begins an explicit transaction within the > current session. The function cannot be called from within an already started > transaction. It will fail with FWP_E_TXN_IN_PROGRESS error. So, a subsequent > call to OvsTunnelAddFilterEx would be unsuccessful if a transaction is in > progress. I don't think that this scenario is common. I can add an error log > in case the function fails. What do you think? > > > > Thanks, > > Sorin > > > > -----Original Message----- > > From: Eitan Eliahu [mailto:elia...@vmware.com] > > Sent: Wednesday, 7 January, 2015 17:04 > > To: Sorin Vinturis; dev@openvswitch.org > > Subject: RE: [ovs-dev] [PATCH] datapath-windows: Support for custom VXLAN > tunnel port > > > > Hi Sorin, thank you for working on this. > > Do you thing the default port filter had to be created during initialization ? > > > > On another thing, since OvsTunnelAddFilterEx is called with IRQL = PASSIVE > and no lock is handled it may be called from multiple thread contexts. I am > not sure if opening. Closing and aborting the transaction engine would be > thread safe. We might need to serialize the calls and to invoke the engine > from a different thread context running on PASSIVE_IRQL. > > Thanks, > > Eitan > > > > -----Original Message----- > > From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Sorin Vinturis > > Sent: Wednesday, January 07, 2015 6:24 AM > > To: dev@openvswitch.org > > Subject: [ovs-dev] [PATCH] datapath-windows: Support for custom VXLAN tunnel > port > > > > The kernel datapath supports only port 4789 for VXLAN tunnel creation. > > Added support in order to allow for the VXLAN tunnel port to be configurable > to any port number set by the userspace. > > > > The patch also checks to see if an existing WFP filter, for the necessary UDP > tunnel port, is already created before adding a new one. > > This is a double check, because currently the userspace also verifies this, > but in my opinion is necessary to avoid future inconsistencies. > > > > Custom VXLAN tunnel port requires the addition of a new WFP filter with the > new UDP tunnel port. The cleanest way for the creation or removal of the new > WFP filters is to add them in OvsInitVxlanTunnel or OvsCleanupVxlanTunnel > functions. > > But the latter functions are running at IRQL = DISPATCH_LEVEL, due to the > NDIS RW lock acquisition, and all WFP calls must be running at IRQL = > PASSIVE_LEVEL. This is why the WFP filter creation/removal is made when the > spinlock is released and the IRQL level is lowered at PASSIVE_LEVEL. > > > > The default VXLAN tunnel port 4789 behaviour is unchanged. The WFP filter for > UDP port 4789 is added when the extension is enabled and is removed when the > extension is disabled. No other NL command is able to remove it. > > > > Now it is necessary that OvsTunnelFilterUninitialize function is called after > OvsClearAllSwitchVports in order to allow for the added WFP filters to be > removed. OvsTunnelFilterUninitialize function closes the global engine handle > used by most of the WFP calls, including filter removal. > > > > Signed-off-by: Sorin Vinturis <svintu...@cloudbasesolutions.com> > > Reported-by: Alin Gabriel Serdean <aserd...@cloudbasesolutions.com> > > Reported-at: > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs-2Dissues_issues_66&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=fVUQFtLj8jLS3oqKrhXRCrc7OwBHhpU4eYO-t9Nbm-g&s=R2DrJ23z-ZavsrI21xMGMD6w-yT16Xz2NCFc_qF7slw&e= > > --- > > datapath-windows/ovsext/Switch.c | 2 +- > > datapath-windows/ovsext/TunnelFilter.c | 88 > ++++++++++++++++++++++++++++++++-- > > datapath-windows/ovsext/TunnelIntf.h | 4 ++ > > datapath-windows/ovsext/Vport.c | 78 ++++++++++++++++++++++++++++-- > > datapath-windows/ovsext/Vxlan.c | 8 +--- > > datapath-windows/ovsext/Vxlan.h | 1 + > > 6 files changed, 166 insertions(+), 15 deletions(-) > > > > diff --git a/datapath-windows/ovsext/Switch.c > b/datapath-windows/ovsext/Switch.c > > index a228d8e..cf5e3c4 100644 > > --- a/datapath-windows/ovsext/Switch.c > > +++ b/datapath-windows/ovsext/Switch.c > > @@ -261,8 +261,8 @@ OvsDeleteSwitch(POVS_SWITCH_CONTEXT switchContext) > > if (switchContext) > > { > > dpNo = switchContext->dpNo; > > - OvsTunnelFilterUninitialize(gOvsExtDriverObject); > > OvsClearAllSwitchVports(switchContext); > > + OvsTunnelFilterUninitialize(gOvsExtDriverObject); > > OvsUninitSwitchContext(switchContext); > > OvsFreeMemory(switchContext); > > } > > diff --git a/datapath-windows/ovsext/TunnelFilter.c > b/datapath-windows/ovsext/TunnelFilter.c > > index e0adc37..dac14c7 100644 > > --- a/datapath-windows/ovsext/TunnelFilter.c > > +++ b/datapath-windows/ovsext/TunnelFilter.c > > @@ -239,7 +239,8 @@ OvsTunnelAddFilter(PWSTR filterName, > > UINT64 context, > > const GUID *filterKey, > > const GUID *layerKey, > > - const GUID *calloutKey) > > + const GUID *calloutKey, > > + UINT64 *filterID) > > { > > NTSTATUS status = STATUS_SUCCESS; > > FWPM_FILTER filter = {0}; > > @@ -249,7 +250,9 @@ OvsTunnelAddFilter(PWSTR filterName, > > UNREFERENCED_PARAMETER(remotePort); > > UNREFERENCED_PARAMETER(direction); > > > > - filter.filterKey = *filterKey; > > + if (filterKey) { > > + filter.filterKey = *filterKey; > > + } > > filter.layerKey = *layerKey; > > filter.displayData.name = (wchar_t*)filterName; > > filter.displayData.description = (wchar_t*)filterDesc; @@ -282,7 +285,7 > @@ OvsTunnelAddFilter(PWSTR filterName, > > status = FwpmFilterAdd(gEngineHandle, > > &filter, > > NULL, > > - NULL); > > + filterID); > > > > return status; > > } > > @@ -395,7 +398,8 @@ OvsTunnelRegisterDatagramDataCallouts(const GUID > *layerKey, > > 0, > > &OVS_TUNNEL_FILTER_KEY, > > layerKey, > > - calloutKey); > > + calloutKey, > > + NULL); > > > > Exit: > > > > @@ -541,3 +545,79 @@ Exit: > > > > return status; > > } > > + > > +NTSTATUS > > +OvsTunnelAddFilterEx(UINT32 remotePort, > > + UINT64* filterID) > > +{ > > + NTSTATUS status = STATUS_SUCCESS; > > + BOOLEAN inTransaction = FALSE; > > + > > + do { > > + status = FwpmTransactionBegin(gEngineHandle, 0); > > + if (!NT_SUCCESS(status)) { > > + break; > > + } > > + > > + inTransaction = TRUE; > > + > > + status = OvsTunnelAddFilter(L"Datagram-Data OVS Filter (Inbound)", > > + L"address/port for UDP", > > + (USHORT)remotePort, > > + FWP_DIRECTION_INBOUND, > > + 0, > > + NULL, > > + &FWPM_LAYER_DATAGRAM_DATA_V4, > > + &OVS_TUNNEL_CALLOUT_V4, > > + filterID); > > + if (!NT_SUCCESS(status)) { > > + break; > > + } > > + > > + status = FwpmTransactionCommit(gEngineHandle); > > + if (!NT_SUCCESS(status)){ > > + break; > > + } > > + > > + inTransaction = FALSE; > > + } while (inTransaction); > > + > > + if (inTransaction) { > > + FwpmTransactionAbort(gEngineHandle); > > + } > > + return status; > > +} > > + > > +NTSTATUS > > +OvsTunnelRemoveFilterEx(UINT64 filterID) { > > + NTSTATUS status = STATUS_SUCCESS; > > + BOOLEAN inTransaction = FALSE; > > + > > + do { > > + if (!filterID) { > > + break; > > + } > > + > > + status = FwpmTransactionBegin(gEngineHandle, 0); > > + if (!NT_SUCCESS(status)) { > > + break; > > + } > > + inTransaction = TRUE; > > + > > + status = FwpmFilterDeleteById(gEngineHandle, filterID); > > + if (!NT_SUCCESS(status)) { > > + break; > > + } > > + > > + status = FwpmTransactionCommit(gEngineHandle); > > + > > + inTransaction = FALSE; > > + } while (inTransaction); > > + > > + if (inTransaction) { > > + FwpmTransactionAbort(gEngineHandle); > > + } > > + > > + return status; > > +} > > diff --git a/datapath-windows/ovsext/TunnelIntf.h > b/datapath-windows/ovsext/TunnelIntf.h > > index b2bba30..5084a94 100644 > > --- a/datapath-windows/ovsext/TunnelIntf.h > > +++ b/datapath-windows/ovsext/TunnelIntf.h > > @@ -30,4 +30,8 @@ VOID OvsTunnelAddSystemProvider(HANDLE handle); > > > > VOID OvsTunnelRemoveSystemProvider(HANDLE handle); > > > > +NTSTATUS OvsTunnelAddFilterEx(UINT32 remotePort, UINT64* filterID); > > + > > +NTSTATUS OvsTunnelRemoveFilterEx(UINT64 filterID); > > + > > #endif /* __TUNNEL_INTF_H_ */ > > diff --git a/datapath-windows/ovsext/Vport.c > b/datapath-windows/ovsext/Vport.c index c9dfaea..c9ba19a 100644 > > --- a/datapath-windows/ovsext/Vport.c > > +++ b/datapath-windows/ovsext/Vport.c > > @@ -70,6 +70,8 @@ static POVS_VPORT_ENTRY > OvsFindVportByHvNameW(POVS_SWITCH_CONTEXT switchContext, static NDIS_STATUS > InitHvVportCommon(POVS_SWITCH_CONTEXT switchContext, > > POVS_VPORT_ENTRY vport, > > BOOLEAN newPort); > > +static BOOLEAN OvsIsTunnelFilterCreated(POVS_SWITCH_CONTEXT switchContext, > > + UINT16 udpPortDest); > > > > /* > > * Functions implemented in relaton to NDIS port manipulation. > > @@ -1316,6 +1318,12 @@ OvsClearAllSwitchVports(POVS_SWITCH_CONTEXT > switchContext) > > ASSERT(OvsIsTunnelVportType(vport->ovsType) || > > (vport->ovsType == OVS_VPORT_TYPE_INTERNAL && > > vport->isBridgeInternal) || vport->isPresentOnHv == > TRUE); > > + > > + if (vport->priv) { > > + OvsTunnelRemoveFilterEx( > > + ((POVS_VXLAN_VPORT)vport->priv)->filterID); > > + } > > + > > OvsRemoveAndDeleteVport(switchContext, vport, TRUE, TRUE, NULL); > > } > > } > > @@ -1908,7 +1916,7 @@ Cleanup: > > > > /* > > * -------------------------------------------------------------------------- > > - * Command Handler for 'OVS_VPORT_CMD_NEW'. > > + * Command Handler for 'OVS_VPORT_CMD_GET'. > > * > > * The function handles the initial call to setup the dump state, as well as > > * subsequent calls to continue dumping data. > > @@ -1971,6 +1979,7 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT > usrParamsCtx, > > PCHAR portName; > > ULONG portNameLen; > > UINT32 portType; > > + UINT16 udpPortDest = VXLAN_UDP_PORT; > > BOOLEAN isBridgeInternal = FALSE; > > BOOLEAN vportAllocated = FALSE, vportInitialized = FALSE; > > BOOLEAN addInternalPortAsNetdev = FALSE; @@ -2044,7 +2053,10 @@ > OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, > > vportAllocated = TRUE; > > > > if (OvsIsTunnelVportType(portType)) { > > - status = OvsInitTunnelVport(vport, portType, VXLAN_UDP_PORT); > > + PNL_ATTR attr = > NlAttrFindNested(vportAttrs[OVS_VPORT_ATTR_OPTIONS], > > + OVS_TUNNEL_ATTR_DST_PORT); > > + udpPortDest = NlAttrGetU16(attr); > > + status = OvsInitTunnelVport(vport, portType, udpPortDest); > > nlError = NlMapStatusToNlErr(status); > > } else { > > OvsInitBridgeInternalVport(vport); > > @@ -2119,7 +2131,21 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT > usrParamsCtx, > > Cleanup: > > NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState); > > > > - if (nlError != NL_ERROR_SUCCESS) { > > + if (nlError == NL_ERROR_SUCCESS) { > > + if (VXLAN_UDP_PORT != udpPortDest && > > + !OvsIsTunnelFilterCreated(gOvsSwitchContext, udpPortDest)) { > > + /* > > + * All necessary calls to the WFP filtering engine must be > running > > + * at IRQL = PASSIVE_LEVEL. Thus the new WFP tunnel filter > creation > > + * is added here, outside the lock which raises the IRQL to > > + * DISPATCH_LEVEL. > > + */ > > + if (vport->priv) { > > + OvsTunnelAddFilterEx(udpPortDest, > > + > &((POVS_VXLAN_VPORT)vport->priv)->filterID); > > + } > > + } > > + } else { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > > > > @@ -2269,6 +2295,7 @@ OvsDeleteVportCmdHandler(POVS_USER_PARAMS_CONTEXT > usrParamsCtx, > > NL_ERROR nlError = NL_ERROR_SUCCESS; > > PSTR portName = NULL; > > UINT32 portNameLen = 0; > > + UINT64 vxlanFilterID = 0; > > > > static const NL_POLICY ovsVportPolicy[] = { > > [OVS_VPORT_ATTR_PORT_NO] = { .type = NL_A_U32, .optional = TRUE }, > @@ -2308,6 +2335,14 @@ OvsDeleteVportCmdHandler(POVS_USER_PARAMS_CONTEXT > usrParamsCtx, > > nlError = NL_ERROR_NODEV; > > goto Cleanup; > > } > > + > > + if (vport->priv) { > > + /* > > + * Get VXLAN filterID before vport deletion in order to remove the > > + * WFP filter. > > + */ > > + vxlanFilterID = ((POVS_VXLAN_VPORT)vport->priv)->filterID; > > + } > > > > status = OvsCreateMsgFromVport(vport, msgIn, usrParamsCtx->outputBuffer, > > usrParamsCtx->outputLength, @@ -2324,6 > +2359,10 @@ OvsDeleteVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, > > Cleanup: > > NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState); > > > > + if (vxlanFilterID) { > > + OvsTunnelRemoveFilterEx(vxlanFilterID); > > + } > > + > > if (nlError != NL_ERROR_SUCCESS) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; @@ -2334,3 +2373,36 @@ Cleanup: > > > > return STATUS_SUCCESS; > > } > > + > > +static BOOLEAN > > +OvsIsTunnelFilterCreated(POVS_SWITCH_CONTEXT switchContext, > > + UINT16 udpPortDest) { > > + for (UINT hash = 0; hash < OVS_MAX_VPORT_ARRAY_SIZE; hash++) { > > + PLIST_ENTRY head, link, next; > > + > > + head = &(switchContext->portNoHashArray[hash & OVS_VPORT_MASK]); > > + LIST_FORALL_SAFE(head, link, next) { > > + POVS_VPORT_ENTRY vport = NULL; > > + POVS_VXLAN_VPORT vxlanPort = NULL; > > + vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, portNoLink); > > + vxlanPort = (POVS_VXLAN_VPORT)vport->priv; > > + if (vxlanPort) { > > + if ((udpPortDest == vxlanPort->dstPort) && > > + (0 != vxlanPort->filterID)) { > > + /* > > + * VXLAN destination port matching is not enough to > decide > > + * if the WFP filter was created or not, because the > newly > > + * allocated VXLAN vport is already added to the > > + * portNoHashArray in InitOvsVportCommon function. If the > > + * filterID of the VXLAN vport is zero it means that the > > + * WFP filter is not created yet. > > + */ > > + return TRUE; > > + } > > + } > > + } > > + } > > + > > + return FALSE; > > +} > > \ No newline at end of file > > diff --git a/datapath-windows/ovsext/Vxlan.c > b/datapath-windows/ovsext/Vxlan.c index 1ce5af2..8f40b6a 100644 > > --- a/datapath-windows/ovsext/Vxlan.c > > +++ b/datapath-windows/ovsext/Vxlan.c > > @@ -57,7 +57,7 @@ NTSTATUS > > OvsInitVxlanTunnel(POVS_VPORT_ENTRY vport, > > UINT16 udpDestPort) > > { > > - POVS_VXLAN_VPORT vxlanPort; > > + POVS_VXLAN_VPORT vxlanPort = NULL; > > > > vxlanPort = OvsAllocateMemory(sizeof (*vxlanPort)); > > if (vxlanPort == NULL) { > > @@ -66,12 +66,6 @@ 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; > > diff --git a/datapath-windows/ovsext/Vxlan.h > b/datapath-windows/ovsext/Vxlan.h index d84796d..4343da1 100644 > > --- a/datapath-windows/ovsext/Vxlan.h > > +++ b/datapath-windows/ovsext/Vxlan.h > > @@ -24,6 +24,7 @@ typedef struct _OVS_VXLAN_VPORT { > > UINT64 outPkts; > > UINT64 slowInPkts; > > UINT64 slowOutPkts; > > + UINT64 filterID; > > /* > > * To be filled > > */ > > -- > > 1.9.0.msysgit.0 > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailman_listinfo_dev&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=fVUQFtLj8jLS3oqKrhXRCrc7OwBHhpU4eYO-t9Nbm-g&s=J3wP7n4jBwn5krFRF2RrwDRv1tRN_yxfZF6K_tpNT7s&e= > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailman_listinfo_dev&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=0PJMRqoiQGgjgV8XRSA6FUJr47k0iVlG9CRNKAqh3pY&s=ciUMGo6A6FQguvNbGkODMbvZauqOvdddoL9gG6Sdtbk&e= > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
