Nithin, I thought I have addressed your comment. My understanding was that was an issue in OvsDeleteVportCmdHandler() after the call to OvsRemoveAndDeleteVport(). The latter function might deallocate the vport, which would be used by the OvsTunnelVportPendingUninit() callback. I have modified OvsRemoveAndDeleteVport() to deallocate the vport only if the OvsCleanupVxlanTunnel() returns STATUS_SUCCESS. If OvsCleanupVxlanTunnel() returns STATUS_PENDING, then the vport is deallocated in OvsTunnelVportPendingUninit() callback.
Is there another use case I am not seeing? Thanks, Sorin -----Original Message----- From: Nithin Raju [mailto:nit...@vmware.com] Sent: Thursday, 21 May, 2015 20:38 To: Sorin Vinturis Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH v5 1/4] datapath-windows: Support for custom VXLAN tunnel port hi Sorin, Thanks for the respin. I had 2 comments in the previous review regarding access to a freed up ‘vport’ in OvsDeleteVportCmdHandler() and OvsTunnelVportPendingUninit(). It looks like you addressed the issue with OvsTunnelVportPendingUninit(), by making sure that the Vport is not deallocated as well as unlinked from the hash tables. The fix is partly correct, but we can improvise on it. The only quip I have is that a duplicate ‘vport’ delete request from userspace might end up confusing the kernel. I don’t see the other comment being addressed. I am pasting the comment here for convenience from the previous review: --- ‘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. --- Can you pls. address that and resend the patch? We can speak on IRC if you think this merits a discussion. -- Nithin > On May 21, 2015, at 4:30 AM, Sorin Vinturis > <svintu...@cloudbasesolutions.com> wrote: > > 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 it is necessary to avoid future issues. > > Custom VXLAN tunnel port requires the addition of a new WFP filter > with the new UDP tunnel port. The creation of a new WFP filter is > triggered in OvsInitVxlanTunnel function and the removal of the WFP > filter in OvsCleanupVxlanTunnel function. > 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 I have created a system thread which > records all filter addition/removal requests into a list for later > processing by the system thread. The ThreadStart routine processes all > received requests at IRQL = PASSIVE_LEVEL, which is the required IRQL > for the necessary WFP calls for adding/removal of the WFP filters. > > The WFP filter for the default VXLAN port 4789 is not added anymore at > filter attach. All WFP filters for the tunnel ports are added when the > tunnel ports are initialized and are removed at cleanup. WFP operation > status is then reported to userspace. > > 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_openvs > witch_ovs-2Dissues_issues_66&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw- > YihVMNtXt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=a6vKUvOq > 9zeqgPiwG03r2FU8bkmAQm54KhGn7-l9rHU&s=b4OU0oNav7uYXzW94eGhTwpYTNfj04Nd > kup6zLhqir0&e= _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
