| Message ID | 1508432048-2265-2-git-send-email-antonio.fischetti@intel.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | netdev-dpdk: Fix mempool management and other cleanup. | expand |
>From: Fischetti, Antonio >Sent: Thursday, October 19, 2017 5:54 PM >To: dev@openvswitch.org >Cc: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Aaron Conole ><aconole@redhat.com>; Darrell Ball <dlu998@gmail.com>; Fischetti, Antonio ><antonio.fischetti@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@intel.com> >CC: Aaron Conole <aconole@redhat.com> >CC: Darrell Ball <dlu998@gmail.com> >Acked-by: Kevin Traynor <ktraynor@redhat.com> >Reported-by: Ciara Loftus <ciara.loftus@intel.com> >Tested-by: Ciara Loftus <ciara.loftus@intel.com> >Reported-by: Róbert Mulik <robert.mulik@ericsson.com> >Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each >port.") >Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com> LGTM - thanks for making the suggested mods! Acked-by: Mark Kavanagh <mark.b.kavanagh@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
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;