On Thu, Sep 17, 2020 at 5:51 AM Dumitru Ceara <dce...@redhat.com> wrote: > > Avoid reparsing the *_force_snat_ip addresses for every logical router port. > These addresses are defined once for the whole router. >
The commit message is a little misleading. Originally it wasn't reparsing for every logical router port. It was per-router. This patch avoids one extra parsing. In addition, another minor comment is that the function name "empty_lport_addresses" may be "lport_addresses_is_empty" to follow the naming convention of a lot of xxx_is_empty() function names in OVN/OVS. With these addressed: Acked-by: Han Zhou <hz...@ovn.org> > Signed-off-by: Dumitru Ceara <dce...@redhat.com> > --- > lib/ovn-util.c | 6 ++++ > lib/ovn-util.h | 2 + > northd/ovn-northd.c | 69 ++++++++++++++++++++++++--------------------------- > 3 files changed, 40 insertions(+), 37 deletions(-) > > diff --git a/lib/ovn-util.c b/lib/ovn-util.c > index cdb5e18..a8cf6c9 100644 > --- a/lib/ovn-util.c > +++ b/lib/ovn-util.c > @@ -320,6 +320,12 @@ extract_sbrec_binding_first_mac(const struct sbrec_port_binding *binding, > return ret; > } > > +bool > +empty_lport_addresses(struct lport_addresses *laddrs) > +{ > + return !laddrs->n_ipv4_addrs && !laddrs->n_ipv6_addrs; > +} > + > void > destroy_lport_addresses(struct lport_addresses *laddrs) > { > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > index d9aadcb..3343f28 100644 > --- a/lib/ovn-util.h > +++ b/lib/ovn-util.h > @@ -74,7 +74,7 @@ bool extract_lrp_networks(const struct nbrec_logical_router_port *, > struct lport_addresses *); > bool extract_sbrec_binding_first_mac(const struct sbrec_port_binding *binding, > struct eth_addr *ea); > - > +bool empty_lport_addresses(struct lport_addresses *); > void destroy_lport_addresses(struct lport_addresses *); > > char *alloc_nat_zone_key(const struct uuid *key, const char *type); > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index f79ed99..43bd7b5 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -625,6 +625,8 @@ struct ovn_datapath { > > /* SNAT IPs used by the router. */ > struct sset snat_ips; > + struct lport_addresses dnat_force_snat_addrs; > + struct lport_addresses lb_force_snat_addrs; > > struct ovn_port **localnet_ports; > size_t n_localnet_ports; > @@ -670,32 +672,31 @@ nat_entry_is_v6(const struct ovn_nat *nat_entry) > static void > init_nat_entries(struct ovn_datapath *od) > { > - struct lport_addresses snat_addrs; > - > if (!od->nbr) { > return; > } > > sset_init(&od->snat_ips); > - if (get_force_snat_ip(od, "dnat", &snat_addrs)) { > - if (snat_addrs.n_ipv4_addrs) { > - sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s); > + if (get_force_snat_ip(od, "dnat", &od->dnat_force_snat_addrs)) { > + if (&od->dnat_force_snat_addrs.n_ipv4_addrs) { > + sset_add(&od->snat_ips, > + od->dnat_force_snat_addrs.ipv4_addrs[0].addr_s); > } > - if (snat_addrs.n_ipv6_addrs) { > - sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s); > + if (&od->dnat_force_snat_addrs.n_ipv6_addrs) { > + sset_add(&od->snat_ips, > + od->dnat_force_snat_addrs.ipv6_addrs[0].addr_s); > } > - destroy_lport_addresses(&snat_addrs); > } > > - memset(&snat_addrs, 0, sizeof(snat_addrs)); > - if (get_force_snat_ip(od, "lb", &snat_addrs)) { > - if (snat_addrs.n_ipv4_addrs) { > - sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s); > + if (get_force_snat_ip(od, "lb", &od->lb_force_snat_addrs)) { > + if (od->lb_force_snat_addrs.n_ipv4_addrs) { > + sset_add(&od->snat_ips, > + od->lb_force_snat_addrs.ipv4_addrs[0].addr_s); > } > - if (snat_addrs.n_ipv6_addrs) { > - sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s); > + if (od->lb_force_snat_addrs.n_ipv6_addrs) { > + sset_add(&od->snat_ips, > + od->lb_force_snat_addrs.ipv6_addrs[0].addr_s); > } > - destroy_lport_addresses(&snat_addrs); > } > > if (!od->nbr->n_nat) { > @@ -736,6 +737,9 @@ destroy_nat_entries(struct ovn_datapath *od) > } > > sset_destroy(&od->snat_ips); > + destroy_lport_addresses(&od->dnat_force_snat_addrs); > + destroy_lport_addresses(&od->lb_force_snat_addrs); > + > for (size_t i = 0; i < od->nbr->n_nat; i++) { > destroy_lport_addresses(&od->nat_entries[i].ext_addrs); > } > @@ -9477,12 +9481,10 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > > struct sset nat_entries = SSET_INITIALIZER(&nat_entries); > > - struct lport_addresses dnat_force_snat_addrs; > - struct lport_addresses lb_force_snat_addrs; > - bool dnat_force_snat_ip = get_force_snat_ip(od, "dnat", > - &dnat_force_snat_addrs); > - bool lb_force_snat_ip = get_force_snat_ip(od, "lb", > - &lb_force_snat_addrs); > + bool dnat_force_snat_ip = > + !empty_lport_addresses(&od->dnat_force_snat_addrs); > + bool lb_force_snat_ip = > + !empty_lport_addresses(&od->lb_force_snat_addrs); > > for (int i = 0; i < od->nbr->n_nat; i++) { > const struct nbrec_nat *nat; > @@ -9982,23 +9984,25 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > /* Handle force SNAT options set in the gateway router. */ > if (!od->l3dgw_port) { > if (dnat_force_snat_ip) { > - if (dnat_force_snat_addrs.n_ipv4_addrs) { > + if (od->dnat_force_snat_addrs.n_ipv4_addrs) { > build_lrouter_force_snat_flows(lflows, od, "4", > - dnat_force_snat_addrs.ipv4_addrs[0].addr_s, "dnat"); > + od->dnat_force_snat_addrs.ipv4_addrs[0].addr_s, > + "dnat"); > } > - if (dnat_force_snat_addrs.n_ipv6_addrs) { > + if (od->dnat_force_snat_addrs.n_ipv6_addrs) { > build_lrouter_force_snat_flows(lflows, od, "6", > - dnat_force_snat_addrs.ipv6_addrs[0].addr_s, "dnat"); > + od->dnat_force_snat_addrs.ipv6_addrs[0].addr_s, > + "dnat"); > } > } > if (lb_force_snat_ip) { > - if (lb_force_snat_addrs.n_ipv4_addrs) { > + if (od->lb_force_snat_addrs.n_ipv4_addrs) { > build_lrouter_force_snat_flows(lflows, od, "4", > - lb_force_snat_addrs.ipv4_addrs[0].addr_s, "lb"); > + od->lb_force_snat_addrs.ipv4_addrs[0].addr_s, "lb"); > } > - if (lb_force_snat_addrs.n_ipv6_addrs) { > + if (od->lb_force_snat_addrs.n_ipv6_addrs) { > build_lrouter_force_snat_flows(lflows, od, "6", > - lb_force_snat_addrs.ipv6_addrs[0].addr_s, "lb"); > + od->lb_force_snat_addrs.ipv6_addrs[0].addr_s, "lb"); > } > } > > @@ -10015,13 +10019,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > "ip", "flags.loopback = 1; ct_dnat;"); > } > > - if (dnat_force_snat_ip) { > - destroy_lport_addresses(&dnat_force_snat_addrs); > - } > - if (lb_force_snat_ip) { > - destroy_lport_addresses(&lb_force_snat_addrs); > - } > - > /* Load balancing and packet defrag are only valid on > * Gateway routers or router with gateway port. */ > if (!smap_get(&od->nbr->options, "chassis") && !od->l3dgw_port) { > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev