DPDK mempools are freed when they are no longer needed.
This can happen when a port is removed or a port's mtu
is reconfigured so that a new mempool is used.

It is possible that an mbuf is attempted to be returned
to a freed mempool from NIC Tx queues and this can lead
to a segfault.

In order to prevent this, only free mempools when they
are not needed and have no in-use mbufs. As this might
not be possible immediately, create a free list of
mempools and sweep it anytime a port tries to get a
mempool.

Fixes: 8d38823bdf8b ("netdev-dpdk: fix memory leak")
Cc: mark.b.kavanag...@gmail.com
Signed-off-by: Kevin Traynor <ktray...@redhat.com>
---
 lib/netdev-dpdk.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 63 insertions(+), 5 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index ee39cbe..e6cd3f3 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -297,4 +297,14 @@ static struct ovs_mutex dpdk_mp_mutex 
OVS_ACQ_AFTER(dpdk_mutex)
     = OVS_MUTEX_INITIALIZER;
 
+/* Contains all 'struct dpdk_mp's. */
+static struct ovs_list dpdk_mp_free_list OVS_GUARDED_BY(dpdk_mp_mutex)
+    = OVS_LIST_INITIALIZER(&dpdk_mp_free_list);
+
+/* Wrapper for a mempool released but not yet freed. */
+struct dpdk_mp {
+     struct rte_mempool *mp;
+     struct ovs_list list_node OVS_GUARDED_BY(dpdk_mp_mutex);
+ };
+
 /* There should be one 'struct dpdk_tx_queue' created for
  * each cpu core. */
@@ -512,4 +522,38 @@ ovs_rte_pktmbuf_init(struct rte_mempool *mp OVS_UNUSED,
 }
 
+/* Free unused mempools. */
+static void
+dpdk_mp_sweep(void)
+{
+    struct dpdk_mp *dmp, *next;
+
+    ovs_mutex_lock(&dpdk_mp_mutex);
+    LIST_FOR_EACH_SAFE (dmp, next, list_node, &dpdk_mp_free_list) {
+        if (rte_mempool_full(dmp->mp)) {
+            VLOG_DBG("Freeing mempool \"%s\"", dmp->mp->name);
+            ovs_list_remove(&dmp->list_node);
+            rte_mempool_free(dmp->mp);
+            rte_free(dmp);
+        }
+    }
+    ovs_mutex_unlock(&dpdk_mp_mutex);
+}
+
+/* Ensure a mempool will not be freed. */
+static void
+dpdk_mp_do_not_free(struct rte_mempool *mp) OVS_REQUIRES(dpdk_mp_mutex)
+{
+    struct dpdk_mp *dmp, *next;
+
+    LIST_FOR_EACH_SAFE (dmp, next, list_node, &dpdk_mp_free_list) {
+        if (dmp->mp == mp) {
+            VLOG_DBG("Removing mempool \"%s\" from free list", dmp->mp->name);
+            ovs_list_remove(&dmp->list_node);
+            rte_free(dmp);
+            break;
+        }
+    }
+}
+
 /* Returns a valid pointer when either of the following is true:
  *  - a new mempool was just created;
@@ -578,4 +622,6 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu)
             VLOG_DBG("A mempool with name \"%s\" already exists at %p.",
                      mp_name, mp);
+            /* Ensure this reused mempool will not be freed. */
+            dpdk_mp_do_not_free(mp);
         } else {
             VLOG_ERR("Failed mempool \"%s\" create request of %u mbufs",
@@ -590,5 +636,5 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu)
 /* Release an existing mempool. */
 static void
-dpdk_mp_free(struct rte_mempool *mp)
+dpdk_mp_release(struct rte_mempool *mp)
 {
     if (!mp) {
@@ -597,6 +643,16 @@ dpdk_mp_free(struct rte_mempool *mp)
 
     ovs_mutex_lock(&dpdk_mp_mutex);
-    VLOG_DBG("Releasing \"%s\" mempool", mp->name);
-    rte_mempool_free(mp);
+    if (rte_mempool_full(mp)) {
+        VLOG_DBG("Freeing mempool \"%s\"", mp->name);
+        rte_mempool_free(mp);
+    } else {
+        struct dpdk_mp *dmp;
+
+        dmp = dpdk_rte_mzalloc(sizeof *dmp);
+        if (dmp) {
+            dmp->mp = mp;
+            ovs_list_push_back(&dpdk_mp_free_list, &dmp->list_node);
+        }
+    }
     ovs_mutex_unlock(&dpdk_mp_mutex);
 }
@@ -616,4 +672,6 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev)
     int ret = 0;
 
+    dpdk_mp_sweep();
+
     mp = dpdk_mp_create(dev, FRAME_LEN_TO_MTU(buf_size));
     if (!mp) {
@@ -628,5 +686,5 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev)
         if (dev->mp != mp) {
             /* A new mempool was created, release the previous one. */
-            dpdk_mp_free(dev->mp);
+            dpdk_mp_release(dev->mp);
         } else {
             ret = EEXIST;
@@ -1083,5 +1141,5 @@ common_destruct(struct netdev_dpdk *dev)
 {
     rte_free(dev->tx_q);
-    dpdk_mp_free(dev->mp);
+    dpdk_mp_release(dev->mp);
 
     ovs_list_remove(&dev->list_node);
-- 
1.8.3.1

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to