> On 5/13/22 12:54, Dumitru Ceara wrote:
> > On 5/3/22 21:20, Lorenzo Bianconi wrote:
> >> This is a preliminary patch to reduce load balancer logical flows
> >> computation cost for gw routers.
> >>
> >> Signed-off-by: Lorenzo Bianconi <[email protected]>
> >> ---
> >> northd/northd.c | 61 +++++++++++++++++++++++--------------------------
> >> 1 file changed, 29 insertions(+), 32 deletions(-)
> >>
> >> diff --git a/northd/northd.c b/northd/northd.c
> >> index e7901f348..53edf2efd 100644
> >> --- a/northd/northd.c
> >> +++ b/northd/northd.c
> >> @@ -10005,10 +10005,6 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> >> *lb_vip,
> >> char *est_match_p = est_match;
> >> const char *meter = NULL;
> >>
> >> - if (reject) {
> >> - meter = copp_meter_get(COPP_REJECT, od->nbr->copp,
> >> meter_groups);
> >> - }
> >> -
> >> if (sset_contains(&od->external_ips, lb_vip->vip_str)) {
> >> /* The load balancer vip is also present in the NAT entries.
> >> * So add a high priority lflow to advance the the packet
> >> @@ -10026,8 +10022,15 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> >> *lb_vip,
> >> &lb->nlb->header_);
> >> }
> >>
> >> - if (od->n_l3dgw_ports &&
> >> - (lb_vip->n_backends || !lb_vip->empty_backend_rej)) {
> >> + if (!od->n_l3dgw_ports) {
> >> + continue;
> >> + }
> >
> > This seems incorrect to me. With this change we now never add the
> > force_snat* flows for any gateway router.
> >
>
> Thanks for the offline clarification, Lorenzo! Sharing it here for
> reference: with the previous patches in the series, what's after this
> point in the body of the loop is only relevant for non-gw-routers. We
> have already processed the gw-routers (calls to
> build_gw_lrouter_nat_flows_for_lb()) so it's safe to skip them here.
>
> It would be great if you could add a comment about this.
ack, I will do in v2.
>
> Also, maybe we can split the loop further?
>
> The first part of the loop, before this if, applies to both gw and
> non-gw routers (flows in S_ROUTER_IN_UNSNAT). That can be factored out
> to a different loop.
ack, I will fix it in v2.
Regards,
Lorenzo
>
> Thanks!
>
> > Thanks,
> > Dumitru
> >
> >> +
> >> + if (reject) {
> >> + meter = copp_meter_get(COPP_REJECT, od->nbr->copp,
> >> meter_groups);
> >> + }
> >> +
> >> + if (lb_vip->n_backends || !lb_vip->empty_backend_rej) {
> >> new_match_p = xasprintf("%s && is_chassis_resident(%s)",
> >> new_match,
> >>
> >> od->l3dgw_ports[0]->cr_port->json_key);
> >> @@ -10037,33 +10040,27 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> >> *lb_vip,
> >> }
> >>
> >> if (snat_type == SKIP_SNAT) {
> >> - if (od->n_l3dgw_ports) {
> >> - ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT,
> >> prio,
> >> - new_match_p,
> >> skip_snat_new_action,
> >> - NULL, meter, &lb->nlb->header_);
> >> - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT,
> >> prio,
> >> - est_match_p, skip_snat_est_action,
> >> - &lb->nlb->header_);
> >> - }
> >> + ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, prio,
> >> + new_match_p, skip_snat_new_action,
> >> + NULL, meter, &lb->nlb->header_);
> >> + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, prio,
> >> + est_match_p, skip_snat_est_action,
> >> + &lb->nlb->header_);
> >> } else if (snat_type == FORCE_SNAT) {
> >> - if (od->n_l3dgw_ports) {
> >> - ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT,
> >> prio,
> >> - new_match_p, new_actions, NULL,
> >> - meter, &lb->nlb->header_);
> >> - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT,
> >> prio,
> >> - est_match_p,
> >> - "flags.force_snat_for_lb = 1;
> >> next;",
> >> - &lb->nlb->header_);
> >> - }
> >> + ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, prio,
> >> + new_match_p, new_actions, NULL,
> >> + meter, &lb->nlb->header_);
> >> + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, prio,
> >> + est_match_p,
> >> + "flags.force_snat_for_lb = 1; next;",
> >> + &lb->nlb->header_);
> >> } else if (snat_type == NO_FORCE_SNAT) {
> >> - if (od->n_l3dgw_ports) {
> >> - ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT,
> >> prio,
> >> - new_match_p, ds_cstr(action),
> >> NULL,
> >> - meter, &lb->nlb->header_);
> >> - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT,
> >> prio,
> >> - est_match_p, "next;",
> >> - &lb->nlb->header_);
> >> - }
> >> + ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, prio,
> >> + new_match_p, ds_cstr(action), NULL,
> >> + meter, &lb->nlb->header_);
> >> + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, prio,
> >> + est_match_p, "next;",
> >> + &lb->nlb->header_);
> >> }
> >>
> >> if (new_match_p != new_match) {
> >> @@ -10073,7 +10070,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> >> *lb_vip,
> >> free(est_match_p);
> >> }
> >>
> >> - if (!od->n_l3dgw_ports || !lb_vip->n_backends) {
> >> + if (!lb_vip->n_backends) {
> >> continue;
> >> }
> >>
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
