| Message ID | 20230612083926.78585-1-zhangzujian.7@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] netdev-linux: fix memory leak in qos | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Mon, Jun 12, 2023 at 4:39 AM 张祖建 <zhangzujian.7@gmail.com> wrote: > > Reported by Valgrind: > > ==00:00:00:46.715 951138== 56 bytes in 1 blocks are definitely lost in loss record 363 of 408 > ==00:00:00:46.715 951138== at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) > ==00:00:00:46.715 951138== by 0x278E98: xmalloc__ (util.c:140) > ==00:00:00:46.715 951138== by 0x2998C2: netem_install__ (netdev-linux.c:4365) > ==00:00:00:46.715 951138== by 0x2998C2: netem_tc_install (netdev-linux.c:4454) > ==00:00:00:46.715 951138== by 0x29A5E8: netdev_linux_set_qos (netdev-linux.c:2994) > ==00:00:00:46.715 951138== by 0x1390AC: iface_configure_qos (bridge.c:4822) > ==00:00:00:46.715 951138== by 0x1390AC: bridge_reconfigure (bridge.c:929) > ==00:00:00:46.715 951138== by 0x13C3B8: bridge_run (bridge.c:3298) > ==00:00:00:46.715 951138== by 0x131314: main (ovs-vswitchd.c:129) > ==00:00:00:46.715 951138== > ==00:00:00:46.715 951138== 56 bytes in 1 blocks are definitely lost in loss record 364 of 408 > ==00:00:00:46.715 951138== at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) > ==00:00:00:46.715 951138== by 0x278E98: xmalloc__ (util.c:140) > ==00:00:00:46.715 951138== by 0x293688: netem_install__ (netdev-linux.c:4365) > ==00:00:00:46.715 951138== by 0x293688: netem_qdisc_set (netdev-linux.c:4504) > ==00:00:00:46.715 951138== by 0x29A696: netdev_linux_set_qos (netdev-linux.c:2982) > ==00:00:00:46.715 951138== by 0x1390AC: iface_configure_qos (bridge.c:4822) > ==00:00:00:46.715 951138== by 0x1390AC: bridge_reconfigure (bridge.c:929) > ==00:00:00:46.715 951138== by 0x13C3B8: bridge_run (bridge.c:3298) > ==00:00:00:46.715 951138== by 0x131314: main (ovs-vswitchd.c:129) > ==00:00:00:46.715 951138== > > Signed-off-by: 张祖建 <zhangzujian.7@gmail.com> Good find, it looks like some but not all of the QoS .set_qos() functions call install as well. htb_qdisc_set() doesn't, but many of the others do. Could we just delete those extra calls to the install functions in netem_qdisc_set() and the other qdisc_set functions? Thanks, Mike > --- > lib/netdev-linux.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 56 insertions(+) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index 49c74346a..43fd9ccfe 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -3764,6 +3764,13 @@ codel_install__(struct netdev *netdev_, uint32_t target, uint32_t limit, > struct netdev_linux *netdev = netdev_linux_cast(netdev_); > struct codel *codel; > > + if (netdev->tc) { > + if (netdev->tc->ops->tc_destroy) { > + netdev->tc->ops->tc_destroy(netdev->tc); > + } > + netdev->tc = NULL; > + } > + > codel = xmalloc(sizeof *codel); > tc_init(&codel->tc, &tc_ops_codel); > codel->target = target; > @@ -3975,6 +3982,13 @@ fqcodel_install__(struct netdev *netdev_, uint32_t target, uint32_t limit, > struct netdev_linux *netdev = netdev_linux_cast(netdev_); > struct fqcodel *fqcodel; > > + if (netdev->tc) { > + if (netdev->tc->ops->tc_destroy) { > + netdev->tc->ops->tc_destroy(netdev->tc); > + } > + netdev->tc = NULL; > + } > + > fqcodel = xmalloc(sizeof *fqcodel); > tc_init(&fqcodel->tc, &tc_ops_fqcodel); > fqcodel->target = target; > @@ -4197,6 +4211,13 @@ sfq_install__(struct netdev *netdev_, uint32_t quantum, uint32_t perturb) > struct netdev_linux *netdev = netdev_linux_cast(netdev_); > struct sfq *sfq; > > + if (netdev->tc) { > + if (netdev->tc->ops->tc_destroy) { > + netdev->tc->ops->tc_destroy(netdev->tc); > + } > + netdev->tc = NULL; > + } > + > sfq = xmalloc(sizeof *sfq); > tc_init(&sfq->tc, &tc_ops_sfq); > sfq->perturb = perturb; > @@ -4372,6 +4393,13 @@ netem_install__(struct netdev *netdev_, uint32_t latency, > struct netdev_linux *netdev = netdev_linux_cast(netdev_); > struct netem *netem; > > + if (netdev->tc) { > + if (netdev->tc->ops->tc_destroy) { > + netdev->tc->ops->tc_destroy(netdev->tc); > + } > + netdev->tc = NULL; > + } > + > netem = xmalloc(sizeof *netem); > tc_init(&netem->tc, &tc_ops_netem); > netem->latency = latency; > @@ -4562,6 +4590,13 @@ htb_install__(struct netdev *netdev_, uint64_t max_rate) > struct netdev_linux *netdev = netdev_linux_cast(netdev_); > struct htb *htb; > > + if (netdev->tc) { > + if (netdev->tc->ops->tc_destroy) { > + netdev->tc->ops->tc_destroy(netdev->tc); > + } > + netdev->tc = NULL; > + } > + > htb = xmalloc(sizeof *htb); > tc_init(&htb->tc, &tc_ops_htb); > htb->max_rate = max_rate; > @@ -5047,6 +5082,13 @@ hfsc_install__(struct netdev *netdev_, uint32_t max_rate) > struct netdev_linux *netdev = netdev_linux_cast(netdev_); > struct hfsc *hfsc; > > + if (netdev->tc) { > + if (netdev->tc->ops->tc_destroy) { > + netdev->tc->ops->tc_destroy(netdev->tc); > + } > + netdev->tc = NULL; > + } > + > hfsc = xmalloc(sizeof *hfsc); > tc_init(&hfsc->tc, &tc_ops_hfsc); > hfsc->max_rate = max_rate; > @@ -5518,6 +5560,13 @@ noop_install__(struct netdev *netdev_) > struct netdev_linux *netdev = netdev_linux_cast(netdev_); > static const struct tc tc = TC_INITIALIZER(&tc, &tc_ops_default); > > + if (netdev->tc) { > + if (netdev->tc->ops->tc_destroy) { > + netdev->tc->ops->tc_destroy(netdev->tc); > + } > + netdev->tc = NULL; > + } > + > netdev->tc = CONST_CAST(struct tc *, &tc); > } > > @@ -5553,6 +5602,13 @@ default_install__(struct netdev *netdev_) > struct netdev_linux *netdev = netdev_linux_cast(netdev_); > static const struct tc tc = TC_INITIALIZER(&tc, &tc_ops_default); > > + if (netdev->tc) { > + if (netdev->tc->ops->tc_destroy) { > + netdev->tc->ops->tc_destroy(netdev->tc); > + } > + netdev->tc = NULL; > + } > + > /* Nothing but a tc class implementation is allowed to write to a tc. This > * class never does that, so we can legitimately use a const tc object. */ > netdev->tc = CONST_CAST(struct tc *, &tc); > -- > 2.41.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 49c74346a..43fd9ccfe 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -3764,6 +3764,13 @@ codel_install__(struct netdev *netdev_, uint32_t target, uint32_t limit, struct netdev_linux *netdev = netdev_linux_cast(netdev_); struct codel *codel; + if (netdev->tc) { + if (netdev->tc->ops->tc_destroy) { + netdev->tc->ops->tc_destroy(netdev->tc); + } + netdev->tc = NULL; + } + codel = xmalloc(sizeof *codel); tc_init(&codel->tc, &tc_ops_codel); codel->target = target; @@ -3975,6 +3982,13 @@ fqcodel_install__(struct netdev *netdev_, uint32_t target, uint32_t limit, struct netdev_linux *netdev = netdev_linux_cast(netdev_); struct fqcodel *fqcodel; + if (netdev->tc) { + if (netdev->tc->ops->tc_destroy) { + netdev->tc->ops->tc_destroy(netdev->tc); + } + netdev->tc = NULL; + } + fqcodel = xmalloc(sizeof *fqcodel); tc_init(&fqcodel->tc, &tc_ops_fqcodel); fqcodel->target = target; @@ -4197,6 +4211,13 @@ sfq_install__(struct netdev *netdev_, uint32_t quantum, uint32_t perturb) struct netdev_linux *netdev = netdev_linux_cast(netdev_); struct sfq *sfq; + if (netdev->tc) { + if (netdev->tc->ops->tc_destroy) { + netdev->tc->ops->tc_destroy(netdev->tc); + } + netdev->tc = NULL; + } + sfq = xmalloc(sizeof *sfq); tc_init(&sfq->tc, &tc_ops_sfq); sfq->perturb = perturb; @@ -4372,6 +4393,13 @@ netem_install__(struct netdev *netdev_, uint32_t latency, struct netdev_linux *netdev = netdev_linux_cast(netdev_); struct netem *netem; + if (netdev->tc) { + if (netdev->tc->ops->tc_destroy) { + netdev->tc->ops->tc_destroy(netdev->tc); + } + netdev->tc = NULL; + } + netem = xmalloc(sizeof *netem); tc_init(&netem->tc, &tc_ops_netem); netem->latency = latency; @@ -4562,6 +4590,13 @@ htb_install__(struct netdev *netdev_, uint64_t max_rate) struct netdev_linux *netdev = netdev_linux_cast(netdev_); struct htb *htb; + if (netdev->tc) { + if (netdev->tc->ops->tc_destroy) { + netdev->tc->ops->tc_destroy(netdev->tc); + } + netdev->tc = NULL; + } + htb = xmalloc(sizeof *htb); tc_init(&htb->tc, &tc_ops_htb); htb->max_rate = max_rate; @@ -5047,6 +5082,13 @@ hfsc_install__(struct netdev *netdev_, uint32_t max_rate) struct netdev_linux *netdev = netdev_linux_cast(netdev_); struct hfsc *hfsc; + if (netdev->tc) { + if (netdev->tc->ops->tc_destroy) { + netdev->tc->ops->tc_destroy(netdev->tc); + } + netdev->tc = NULL; + } + hfsc = xmalloc(sizeof *hfsc); tc_init(&hfsc->tc, &tc_ops_hfsc); hfsc->max_rate = max_rate; @@ -5518,6 +5560,13 @@ noop_install__(struct netdev *netdev_) struct netdev_linux *netdev = netdev_linux_cast(netdev_); static const struct tc tc = TC_INITIALIZER(&tc, &tc_ops_default); + if (netdev->tc) { + if (netdev->tc->ops->tc_destroy) { + netdev->tc->ops->tc_destroy(netdev->tc); + } + netdev->tc = NULL; + } + netdev->tc = CONST_CAST(struct tc *, &tc); } @@ -5553,6 +5602,13 @@ default_install__(struct netdev *netdev_) struct netdev_linux *netdev = netdev_linux_cast(netdev_); static const struct tc tc = TC_INITIALIZER(&tc, &tc_ops_default); + if (netdev->tc) { + if (netdev->tc->ops->tc_destroy) { + netdev->tc->ops->tc_destroy(netdev->tc); + } + netdev->tc = NULL; + } + /* Nothing but a tc class implementation is allowed to write to a tc. This * class never does that, so we can legitimately use a const tc object. */ netdev->tc = CONST_CAST(struct tc *, &tc);
Reported by Valgrind: ==00:00:00:46.715 951138== 56 bytes in 1 blocks are definitely lost in loss record 363 of 408 ==00:00:00:46.715 951138== at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) ==00:00:00:46.715 951138== by 0x278E98: xmalloc__ (util.c:140) ==00:00:00:46.715 951138== by 0x2998C2: netem_install__ (netdev-linux.c:4365) ==00:00:00:46.715 951138== by 0x2998C2: netem_tc_install (netdev-linux.c:4454) ==00:00:00:46.715 951138== by 0x29A5E8: netdev_linux_set_qos (netdev-linux.c:2994) ==00:00:00:46.715 951138== by 0x1390AC: iface_configure_qos (bridge.c:4822) ==00:00:00:46.715 951138== by 0x1390AC: bridge_reconfigure (bridge.c:929) ==00:00:00:46.715 951138== by 0x13C3B8: bridge_run (bridge.c:3298) ==00:00:00:46.715 951138== by 0x131314: main (ovs-vswitchd.c:129) ==00:00:00:46.715 951138== ==00:00:00:46.715 951138== 56 bytes in 1 blocks are definitely lost in loss record 364 of 408 ==00:00:00:46.715 951138== at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) ==00:00:00:46.715 951138== by 0x278E98: xmalloc__ (util.c:140) ==00:00:00:46.715 951138== by 0x293688: netem_install__ (netdev-linux.c:4365) ==00:00:00:46.715 951138== by 0x293688: netem_qdisc_set (netdev-linux.c:4504) ==00:00:00:46.715 951138== by 0x29A696: netdev_linux_set_qos (netdev-linux.c:2982) ==00:00:00:46.715 951138== by 0x1390AC: iface_configure_qos (bridge.c:4822) ==00:00:00:46.715 951138== by 0x1390AC: bridge_reconfigure (bridge.c:929) ==00:00:00:46.715 951138== by 0x13C3B8: bridge_run (bridge.c:3298) ==00:00:00:46.715 951138== by 0x131314: main (ovs-vswitchd.c:129) ==00:00:00:46.715 951138== Signed-off-by: 张祖建 <zhangzujian.7@gmail.com> --- lib/netdev-linux.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)