> -----Original Message----- > From: Ferruh Yigit [mailto:ferruh.yi...@intel.com] > Sent: Tuesday, April 7, 2020 8:55 PM > To: wangyunjian <wangyunj...@huawei.com>; dev@dpdk.org > Cc: keith.wi...@intel.com; Lilijun (Jerry) <jerry.lili...@huawei.com>; > xudingke > <xudin...@huawei.com>; sta...@dpdk.org > Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v3 2/5] net/tap: fix mbuf and mem > leak during queue release > > On 4/7/2020 5:22 AM, wangyunjian wrote: > > From: Yunjian Wang <wangyunj...@huawei.com> > > > > For the tap PMD, we should release mbufs and iovecs from the Rx queue > > when close or remove device. > > > > Fixes: 0781f5762cfe ("net/tap: support segmented mbufs") > > CC: sta...@dpdk.org > > > > Signed-off-by: Yunjian Wang <wangyunj...@huawei.com> > > --- > > drivers/net/tap/rte_eth_tap.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/net/tap/rte_eth_tap.c > > b/drivers/net/tap/rte_eth_tap.c index 4c4b6b0b2..a9ba0ca68 100644 > > --- a/drivers/net/tap/rte_eth_tap.c > > +++ b/drivers/net/tap/rte_eth_tap.c > > @@ -1022,6 +1022,7 @@ tap_dev_close(struct rte_eth_dev *dev) > > int i; > > struct pmd_internals *internals = dev->data->dev_private; > > struct pmd_process_private *process_private = dev->process_private; > > + struct rx_queue *rxq; > > > > tap_link_set_down(dev); > > tap_flow_flush(dev, NULL); > > @@ -1029,8 +1030,13 @@ tap_dev_close(struct rte_eth_dev *dev) > > > > for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) { > > if (process_private->rxq_fds[i] != -1) { > > + rxq = &internals->rxq[i]; > > close(process_private->rxq_fds[i]); > > process_private->rxq_fds[i] = -1; > > + rte_pktmbuf_free(rxq->pool); > > + rte_free(rxq->iovecs); > > + rxq->pool = NULL; > > + rxq->iovecs = NULL; > > } > > if (process_private->txq_fds[i] != -1) { > > close(process_private->txq_fds[i]); > > @@ -2399,6 +2405,7 @@ rte_pmd_tap_remove(struct rte_vdev_device > *dev) > > struct rte_eth_dev *eth_dev = NULL; > > struct pmd_internals *internals; > > struct pmd_process_private *process_private; > > + struct rx_queue *rxq; > > int i; > > > > /* find the ethdev entry */ > > @@ -2425,8 +2432,13 @@ rte_pmd_tap_remove(struct rte_vdev_device > *dev) > > } > > for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) { > > if (process_private->rxq_fds[i] != -1) { > > + rxq = &internals->rxq[i]; > > close(process_private->rxq_fds[i]); > > process_private->rxq_fds[i] = -1; > > + rte_pktmbuf_free(rxq->pool); > > + rte_free(rxq->iovecs); > > + rxq->pool = NULL; > > + rxq->iovecs = NULL; > > } > > if (process_private->txq_fds[i] != -1) { > > close(process_private->txq_fds[i]); > > > > Thanks for the fix, but instead of duplicating this for 'close()' & > 'remove()', can > 'remove()' call the 'close()'? > They should be doing almost same thing but I can see there is difference > between two, which may mean something is missed, unifying them fixes those > missed parts too. > Just a reminder that there can be tree valid path and should work fine: > 1- user 'close()' the PMD > 2- user directly 'remove()' the PMD > 3- user first 'close()', later 'remove()' the PMD > > Thanks, > Ferruh
OK, I got your point, will do this in v4. Thanks Yunjian
