[ovs-dev] [PATCH v8 1/6] netdev-dpdk: fix management of pre-existing mempools.

Kavanagh, Mark B mark.b.kavanagh at intel.com
Fri Oct 20 09:49:08 UTC 2017


>From: Fischetti, Antonio
>Sent: Thursday, October 19, 2017 5:54 PM
>To: dev at openvswitch.org
>Cc: Kavanagh, Mark B <mark.b.kavanagh at intel.com>; Aaron Conole
><aconole at redhat.com>; Darrell Ball <dlu998 at gmail.com>; Fischetti, Antonio
><antonio.fischetti at intel.com>
>Subject: [PATCH v8 1/6] netdev-dpdk: fix management of pre-existing mempools.
>
>Fix an issue on reconfiguration of pre-existing mempools.
>This patch avoids to call dpdk_mp_put() - and erroneously
>release the mempool - when it already exists.
>
>CC: Mark B Kavanagh <mark.b.kavanagh at intel.com>
>CC: Aaron Conole <aconole at redhat.com>
>CC: Darrell Ball <dlu998 at gmail.com>
>Acked-by: Kevin Traynor <ktraynor at redhat.com>
>Reported-by: Ciara Loftus <ciara.loftus at intel.com>
>Tested-by: Ciara Loftus <ciara.loftus at intel.com>
>Reported-by: Róbert Mulik <robert.mulik at ericsson.com>
>Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each
>port.")
>Signed-off-by: Antonio Fischetti <antonio.fischetti at intel.com>

LGTM - thanks for making the suggested mods!

Acked-by: Mark Kavanagh <mark.b.kavanagh at intel.com>


>---
>I've tested this patch by
>  - changing at run-time the number of Rx queues:
>      ovs-vsctl set Interface dpdk0 type=dpdk options:n_rxq=4
>
>  - reducing the MTU of the dpdk ports of 1 byte to force
>    the configuration of an existing mempool:
>      ovs-vsctl set Interface dpdk0 mtu_request=1499
>
>This issue was observed in a PVP test topology with dpdkvhostuserclient
>ports. It can happen also with dpdk type ports, eg by reducing the MTU
>of 1 byte.
>
>To replicate the bug scenario in the PVP case it's sufficient to
>set 1 dpdkvhostuserclient port, and just boot the VM.
>
>Below some more details on my own test setup.
>
> PVP test setup
> --------------
>CLIENT_SOCK_DIR=/tmp
>SOCK0=dpdkvhostuser0
>SOCK1=dpdkvhostuser1
>
>1 PMD
>Add 2 dpdk ports, n_rxq=1
>Add 2 vhu ports both of type dpdkvhostuserclient and specify vhost-server-path
> ovs-vsctl set Interface dpdkvhostuser0 options:vhost-server-
>path="$CLIENT_SOCK_DIR/$SOCK0"
> ovs-vsctl set Interface dpdkvhostuser1 options:vhost-server-
>path="$CLIENT_SOCK_DIR/$SOCK1"
>
>Set port-based rules: dpdk0 <--> vhu0 and dpdk1 <--> vhu1
> add-flow br0 in_port=1,action=output:3
> add-flow br0 in_port=3,action=output:1
> add-flow br0 in_port=4,action=output:2
> add-flow br0 in_port=2,action=output:4
>
> Launch QEMU
> -----------
>As OvS vhu ports are acting as clients, we must specify 'server' in the next
>command.
>VM_IMAGE=<path/to/your/vm/image>
>
> sudo -E taskset 0x3F00 $QEMU_DIR/x86_64-softmmu/qemu-system-x86_64 -name us-
>vhost-vm1 -cpu host -enable-kvm -m 4096M -object memory-backend-
>file,id=mem,size=4096M,mem-path=/dev/hugepages,share=on -numa node,memdev=mem
>-mem-prealloc -smp 4 -drive file=$VM_IMAGE -chardev
>socket,id=char0,path=$CLIENT_SOCK_DIR/$SOCK0,server -netdev type=vhost-
>user,id=mynet1,chardev=char0,vhostforce -device virtio-net-
>pci,mac=00:00:00:00:00:01,netdev=mynet1,mrg_rxbuf=off -chardev
>socket,id=char1,path=$CLIENT_SOCK_DIR/$SOCK1,server -netdev type=vhost-
>user,id=mynet2,chardev=char1,vhostforce -device virtio-net-
>pci,mac=00:00:00:00:00:02,netdev=mynet2,mrg_rxbuf=off --nographic
>
> Expected behavior
> -----------------
>With this fix OvS shouldn't crash.
>---
> lib/netdev-dpdk.c | 44 ++++++++++++++++++++++++++++----------------
> 1 file changed, 28 insertions(+), 16 deletions(-)
>
>diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>index c60f46f..45a81f2 100644
>--- a/lib/netdev-dpdk.c
>+++ b/lib/netdev-dpdk.c
>@@ -508,12 +508,13 @@ dpdk_mp_name(struct dpdk_mp *dmp)
> }
>
> 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);
>@@ -530,8 +531,6 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu)
>             + 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);
>
>@@ -559,7 +558,7 @@ 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);
>@@ -573,20 +572,23 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu)
>             rte_mempool_obj_iter(dmp->mp, ovs_rte_pktmbuf_init, NULL);
>             return dmp;
>         }
>-    } while (!mp_exists &&
>+    } while (!(*mp_exists) &&
>             (rte_errno == ENOMEM && (dmp->mp_size /= 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;
>@@ -610,9 +612,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,14 +624,22 @@ 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);
>         dev->dpdk_mp = mp;
>@@ -3207,7 +3219,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 +3259,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;
>--
>2.4.11



More information about the dev mailing list