diff mbox series

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

Message ID 20181002230045.8708-1-kumaranand@vmware.com
State Changes Requested
Delegated to: Alin Gabriel Serdean
Headers show
Series [ovs-dev,RFC] datapath-windows: Remove neighbor entries when Iphelper instance is deleted | expand

Commit Message

Anand Kumar Oct. 2, 2018, 11 p.m. UTC 
'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 <kumaranand@vmware.com>
---
 datapath-windows/ovsext/IpHelper.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Comments

Shashank Ram Oct. 5, 2018, 1:31 a.m. UTC | #1 
On 10/3/18, 4:30 AM, "ovs-dev-bounces@openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of kumaranand@vmware.com> 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 <kumaranand@vmware.com>
    ---
     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(&ovsSortedIPNeighList, 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(&instance->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(&ovsIpHelperLock);
    -            ExAcquireResourceExclusiveLite(&ovsInstanceListLock, TRUE);
    +            ExAcquireResourceExclusiveLite(&instance->lock, TRUE);
     
                 status = OvsGetOrResolveIPNeigh(&instance->internalRow,
                                                 ipAddr, &ipNeigh);
                 OvsUpdateIPNeighEntry(ipAddr, &ipNeigh, status);
     
    -            ExReleaseResourceLite(&ovsInstanceListLock);
    +            ExReleaseResourceLite(&instance->lock);
     
                 NdisAcquireSpinLock(&ovsIpHelperLock);
             }
    @@ -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(&ovsIpHelperLock);
     
    -    OvsIpHelperDeleteAllInstances();
         ExDeleteResourceLite(&ovsInstanceListLock);
     }
     
    -- 
    2.9.3.windows.1
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&amp;data=02%7C01%7Crams%40vmware.com%7Cc2c81b5b031b48819dca08d628bb034a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636741181029502269&amp;sdata=WfD24VN49hX0vKBrxBaxf7FVIF5JhkTpk1YI%2BzdtwX4%3D&amp;reserved=0
Anand Kumar Oct. 5, 2018, 5 a.m. UTC | #2 
Hi Shashank,

Thanks for the review, please find my response inline.

Regards,
Anand Kumar

On 10/4/18, 6:31 PM, "Shashank Ram" <rams@vmware.com> wrote:

    
    On 10/3/18, 4:30 AM, "ovs-dev-bounces@openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of kumaranand@vmware.com> 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 <kumaranand@vmware.com>
        ---
         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(&ovsSortedIPNeighList, 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(&instance->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(&ovsIpHelperLock);
        -            ExAcquireResourceExclusiveLite(&ovsInstanceListLock, TRUE);
        +            ExAcquireResourceExclusiveLite(&instance->lock, TRUE);
         
                     status = OvsGetOrResolveIPNeigh(&instance->internalRow,
                                                     ipAddr, &ipNeigh);
                     OvsUpdateIPNeighEntry(ipAddr, &ipNeigh, status);
         
        -            ExReleaseResourceLite(&ovsInstanceListLock);
        +            ExReleaseResourceLite(&instance->lock);
         
                     NdisAcquireSpinLock(&ovsIpHelperLock);
                 }
        @@ -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(&ovsIpHelperLock);
         
        -    OvsIpHelperDeleteAllInstances();
             ExDeleteResourceLite(&ovsInstanceListLock);
         }
         
        -- 
        2.9.3.windows.1
        
        _______________________________________________
        dev mailing list
        dev@openvswitch.org
        https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&amp;data=02%7C01%7Crams%40vmware.com%7Cc2c81b5b031b48819dca08d628bb034a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636741181029502269&amp;sdata=WfD24VN49hX0vKBrxBaxf7FVIF5JhkTpk1YI%2BzdtwX4%3D&amp;reserved=0
diff mbox series

Patch

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(&ovsSortedIPNeighList, 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(&instance->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(&ovsIpHelperLock);
-            ExAcquireResourceExclusiveLite(&ovsInstanceListLock, TRUE);
+            ExAcquireResourceExclusiveLite(&instance->lock, TRUE);
 
             status = OvsGetOrResolveIPNeigh(&instance->internalRow,
                                             ipAddr, &ipNeigh);
             OvsUpdateIPNeighEntry(ipAddr, &ipNeigh, status);
 
-            ExReleaseResourceLite(&ovsInstanceListLock);
+            ExReleaseResourceLite(&instance->lock);
 
             NdisAcquireSpinLock(&ovsIpHelperLock);
         }
@@ -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(&ovsIpHelperLock);
 
-    OvsIpHelperDeleteAllInstances();
     ExDeleteResourceLite(&ovsInstanceListLock);
 }