Merge branch 'dpdk_merge' of https://github.com/istokes/ovs into HEAD

This patchset specifically deals with fixing a bug related to mempool
management for DPDK and vhost ports.

Patchset has been reviewed and Acked by a number of trusted contributors.

Patchset has been validated follows:

Clang/Sparse compilation and analysis.
OVS Unit tests
Vsperf Performance tests for OVS DPDK for p2p, pvp, pvvp,pvpv
Vpserf integration tests for OVS DPDK for p2p, pvp, pvvp.
MTU tests for both physical and vhost DPDK port types.
Multi queue for DPDK and vhost port types with both kernel virtio and DPDK
virtio interfaces in the guest.

Signed-off-by: Ben Pfaff <blp@ovn.org>

1 files changed, 59 insertions, 37 deletions

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index c60f46f87..82652f0c0 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -308,7 +308,7 @@ struct dpdk_mp {
     int mtu;
     int socket_id;
     char if_name[IFNAMSIZ];
-    unsigned mp_size;
+    unsigned n_mbufs;   /* Number of mbufs inside the mempool. */
     struct ovs_list list_node OVS_GUARDED_BY(dpdk_mp_mutex);
 };
 
@@ -499,48 +499,55 @@ dpdk_mp_name(struct dpdk_mp *dmp)
 {
     uint32_t h = hash_string(dmp->if_name, 0);
     char *mp_name = xcalloc(RTE_MEMPOOL_NAMESIZE, sizeof *mp_name);
-    int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%u",
-                       h, dmp->mtu, dmp->mp_size);
+    int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
+                       h, dmp->socket_id, dmp->mtu, dmp->n_mbufs);
     if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
+        VLOG_DBG("snprintf returned %d. Failed to generate a mempool "
+            "name for \"%s\". Hash:0x%x, mtu:%d, mbufs:%u.",
+            ret, dmp->if_name, h, dmp->mtu, dmp->n_mbufs);
         return NULL;
     }
     return mp_name;
 }
 
 static struct dpdk_mp *
-dpdk_mp_create(struct netdev_dpdk *dev, int mtu)
+dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool *mp_exists)
 {
     struct dpdk_mp *dmp = dpdk_rte_mzalloc(sizeof *dmp);
     if (!dmp) {
         return NULL;
     }
+    *mp_exists = false;
     dmp->socket_id = dev->requested_socket_id;
     dmp->mtu = mtu;
     ovs_strzcpy(dmp->if_name, dev->up.name, IFNAMSIZ);
 
     /*
-     * XXX: rough estimation of memory required for port:
+     * XXX: rough estimation of number of mbufs required for this port:
      * <packets required to fill the device rxqs>
      * + <packets that could be stuck on other ports txqs>
      * + <packets in the pmd threads>
      * + <additional memory for corner cases>
      */
-    dmp->mp_size = dev->requested_n_rxq * dev->requested_rxq_size
+    dmp->n_mbufs = dev->requested_n_rxq * dev->requested_rxq_size
             + dev->requested_n_txq * dev->requested_txq_size
             + MIN(RTE_MAX_LCORE, dev->requested_n_rxq) * NETDEV_MAX_BURST
             + MIN_NB_MBUF;
 
-    bool mp_exists = false;
-
     do {
         char *mp_name = dpdk_mp_name(dmp);
+        if (!mp_name) {
+            rte_free(dmp);
+            return NULL;
+        }
 
-        VLOG_DBG("Requesting a mempool of %u mbufs for netdev %s "
-                 "with %d Rx and %d Tx queues.",
-                 dmp->mp_size, dev->up.name,
-                 dev->requested_n_rxq, dev->requested_n_txq);
+        VLOG_DBG("Port %s: Requesting a mempool of %u mbufs "
+                  "on socket %d for %d Rx and %d Tx queues.",
+                  dev->up.name, dmp->n_mbufs,
+                  dev->requested_socket_id,
+                  dev->requested_n_rxq, dev->requested_n_txq);
 
-        dmp->mp = rte_pktmbuf_pool_create(mp_name, dmp->mp_size,
+        dmp->mp = rte_pktmbuf_pool_create(mp_name, dmp->n_mbufs,
                                           MP_CACHE_SZ,
                                           sizeof (struct dp_packet)
                                                  - sizeof (struct rte_mbuf),
@@ -549,7 +556,12 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu)
                                           dmp->socket_id);
         if (dmp->mp) {
             VLOG_DBG("Allocated \"%s\" mempool with %u mbufs", mp_name,
-                     dmp->mp_size);
+                     dmp->n_mbufs);
+            /* rte_pktmbuf_pool_create has done some initialization of the
+             * rte_mbuf part of each dp_packet. Some OvS specific fields
+             * of the packet still need to be initialized by
+             * ovs_rte_pktmbuf_init. */
+            rte_mempool_obj_iter(dmp->mp, ovs_rte_pktmbuf_init, NULL);
         } else if (rte_errno == EEXIST) {
             /* A mempool with the same name already exists.  We just
              * retrieve its pointer to be returned to the caller. */
@@ -559,41 +571,40 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu)
             /* As the mempool create returned EEXIST we can expect the
              * lookup has returned a valid pointer.  If for some reason
              * that's not the case we keep track of it. */
-            mp_exists = true;
+            *mp_exists = true;
         } else {
             VLOG_ERR("Failed mempool \"%s\" create request of %u mbufs",
-                     mp_name, dmp->mp_size);
+                     mp_name, dmp->n_mbufs);
         }
         free(mp_name);
         if (dmp->mp) {
-            /* rte_pktmbuf_pool_create has done some initialization of the
-             * rte_mbuf part of each dp_packet, while ovs_rte_pktmbuf_init
-             * initializes some OVS specific fields of dp_packet.
-             */
-            rte_mempool_obj_iter(dmp->mp, ovs_rte_pktmbuf_init, NULL);
             return dmp;
         }
