diff mbox series

[ovs-dev,v1,3/4] northd: Fix lflow ref node's reference counting.

Message ID 20240208214932.12783-1-numans@ovn.org
State Accepted
Headers show
Series northd memory and CPU increase fix due to lflow-mgr. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_ovn-kubernetes success github build: passed
ovsrobot/github-robot-_Build_and_Test fail github build: failed

Commit Message

Numan Siddique Feb. 8, 2024, 9:49 p.m. UTC 
From: Numan Siddique <numans@ovn.org>

When the lflows in an lflow_ref are unlinked by calling
lflow_ref_unlink_lflows(lflow_ref), the dp_ref counter
for each lflow in the lflow_ref is decremented (by calling
dp_refcnt_release()),  but it is not incremented later
when the same lflow is linked back to the lflow_ref.

This patch fixes it.

Fixes: a623606052ea ("northd: Refactor lflow management into a separate module.")
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 northd/lflow-mgr.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Han Zhou Feb. 13, 2024, 7:14 a.m. UTC | #1 
On Thu, Feb 8, 2024 at 1:50 PM <numans@ovn.org> wrote:
>
> From: Numan Siddique <numans@ovn.org>
>
> When the lflows in an lflow_ref are unlinked by calling
> lflow_ref_unlink_lflows(lflow_ref), the dp_ref counter
> for each lflow in the lflow_ref is decremented (by calling
> dp_refcnt_release()),  but it is not incremented later
> when the same lflow is linked back to the lflow_ref.
>
> This patch fixes it.
>
> Fixes: a623606052ea ("northd: Refactor lflow management into a separate
module.")
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>  northd/lflow-mgr.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
> index 61729e9039..df62cd6ab4 100644
> --- a/northd/lflow-mgr.c
> +++ b/northd/lflow-mgr.c
> @@ -690,19 +690,24 @@ lflow_table_add_lflow(struct lflow_table
*lflow_table,
>              if (lrn->dpgrp_lflow) {
>                  lrn->dpgrp_bitmap = bitmap_clone(dp_bitmap,
dp_bitmap_len);
>                  lrn->dpgrp_bitmap_len = dp_bitmap_len;
> +            } else {
> +                lrn->dp_index = od->index;
> +            }
> +            ovs_list_insert(&lflow->referenced_by, &lrn->ref_list_node);
> +            hmap_insert(&lflow_ref->lflow_ref_nodes, &lrn->ref_node,
hash);
> +        }
>
> +        if (!lrn->linked) {
> +            if (lrn->dpgrp_lflow) {
> +                ovs_assert(lrn->dpgrp_bitmap_len == dp_bitmap_len);
>                  size_t index;
>                  BITMAP_FOR_EACH_1 (index, dp_bitmap_len, dp_bitmap) {
>                      dp_refcnt_use(&lflow->dp_refcnts_map, index);
>                  }
>              } else {
> -                lrn->dp_index = od->index;
>                  dp_refcnt_use(&lflow->dp_refcnts_map, lrn->dp_index);
>              }
> -            ovs_list_insert(&lflow->referenced_by, &lrn->ref_list_node);
> -            hmap_insert(&lflow_ref->lflow_ref_nodes, &lrn->ref_node,
hash);
>          }
> -
>          lrn->linked = true;
>      }
>
> --
> 2.43.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Thanks Numan for the fix.
Acked-by: Han Zhou <hzhou@ovn.org>
Dumitru Ceara Feb. 14, 2024, 2:38 p.m. UTC | #2 
On 2/13/24 08:14, Han Zhou wrote:
> On Thu, Feb 8, 2024 at 1:50 PM <numans@ovn.org> wrote:
>>
>> From: Numan Siddique <numans@ovn.org>
>>
>> When the lflows in an lflow_ref are unlinked by calling
>> lflow_ref_unlink_lflows(lflow_ref), the dp_ref counter
>> for each lflow in the lflow_ref is decremented (by calling
>> dp_refcnt_release()),  but it is not incremented later
>> when the same lflow is linked back to the lflow_ref.
>>
>> This patch fixes it.
>>
>> Fixes: a623606052ea ("northd: Refactor lflow management into a separate
> module.")
>> Signed-off-by: Numan Siddique <numans@ovn.org>
>> ---
>>  northd/lflow-mgr.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
>> index 61729e9039..df62cd6ab4 100644
>> --- a/northd/lflow-mgr.c
>> +++ b/northd/lflow-mgr.c
>> @@ -690,19 +690,24 @@ lflow_table_add_lflow(struct lflow_table
> *lflow_table,
>>              if (lrn->dpgrp_lflow) {
>>                  lrn->dpgrp_bitmap = bitmap_clone(dp_bitmap,
> dp_bitmap_len);
>>                  lrn->dpgrp_bitmap_len = dp_bitmap_len;
>> +            } else {
>> +                lrn->dp_index = od->index;
>> +            }
>> +            ovs_list_insert(&lflow->referenced_by, &lrn->ref_list_node);
>> +            hmap_insert(&lflow_ref->lflow_ref_nodes, &lrn->ref_node,
> hash);
>> +        }
>>
>> +        if (!lrn->linked) {
>> +            if (lrn->dpgrp_lflow) {
>> +                ovs_assert(lrn->dpgrp_bitmap_len == dp_bitmap_len);
>>                  size_t index;
>>                  BITMAP_FOR_EACH_1 (index, dp_bitmap_len, dp_bitmap) {
>>                      dp_refcnt_use(&lflow->dp_refcnts_map, index);
>>                  }
>>              } else {
>> -                lrn->dp_index = od->index;
>>                  dp_refcnt_use(&lflow->dp_refcnts_map, lrn->dp_index);
>>              }
>> -            ovs_list_insert(&lflow->referenced_by, &lrn->ref_list_node);
>> -            hmap_insert(&lflow_ref->lflow_ref_nodes, &lrn->ref_node,
> hash);
>>          }
>> -
>>          lrn->linked = true;
>>      }
>>
>> --
>> 2.43.0
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
> Thanks Numan for the fix.
> Acked-by: Han Zhou <hzhou@ovn.org>

