DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] kni: fix possible memory leak
@ 2017-04-18 14:21 Ferruh Yigit
  2017-04-20 23:23 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
  0 siblings, 1 reply; 2+ messages in thread
From: Ferruh Yigit @ 2017-04-18 14:21 UTC (permalink / raw)
  To: Ferruh Yigit, Thomas Monjalon; +Cc: dev, stable

alloc_q and rx_q fifos holds physical address of the mbufs, and not able
to free those mbufs explicitly.

But kernel thread reads from rx_q and puts used mbufs into free_q (with
their virtual addresses.) And kernel thread stopped when application
close the /dev/kni file on exit. So rx_q has time to be consumed by
kernel thread but leak is technically possible.

Another fifo, alloc_q has physical addresses too, but all those coming
from same mempool provided by application, when application quit, all
mempool already returned back, so this leak can be ignored.

Added check and wait logic for rx_q to be sure kernel consumed the fifo,
an error message printed after some ammount of wait, and an explicit
mempool free added for alloc_q.

Fixes: 8451269e6d7b ("kni: remove continuous memory restriction")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 lib/librte_kni/rte_kni.c      | 20 ++++++++++++++------
 lib/librte_kni/rte_kni_fifo.h |  9 +++++++++
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
index a80cefd..52fcd4b 100644
--- a/lib/librte_kni/rte_kni.c
+++ b/lib/librte_kni/rte_kni.c
@@ -452,16 +452,16 @@ kni_free_fifo(struct rte_kni_fifo *fifo)
 }
 
 static void
-kni_free_fifo_phy(struct rte_kni_fifo *fifo)
+kni_free_fifo_phy(struct rte_mempool *pktmbuf_pool, struct rte_kni_fifo *fifo)
 {
 	void *mbuf_phys;
 	int ret;
 
+	rte_mempool_free(pktmbuf_pool);
+
+	/* All mbufs alredy freed with rte_mempoll_free, just free the fifo */
 	do {
 		ret = kni_fifo_get(fifo, &mbuf_phys, 1);
-		/*
-		 * TODO: free mbufs
-		 */
 	} while (ret);
 }
 
@@ -470,6 +470,7 @@ rte_kni_release(struct rte_kni *kni)
 {
 	struct rte_kni_device_info dev_info;
 	uint32_t slot_id;
+	uint32_t retry = 5;
 
 	if (!kni || !kni->in_use)
 		return -1;
@@ -481,9 +482,16 @@ rte_kni_release(struct rte_kni *kni)
 	}
 
 	/* mbufs in all fifo should be released, except request/response */
+
+	/* wait until all rxq packets processed by kernel */
+	while (kni_fifo_count(kni->rx_q) && retry--)
+		usleep(1000);
+
+	if (kni_fifo_count(kni->rx_q))
+		RTE_LOG(ERR, KNI, "Fail to free all Rx-q items\n");
+
+	kni_free_fifo_phy(kni->pktmbuf_pool, kni->alloc_q);
 	kni_free_fifo(kni->tx_q);
-	kni_free_fifo_phy(kni->rx_q);
-	kni_free_fifo_phy(kni->alloc_q);
 	kni_free_fifo(kni->free_q);
 
 	slot_id = kni->slot_id;
diff --git a/lib/librte_kni/rte_kni_fifo.h b/lib/librte_kni/rte_kni_fifo.h
index 8cb8587..c7cd5c2 100644
--- a/lib/librte_kni/rte_kni_fifo.h
+++ b/lib/librte_kni/rte_kni_fifo.h
@@ -91,3 +91,12 @@ kni_fifo_get(struct rte_kni_fifo *fifo, void **data, unsigned num)
 	fifo->read = new_read;
 	return i;
 }
+
+/**
+ * Get the num of elements in the fifo
+ */
+static inline uint32_t
+kni_fifo_count(struct rte_kni_fifo *fifo)
+{
+	return (fifo->len + fifo->write - fifo->read) & (fifo->len - 1);
+}
-- 
2.9.3

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] kni: fix possible memory leak
  2017-04-18 14:21 [dpdk-dev] [PATCH] kni: fix possible memory leak Ferruh Yigit
@ 2017-04-20 23:23 ` Thomas Monjalon
  0 siblings, 0 replies; 2+ messages in thread
From: Thomas Monjalon @ 2017-04-20 23:23 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: stable, dev

18/04/2017 16:21, Ferruh Yigit:
> alloc_q and rx_q fifos holds physical address of the mbufs, and not able
> to free those mbufs explicitly.
> 
> But kernel thread reads from rx_q and puts used mbufs into free_q (with
> their virtual addresses.) And kernel thread stopped when application
> close the /dev/kni file on exit. So rx_q has time to be consumed by
> kernel thread but leak is technically possible.
> 
> Another fifo, alloc_q has physical addresses too, but all those coming
> from same mempool provided by application, when application quit, all
> mempool already returned back, so this leak can be ignored.
> 
> Added check and wait logic for rx_q to be sure kernel consumed the fifo,
> an error message printed after some ammount of wait, and an explicit
> mempool free added for alloc_q.
> 
> Fixes: 8451269e6d7b ("kni: remove continuous memory restriction")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied, thanks

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2017-04-20 23:23 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-18 14:21 [dpdk-dev] [PATCH] kni: fix possible memory leak Ferruh Yigit
2017-04-20 23:23 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).