| Message ID | 20240208214926.12763-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> > > If an SNAT external_ip belongs to the router IP, then there > is no need to generate ARP request responder flows in > build_lswitch_rport_arp_req_flows_for_lbnats() as these flows > for router ips are generated by build_lswitch_rport_arp_req_flows(). > Otherwise 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. > > Fixes: fe1c5df98b6f ("northd: forward arp request to lrp snat on.") > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > northd/en-lr-nat.c | 6 ++++++ > northd/en-lr-nat.h | 2 ++ > northd/northd.c | 28 ++++++++++++++++++++++++---- > northd/northd.h | 1 + > 4 files changed, 33 insertions(+), 4 deletions(-) > > diff --git a/northd/en-lr-nat.c b/northd/en-lr-nat.c > index 7664ec0ca9..ad11025c69 100644 > --- a/northd/en-lr-nat.c > +++ b/northd/en-lr-nat.c > @@ -288,6 +288,8 @@ lr_nat_record_init(struct lr_nat_record *lrnat_rec, > struct ovn_nat *nat_entry = &lrnat_rec->nat_entries[i]; > > nat_entry->nb = nat; > + nat_entry->is_router_ip = false; > + > if (!extract_ip_addresses(nat->external_ip, > &nat_entry->ext_addrs) || > !nat_entry_is_valid(nat_entry)) { > @@ -302,6 +304,10 @@ lr_nat_record_init(struct lr_nat_record *lrnat_rec, > > /* If this is a SNAT rule add the IP to the set of unique SNAT IPs. */ > if (!strcmp(nat->type, "snat")) { > + if (sset_contains(&od->router_ips, nat->external_ip)) { > + nat_entry->is_router_ip = true; > + } > + > if (!nat_entry_is_v6(nat_entry)) { > snat_ip_add(lrnat_rec, > nat_entry->ext_addrs.ipv4_addrs[0].addr_s, > diff --git a/northd/en-lr-nat.h b/northd/en-lr-nat.h > index 16b166ee05..6d3b2b6d65 100644 > --- a/northd/en-lr-nat.h > +++ b/northd/en-lr-nat.h > @@ -37,6 +37,8 @@ struct ovn_nat { > * list of nat entries. Currently > * only used for SNAT. > */ > + bool is_router_ip; /* Indicates if the NAT external_ip is also one of > + * router's lrp ip. Initialized only for SNAT. */ > }; > > /* Stores the list of SNAT entries referencing a unique SNAT IP address. > diff --git a/northd/northd.c b/northd/northd.c > index a5d5e67117..c59aa8d304 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -431,6 +431,7 @@ ovn_datapath_create(struct hmap *datapaths, const struct uuid *key, > hmap_insert(datapaths, &od->key_node, uuid_hash(&od->key)); > od->lr_group = NULL; > hmap_init(&od->ports); > + sset_init(&od->router_ips); > return od; > } > > @@ -459,6 +460,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct ovn_datapath *od) > free(od->l3dgw_ports); > destroy_mcast_info_for_datapath(od); > destroy_ports_for_datapath(od); > + sset_destroy(&od->router_ips); > free(od); > } > } > @@ -2190,6 +2192,19 @@ join_logical_ports(const struct sbrec_port_binding_table *sbrec_pb_table, > > op->lrp_networks = lrp_networks; > op->od = od; > + > + for (size_t j = 0; j < op->lrp_networks.n_ipv4_addrs; j++) { > + sset_add(&op->od->router_ips, > + op->lrp_networks.ipv4_addrs[j].addr_s); > + } > + for (size_t j = 0; j < op->lrp_networks.n_ipv6_addrs; j++) { > + /* Exclude the LLA. */ > + if (!in6_is_lla(&op->lrp_networks.ipv6_addrs[j].addr)) { > + sset_add(&op->od->router_ips, > + op->lrp_networks.ipv6_addrs[j].addr_s); > + } > + } > + > hmap_insert(&od->ports, &op->dp_node, > hmap_node_hash(&op->key_node)); > > @@ -8302,22 +8317,27 @@ build_lswitch_rport_arp_req_flows_for_lbnats( > struct ovn_nat *nat_entry = > CONTAINER_OF(ovs_list_front(&snat_ip->snat_entries), > struct ovn_nat, ext_addr_list_node); > + if (nat_entry->is_router_ip) { > + /* If its a router ip, then there is no need to add the ARP > + * request forwarder flows as it will be added by > + * build_lswitch_rport_arp_req_flows(). */ > + continue; > + } > + > const struct nbrec_nat *nat = nat_entry->nb; > > /* Check if the ovn port has a network configured on which we could > * expect ARP requests/NS for the SNAT external_ip. > */ > if (nat_entry_is_v6(nat_entry)) { > - if (!lr_stateful_rec || > - !sset_contains(&lr_stateful_rec->lb_ips->ips_v6, > + if (!sset_contains(&lr_stateful_rec->lb_ips->ips_v6, > nat->external_ip)) { > build_lswitch_rport_arp_req_flow( > nat->external_ip, AF_INET6, sw_op, sw_od, 80, lflows, > stage_hint, lflow_ref); > } > } else { > - if (!lr_stateful_rec || > - !sset_contains(&lr_stateful_rec->lb_ips->ips_v4, > + if (!sset_contains(&lr_stateful_rec->lb_ips->ips_v4, > nat->external_ip)) { > build_lswitch_rport_arp_req_flow( > nat->external_ip, AF_INET, sw_op, sw_od, 80, lflows, > diff --git a/northd/northd.h b/northd/northd.h > index b5c175929e..3f1cd83413 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -293,6 +293,7 @@ struct ovn_datapath { > struct ovn_datapath **ls_peers; > size_t n_ls_peers; > size_t n_allocated_ls_peers; > + struct sset router_ips; /* Router port IPs except the IPv6 LLAs. */ > > /* Logical switch data. */ > struct ovn_port **router_ports; > -- > 2.43.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Thanks Numan. Acked-by: Han Zhou <hzhou@ovn.org>
On 2/13/24 08:04, Han Zhou wrote: > On Thu, Feb 8, 2024 at 1:49 PM <numans@ovn.org> wrote: >> >> From: Numan Siddique <numans@ovn.org> >> >> If an SNAT external_ip belongs to the router IP, then there >> is no need to generate ARP request responder flows in >> build_lswitch_rport_arp_req_flows_for_lbnats() as these flows >> for router ips are generated by build_lswitch_rport_arp_req_flows(). >> Otherwise 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. >> >> Fixes: fe1c5df98b6f ("northd: forward arp request to lrp snat on.") >> Signed-off-by: Numan Siddique <numans@ovn.org> >> --- >> northd/en-lr-nat.c | 6 ++++++ >> northd/en-lr-nat.h | 2 ++ >> northd/northd.c | 28 ++++++++++++++++++++++++---- >> northd/northd.h | 1 + >> 4 files changed, 33 insertions(+), 4 deletions(-) >> >> diff --git a/northd/en-lr-nat.c b/northd/en-lr-nat.c >> index 7664ec0ca9..ad11025c69 100644 >> --- a/northd/en-lr-nat.c >> +++ b/northd/en-lr-nat.c >> @@ -288,6 +288,8 @@ lr_nat_record_init(struct lr_nat_record *lrnat_rec, >> struct ovn_nat *nat_entry = &lrnat_rec->nat_entries[i]; >> >> nat_entry->nb = nat; >> + nat_entry->is_router_ip = false; >> + >> if (!extract_ip_addresses(nat->external_ip, >> &nat_entry->ext_addrs) || >> !nat_entry_is_valid(nat_entry)) { >> @@ -302,6 +304,10 @@ lr_nat_record_init(struct lr_nat_record *lrnat_rec, >> >> /* If this is a SNAT rule add the IP to the set of unique SNAT > IPs. */ >> if (!strcmp(nat->type, "snat")) { >> + if (sset_contains(&od->router_ips, nat->external_ip)) { >> + nat_entry->is_router_ip = true; >> + } >> + >> if (!nat_entry_is_v6(nat_entry)) { >> snat_ip_add(lrnat_rec, >> nat_entry->ext_addrs.ipv4_addrs[0].addr_s, >> diff --git a/northd/en-lr-nat.h b/northd/en-lr-nat.h >> index 16b166ee05..6d3b2b6d65 100644 >> --- a/northd/en-lr-nat.h >> +++ b/northd/en-lr-nat.h >> @@ -37,6 +37,8 @@ struct ovn_nat { >> * list of nat entries. Currently >> * only used for SNAT. >> */ >> + bool is_router_ip; /* Indicates if the NAT external_ip is also one of >> + * router's lrp ip. Initialized only for SNAT. */ Nit: we always initialize it, we just don't set it to true unless this is a SNAT rule which uses a router IP. I'd change this to "Can be 'true' only for SNAT." >> }; >> >> /* Stores the list of SNAT entries referencing a unique SNAT IP address. >> diff --git a/northd/northd.c b/northd/northd.c >> index a5d5e67117..c59aa8d304 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -431,6 +431,7 @@ ovn_datapath_create(struct hmap *datapaths, const > struct uuid *key, >> hmap_insert(datapaths, &od->key_node, uuid_hash(&od->key)); >> od->lr_group = NULL; >> hmap_init(&od->ports); >> + sset_init(&od->router_ips); >> return od; >> } >> >> @@ -459,6 +460,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct > ovn_datapath *od) >> free(od->l3dgw_ports); >> destroy_mcast_info_for_datapath(od); >> destroy_ports_for_datapath(od); >> + sset_destroy(&od->router_ips); >> free(od); >> } >> } >> @@ -2190,6 +2192,19 @@ join_logical_ports(const struct > sbrec_port_binding_table *sbrec_pb_table, >> >> op->lrp_networks = lrp_networks; >> op->od = od; >> + >> + for (size_t j = 0; j < op->lrp_networks.n_ipv4_addrs; j++) { >> + sset_add(&op->od->router_ips, >> + op->lrp_networks.ipv4_addrs[j].addr_s); >> + } >> + for (size_t j = 0; j < op->lrp_networks.n_ipv6_addrs; j++) { >> + /* Exclude the LLA. */ >> + if (!in6_is_lla(&op->lrp_networks.ipv6_addrs[j].addr)) { >> + sset_add(&op->od->router_ips, >> + op->lrp_networks.ipv6_addrs[j].addr_s); >> + } >> + } >> + >> hmap_insert(&od->ports, &op->dp_node, >> hmap_node_hash(&op->key_node)); >> >> @@ -8302,22 +8317,27 @@ build_lswitch_rport_arp_req_flows_for_lbnats( >> struct ovn_nat *nat_entry = >> CONTAINER_OF(ovs_list_front(&snat_ip->snat_entries), >> struct ovn_nat, ext_addr_list_node); >> + if (nat_entry->is_router_ip) { >> + /* If its a router ip, then there is no need to add the ARP >> + * request forwarder flows as it will be added by >> + * build_lswitch_rport_arp_req_flows(). */ >> + continue; >> + } >> + >> const struct nbrec_nat *nat = nat_entry->nb; >> >> /* Check if the ovn port has a network configured on which we > could >> * expect ARP requests/NS for the SNAT external_ip. >> */ >> if (nat_entry_is_v6(nat_entry)) { >> - if (!lr_stateful_rec || >> - !sset_contains(&lr_stateful_rec->lb_ips->ips_v6, >> + if (!sset_contains(&lr_stateful_rec->lb_ips->ips_v6, >> nat->external_ip)) { >> build_lswitch_rport_arp_req_flow( >> nat->external_ip, AF_INET6, sw_op, sw_od, 80, lflows, >> stage_hint, lflow_ref); >> } >> } else { >> - if (!lr_stateful_rec || >> - !sset_contains(&lr_stateful_rec->lb_ips->ips_v4, >> + if (!sset_contains(&lr_stateful_rec->lb_ips->ips_v4, >> nat->external_ip)) { >> build_lswitch_rport_arp_req_flow( >> nat->external_ip, AF_INET, sw_op, sw_od, 80, lflows, >> diff --git a/northd/northd.h b/northd/northd.h >> index b5c175929e..3f1cd83413 100644 >> --- a/northd/northd.h >> +++ b/northd/northd.h >> @@ -293,6 +293,7 @@ struct ovn_datapath { >> struct ovn_datapath **ls_peers; >> size_t n_ls_peers; >> size_t n_allocated_ls_peers; >> + struct sset router_ips; /* Router port IPs except the IPv6 LLAs. */ >> >> /* Logical switch data. */ >> struct ovn_port **router_ports; >> -- >> 2.43.0 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks Numan. > Acked-by: Han Zhou <hzhou@ovn.org> Looks good to me too: Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks!
diff --git a/northd/en-lr-nat.c b/northd/en-lr-nat.c index 7664ec0ca9..ad11025c69 100644 --- a/northd/en-lr-nat.c +++ b/northd/en-lr-nat.c @@ -288,6 +288,8 @@ lr_nat_record_init(struct lr_nat_record *lrnat_rec, struct ovn_nat *nat_entry = &lrnat_rec->nat_entries[i]; nat_entry->nb = nat; + nat_entry->is_router_ip = false; + if (!extract_ip_addresses(nat->external_ip, &nat_entry->ext_addrs) || !nat_entry_is_valid(nat_entry)) { @@ -302,6 +304,10 @@ lr_nat_record_init(struct lr_nat_record *lrnat_rec, /* If this is a SNAT rule add the IP to the set of unique SNAT IPs. */ if (!strcmp(nat->type, "snat")) { + if (sset_contains(&od->router_ips, nat->external_ip)) { + nat_entry->is_router_ip = true; + } + if (!nat_entry_is_v6(nat_entry)) { snat_ip_add(lrnat_rec, nat_entry->ext_addrs.ipv4_addrs[0].addr_s, diff --git a/northd/en-lr-nat.h b/northd/en-lr-nat.h index 16b166ee05..6d3b2b6d65 100644 --- a/northd/en-lr-nat.h +++ b/northd/en-lr-nat.h @@ -37,6 +37,8 @@ struct ovn_nat { * list of nat entries. Currently * only used for SNAT. */ + bool is_router_ip; /* Indicates if the NAT external_ip is also one of + * router's lrp ip. Initialized only for SNAT. */ }; /* Stores the list of SNAT entries referencing a unique SNAT IP address. diff --git a/northd/northd.c b/northd/northd.c index a5d5e67117..c59aa8d304 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -431,6 +431,7 @@ ovn_datapath_create(struct hmap *datapaths, const struct uuid *key, hmap_insert(datapaths, &od->key_node, uuid_hash(&od->key)); od->lr_group = NULL; hmap_init(&od->ports); + sset_init(&od->router_ips); return od; } @@ -459,6 +460,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct ovn_datapath *od) free(od->l3dgw_ports); destroy_mcast_info_for_datapath(od); destroy_ports_for_datapath(od); + sset_destroy(&od->router_ips); free(od); } } @@ -2190,6 +2192,19 @@ join_logical_ports(const struct sbrec_port_binding_table *sbrec_pb_table, op->lrp_networks = lrp_networks; op->od = od; + + for (size_t j = 0; j < op->lrp_networks.n_ipv4_addrs; j++) { + sset_add(&op->od->router_ips, + op->lrp_networks.ipv4_addrs[j].addr_s); + } + for (size_t j = 0; j < op->lrp_networks.n_ipv6_addrs; j++) { + /* Exclude the LLA. */ + if (!in6_is_lla(&op->lrp_networks.ipv6_addrs[j].addr)) { + sset_add(&op->od->router_ips, + op->lrp_networks.ipv6_addrs[j].addr_s); + } + } + hmap_insert(&od->ports, &op->dp_node, hmap_node_hash(&op->key_node)); @@ -8302,22 +8317,27 @@ build_lswitch_rport_arp_req_flows_for_lbnats( struct ovn_nat *nat_entry = CONTAINER_OF(ovs_list_front(&snat_ip->snat_entries), struct ovn_nat, ext_addr_list_node); + if (nat_entry->is_router_ip) { + /* If its a router ip, then there is no need to add the ARP + * request forwarder flows as it will be added by + * build_lswitch_rport_arp_req_flows(). */ + continue; + } + const struct nbrec_nat *nat = nat_entry->nb; /* Check if the ovn port has a network configured on which we could * expect ARP requests/NS for the SNAT external_ip. */ if (nat_entry_is_v6(nat_entry)) { - if (!lr_stateful_rec || - !sset_contains(&lr_stateful_rec->lb_ips->ips_v6, + if (!sset_contains(&lr_stateful_rec->lb_ips->ips_v6, nat->external_ip)) { build_lswitch_rport_arp_req_flow( nat->external_ip, AF_INET6, sw_op, sw_od, 80, lflows, stage_hint, lflow_ref); } } else { - if (!lr_stateful_rec || - !sset_contains(&lr_stateful_rec->lb_ips->ips_v4, + if (!sset_contains(&lr_stateful_rec->lb_ips->ips_v4, nat->external_ip)) { build_lswitch_rport_arp_req_flow( nat->external_ip, AF_INET, sw_op, sw_od, 80, lflows, diff --git a/northd/northd.h b/northd/northd.h index b5c175929e..3f1cd83413 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -293,6 +293,7 @@ struct ovn_datapath { struct ovn_datapath **ls_peers; size_t n_ls_peers; size_t n_allocated_ls_peers; + struct sset router_ips; /* Router port IPs except the IPv6 LLAs. */ /* Logical switch data. */ struct ovn_port **router_ports;