diff mbox series

[ovs-dev,v1,1/4] northd: Don't add lr_out_delivery default drop flow for each lrp.

Message ID 20240208214920.12747-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-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

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

The default drop flow in lr_out_delivery stage is generated
for every router port of a logical router.  This results in the
lflow_table_add_lflow() to be called multiple times for the
same match and actions and the ovn_lflow to have multiple
dp_refcnts.  Fix this by generating this lflow only once for
each router.

Fixes: 27a92cc272aa ("northd: make default drops explicit")
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 northd/northd.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Comments

Han Zhou Feb. 13, 2024, 6:44 a.m. UTC | #1 
On Thu, Feb 8, 2024 at 1:49 PM <numans@ovn.org> wrote:
>
> From: Numan Siddique <numans@ovn.org>
>
> The default drop flow in lr_out_delivery stage is generated
> for every router port of a logical router.  This results in the
> lflow_table_add_lflow() to be called multiple times for the
> same match and actions and the ovn_lflow to have multiple
> dp_refcnts.  Fix this by generating this lflow only once for
> each router.
>
> Fixes: 27a92cc272aa ("northd: make default drops explicit")
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>  northd/northd.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index a174a4dcd1..a5d5e67117 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -13470,9 +13470,6 @@ build_egress_delivery_flows_for_lrouter_port(
>      ds_put_format(match, "outport == %s", op->json_key);
>      ovn_lflow_add(lflows, op->od, S_ROUTER_OUT_DELIVERY, 100,
>                    ds_cstr(match), "output;", lflow_ref);
> -
> -    ovn_lflow_add_default_drop(lflows, op->od, S_ROUTER_OUT_DELIVERY,
> -                               lflow_ref);
>  }
>
>  static void
> @@ -14838,9 +14835,9 @@ lrouter_check_nat_entry(const struct ovn_datapath
*od,
>  }
>
>  /* NAT, Defrag and load balancing. */
> -static void build_lr_nat_defrag_and_lb_default_flows(struct ovn_datapath
*od,
> -                                                struct lflow_table
*lflows,
> -                                                struct lflow_ref
*lflow_ref)
> +static void build_lr_nat_defrag_and_lb_default_flows(
> +    struct ovn_datapath *od, struct lflow_table *lflows,
> +    struct lflow_ref *lflow_ref)
>  {
>      ovs_assert(od->nbr);
>
> @@ -14866,6 +14863,12 @@ static void
build_lr_nat_defrag_and_lb_default_flows(struct ovn_datapath *od,
>       * packet would go through conntrack - which is not required. */
>      ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 120, "nd_ns", "next;",
>                    lflow_ref);
> +
> +    /* Default drop rule in lr_out_delivery stage.  See
> +     * build_egress_delivery_flows_for_lrouter_port() which adds a rule
> +     * for each router port. */
> +    ovn_lflow_add_default_drop(lflows, od, S_ROUTER_OUT_DELIVERY,
> +                               lflow_ref);
>  }
>
>  static void
> --
> 2.43.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Thanks Numan. The patch LGTM except that the function
build_lr_nat_defrag_and_lb_default_flows() doesn't seem to be the right
place to add the flow. I'd either add a new function, or just call the
ovn_lflow_add_default_drop directly in
build_lswitch_and_lrouter_iterate_by_lr for this flow.
With this addressed:

Acked-by: Han Zhou <hzhou@ovn.org>

