Re: [ovs-dev] [PATCH RFC] datapath-windows: Remove neighbor entries when Iphelper instance is deleted

2018-10-04 Thread Anand Kumar 
Hi Shashank,

Thanks for the review, please find my response inline.

Regards,
Anand Kumar

On 10/4/18, 6:31 PM, "Shashank Ram"  wrote:


On 10/3/18, 4:30 AM, "ovs-dev-boun...@openvswitch.org on behalf of Anand 
Kumar"  
wrote:

'OVS_IPHELPER_INSTANCE' is linked to ovsSortedIPNeighList.
So when an Iphelper instance is deleted, also delete the ip
neighboring entries associated with that instance.

Also fix accessing Iphelper instance without acquiring thelock.

Signed-off-by: Anand Kumar 
---
 datapath-windows/ovsext/IpHelper.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/datapath-windows/ovsext/IpHelper.c 
b/datapath-windows/ovsext/IpHelper.c
index 6bbd096..581be61 100644
--- a/datapath-windows/ovsext/IpHelper.c
+++ b/datapath-windows/ovsext/IpHelper.c
@@ -1446,6 +1446,17 @@ static VOID
 OvsIpHelperDeleteInstance(POVS_IPHELPER_INSTANCE instance)
 {
 if (instance) {
+if (ovsNumFwdEntries) {
Is this check really needed? If there are no entries, then LIST_FORALL_SAFE 
will not enter the loop?
Yes, this is required. Ip Neighboring entry (ipn)  and Ip forwarding entry 
(ipf) have 1:1 mapping, 
i.e. each ipf will have ipn associated with it. 
+POVS_IPNEIGH_ENTRY ipn;
+PLIST_ENTRY link, next;
+LIST_FORALL_SAFE(, link, next) {
+ipn = CONTAINING_RECORD(link, OVS_IPNEIGH_ENTRY, 
slink);
+POVS_IPHELPER_INSTANCE ipnInstance = 
(POVS_IPHELPER_INSTANCE)ipn->context;
+if (ipnInstance == instance) {
+OvsRemoveIPNeighEntry(ipn);
+}
+}
+}
 ExDeleteResourceLite(>lock);
 OvsFreeMemoryWithTag(instance, OVS_IPHELPER_POOL_TAG);
 }
@@ -1942,13 +1953,13 @@ OvsStartIpHelper(PVOID data)
 NTSTATUS status;
 POVS_IPHELPER_INSTANCE instance = 
(POVS_IPHELPER_INSTANCE)ipn->context;
 NdisReleaseSpinLock();
-ExAcquireResourceExclusiveLite(, TRUE);
+ExAcquireResourceExclusiveLite(>lock, TRUE);
 
 status = OvsGetOrResolveIPNeigh(>internalRow,
 ipAddr, );
 OvsUpdateIPNeighEntry(ipAddr, , status);
 
-ExReleaseResourceLite();
+ExReleaseResourceLite(>lock);
 
 NdisAcquireSpinLock();
 }
@@ -2098,11 +2109,10 @@ OvsCleanupIpHelper(VOID)
 OvsFreeMemoryWithTag(ovsFwdHashTable, OVS_IPHELPER_POOL_TAG);
 OvsFreeMemoryWithTag(ovsRouteHashTable, OVS_IPHELPER_POOL_TAG);
 OvsFreeMemoryWithTag(ovsNeighHashTable, OVS_IPHELPER_POOL_TAG);
-
+OvsIpHelperDeleteAllInstances();
Why is this being changed?
This is required because any write operation to 'ovsSortedIPNeighList ' is 
protected by  'ovsIpHelperLock'. 
With this patch, ipn entry is removed from sorted list in ' 
OvsIpHelperDeleteInstance '  .
 NdisFreeRWLock(ovsTableLock);
 NdisFreeSpinLock();
 
-OvsIpHelperDeleteAllInstances();
 ExDeleteResourceLite();
 }
 
-- 
2.9.3.windows.1

___
dev mailing list
d...@openvswitch.org

https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-devdata=02%7C01%7Crams%40vmware.com%7Cc2c81b5b031b48819dca08d628bb034a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636741181029502269sdata=WfD24VN49hX0vKBrxBaxf7FVIF5JhkTpk1YI%2BzdtwX4%3Dreserved=0




___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH RFC] datapath-windows: Remove neighbor entries when Iphelper instance is deleted

2018-10-04 Thread Shashank Ram 


On 10/3/18, 4:30 AM, "ovs-dev-boun...@openvswitch.org on behalf of Anand Kumar" 
 wrote:

'OVS_IPHELPER_INSTANCE' is linked to ovsSortedIPNeighList.
So when an Iphelper instance is deleted, also delete the ip
neighboring entries associated with that instance.

Also fix accessing Iphelper instance without acquiring thelock.

Signed-off-by: Anand Kumar 
---
 datapath-windows/ovsext/IpHelper.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/datapath-windows/ovsext/IpHelper.c 
