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.
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.
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
