| Message ID | 20200511212152.826573-1-i.maximets@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,ovn] ovn-northd: Fix leak of lport addresses during DHCPv6 reply handling. | expand |
On 5/11/20 11:21 PM, Ilya Maximets wrote: > 'lrp_networks' never destroyed but constantly overwritten in a loop that > handles DHCPv6 replies. In some cases this point leaks several MB per > minute making ovn-northd to constantly growing its memory consumption: > > 399,820,764 bytes in 1,885,947 blocks are definitely lost in loss record 182 of 182 > at 0x4839748: malloc (vg_replace_malloc.c:308) > by 0x483BD63: realloc (vg_replace_malloc.c:836) > by 0x1E7BF8: xrealloc (util.c:149) > by 0x152723: add_ipv6_netaddr.isra.0 (ovn-util.c:55) > by 0x152F1C: extract_lrp_networks (ovn-util.c:275) > by 0x142EE2: build_lrouter_flows (ovn-northd.c:8607) > by 0x142EE2: build_lflows.isra.0 (ovn-northd.c:10296) > by 0x14E4F8: ovnnb_db_run (ovn-northd.c:11128) > by 0x14E4F8: ovn_db_run (ovn-northd.c:11672) > by 0x13304D: main (ovn-northd.c:12035) > > Additionally fixed similar leak in case of a duplicate logical router > port. > > Reported-by: Joe Talerico <jtaleric@redhat.com> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1827769 > Fixes: 5c1d2d230773 ("northd: Add logical flows for dhcpv6 pfd parsing") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > northd/ovn-northd.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 76b4a14ee..8200af24b 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -2129,6 +2129,7 @@ join_logical_ports(struct northd_context *ctx, > = VLOG_RATE_LIMIT_INIT(5, 1); > VLOG_WARN_RL(&rl, "duplicate logical router port %s", > nbrp->name); > + destroy_lport_addresses(&lrp_networks); > continue; > } > ovn_port_set_nb(op, NULL, nbrp); > @@ -8622,6 +8623,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 100, > ds_cstr(&match), ds_cstr(&actions)); > } > + destroy_lport_addresses(&lrp_networks); This is basically a mechanical fix, but maybe we could do something a bit more clever. I'm not sure why we're extracting 'lrp_networks' in this loop in the first place. Can we just use 'op->lrp_networks' like all other loops in this function? In this case we will not need to allocate and destroy at all. Best regards, Ilya Maximets.
On 5/11/20 5:31 PM, Ilya Maximets wrote: > On 5/11/20 11:21 PM, Ilya Maximets wrote: >> 'lrp_networks' never destroyed but constantly overwritten in a loop that >> handles DHCPv6 replies. In some cases this point leaks several MB per >> minute making ovn-northd to constantly growing its memory consumption: >> >> 399,820,764 bytes in 1,885,947 blocks are definitely lost in loss record 182 of 182 >> at 0x4839748: malloc (vg_replace_malloc.c:308) >> by 0x483BD63: realloc (vg_replace_malloc.c:836) >> by 0x1E7BF8: xrealloc (util.c:149) >> by 0x152723: add_ipv6_netaddr.isra.0 (ovn-util.c:55) >> by 0x152F1C: extract_lrp_networks (ovn-util.c:275) >> by 0x142EE2: build_lrouter_flows (ovn-northd.c:8607) >> by 0x142EE2: build_lflows.isra.0 (ovn-northd.c:10296) >> by 0x14E4F8: ovnnb_db_run (ovn-northd.c:11128) >> by 0x14E4F8: ovn_db_run (ovn-northd.c:11672) >> by 0x13304D: main (ovn-northd.c:12035) >> >> Additionally fixed similar leak in case of a duplicate logical router >> port. >> >> Reported-by: Joe Talerico <jtaleric@redhat.com> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1827769 >> Fixes: 5c1d2d230773 ("northd: Add logical flows for dhcpv6 pfd parsing") >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> northd/ovn-northd.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >> index 76b4a14ee..8200af24b 100644 >> --- a/northd/ovn-northd.c >> +++ b/northd/ovn-northd.c >> @@ -2129,6 +2129,7 @@ join_logical_ports(struct northd_context *ctx, >> = VLOG_RATE_LIMIT_INIT(5, 1); >> VLOG_WARN_RL(&rl, "duplicate logical router port %s", >> nbrp->name); >> + destroy_lport_addresses(&lrp_networks); >> continue; >> } >> ovn_port_set_nb(op, NULL, nbrp); >> @@ -8622,6 +8623,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, >> ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 100, >> ds_cstr(&match), ds_cstr(&actions)); >> } >> + destroy_lport_addresses(&lrp_networks); > > > This is basically a mechanical fix, but maybe we could do something a bit more clever. > I'm not sure why we're extracting 'lrp_networks' in this loop in the first place. > Can we just use 'op->lrp_networks' like all other loops in this function? In this > case we will not need to allocate and destroy at all. > > Best regards, Ilya Maximets. I agree. The sections below and above this one in build_router_lflows() use op->lrp_networks. There's no reason I can see to extract nbrp->lrp_networks. > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 5/12/20 1:52 AM, Mark Michelson wrote: > On 5/11/20 5:31 PM, Ilya Maximets wrote: >> On 5/11/20 11:21 PM, Ilya Maximets wrote: >>> 'lrp_networks' never destroyed but constantly overwritten in a loop that >>> handles DHCPv6 replies. In some cases this point leaks several MB per >>> minute making ovn-northd to constantly growing its memory consumption: >>> >>> 399,820,764 bytes in 1,885,947 blocks are definitely lost in loss record 182 of 182 >>> at 0x4839748: malloc (vg_replace_malloc.c:308) >>> by 0x483BD63: realloc (vg_replace_malloc.c:836) >>> by 0x1E7BF8: xrealloc (util.c:149) >>> by 0x152723: add_ipv6_netaddr.isra.0 (ovn-util.c:55) >>> by 0x152F1C: extract_lrp_networks (ovn-util.c:275) >>> by 0x142EE2: build_lrouter_flows (ovn-northd.c:8607) >>> by 0x142EE2: build_lflows.isra.0 (ovn-northd.c:10296) >>> by 0x14E4F8: ovnnb_db_run (ovn-northd.c:11128) >>> by 0x14E4F8: ovn_db_run (ovn-northd.c:11672) >>> by 0x13304D: main (ovn-northd.c:12035) >>> >>> Additionally fixed similar leak in case of a duplicate logical router >>> port. >>> >>> Reported-by: Joe Talerico <jtaleric@redhat.com> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1827769 >>> Fixes: 5c1d2d230773 ("northd: Add logical flows for dhcpv6 pfd parsing") >>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>> --- >>> northd/ovn-northd.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >>> index 76b4a14ee..8200af24b 100644 >>> --- a/northd/ovn-northd.c >>> +++ b/northd/ovn-northd.c >>> @@ -2129,6 +2129,7 @@ join_logical_ports(struct northd_context *ctx, >>> = VLOG_RATE_LIMIT_INIT(5, 1); >>> VLOG_WARN_RL(&rl, "duplicate logical router port %s", >>> nbrp->name); >>> + destroy_lport_addresses(&lrp_networks); >>> continue; >>> } >>> ovn_port_set_nb(op, NULL, nbrp); >>> @@ -8622,6 +8623,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, >>> ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 100, >>> ds_cstr(&match), ds_cstr(&actions)); >>> } >>> + destroy_lport_addresses(&lrp_networks); >> >> >> This is basically a mechanical fix, but maybe we could do something a bit more clever. >> I'm not sure why we're extracting 'lrp_networks' in this loop in the first place. >> Can we just use 'op->lrp_networks' like all other loops in this function? In this >> case we will not need to allocate and destroy at all. >> >> Best regards, Ilya Maximets. > > I agree. The sections below and above this one in build_router_lflows() use op->lrp_networks. There's no reason I can see to extract nbrp->lrp_networks. OK. I'll prepare v2 with this change.
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 76b4a14ee..8200af24b 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -2129,6 +2129,7 @@ join_logical_ports(struct northd_context *ctx, = VLOG_RATE_LIMIT_INIT(5, 1); VLOG_WARN_RL(&rl, "duplicate logical router port %s", nbrp->name); + destroy_lport_addresses(&lrp_networks); continue; } ovn_port_set_nb(op, NULL, nbrp); @@ -8622,6 +8623,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 100, ds_cstr(&match), ds_cstr(&actions)); } + destroy_lport_addresses(&lrp_networks); } /* Logical router ingress table 1: IP Input for IPv6. */
'lrp_networks' never destroyed but constantly overwritten in a loop that handles DHCPv6 replies. In some cases this point leaks several MB per minute making ovn-northd to constantly growing its memory consumption: 399,820,764 bytes in 1,885,947 blocks are definitely lost in loss record 182 of 182 at 0x4839748: malloc (vg_replace_malloc.c:308) by 0x483BD63: realloc (vg_replace_malloc.c:836) by 0x1E7BF8: xrealloc (util.c:149) by 0x152723: add_ipv6_netaddr.isra.0 (ovn-util.c:55) by 0x152F1C: extract_lrp_networks (ovn-util.c:275) by 0x142EE2: build_lrouter_flows (ovn-northd.c:8607) by 0x142EE2: build_lflows.isra.0 (ovn-northd.c:10296) by 0x14E4F8: ovnnb_db_run (ovn-northd.c:11128) by 0x14E4F8: ovn_db_run (ovn-northd.c:11672) by 0x13304D: main (ovn-northd.c:12035) Additionally fixed similar leak in case of a duplicate logical router port. Reported-by: Joe Talerico <jtaleric@redhat.com> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1827769 Fixes: 5c1d2d230773 ("northd: Add logical flows for dhcpv6 pfd parsing") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- northd/ovn-northd.c | 2 ++ 1 file changed, 2 insertions(+)