Regards,
Han
Dumitru Ceara Feb. 14, 2024, 2:31 p.m. UTC | #2 
On 2/13/24 07:44, Han Zhou wrote:
> On Thu, Feb 8, 2024 at 1:49 PM <numans@ovn.org> wrote:
>>
>> From: Numan Siddique <numans@ovn.org>
>>
>> The default drop flow in lr_out_delivery stage is generated
>> for every router port of a logical router.  This results in the
>> lflow_table_add_lflow() to be called multiple times for the
>> same match and actions and the ovn_lflow to have multiple
>> dp_refcnts.  Fix this by generating this lflow only once for
>> each router.
>>
>> Fixes: 27a92cc272aa ("northd: make default drops explicit")
>> Signed-off-by: Numan Siddique <numans@ovn.org>
>> ---
>>  northd/northd.c | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index a174a4dcd1..a5d5e67117 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -13470,9 +13470,6 @@ build_egress_delivery_flows_for_lrouter_port(
>>      ds_put_format(match, "outport == %s", op->json_key);
>>      ovn_lflow_add(lflows, op->od, S_ROUTER_OUT_DELIVERY, 100,
>>                    ds_cstr(match), "output;", lflow_ref);
>> -
>> -    ovn_lflow_add_default_drop(lflows, op->od, S_ROUTER_OUT_DELIVERY,
>> -                               lflow_ref);
>>  }
>>
>>  static void
>> @@ -14838,9 +14835,9 @@ lrouter_check_nat_entry(const struct ovn_datapath
> *od,
>>  }
>>
>>  /* NAT, Defrag and load balancing. */
>> -static void build_lr_nat_defrag_and_lb_default_flows(struct ovn_datapath
> *od,
>> -                                                struct lflow_table
> *lflows,
>> -                                                struct lflow_ref
> *lflow_ref)
>> +static void build_lr_nat_defrag_and_lb_default_flows(
>> +    struct ovn_datapath *od, struct lflow_table *lflows,
>> +    struct lflow_ref *lflow_ref)
>>  {
>>      ovs_assert(od->nbr);
>>
>> @@ -14866,6 +14863,12 @@ static void
> build_lr_nat_defrag_and_lb_default_flows(struct ovn_datapath *od,
>>       * packet would go through conntrack - which is not required. */
>>      ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 120, "nd_ns", "next;",
>>                    lflow_ref);
>> +
>> +    /* Default drop rule in lr_out_delivery stage.  See
>> +     * build_egress_delivery_flows_for_lrouter_port() which adds a rule
>> +     * for each router port. */
>> +    ovn_lflow_add_default_drop(lflows, od, S_ROUTER_OUT_DELIVERY,
>> +                               lflow_ref);
>>  }
>>
>>  static void
>> --
>> 2.43.0
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
> Thanks Numan. The patch LGTM except that the function
> build_lr_nat_defrag_and_lb_default_flows() doesn't seem to be the right
> place to add the flow. I'd either add a new function, or just call the
> ovn_lflow_add_default_drop directly in
> build_lswitch_and_lrouter_iterate_by_lr for this flow.

I agree with Han, this should be moved somewhere else but that can
happen when the patch is merged.

> With this addressed:
> 
> Acked-by: Han Zhou <hzhou@ovn.org>
> 

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

Patch

diff --git a/northd/northd.c b/northd/northd.c
index a174a4dcd1..a5d5e67117 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -13470,9 +13470,6 @@  build_egress_delivery_flows_for_lrouter_port(
     ds_put_format(match, "outport == %s", op->json_key);
     ovn_lflow_add(lflows, op->od, S_ROUTER_OUT_DELIVERY, 100,
                   ds_cstr(match), "output;", lflow_ref);
-
-    ovn_lflow_add_default_drop(lflows, op->od, S_ROUTER_OUT_DELIVERY,
-                               lflow_ref);
 }
 
 static void
@@ -14838,9 +14835,9 @@  lrouter_check_nat_entry(const struct ovn_datapath *od,
 }
 
 /* NAT, Defrag and load balancing. */
-static void build_lr_nat_defrag_and_lb_default_flows(struct ovn_datapath *od,
-                                                struct lflow_table *lflows,
-                                                struct lflow_ref *lflow_ref)
+static void build_lr_nat_defrag_and_lb_default_flows(
+    struct ovn_datapath *od, struct lflow_table *lflows,
+    struct lflow_ref *lflow_ref)
 {
     ovs_assert(od->nbr);
 
@@ -14866,6 +14863,12 @@  static void build_lr_nat_defrag_and_lb_default_flows(struct ovn_datapath *od,
      * packet would go through conntrack - which is not required. */
     ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 120, "nd_ns", "next;",
                   lflow_ref);
+
+    /* Default drop rule in lr_out_delivery stage.  See
+     * build_egress_delivery_flows_for_lrouter_port() which adds a rule
+     * for each router port. */
+    ovn_lflow_add_default_drop(lflows, od, S_ROUTER_OUT_DELIVERY,
+                               lflow_ref);
 }
 
 static void