| Message ID | 1507218725-2355-1-git-send-email-antonio.fischetti@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v3,1/5] netdev-dpdk: fix mempool management with vhu client. | expand |
> > In a PVP test where vhostuser ports are configured as > clients, OvS crashes when QEMU is launched. > This patch avoids to call dpdk_mp_put() - and erroneously > release the mempool - when it already exists. Thanks for investigating this issue and for the patch. I think the commit message could be made more generic since the freeing of the pre-existing mempool could potentially happen for other port types and topologies, not just vhostuserclient & PVP. > > CC: Kevin Traynor <ktraynor@redhat.com> > CC: Aaron Conole <aconole@redhat.com> > Reported-by: Ciara Loftus <ciara.loftus@intel.com> > Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each > port.") > Signed-off-by: Antonio Fischetti <antonio.fischetti@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 > > To replicate the bug scenario: > > 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 Nit - the steps to reproduce the bug are over-complicated. One only needs 1 vhostuserclient port (no dpdk ports, no flows), and just boot the VM = crash. > > 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 | 27 ++++++++++++++------------- > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index c60f46f..80a6ff3 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -508,7 +508,7 @@ 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) { > @@ -530,8 +530,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); Slightly unrelated to this patch but another issue with the d555d9bded5f "netdev-dpdk: Create separate memory pool for each port." commit I came across when reviewing this fix: dpdk_mp_name doesn't reflect what socket the mempool is allocated on. So that commit also breaks the vHost User NUMA feature. Could you include a fix for that bug too in this patch / part of this patchset? > > @@ -559,7 +557,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,7 +571,7 @@ 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); > @@ -581,12 +579,12 @@ dpdk_mp_create(struct netdev_dpdk *dev, int > mtu) > } > > 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; > @@ -620,14 +618,17 @@ 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 = false; > > - 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) { > + return EEXIST; > } else { > dpdk_mp_put(dev->dpdk_mp); > dev->dpdk_mp = mp; > @@ -3207,7 +3208,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 +3248,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; I've verified this patch fixes the issue with reconfigure of vHost User client ports & I no longer see a crash on VM boot. Tested-by: Ciara Loftus <ciara.loftus@intel.com> Thanks, Ciara > -- > 2.4.11 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Thanks Ciara, will respin a v4. Comments inline. Antonio > -----Original Message----- > From: Loftus, Ciara > Sent: Friday, October 6, 2017 11:40 AM > To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org > Subject: RE: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management with > vhu client. > > > > > In a PVP test where vhostuser ports are configured as > > clients, OvS crashes when QEMU is launched. > > This patch avoids to call dpdk_mp_put() - and erroneously > > release the mempool - when it already exists. > > Thanks for investigating this issue and for the patch. > I think the commit message could be made more generic since the freeing of the > pre-existing mempool could potentially happen for other port types and > topologies, not just vhostuserclient & PVP. [Antonio] ok. > > > > > CC: Kevin Traynor <ktraynor@redhat.com> > > CC: Aaron Conole <aconole@redhat.com> > > Reported-by: Ciara Loftus <ciara.loftus@intel.com> > > Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each > > port.") > > Signed-off-by: Antonio Fischetti <antonio.fischetti@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 > > > > To replicate the bug scenario: > > > > 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 > > Nit - the steps to reproduce the bug are over-complicated. One only needs 1 > vhostuserclient port (no dpdk ports, no flows), and just boot the VM = crash. [Antonio] ok, will change this description. > > > > > 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 | 27 ++++++++++++++------------- > > 1 file changed, 14 insertions(+), 13 deletions(-) > > > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > > index c60f46f..80a6ff3 100644 > > --- a/lib/netdev-dpdk.c > > +++ b/lib/netdev-dpdk.c > > @@ -508,7 +508,7 @@ 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) { > > @@ -530,8 +530,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); > > Slightly unrelated to this patch but another issue with the d555d9bded5f > "netdev-dpdk: Create separate memory pool for each port." commit I came across > when reviewing this fix: > dpdk_mp_name doesn't reflect what socket the mempool is allocated on. So that > commit also breaks the vHost User NUMA feature. Could you include a fix for > that bug too in this patch / part of this patchset? [Antonio] ok, I'll keep it in a separate patch as I think it is not strictly related to this particular issue we were seeing. > > > > > @@ -559,7 +557,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,7 +571,7 @@ 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); > > @@ -581,12 +579,12 @@ dpdk_mp_create(struct netdev_dpdk *dev, int > > mtu) > > } > > > > 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; > > @@ -620,14 +618,17 @@ 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 = false; > > > > - 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) { > > + return EEXIST; > > } else { > > dpdk_mp_put(dev->dpdk_mp); > > dev->dpdk_mp = mp; > > @@ -3207,7 +3208,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 +3248,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; > > I've verified this patch fixes the issue with reconfigure of vHost User client > ports & I no longer see a crash on VM boot. > Tested-by: Ciara Loftus <ciara.loftus@intel.com> > > Thanks, > Ciara > > > -- > > 2.4.11 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Antonio,
Last week I run into this mempool issue during the development of a new feature. I have made a bugfix, but then we saw yours too, so I tested if it solves my problem. It did, but I realized another problem with it. The mempool name generation is partly based on the MTU size, which is handled in 1024 bytes long ranges. For example MTU 1000 and 1500 are in the same range, 2000 and 2500 are in a different range. So I tested this patch and got the following.
# ovs-vsctl list interface dpdk0 |grep mtu
mtu : 2500
mtu_request : 2500
# ovs-vsctl set interface dpdk0 mtu_request=2000
# ovs-vsctl list interface dpdk0 |grep mtu
mtu : 2500
mtu_request : 2000
# ovs-vsctl set interface dpdk0 mtu_request=1500
# ovs-vsctl list interface dpdk0 |grep mtu
mtu : 1500
mtu_request : 1500
# ovs-vsctl set interface dpdk0 mtu_request=1000
# ovs-vsctl list interface dpdk0 |grep mtu
mtu : 1500
mtu_request : 1000
# ovs-vsctl set interface dpdk0 mtu_request=2000
# ovs-vsctl list interface dpdk0 |grep mtu
mtu : 2000
mtu_request : 2000
# ovs-vsctl set interface dpdk0 mtu_request=1000
# ovs-vsctl list interface dpdk0 |grep mtu
mtu : 1000
mtu_request : 1000
# ovs-vsctl set interface dpdk0 mtu_request=1500
# ovs-vsctl list interface dpdk0 |grep mtu
mtu : 1000
mtu_request : 1500
# service openvswitch-switch restart
# ovs-vsctl list interface dpdk0 |grep mtu
mtu : 1500
mtu_request : 1500
This was my setup:
Bridge br-prv
Port bond-prv
Interface "dpdk0"
type: dpdk
options: {dpdk-devargs="0000:05:00.0", n_rxq_desc="1024", n_txq_desc="1024"}
ovs_version: "2.8.90"
And I used DPDK v17.08.
So, as it can be see from the example above, with the patch applied when a new mtu_request is in the same range as the previously set MTU, then it has no effect until service restart. The mtu_request has immediate effect when it is in different range as the previously set MTU. Or did I miss something during the testing?
My patch what I used last week does the following. During reconfiguration the mempool is always deleted before a new one is created. It solved the problem without side effects, but it is not optimized (always recreates the mempool when this function is called).
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index c60f46f..de38f95 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -621,6 +621,7 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev)
uint32_t buf_size = dpdk_buf_size(dev->requested_mtu);
struct dpdk_mp *mp;
+ dpdk_mp_put(dev->dpdk_mp);
mp = dpdk_mp_get(dev, FRAME_LEN_TO_MTU(buf_size));
if (!mp) {
VLOG_ERR("Failed to create memory pool for netdev "
@@ -629,7 +630,6 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev)
rte_strerror(rte_errno));
return rte_errno;
} else {
- dpdk_mp_put(dev->dpdk_mp);
dev->dpdk_mp = mp;
dev->mtu = dev->requested_mtu;
dev->socket_id = dev->requested_socket_id;
What do you think about this solution?
Regards,
Robert
Thanks Robert for reporting this and for all the clear details you provided. I'll look into this and get back to you. Antonio > -----Original Message----- > From: Róbert Mulik [mailto:robert.mulik@ericsson.com] > Sent: Tuesday, October 10, 2017 4:19 PM > To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org > Subject: RE: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management with > vhu client. > > Hi Antonio, > > Last week I run into this mempool issue during the development of a new > feature. I have made a bugfix, but then we saw yours too, so I tested if it > solves my problem. It did, but I realized another problem with it. The mempool > name generation is partly based on the MTU size, which is handled in 1024 bytes > long ranges. For example MTU 1000 and 1500 are in the same range, 2000 and 2500 > are in a different range. So I tested this patch and got the following. > > # ovs-vsctl list interface dpdk0 |grep mtu > mtu : 2500 > mtu_request : 2500 > # ovs-vsctl set interface dpdk0 mtu_request=2000 > # ovs-vsctl list interface dpdk0 |grep mtu > mtu : 2500 > mtu_request : 2000 > # ovs-vsctl set interface dpdk0 mtu_request=1500 > # ovs-vsctl list interface dpdk0 |grep mtu > mtu : 1500 > mtu_request : 1500 > # ovs-vsctl set interface dpdk0 mtu_request=1000 > # ovs-vsctl list interface dpdk0 |grep mtu > mtu : 1500 > mtu_request : 1000 > # ovs-vsctl set interface dpdk0 mtu_request=2000 > # ovs-vsctl list interface dpdk0 |grep mtu > mtu : 2000 > mtu_request : 2000 > # ovs-vsctl set interface dpdk0 mtu_request=1000 > # ovs-vsctl list interface dpdk0 |grep mtu > mtu : 1000 > mtu_request : 1000 > # ovs-vsctl set interface dpdk0 mtu_request=1500 > # ovs-vsctl list interface dpdk0 |grep mtu > mtu : 1000 > mtu_request : 1500 > # service openvswitch-switch restart > # ovs-vsctl list interface dpdk0 |grep mtu > mtu : 1500 > mtu_request : 1500 > > > This was my setup: > Bridge br-prv > Port bond-prv > Interface "dpdk0" > type: dpdk > options: {dpdk-devargs="0000:05:00.0", n_rxq_desc="1024", > n_txq_desc="1024"} > ovs_version: "2.8.90" > > And I used DPDK v17.08. > > > So, as it can be see from the example above, with the patch applied when a new > mtu_request is in the same range as the previously set MTU, then it has no > effect until service restart. The mtu_request has immediate effect when it is > in different range as the previously set MTU. Or did I miss something during > the testing? > > My patch what I used last week does the following. During reconfiguration the > mempool is always deleted before a new one is created. It solved the problem > without side effects, but it is not optimized (always recreates the mempool > when this function is called). > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index c60f46f..de38f95 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -621,6 +621,7 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev) > uint32_t buf_size = dpdk_buf_size(dev->requested_mtu); > struct dpdk_mp *mp; > > + dpdk_mp_put(dev->dpdk_mp); > mp = dpdk_mp_get(dev, FRAME_LEN_TO_MTU(buf_size)); > if (!mp) { > VLOG_ERR("Failed to create memory pool for netdev " > @@ -629,7 +630,6 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev) > rte_strerror(rte_errno)); > return rte_errno; > } else { > - dpdk_mp_put(dev->dpdk_mp); > dev->dpdk_mp = mp; > dev->mtu = dev->requested_mtu; > dev->socket_id = dev->requested_socket_id; > > > What do you think about this solution? > > Regards, > Robert
Hi Robert, that's happening because the requested MTU is rounded up to the current boundary. So if the current upper boundary is 2500 and we request 2000 => 2000 is rounded up to 2500 and the same mempool is returned. I may be wrong but this seems the wanted behavior, maybe Kevin can shed some light? I may have missed some detail as I didn't follow this implementation since the very beginning. Antonio > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] > On Behalf Of Fischetti, Antonio > Sent: Wednesday, October 11, 2017 9:04 AM > To: Róbert Mulik <robert.mulik@ericsson.com>; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management with > vhu client. > > Thanks Robert for reporting this and for all the clear details you provided. > I'll look into this and get back to you. > > Antonio > > > -----Original Message----- > > From: Róbert Mulik [mailto:robert.mulik@ericsson.com] > > Sent: Tuesday, October 10, 2017 4:19 PM > > To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org > > Subject: RE: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management > with > > vhu client. > > > > Hi Antonio, > > > > Last week I run into this mempool issue during the development of a new > > feature. I have made a bugfix, but then we saw yours too, so I tested if it > > solves my problem. It did, but I realized another problem with it. The > mempool > > name generation is partly based on the MTU size, which is handled in 1024 > bytes > > long ranges. For example MTU 1000 and 1500 are in the same range, 2000 and > 2500 > > are in a different range. So I tested this patch and got the following. > > > > # ovs-vsctl list interface dpdk0 |grep mtu > > mtu : 2500 > > mtu_request : 2500 > > # ovs-vsctl set interface dpdk0 mtu_request=2000 > > # ovs-vsctl list interface dpdk0 |grep mtu > > mtu : 2500 > > mtu_request : 2000 > > # ovs-vsctl set interface dpdk0 mtu_request=1500 > > # ovs-vsctl list interface dpdk0 |grep mtu > > mtu : 1500 > > mtu_request : 1500 > > # ovs-vsctl set interface dpdk0 mtu_request=1000 > > # ovs-vsctl list interface dpdk0 |grep mtu > > mtu : 1500 > > mtu_request : 1000 > > # ovs-vsctl set interface dpdk0 mtu_request=2000 > > # ovs-vsctl list interface dpdk0 |grep mtu > > mtu : 2000 > > mtu_request : 2000 > > # ovs-vsctl set interface dpdk0 mtu_request=1000 > > # ovs-vsctl list interface dpdk0 |grep mtu > > mtu : 1000 > > mtu_request : 1000 > > # ovs-vsctl set interface dpdk0 mtu_request=1500 > > # ovs-vsctl list interface dpdk0 |grep mtu > > mtu : 1000 > > mtu_request : 1500 > > # service openvswitch-switch restart > > # ovs-vsctl list interface dpdk0 |grep mtu > > mtu : 1500 > > mtu_request : 1500 > > > > > > This was my setup: > > Bridge br-prv > > Port bond-prv > > Interface "dpdk0" > > type: dpdk > > options: {dpdk-devargs="0000:05:00.0", n_rxq_desc="1024", > > n_txq_desc="1024"} > > ovs_version: "2.8.90" > > > > And I used DPDK v17.08. > > > > > > So, as it can be see from the example above, with the patch applied when a > new > > mtu_request is in the same range as the previously set MTU, then it has no > > effect until service restart. The mtu_request has immediate effect when it is > > in different range as the previously set MTU. Or did I miss something during > > the testing? > > > > My patch what I used last week does the following. During reconfiguration the > > mempool is always deleted before a new one is created. It solved the problem > > without side effects, but it is not optimized (always recreates the mempool > > when this function is called). > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > > index c60f46f..de38f95 100644 > > --- a/lib/netdev-dpdk.c > > +++ b/lib/netdev-dpdk.c > > @@ -621,6 +621,7 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev) > > uint32_t buf_size = dpdk_buf_size(dev->requested_mtu); > > struct dpdk_mp *mp; > > > > + dpdk_mp_put(dev->dpdk_mp); > > mp = dpdk_mp_get(dev, FRAME_LEN_TO_MTU(buf_size)); > > if (!mp) { > > VLOG_ERR("Failed to create memory pool for netdev " > > @@ -629,7 +630,6 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev) > > rte_strerror(rte_errno)); > > return rte_errno; > > } else { > > - dpdk_mp_put(dev->dpdk_mp); > > dev->dpdk_mp = mp; > > dev->mtu = dev->requested_mtu; > > dev->socket_id = dev->requested_socket_id; > > > > > > What do you think about this solution? > > > > Regards, > > Robert > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 10/11/2017 11:28 AM, Fischetti, Antonio wrote: > Hi Robert, > that's happening because the requested MTU is rounded up to the current boundary. > So if the current upper boundary is 2500 and we request 2000 => > 2000 is rounded up to 2500 and the same mempool is returned. > > I may be wrong but this seems the wanted behavior, maybe Kevin can shed some light? > I may have missed some detail as I didn't follow this implementation since the > very beginning. > I think it's related to review comments I sent earlier today. mtu is mtu, but a value that is rounded from it is used in calculating the size of the mbuf. I suspect in this case, when the new mtu size results in the same rounded value, the current mempool is being reused (which is fine) but the EEXISTS error value returned from reconfigure means that the change is not seen as successful and the old mtu value is restored to ovsdb. Kevin. > Antonio > >> -----Original Message----- >> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] >> On Behalf Of Fischetti, Antonio >> Sent: Wednesday, October 11, 2017 9:04 AM >> To: Róbert Mulik <robert.mulik@ericsson.com>; dev@openvswitch.org >> Subject: Re: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management with >> vhu client. >> >> Thanks Robert for reporting this and for all the clear details you provided. >> I'll look into this and get back to you. >> >> Antonio >> >>> -----Original Message----- >>> From: Róbert Mulik [mailto:robert.mulik@ericsson.com] >>> Sent: Tuesday, October 10, 2017 4:19 PM >>> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org >>> Subject: RE: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management >> with >>> vhu client. >>> >>> Hi Antonio, >>> >>> Last week I run into this mempool issue during the development of a new >>> feature. I have made a bugfix, but then we saw yours too, so I tested if it >>> solves my problem. It did, but I realized another problem with it. The >> mempool >>> name generation is partly based on the MTU size, which is handled in 1024 >> bytes >>> long ranges. For example MTU 1000 and 1500 are in the same range, 2000 and >> 2500 >>> are in a different range. So I tested this patch and got the following. >>> >>> # ovs-vsctl list interface dpdk0 |grep mtu >>> mtu : 2500 >>> mtu_request : 2500 >>> # ovs-vsctl set interface dpdk0 mtu_request=2000 >>> # ovs-vsctl list interface dpdk0 |grep mtu >>> mtu : 2500 >>> mtu_request : 2000 >>> # ovs-vsctl set interface dpdk0 mtu_request=1500 >>> # ovs-vsctl list interface dpdk0 |grep mtu >>> mtu : 1500 >>> mtu_request : 1500 >>> # ovs-vsctl set interface dpdk0 mtu_request=1000 >>> # ovs-vsctl list interface dpdk0 |grep mtu >>> mtu : 1500 >>> mtu_request : 1000 >>> # ovs-vsctl set interface dpdk0 mtu_request=2000 >>> # ovs-vsctl list interface dpdk0 |grep mtu >>> mtu : 2000 >>> mtu_request : 2000 >>> # ovs-vsctl set interface dpdk0 mtu_request=1000 >>> # ovs-vsctl list interface dpdk0 |grep mtu >>> mtu : 1000 >>> mtu_request : 1000 >>> # ovs-vsctl set interface dpdk0 mtu_request=1500 >>> # ovs-vsctl list interface dpdk0 |grep mtu >>> mtu : 1000 >>> mtu_request : 1500 >>> # service openvswitch-switch restart >>> # ovs-vsctl list interface dpdk0 |grep mtu >>> mtu : 1500 >>> mtu_request : 1500 >>> >>> >>> This was my setup: >>> Bridge br-prv >>> Port bond-prv >>> Interface "dpdk0" >>> type: dpdk >>> options: {dpdk-devargs="0000:05:00.0", n_rxq_desc="1024", >>> n_txq_desc="1024"} >>> ovs_version: "2.8.90" >>> >>> And I used DPDK v17.08. >>> >>> >>> So, as it can be see from the example above, with the patch applied when a >> new >>> mtu_request is in the same range as the previously set MTU, then it has no >>> effect until service restart. The mtu_request has immediate effect when it is >>> in different range as the previously set MTU. Or did I miss something during >>> the testing? >>> >>> My patch what I used last week does the following. During reconfiguration the >>> mempool is always deleted before a new one is created. It solved the problem >>> without side effects, but it is not optimized (always recreates the mempool >>> when this function is called). >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>> index c60f46f..de38f95 100644 >>> --- a/lib/netdev-dpdk.c >>> +++ b/lib/netdev-dpdk.c >>> @@ -621,6 +621,7 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev) >>> uint32_t buf_size = dpdk_buf_size(dev->requested_mtu); >>> struct dpdk_mp *mp; >>> >>> + dpdk_mp_put(dev->dpdk_mp); >>> mp = dpdk_mp_get(dev, FRAME_LEN_TO_MTU(buf_size)); >>> if (!mp) { >>> VLOG_ERR("Failed to create memory pool for netdev " >>> @@ -629,7 +630,6 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev) >>> rte_strerror(rte_errno)); >>> return rte_errno; >>> } else { >>> - dpdk_mp_put(dev->dpdk_mp); >>> dev->dpdk_mp = mp; >>> dev->mtu = dev->requested_mtu; >>> dev->socket_id = dev->requested_socket_id; >>> >>> >>> What do you think about this solution? >>> >>> Regards, >>> Robert >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> -----Original Message----- > From: Kevin Traynor [mailto:ktraynor@redhat.com] > Sent: Wednesday, October 11, 2017 11:40 AM > To: Fischetti, Antonio <antonio.fischetti@intel.com>; Róbert Mulik > <robert.mulik@ericsson.com>; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management with > vhu client. > > On 10/11/2017 11:28 AM, Fischetti, Antonio wrote: > > Hi Robert, > > that's happening because the requested MTU is rounded up to the current > boundary. > > So if the current upper boundary is 2500 and we request 2000 => > > 2000 is rounded up to 2500 and the same mempool is returned. > > > > I may be wrong but this seems the wanted behavior, maybe Kevin can shed some > light? > > I may have missed some detail as I didn't follow this implementation since > the > > very beginning. > > > > I think it's related to review comments I sent earlier today. mtu is > mtu, but a value that is rounded from it is used in calculating the size > of the mbuf. I suspect in this case, when the new mtu size results in > the same rounded value, the current mempool is being reused (which is > fine) [Antonio] exactly. > but the EEXISTS error value returned from reconfigure means that > the change is not seen as successful and the old mtu value is restored > to ovsdb. [Antonio] I think this can be fixed by the following changes. The new mtu value is stored when a pre-existing mempool is returned: __________________________________________________________________________ @@ -631,6 +631,11 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev) rte_strerror(rte_errno)); return rte_errno; } else if (mp_exists) { + /* If a new MTU was requested and its rounded value is the same + * that is currently used, then the existing mempool was returned. + * Update the new MTU value. */ + dev->mtu = dev->requested_mtu; + dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu); return EEXIST; } else { dpdk_mp_put(dev->dpdk_mp); __________________________________________________________________________ Instead, inside netdev_dpdk_reconfigure() I think it should be correct to have err = netdev_dpdk_mempool_configure(dev); if (err && err != EEXIST) { goto out; } because as in the case of a new mempool just created as in the case of EEXIST (=17) we don't want to execute "goto out" and fall through to do dev->rxq_size = dev->requested_rxq_size; dev->txq_size = dev->requested_txq_size; ... With these code changes it seems to work, at the beginning I have MTU=1500 and I set it to 1000: # ovs-vsctl list interface dpdk0 | grep mtu mtu : 1500 mtu_request : [] # ovs-vsctl set interface dpdk0 mtu_request=1000 my log says netdev_dpdk|DBG|Requesting a mempool of 40992 mbufs for netdev dpdk0 with 1 Rx and 11 Tx queues, socket id:0. netdev_dpdk|DBG|A mempool with name ovs_62a2ca2f_0_2030_40992 already exists at 0x7fad9d77dd00. dpdk|INFO|PMD: ixgbe_dev_link_status_print(): Port 0: Link Up - speed 0 Mbps - half-duplex # ovs-vsctl list interface dpdk0 | grep mtu mtu : 1000 mtu_request : 1000 Does that make sense? Thanks, -Antonio > > Kevin. > > > Antonio > > > >> -----Original Message----- > >> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] > >> On Behalf Of Fischetti, Antonio > >> Sent: Wednesday, October 11, 2017 9:04 AM > >> To: Róbert Mulik <robert.mulik@ericsson.com>; dev@openvswitch.org > >> Subject: Re: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management > with > >> vhu client. > >> > >> Thanks Robert for reporting this and for all the clear details you provided. > >> I'll look into this and get back to you. > >> > >> Antonio > >> > >>> -----Original Message----- > >>> From: Róbert Mulik [mailto:robert.mulik@ericsson.com] > >>> Sent: Tuesday, October 10, 2017 4:19 PM > >>> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org > >>> Subject: RE: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management > >> with > >>> vhu client. > >>> > >>> Hi Antonio, > >>> > >>> Last week I run into this mempool issue during the development of a new > >>> feature. I have made a bugfix, but then we saw yours too, so I tested if it > >>> solves my problem. It did, but I realized another problem with it. The > >> mempool > >>> name generation is partly based on the MTU size, which is handled in 1024 > >> bytes > >>> long ranges. For example MTU 1000 and 1500 are in the same range, 2000 and > >> 2500 > >>> are in a different range. So I tested this patch and got the following. > >>> > >>> # ovs-vsctl list interface dpdk0 |grep mtu > >>> mtu : 2500 > >>> mtu_request : 2500 > >>> # ovs-vsctl set interface dpdk0 mtu_request=2000 > >>> # ovs-vsctl list interface dpdk0 |grep mtu > >>> mtu : 2500 > >>> mtu_request : 2000 > >>> # ovs-vsctl set interface dpdk0 mtu_request=1500 > >>> # ovs-vsctl list interface dpdk0 |grep mtu > >>> mtu : 1500 > >>> mtu_request : 1500 > >>> # ovs-vsctl set interface dpdk0 mtu_request=1000 > >>> # ovs-vsctl list interface dpdk0 |grep mtu > >>> mtu : 1500 > >>> mtu_request : 1000 > >>> # ovs-vsctl set interface dpdk0 mtu_request=2000 > >>> # ovs-vsctl list interface dpdk0 |grep mtu > >>> mtu : 2000 > >>> mtu_request : 2000 > >>> # ovs-vsctl set interface dpdk0 mtu_request=1000 > >>> # ovs-vsctl list interface dpdk0 |grep mtu > >>> mtu : 1000 > >>> mtu_request : 1000 > >>> # ovs-vsctl set interface dpdk0 mtu_request=1500 > >>> # ovs-vsctl list interface dpdk0 |grep mtu > >>> mtu : 1000 > >>> mtu_request : 1500 > >>> # service openvswitch-switch restart > >>> # ovs-vsctl list interface dpdk0 |grep mtu > >>> mtu : 1500 > >>> mtu_request : 1500 > >>> > >>> > >>> This was my setup: > >>> Bridge br-prv > >>> Port bond-prv > >>> Interface "dpdk0" > >>> type: dpdk > >>> options: {dpdk-devargs="0000:05:00.0", n_rxq_desc="1024", > >>> n_txq_desc="1024"} > >>> ovs_version: "2.8.90" > >>> > >>> And I used DPDK v17.08. > >>> > >>> > >>> So, as it can be see from the example above, with the patch applied when a > >> new > >>> mtu_request is in the same range as the previously set MTU, then it has no > >>> effect until service restart. The mtu_request has immediate effect when it > is > >>> in different range as the previously set MTU. Or did I miss something > during > >>> the testing? > >>> > >>> My patch what I used last week does the following. During reconfiguration > the > >>> mempool is always deleted before a new one is created. It solved the > problem > >>> without side effects, but it is not optimized (always recreates the mempool > >>> when this function is called). > >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > >>> index c60f46f..de38f95 100644 > >>> --- a/lib/netdev-dpdk.c > >>> +++ b/lib/netdev-dpdk.c > >>> @@ -621,6 +621,7 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev) > >>> uint32_t buf_size = dpdk_buf_size(dev->requested_mtu); > >>> struct dpdk_mp *mp; > >>> > >>> + dpdk_mp_put(dev->dpdk_mp); > >>> mp = dpdk_mp_get(dev, FRAME_LEN_TO_MTU(buf_size)); > >>> if (!mp) { > >>> VLOG_ERR("Failed to create memory pool for netdev " > >>> @@ -629,7 +630,6 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev) > >>> rte_strerror(rte_errno)); > >>> return rte_errno; > >>> } else { > >>> - dpdk_mp_put(dev->dpdk_mp); > >>> dev->dpdk_mp = mp; > >>> dev->mtu = dev->requested_mtu; > >>> dev->socket_id = dev->requested_socket_id; > >>> > >>> > >>> What do you think about this solution? > >>> > >>> Regards, > >>> Robert > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 10/11/2017 02:04 PM, Fischetti, Antonio wrote: > >> -----Original Message----- >> From: Kevin Traynor [mailto:ktraynor@redhat.com] >> Sent: Wednesday, October 11, 2017 11:40 AM >> To: Fischetti, Antonio <antonio.fischetti@intel.com>; Róbert Mulik >> <robert.mulik@ericsson.com>; dev@openvswitch.org >> Subject: Re: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management with >> vhu client. >> >> On 10/11/2017 11:28 AM, Fischetti, Antonio wrote: >>> Hi Robert, >>> that's happening because the requested MTU is rounded up to the current >> boundary. >>> So if the current upper boundary is 2500 and we request 2000 => >>> 2000 is rounded up to 2500 and the same mempool is returned. >>> >>> I may be wrong but this seems the wanted behavior, maybe Kevin can shed some >> light? >>> I may have missed some detail as I didn't follow this implementation since >> the >>> very beginning. >>> >> >> I think it's related to review comments I sent earlier today. mtu is >> mtu, but a value that is rounded from it is used in calculating the size >> of the mbuf. I suspect in this case, when the new mtu size results in >> the same rounded value, the current mempool is being reused (which is >> fine) > > [Antonio] exactly. > >> but the EEXISTS error value returned from reconfigure means that >> the change is not seen as successful and the old mtu value is restored >> to ovsdb. > > [Antonio] > I think this can be fixed by the following changes. > The new mtu value is stored when a pre-existing mempool is returned: > __________________________________________________________________________ > @@ -631,6 +631,11 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev) > rte_strerror(rte_errno)); > return rte_errno; > } else if (mp_exists) { > + /* If a new MTU was requested and its rounded value is the same > + * that is currently used, then the existing mempool was returned. > + * Update the new MTU value. */ > + dev->mtu = dev->requested_mtu; > + dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu); What about requested_socket_id, isn't that needed also? > return EEXIST; > } else { > dpdk_mp_put(dev->dpdk_mp); > __________________________________________________________________________ > > Instead, inside netdev_dpdk_reconfigure() I think it should be correct to have > > err = netdev_dpdk_mempool_configure(dev); > if (err && err != EEXIST) { > goto out; > } > > because as in the case of a new mempool just created as in the case of EEXIST (=17) > we don't want to execute "goto out" and fall through to do > > dev->rxq_size = dev->requested_rxq_size; > dev->txq_size = dev->requested_txq_size; > ... > > With these code changes it seems to work, at the beginning I have MTU=1500 and I set it to 1000: > > # ovs-vsctl list interface dpdk0 | grep mtu > mtu : 1500 > mtu_request : [] > > # ovs-vsctl set interface dpdk0 mtu_request=1000 > my log says > netdev_dpdk|DBG|Requesting a mempool of 40992 mbufs for netdev dpdk0 with 1 Rx and 11 Tx queues, socket id:0. > netdev_dpdk|DBG|A mempool with name ovs_62a2ca2f_0_2030_40992 already exists at 0x7fad9d77dd00. > dpdk|INFO|PMD: ixgbe_dev_link_status_print(): Port 0: Link Up - speed 0 Mbps - half-duplex > > # ovs-vsctl list interface dpdk0 | grep mtu > mtu : 1000 > mtu_request : 1000 > > Does that make sense? > Looks ok to me. Kevin. > Thanks, > -Antonio > >> >> Kevin. >> >>> Antonio >>> >>>> -----Original Message----- >>>> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- >> bounces@openvswitch.org] >>>> On Behalf Of Fischetti, Antonio >>>> Sent: Wednesday, October 11, 2017 9:04 AM >>>> To: Róbert Mulik <robert.mulik@ericsson.com>; dev@openvswitch.org >>>> Subject: Re: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management >> with >>>> vhu client. >>>> >>>> Thanks Robert for reporting this and for all the clear details you provided. >>>> I'll look into this and get back to you. >>>> >>>> Antonio >>>> >>>>> -----Original Message----- >>>>> From: Róbert Mulik [mailto:robert.mulik@ericsson.com] >>>>> Sent: Tuesday, October 10, 2017 4:19 PM >>>>> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org >>>>> Subject: RE: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management >>>> with >>>>> vhu client. >>>>> >>>>> Hi Antonio, >>>>> >>>>> Last week I run into this mempool issue during the development of a new >>>>> feature. I have made a bugfix, but then we saw yours too, so I tested if it >>>>> solves my problem. It did, but I realized another problem with it. The >>>> mempool >>>>> name generation is partly based on the MTU size, which is handled in 1024 >>>> bytes >>>>> long ranges. For example MTU 1000 and 1500 are in the same range, 2000 and >>>> 2500 >>>>> are in a different range. So I tested this patch and got the following. >>>>> >>>>> # ovs-vsctl list interface dpdk0 |grep mtu >>>>> mtu : 2500 >>>>> mtu_request : 2500 >>>>> # ovs-vsctl set interface dpdk0 mtu_request=2000 >>>>> # ovs-vsctl list interface dpdk0 |grep mtu >>>>> mtu : 2500 >>>>> mtu_request : 2000 >>>>> # ovs-vsctl set interface dpdk0 mtu_request=1500 >>>>> # ovs-vsctl list interface dpdk0 |grep mtu >>>>> mtu : 1500 >>>>> mtu_request : 1500 >>>>> # ovs-vsctl set interface dpdk0 mtu_request=1000 >>>>> # ovs-vsctl list interface dpdk0 |grep mtu >>>>> mtu : 1500 >>>>> mtu_request : 1000 >>>>> # ovs-vsctl set interface dpdk0 mtu_request=2000 >>>>> # ovs-vsctl list interface dpdk0 |grep mtu >>>>> mtu : 2000 >>>>> mtu_request : 2000 >>>>> # ovs-vsctl set interface dpdk0 mtu_request=1000 >>>>> # ovs-vsctl list interface dpdk0 |grep mtu >>>>> mtu : 1000 >>>>> mtu_request : 1000 >>>>> # ovs-vsctl set interface dpdk0 mtu_request=1500 >>>>> # ovs-vsctl list interface dpdk0 |grep mtu >>>>> mtu : 1000 >>>>> mtu_request : 1500 >>>>> # service openvswitch-switch restart >>>>> # ovs-vsctl list interface dpdk0 |grep mtu >>>>> mtu : 1500 >>>>> mtu_request : 1500 >>>>> >>>>> >>>>> This was my setup: >>>>> Bridge br-prv >>>>> Port bond-prv >>>>> Interface "dpdk0" >>>>> type: dpdk >>>>> options: {dpdk-devargs="0000:05:00.0", n_rxq_desc="1024", >>>>> n_txq_desc="1024"} >>>>> ovs_version: "2.8.90" >>>>> >>>>> And I used DPDK v17.08. >>>>> >>>>> >>>>> So, as it can be see from the example above, with the patch applied when a >>>> new >>>>> mtu_request is in the same range as the previously set MTU, then it has no >>>>> effect until service restart. The mtu_request has immediate effect when it >> is >>>>> in different range as the previously set MTU. Or did I miss something >> during >>>>> the testing? >>>>> >>>>> My patch what I used last week does the following. During reconfiguration >> the >>>>> mempool is always deleted before a new one is created. It solved the >> problem >>>>> without side effects, but it is not optimized (always recreates the mempool >>>>> when this function is called). >>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>>>> index c60f46f..de38f95 100644 >>>>> --- a/lib/netdev-dpdk.c >>>>> +++ b/lib/netdev-dpdk.c >>>>> @@ -621,6 +621,7 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev) >>>>> uint32_t buf_size = dpdk_buf_size(dev->requested_mtu); >>>>> struct dpdk_mp *mp; >>>>> >>>>> + dpdk_mp_put(dev->dpdk_mp); >>>>> mp = dpdk_mp_get(dev, FRAME_LEN_TO_MTU(buf_size)); >>>>> if (!mp) { >>>>> VLOG_ERR("Failed to create memory pool for netdev " >>>>> @@ -629,7 +630,6 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev) >>>>> rte_strerror(rte_errno)); >>>>> return rte_errno; >>>>> } else { >>>>> - dpdk_mp_put(dev->dpdk_mp); >>>>> dev->dpdk_mp = mp; >>>>> dev->mtu = dev->requested_mtu; >>>>> dev->socket_id = dev->requested_socket_id; >>>>> >>>>> >>>>> What do you think about this solution? >>>>> >>>>> Regards, >>>>> Robert >>>> _______________________________________________ >>>> dev mailing list >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> -----Original Message----- > From: Kevin Traynor [mailto:ktraynor@redhat.com] > Sent: Wednesday, October 11, 2017 2:37 PM > To: Fischetti, Antonio <antonio.fischetti@intel.com>; Róbert Mulik > <robert.mulik@ericsson.com>; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management with > vhu client. > > On 10/11/2017 02:04 PM, Fischetti, Antonio wrote: > > > >> -----Original Message----- > >> From: Kevin Traynor [mailto:ktraynor@redhat.com] > >> Sent: Wednesday, October 11, 2017 11:40 AM > >> To: Fischetti, Antonio <antonio.fischetti@intel.com>; Róbert Mulik > >> <robert.mulik@ericsson.com>; dev@openvswitch.org > >> Subject: Re: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management > with > >> vhu client. > >> > >> On 10/11/2017 11:28 AM, Fischetti, Antonio wrote: > >>> Hi Robert, > >>> that's happening because the requested MTU is rounded up to the current > >> boundary. > >>> So if the current upper boundary is 2500 and we request 2000 => > >>> 2000 is rounded up to 2500 and the same mempool is returned. > >>> > >>> I may be wrong but this seems the wanted behavior, maybe Kevin can shed > some > >> light? > >>> I may have missed some detail as I didn't follow this implementation since > >> the > >>> very beginning. > >>> > >> > >> I think it's related to review comments I sent earlier today. mtu is > >> mtu, but a value that is rounded from it is used in calculating the size > >> of the mbuf. I suspect in this case, when the new mtu size results in > >> the same rounded value, the current mempool is being reused (which is > >> fine) > > > > [Antonio] exactly. > > > >> but the EEXISTS error value returned from reconfigure means that > >> the change is not seen as successful and the old mtu value is restored > >> to ovsdb. > > > > [Antonio] > > I think this can be fixed by the following changes. > > The new mtu value is stored when a pre-existing mempool is returned: > > __________________________________________________________________________ > > @@ -631,6 +631,11 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev) > > rte_strerror(rte_errno)); > > return rte_errno; > > } else if (mp_exists) { > > + /* If a new MTU was requested and its rounded value is the same > > + * that is currently used, then the existing mempool was returned. > > + * Update the new MTU value. */ > > + dev->mtu = dev->requested_mtu; > > + dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu); > > What about requested_socket_id, isn't that needed also? Yes will add it. Thanks Kevin and Robert, I'll rework it and post a v5. I'll take into account also the other comments by Kevin on v4 review. -Antonio > > > return EEXIST; > > } else { > > dpdk_mp_put(dev->dpdk_mp); > > __________________________________________________________________________ > > > > Instead, inside netdev_dpdk_reconfigure() I think it should be correct to > have > > > > err = netdev_dpdk_mempool_configure(dev); > > if (err && err != EEXIST) { > > goto out; > > } > > > because as in the case of a new mempool just created as in the case of > EEXIST (=17) > > we don't want to execute "goto out" and fall through to do > > > > dev->rxq_size = dev->requested_rxq_size; > > dev->txq_size = dev->requested_txq_size; > > ... > > > > With these code changes it seems to work, at the beginning I have MTU=1500 > and I set it to 1000: > > > > # ovs-vsctl list interface dpdk0 | grep mtu > > mtu : 1500 > > mtu_request : [] > > > > # ovs-vsctl set interface dpdk0 mtu_request=1000 > > my log says > > netdev_dpdk|DBG|Requesting a mempool of 40992 mbufs for netdev dpdk0 with 1 > Rx and 11 Tx queues, socket id:0. > > netdev_dpdk|DBG|A mempool with name ovs_62a2ca2f_0_2030_40992 already exists > at 0x7fad9d77dd00. > > dpdk|INFO|PMD: ixgbe_dev_link_status_print(): Port 0: Link Up - speed 0 Mbps > - half-duplex > > > > # ovs-vsctl list interface dpdk0 | grep mtu > > mtu : 1000 > > mtu_request : 1000 > > > > Does that make sense? > > > > Looks ok to me. > > Kevin. > > > Thanks, > > -Antonio > > > >> > >> Kevin. > >> > >>> Antonio > >>> > >>>> -----Original Message----- > >>>> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > >> bounces@openvswitch.org] > >>>> On Behalf Of Fischetti, Antonio > >>>> Sent: Wednesday, October 11, 2017 9:04 AM > >>>> To: Róbert Mulik <robert.mulik@ericsson.com>; dev@openvswitch.org > >>>> Subject: Re: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management > >> with > >>>> vhu client. > >>>> > >>>> Thanks Robert for reporting this and for all the clear details you > provided. > >>>> I'll look into this and get back to you. > >>>> > >>>> Antonio > >>>> > >>>>> -----Original Message----- > >>>>> From: Róbert Mulik [mailto:robert.mulik@ericsson.com] > >>>>> Sent: Tuesday, October 10, 2017 4:19 PM > >>>>> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org > >>>>> Subject: RE: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management > >>>> with > >>>>> vhu client. > >>>>> > >>>>> Hi Antonio, > >>>>> > >>>>> Last week I run into this mempool issue during the development of a new > >>>>> feature. I have made a bugfix, but then we saw yours too, so I tested if > it > >>>>> solves my problem. It did, but I realized another problem with it. The > >>>> mempool > >>>>> name generation is partly based on the MTU size, which is handled in 1024 > >>>> bytes > >>>>> long ranges. For example MTU 1000 and 1500 are in the same range, 2000 > and > >>>> 2500 > >>>>> are in a different range. So I tested this patch and got the following. > >>>>> > >>>>> # ovs-vsctl list interface dpdk0 |grep mtu > >>>>> mtu : 2500 > >>>>> mtu_request : 2500 > >>>>> # ovs-vsctl set interface dpdk0 mtu_request=2000 > >>>>> # ovs-vsctl list interface dpdk0 |grep mtu > >>>>> mtu : 2500 > >>>>> mtu_request : 2000 > >>>>> # ovs-vsctl set interface dpdk0 mtu_request=1500 > >>>>> # ovs-vsctl list interface dpdk0 |grep mtu > >>>>> mtu : 1500 > >>>>> mtu_request : 1500 > >>>>> # ovs-vsctl set interface dpdk0 mtu_request=1000 > >>>>> # ovs-vsctl list interface dpdk0 |grep mtu > >>>>> mtu : 1500 > >>>>> mtu_request : 1000 > >>>>> # ovs-vsctl set interface dpdk0 mtu_request=2000 > >>>>> # ovs-vsctl list interface dpdk0 |grep mtu > >>>>> mtu : 2000 > >>>>> mtu_request : 2000 > >>>>> # ovs-vsctl set interface dpdk0 mtu_request=1000 > >>>>> # ovs-vsctl list interface dpdk0 |grep mtu > >>>>> mtu : 1000 > >>>>> mtu_request : 1000 > >>>>> # ovs-vsctl set interface dpdk0 mtu_request=1500 > >>>>> # ovs-vsctl list interface dpdk0 |grep mtu > >>>>> mtu : 1000 > >>>>> mtu_request : 1500 > >>>>> # service openvswitch-switch restart > >>>>> # ovs-vsctl list interface dpdk0 |grep mtu > >>>>> mtu : 1500 > >>>>> mtu_request : 1500 > >>>>> > >>>>> > >>>>> This was my setup: > >>>>> Bridge br-prv > >>>>> Port bond-prv > >>>>> Interface "dpdk0" > >>>>> type: dpdk > >>>>> options: {dpdk-devargs="0000:05:00.0", n_rxq_desc="1024", > >>>>> n_txq_desc="1024"} > >>>>> ovs_version: "2.8.90" > >>>>> > >>>>> And I used DPDK v17.08. > >>>>> > >>>>> > >>>>> So, as it can be see from the example above, with the patch applied when > a > >>>> new > >>>>> mtu_request is in the same range as the previously set MTU, then it has > no > >>>>> effect until service restart. The mtu_request has immediate effect when > it > >> is > >>>>> in different range as the previously set MTU. Or did I miss something > >> during > >>>>> the testing? > >>>>> > >>>>> My patch what I used last week does the following. During reconfiguration > >> the > >>>>> mempool is always deleted before a new one is created. It solved the > >> problem > >>>>> without side effects, but it is not optimized (always recreates the > mempool > >>>>> when this function is called). > >>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > >>>>> index c60f46f..de38f95 100644 > >>>>> --- a/lib/netdev-dpdk.c > >>>>> +++ b/lib/netdev-dpdk.c > >>>>> @@ -621,6 +621,7 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk > *dev) > >>>>> uint32_t buf_size = dpdk_buf_size(dev->requested_mtu); > >>>>> struct dpdk_mp *mp; > >>>>> > >>>>> + dpdk_mp_put(dev->dpdk_mp); > >>>>> mp = dpdk_mp_get(dev, FRAME_LEN_TO_MTU(buf_size)); > >>>>> if (!mp) { > >>>>> VLOG_ERR("Failed to create memory pool for netdev " > >>>>> @@ -629,7 +630,6 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk > *dev) > >>>>> rte_strerror(rte_errno)); > >>>>> return rte_errno; > >>>>> } else { > >>>>> - dpdk_mp_put(dev->dpdk_mp); > >>>>> dev->dpdk_mp = mp; > >>>>> dev->mtu = dev->requested_mtu; > >>>>> dev->socket_id = dev->requested_socket_id; > >>>>> > >>>>> > >>>>> What do you think about this solution? > >>>>> > >>>>> Regards, > >>>>> Robert > >>>> _______________________________________________ > >>>> dev mailing list > >>>> dev@openvswitch.org > >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index c60f46f..80a6ff3 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -508,7 +508,7 @@ 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) { @@ -530,8 +530,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 +557,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,7 +571,7 @@ 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); @@ -581,12 +579,12 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu) } 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; @@ -620,14 +618,17 @@ 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 = false; - 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) { + return EEXIST; } else { dpdk_mp_put(dev->dpdk_mp); dev->dpdk_mp = mp; @@ -3207,7 +3208,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 +3248,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;
In a PVP test where vhostuser ports are configured as clients, OvS crashes when QEMU is launched. This patch avoids to call dpdk_mp_put() - and erroneously release the mempool - when it already exists. CC: Kevin Traynor <ktraynor@redhat.com> CC: Aaron Conole <aconole@redhat.com> Reported-by: Ciara Loftus <ciara.loftus@intel.com> Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each port.") Signed-off-by: Antonio Fischetti <antonio.fischetti@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 To replicate the bug scenario: 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 | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)