Looks good to me too:

Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks,
Dumitru
diff mbox series

Patch

diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
index 61729e9039..df62cd6ab4 100644
--- a/northd/lflow-mgr.c
+++ b/northd/lflow-mgr.c
@@ -690,19 +690,24 @@  lflow_table_add_lflow(struct lflow_table *lflow_table,
             if (lrn->dpgrp_lflow) {
                 lrn->dpgrp_bitmap = bitmap_clone(dp_bitmap, dp_bitmap_len);
                 lrn->dpgrp_bitmap_len = dp_bitmap_len;
+            } else {
+                lrn->dp_index = od->index;
+            }
+            ovs_list_insert(&lflow->referenced_by, &lrn->ref_list_node);
+            hmap_insert(&lflow_ref->lflow_ref_nodes, &lrn->ref_node, hash);
+        }
 
+        if (!lrn->linked) {
+            if (lrn->dpgrp_lflow) {
+                ovs_assert(lrn->dpgrp_bitmap_len == dp_bitmap_len);
                 size_t index;
                 BITMAP_FOR_EACH_1 (index, dp_bitmap_len, dp_bitmap) {
                     dp_refcnt_use(&lflow->dp_refcnts_map, index);
                 }
             } else {
-                lrn->dp_index = od->index;
                 dp_refcnt_use(&lflow->dp_refcnts_map, lrn->dp_index);
             }
-            ovs_list_insert(&lflow->referenced_by, &lrn->ref_list_node);
-            hmap_insert(&lflow_ref->lflow_ref_nodes, &lrn->ref_node, hash);
         }
-
         lrn->linked = true;
     }