[ovs-dev,v2,3/3] netdev-dpdk: Fix mempool creation with large MTU.

Message ID 1510297926-18710-4-git-send-email-i.maximets@samsung.com
State New
Headers show
Series
  • netdev-dpdk: Mempool creation failure + Appctl
Related show

Commit Message

Ilya Maximets Nov. 10, 2017, 7:12 a.m.
Currently mempool name size limited to 25 characters by
RTE_MEMPOOL_NAMESIZE. netdev-dpdk tries to create mempool with the
following name pattern: "ovs_%{hash}_%{socket}_%{mtu}_%{n_mbuf}".

We have 3 chars for "ovs" + 4 chars for delimiters + 8 chars for
hash (because it's the 32 bit integer printed in hex) + 1 char for
socket_id (mostly 1, but it could be 2 on some systems; larger?) = 16.

Only 25 - 16 = 9 characters remains for mtu + n_mbufs.
Minimum usual value for mtu is 1500 --> 2030 (4 chars) after
dpdk_buf_size conversion and the minimum value for n_mbufs is 16384
(5 chars). So, all the 9 characters are used.

If we'll try to create port with mtu = 9500, mempool creation will
fail, because FRAME_LEN_TO_MTU(dpdk_buf_size(9500)) = 10222 (5 chars)
and this value will overflow the RTE_MEMPOOL_NAMESIZE limit.

Same issue will happen if we'll try to create port with big enough
number of queues or will try to create big enough number of PMD
threads (number of tx queues will enlarge the mempool requirements).

Fix that by removing the delimiters. To keep the readability (at least
partial) of the mempool names exact field sizes with zero padding
are used.

Following limits should be suitable for now:
 - Hash length: 8 chars (uint32_t in hex)
 - Socket ID  : 2 chars (For systems with up to 10 sockets)
 - MTU        : 5 chars (MTU (10^5 - 1) should be enough for now)
 - n_mbufs    : 7 chars (Up to 10^7 of mbufs)

   Total      : 22 + 3 (for "ovs") = 25

CC: Antonio Fischetti <antonio.fischetti@intel.com>
CC: Robert Wojciechowicz <robertx.wojciechowicz@intel.com>
Fixes: f06546a51dd8 ("Fix mempool names to reflect socket id.")
Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each port.")
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
Acked-by: Antonio Fischetti <antonio.fischetti@intel.com>
---
 lib/netdev-dpdk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Mark Kavanagh Nov. 15, 2017, 10:48 a.m. | #1
>From: Ilya Maximets [mailto:i.maximets@samsung.com]
>Sent: Friday, November 10, 2017 7:12 AM
>To: ovs-dev@openvswitch.org
>Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Fischetti, Antonio
><antonio.fischetti@intel.com>; Loftus, Ciara <ciara.loftus@intel.com>;
>Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Stokes, Ian
><ian.stokes@intel.com>; Wojciechowicz, RobertX
><robertx.wojciechowicz@intel.com>; Flavio Leitner <fbl@redhat.com>; Ilya
>Maximets <i.maximets@samsung.com>
>Subject: [PATCH v2 3/3] netdev-dpdk: Fix mempool creation with large MTU.
>
>Currently mempool name size limited to 25 characters by
>RTE_MEMPOOL_NAMESIZE. netdev-dpdk tries to create mempool with the
>following name pattern: "ovs_%{hash}_%{socket}_%{mtu}_%{n_mbuf}".
>
>We have 3 chars for "ovs" + 4 chars for delimiters + 8 chars for
>hash (because it's the 32 bit integer printed in hex) + 1 char for
>socket_id (mostly 1, but it could be 2 on some systems; larger?) = 16.
>
>Only 25 - 16 = 9 characters remains for mtu + n_mbufs.
>Minimum usual value for mtu is 1500 --> 2030 (4 chars) after
>dpdk_buf_size conversion and the minimum value for n_mbufs is 16384
>(5 chars). So, all the 9 characters are used.
>
>If we'll try to create port with mtu = 9500, mempool creation will
>fail, because FRAME_LEN_TO_MTU(dpdk_buf_size(9500)) = 10222 (5 chars)
>and this value will overflow the RTE_MEMPOOL_NAMESIZE limit.
>
>Same issue will happen if we'll try to create port with big enough
>number of queues or will try to create big enough number of PMD
>threads (number of tx queues will enlarge the mempool requirements).
>
>Fix that by removing the delimiters. To keep the readability (at least
>partial) of the mempool names exact field sizes with zero padding
>are used.
>
>Following limits should be suitable for now:
> - Hash length: 8 chars (uint32_t in hex)
> - Socket ID  : 2 chars (For systems with up to 10 sockets)
> - MTU        : 5 chars (MTU (10^5 - 1) should be enough for now)
> - n_mbufs    : 7 chars (Up to 10^7 of mbufs)
>
>   Total      : 22 + 3 (for "ovs") = 25
>
>CC: Antonio Fischetti <antonio.fischetti@intel.com>
>CC: Robert Wojciechowicz <robertx.wojciechowicz@intel.com>
>Fixes: f06546a51dd8 ("Fix mempool names to reflect socket id.")
>Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each
>port.")
>Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>Acked-by: Antonio Fischetti <antonio.fischetti@intel.com>

Thanks for this fix Ilya.

I've tested this, and can confirm that it resolves a SEGV that previously occurred in the case of 'large' MTU values (in the case of my test, 9710).

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

Best,
Mark

>---
> lib/netdev-dpdk.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>index 75856bc..dc210cc 100644
>--- a/lib/netdev-dpdk.c
>+++ b/lib/netdev-dpdk.c
>@@ -510,7 +510,8 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu)
>     do {
>         /* Full DPDK memory pool name must be unique and cannot be
>          * longer than RTE_MEMPOOL_NAMESIZE. */
>-        int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
>+        int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
>+                           "ovs%08x%02d%05d%07u",
>                            hash, socket_id, mtu, n_mbufs);
>         if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
>             VLOG_DBG("snprintf returned %d. "
>--
>2.7.4
Stokes, Ian Nov. 15, 2017, 3:09 p.m. | #2
> Thanks for this fix Ilya.
> 
> I've tested this, and can confirm that it resolves a SEGV that previously
> occurred in the case of 'large' MTU values (in the case of my test, 9710).
> 
> Acked-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> Tested-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> 

Thanks all for the work on this, I'll look to push this to the dpdk merge branch today.

Ian
> Best,
> Mark
> 
> >---
> > lib/netdev-dpdk.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> >75856bc..dc210cc 100644
> >--- a/lib/netdev-dpdk.c
> >+++ b/lib/netdev-dpdk.c
> >@@ -510,7 +510,8 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu)
> >     do {
> >         /* Full DPDK memory pool name must be unique and cannot be
> >          * longer than RTE_MEMPOOL_NAMESIZE. */
> >-        int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
> "ovs_%x_%d_%d_%u",
> >+        int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
> >+                           "ovs%08x%02d%05d%07u",
> >                            hash, socket_id, mtu, n_mbufs);
> >         if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
> >             VLOG_DBG("snprintf returned %d. "
> >--
> >2.7.4

Patch

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 75856bc..dc210cc 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -510,7 +510,8 @@  dpdk_mp_create(struct netdev_dpdk *dev, int mtu)
     do {
         /* Full DPDK memory pool name must be unique and cannot be
          * longer than RTE_MEMPOOL_NAMESIZE. */
-        int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
+        int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
+                           "ovs%08x%02d%05d%07u",
                            hash, socket_id, mtu, n_mbufs);
         if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
             VLOG_DBG("snprintf returned %d. "