b/datapath-windows/ovsext/IpHelper.c
index 6bbd096..581be61 100644
--- a/datapath-windows/ovsext/IpHelper.c
+++ b/datapath-windows/ovsext/IpHelper.c
@@ -1446,6 +1446,17 @@ static VOID
 OvsIpHelperDeleteInstance(POVS_IPHELPER_INSTANCE instance)
 {
 if (instance) {
+if (ovsNumFwdEntries) {
Is this check really needed? If there are no entries, then LIST_FORALL_SAFE 
will not enter the loop?
+POVS_IPNEIGH_ENTRY ipn;
+PLIST_ENTRY link, next;
+LIST_FORALL_SAFE(, link, next) {
+ipn = CONTAINING_RECORD(link, OVS_IPNEIGH_ENTRY, slink);
+POVS_IPHELPER_INSTANCE ipnInstance = 
(POVS_IPHELPER_INSTANCE)ipn->context;
+if (ipnInstance == instance) {
+OvsRemoveIPNeighEntry(ipn);
+}
+}
+}
 ExDeleteResourceLite(>lock);
 OvsFreeMemoryWithTag(instance, OVS_IPHELPER_POOL_TAG);
 }
@@ -1942,13 +1953,13 @@ OvsStartIpHelper(PVOID data)
 NTSTATUS status;
 POVS_IPHELPER_INSTANCE instance = 
(POVS_IPHELPER_INSTANCE)ipn->context;
 NdisReleaseSpinLock();
-ExAcquireResourceExclusiveLite(, TRUE);
+ExAcquireResourceExclusiveLite(>lock, TRUE);
 
 status = OvsGetOrResolveIPNeigh(>internalRow,
 ipAddr, );
 OvsUpdateIPNeighEntry(ipAddr, , status);
 
-ExReleaseResourceLite();
+ExReleaseResourceLite(>lock);
 
 NdisAcquireSpinLock();
 }
@@ -2098,11 +2109,10 @@ OvsCleanupIpHelper(VOID)
 OvsFreeMemoryWithTag(ovsFwdHashTable, OVS_IPHELPER_POOL_TAG);
 OvsFreeMemoryWithTag(ovsRouteHashTable, OVS_IPHELPER_POOL_TAG);
 OvsFreeMemoryWithTag(ovsNeighHashTable, OVS_IPHELPER_POOL_TAG);
-
+OvsIpHelperDeleteAllInstances();
Why is this being changed?

 NdisFreeRWLock(ovsTableLock);
 NdisFreeSpinLock();
 
-OvsIpHelperDeleteAllInstances();
 ExDeleteResourceLite();
 }
 
-- 
2.9.3.windows.1

___
dev mailing list
d...@openvswitch.org

https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-devdata=02%7C01%7Crams%40vmware.com%7Cc2c81b5b031b48819dca08d628bb034a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636741181029502269sdata=WfD24VN49hX0vKBrxBaxf7FVIF5JhkTpk1YI%2BzdtwX4%3Dreserved=0


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH RFC] datapath-windows: Remove neighbor entries when Iphelper instance is deleted

2018-10-02 Thread Anand Kumar 
'OVS_IPHELPER_INSTANCE' is linked to ovsSortedIPNeighList.
So when an Iphelper instance is deleted, also delete the ip
neighboring entries associated with that instance.

Also fix accessing Iphelper instance without acquiring thelock.

Signed-off-by: Anand Kumar 
---
 datapath-windows/ovsext/IpHelper.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/datapath-windows/ovsext/IpHelper.c 
b/datapath-windows/ovsext/IpHelper.c
index 6bbd096..581be61 100644
--- a/datapath-windows/ovsext/IpHelper.c
+++ b/datapath-windows/ovsext/IpHelper.c
@@ -1446,6 +1446,17 @@ static VOID
 OvsIpHelperDeleteInstance(POVS_IPHELPER_INSTANCE instance)
 {
 if (instance) {
+if (ovsNumFwdEntries) {
+POVS_IPNEIGH_ENTRY ipn;
+PLIST_ENTRY link, next;
+LIST_FORALL_SAFE(, link, next) {
+ipn = CONTAINING_RECORD(link, OVS_IPNEIGH_ENTRY, slink);
+POVS_IPHELPER_INSTANCE ipnInstance = 
(POVS_IPHELPER_INSTANCE)ipn->context;
+if (ipnInstance == instance) {
+OvsRemoveIPNeighEntry(ipn);
+}
+}
+}
 ExDeleteResourceLite(>lock);
 OvsFreeMemoryWithTag(instance, OVS_IPHELPER_POOL_TAG);
 }
@@ -1942,13 +1953,13 @@ OvsStartIpHelper(PVOID data)
 NTSTATUS status;
 POVS_IPHELPER_INSTANCE instance = 
(POVS_IPHELPER_INSTANCE)ipn->context;
 NdisReleaseSpinLock();
-ExAcquireResourceExclusiveLite(, TRUE);
+ExAcquireResourceExclusiveLite(>lock, TRUE);
 
 status = OvsGetOrResolveIPNeigh(>internalRow,
 ipAddr, );
 OvsUpdateIPNeighEntry(ipAddr, , status);
 
-ExReleaseResourceLite();
+ExReleaseResourceLite(>lock);
 
 NdisAcquireSpinLock();
 }
@@ -2098,11 +2109,10 @@ OvsCleanupIpHelper(VOID)
 OvsFreeMemoryWithTag(ovsFwdHashTable, OVS_IPHELPER_POOL_TAG);
 OvsFreeMemoryWithTag(ovsRouteHashTable, OVS_IPHELPER_POOL_TAG);
 OvsFreeMemoryWithTag(ovsNeighHashTable, OVS_IPHELPER_POOL_TAG);
-
+OvsIpHelperDeleteAllInstances();
 NdisFreeRWLock(ovsTableLock);
 NdisFreeSpinLock();
 
-OvsIpHelperDeleteAllInstances();
 ExDeleteResourceLite();
 }
 
-- 
2.9.3.windows.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev