Message ID | 20240208214920.12747-1-numans@ovn.org |
---|---|
State | Accepted |
Headers | show |
Series | northd memory and CPU increase fix due to lflow-mgr. | expand |
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 |
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
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 --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