[ovs-dev,v3,1/5] netdev-dpdk: fix mempool management with vhu client.

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.
Related show

Commit Message

Fischetti, Antonio Oct. 5, 2017, 3:52 p.m.
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(-)

Comments

Ciara Loftus Oct. 6, 2017, 10:39 a.m. | #1
> 
> 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
Fischetti, Antonio Oct. 6, 2017, 12:39 p.m. | #2
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
Róbert Mulik Oct. 10, 2017, 3:18 p.m. | #3
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
Fischetti, Antonio Oct. 11, 2017, 8:03 a.m. | #4
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
Fischetti, Antonio Oct. 11, 2017, 10:28 a.m. | #5
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
Kevin Traynor Oct. 11, 2017, 10:40 a.m. | #6
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
Fischetti, Antonio Oct. 11, 2017, 1:04 p.m. | #7
> -----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
Kevin Traynor Oct. 11, 2017, 1:37 p.m. | #8
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
>
Fischetti, Antonio Oct. 11, 2017, 2:21 p.m. | #9
> -----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

> >

Patch

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;