[ovs-dev] netdev-dpdk: fix mempool_configure error state

Message ID 1489399917-192190-1-git-send-email-mark.b.kavanagh@intel.com
State Superseded
Headers show

Commit Message

Mark Kavanagh March 13, 2017, 10:11 a.m.
netdev_dpdk_mempool_configure obtains a handle to a
DPDK memory pool via a call to dpdk_mp_get. If dpdk_mp_get
fails, the former informs the user that insufficient memory
is available, and  returns ENOMEM. However, this is
potentially misleading, as there are a number of reasons why
creation of a mempool can fail (as per rte_mempool_create),
including:
   - insufficient memory available
   - mempool already exists
   - other memory allocation error

Update the error log to reflect this fact, and return rte_errno
in the event of error, instead of ENOMEM.

Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
Fixes: 0072e931 ("netdev-dpdk: add support for jumbo frames")
---
 lib/netdev-dpdk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Stokes, Ian March 13, 2017, 10:58 a.m. | #1
> netdev_dpdk_mempool_configure obtains a handle to a DPDK memory pool via a
> call to dpdk_mp_get. If dpdk_mp_get fails, the former informs the user
> that insufficient memory is available, and  returns ENOMEM. However, this
> is potentially misleading, as there are a number of reasons why creation
> of a mempool can fail (as per rte_mempool_create),
> including:
>    - insufficient memory available
>    - mempool already exists
>    - other memory allocation error
> 
> Update the error log to reflect this fact, and return rte_errno in the
> event of error, instead of ENOMEM.
> 
> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> Fixes: 0072e931 ("netdev-dpdk: add support for jumbo frames")
> ---
>  lib/netdev-dpdk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index ddc651b..7eeced8
> 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -571,10 +571,10 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk
> *dev)
> 
>      mp = dpdk_mp_get(dev->requested_socket_id,
> FRAME_LEN_TO_MTU(buf_size));
>      if (!mp) {
> -        VLOG_ERR("Insufficient memory to create memory pool for netdev "
> +        VLOG_ERR("Failed to create memory pool for netdev "
>                   "%s, with MTU %d on socket %d\n",
>                   dev->up.name, dev->requested_mtu, dev-
> >requested_socket_id);
> -        return ENOMEM;
> +        return rte_errno;

Hi Mark,

It could be useful to give an explanation to users for the failure, maybe a call to rte_strerror(rte_errno) in the VLOG_ERR above? Otherwise the cause of the failure is a bit ambiguous.

Ian

>      } else {
>          dpdk_mp_put(dev->dpdk_mp);
>          dev->dpdk_mp = mp;
> --
> 1.9.3
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Mark Kavanagh March 13, 2017, 11:36 a.m. | #2
>
>> netdev_dpdk_mempool_configure obtains a handle to a DPDK memory pool via a
>> call to dpdk_mp_get. If dpdk_mp_get fails, the former informs the user
>> that insufficient memory is available, and  returns ENOMEM. However, this
>> is potentially misleading, as there are a number of reasons why creation
>> of a mempool can fail (as per rte_mempool_create),
>> including:
>>    - insufficient memory available
>>    - mempool already exists
>>    - other memory allocation error
>>
>> Update the error log to reflect this fact, and return rte_errno in the
>> event of error, instead of ENOMEM.
>>
>> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>> Fixes: 0072e931 ("netdev-dpdk: add support for jumbo frames")
>> ---
>>  lib/netdev-dpdk.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index ddc651b..7eeced8
>> 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -571,10 +571,10 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk
>> *dev)
>>
>>      mp = dpdk_mp_get(dev->requested_socket_id,
>> FRAME_LEN_TO_MTU(buf_size));
>>      if (!mp) {
>> -        VLOG_ERR("Insufficient memory to create memory pool for netdev "
>> +        VLOG_ERR("Failed to create memory pool for netdev "
>>                   "%s, with MTU %d on socket %d\n",
>>                   dev->up.name, dev->requested_mtu, dev-
>> >requested_socket_id);
>> -        return ENOMEM;
>> +        return rte_errno;
>
>Hi Mark,
>
>It could be useful to give an explanation to users for the failure, maybe a call to
>rte_strerror(rte_errno) in the VLOG_ERR above? Otherwise the cause of the failure is a bit
>ambiguous.
>
>Ian

Good call Ian - I added a call to rte_strerror in v2.

Cheers,
Mark

>
>>      } else {
>>          dpdk_mp_put(dev->dpdk_mp);
>>          dev->dpdk_mp = mp;
>> --
>> 1.9.3
>>
>> _______________________________________________
>> 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 ddc651b..7eeced8 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -571,10 +571,10 @@  netdev_dpdk_mempool_configure(struct netdev_dpdk *dev)
 
     mp = dpdk_mp_get(dev->requested_socket_id, FRAME_LEN_TO_MTU(buf_size));
     if (!mp) {
-        VLOG_ERR("Insufficient memory to create memory pool for netdev "
+        VLOG_ERR("Failed to create memory pool for netdev "
                  "%s, with MTU %d on socket %d\n",
                  dev->up.name, dev->requested_mtu, dev->requested_socket_id);
-        return ENOMEM;
+        return rte_errno;
     } else {
         dpdk_mp_put(dev->dpdk_mp);
         dev->dpdk_mp = mp;