-    } while (!mp_exists &&
-            (rte_errno == ENOMEM && (dmp->mp_size /= 2) >= MIN_NB_MBUF));
+    } while (!(*mp_exists) &&
+            (rte_errno == ENOMEM && (dmp->n_mbufs /= 2) >= MIN_NB_MBUF));
 
     rte_free(dmp);
     return NULL;
 }
 
+/* Returns a valid pointer when either of the following is true:
+ *  - a new mempool was just created;
+ *  - a matching mempool already exists. */
 static struct dpdk_mp *
-dpdk_mp_get(struct netdev_dpdk *dev, int mtu)
+dpdk_mp_get(struct netdev_dpdk *dev, int mtu, bool *mp_exists)
 {
     struct dpdk_mp *dmp;
 
     ovs_mutex_lock(&dpdk_mp_mutex);
-    dmp = dpdk_mp_create(dev, mtu);
+    dmp = dpdk_mp_create(dev, mtu, mp_exists);
     ovs_mutex_unlock(&dpdk_mp_mutex);
 
     return dmp;
 }
 
+/* Release an existing mempool. */
 static void
-dpdk_mp_put(struct dpdk_mp *dmp)
+dpdk_mp_free(struct dpdk_mp *dmp)
 {
     char *mp_name;
 
@@ -610,9 +621,11 @@ dpdk_mp_put(struct dpdk_mp *dmp)
     ovs_mutex_unlock(&dpdk_mp_mutex);
 }
 
-/* Tries to allocate new mempool on requested_socket_id with
- * mbuf size corresponding to requested_mtu.
- * On success new configuration will be applied.
+/* Tries to allocate a new mempool - or re-use an existing one where
+ * appropriate - on requested_socket_id with a size determined by
+ * requested_mtu and requested Rx/Tx queues.
+ * On success - or when re-using an existing mempool - the new configuration
+ * will be applied.
  * On error, device will be left unchanged. */
 static int
 netdev_dpdk_mempool_configure(struct netdev_dpdk *dev)
@@ -620,16 +633,25 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev)
 {
     uint32_t buf_size = dpdk_buf_size(dev->requested_mtu);
     struct dpdk_mp *mp;
+    bool mp_exists;
 
-    mp = dpdk_mp_get(dev, FRAME_LEN_TO_MTU(buf_size));
+    mp = dpdk_mp_get(dev, FRAME_LEN_TO_MTU(buf_size), &mp_exists);
     if (!mp) {
         VLOG_ERR("Failed to create memory pool for netdev "
                  "%s, with MTU %d on socket %d: %s\n",
                  dev->up.name, dev->requested_mtu, dev->requested_socket_id,
                  rte_strerror(rte_errno));
         return rte_errno;
+    } else if (mp_exists) {
+        /* If a new MTU was requested and its rounded value equals the one
+         * that is currently used, then the existing mempool is returned.
+         * Update dev with the new values. */
+        dev->mtu = dev->requested_mtu;
+        dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);
+        return EEXIST;
     } else {
-        dpdk_mp_put(dev->dpdk_mp);
+        /* A new mempool was created, release the previous one. */
+        dpdk_mp_free(dev->dpdk_mp);
         dev->dpdk_mp = mp;
         dev->mtu = dev->requested_mtu;
         dev->socket_id = dev->requested_socket_id;
@@ -1074,7 +1096,7 @@ common_destruct(struct netdev_dpdk *dev)
     OVS_EXCLUDED(dev->mutex)
 {
     rte_free(dev->tx_q);
-    dpdk_mp_put(dev->dpdk_mp);
+    dpdk_mp_free(dev->dpdk_mp);
 
     ovs_list_remove(&dev->list_node);
     free(ovsrcu_get_protected(struct ingress_policer *,
@@ -3207,7 +3229,7 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
     rte_eth_dev_stop(dev->port_id);
 
     err = netdev_dpdk_mempool_configure(dev);
-    if (err) {
+    if (err && err != EEXIST) {
         goto out;
     }
 
@@ -3247,12 +3269,12 @@ dpdk_vhost_reconfigure_helper(struct netdev_dpdk *dev)
     netdev_dpdk_remap_txqs(dev);
 
     err = netdev_dpdk_mempool_configure(dev);
-    if (err) {
-        return err;
-    } else {
+    if (!err) {
+        /* A new mempool was created. */
         netdev_change_seq_changed(&dev->up);
+    } else if (err != EEXIST){
+        return err;
     }
-
     if (netdev_dpdk_get_vid(dev) >= 0) {
         if (dev->vhost_reconfigured == false) {
             dev->vhost_reconfigured = true;