[ovs-dev,v5,4/6] netdev-dpdk: assert mempool names.

Message ID 1507737656-31627-5-git-send-email-antonio.fischetti@intel.com
State New
Headers show
Series
  • netdev-dpdk: Fix management of pre-existing mempools.
Related show

Commit Message

Fischetti, Antonio Oct. 11, 2017, 4 p.m.
Replace if statement with an assert.

CC: Darrell Ball <dlu998@gmail.com>
CC: Ciara Loftus <ciara.loftus@intel.com>
CC: Kevin Traynor <ktraynor@redhat.com>
CC: Aaron Conole <aconole@redhat.com>
Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
---
 lib/netdev-dpdk.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Mark Kavanagh Oct. 13, 2017, 2:47 p.m. | #1
>From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org]
>On Behalf Of antonio.fischetti@intel.com
>Sent: Wednesday, October 11, 2017 5:01 PM
>To: dev@openvswitch.org
>Subject: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names.
>
>Replace if statement with an assert.
>
>CC: Darrell Ball <dlu998@gmail.com>
>CC: Ciara Loftus <ciara.loftus@intel.com>
>CC: Kevin Traynor <ktraynor@redhat.com>
>CC: Aaron Conole <aconole@redhat.com>
>Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>

Hey Antonio,

As per my previous comments, I believe that this patch should not be included in the patchset.

Other than that, two comments inline below.

Thanks,
Mark


>---
> lib/netdev-dpdk.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
>diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>index 2aa4a55..c451bc9 100644
>--- a/lib/netdev-dpdk.c
>+++ b/lib/netdev-dpdk.c
>@@ -501,9 +501,7 @@ dpdk_mp_name(struct dpdk_mp *dmp)
>     char *mp_name = xcalloc(RTE_MEMPOOL_NAMESIZE, sizeof *mp_name);
>     int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
>                        h, dmp->socket_id, dmp->mtu, dmp->mp_size);
>-    if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
>-        return NULL;
>-    }
>+    ovs_assert(ret >= 0 && ret < RTE_MEMPOOL_NAMESIZE);

The behavior of ovs_assert in the event of failure is to abort execution - are you sure that's what you want to happen here?

In the previous implementation, NULL was returned, and execution continued; are there some undesired side effects of same that you're trying to avoid with this change?


>     return mp_name;
> }
>
>--
>2.4.11
>
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Fischetti, Antonio Oct. 13, 2017, 3:47 p.m. | #2
> -----Original Message-----
> From: Kavanagh, Mark B
> Sent: Friday, October 13, 2017 3:47 PM
> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
> Subject: RE: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names.
> 
> >From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org]
> >On Behalf Of antonio.fischetti@intel.com
> >Sent: Wednesday, October 11, 2017 5:01 PM
> >To: dev@openvswitch.org
> >Subject: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names.
> >
> >Replace if statement with an assert.
> >
> >CC: Darrell Ball <dlu998@gmail.com>
> >CC: Ciara Loftus <ciara.loftus@intel.com>
> >CC: Kevin Traynor <ktraynor@redhat.com>
> >CC: Aaron Conole <aconole@redhat.com>
> >Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> 
> Hey Antonio,
> 
> As per my previous comments, I believe that this patch should not be included
> in the patchset.
> 
> Other than that, two comments inline below.
> 
> Thanks,
> Mark
> 
> 
> >---
> > lib/netdev-dpdk.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> >index 2aa4a55..c451bc9 100644
> >--- a/lib/netdev-dpdk.c
> >+++ b/lib/netdev-dpdk.c
> >@@ -501,9 +501,7 @@ dpdk_mp_name(struct dpdk_mp *dmp)
> >     char *mp_name = xcalloc(RTE_MEMPOOL_NAMESIZE, sizeof *mp_name);
> >     int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
> >                        h, dmp->socket_id, dmp->mtu, dmp->mp_size);
> >-    if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
> >-        return NULL;
> >-    }
> >+    ovs_assert(ret >= 0 && ret < RTE_MEMPOOL_NAMESIZE);
> 
> The behavior of ovs_assert in the event of failure is to abort execution - are
> you sure that's what you want to happen here?
> 
> In the previous implementation, NULL was returned, and execution continued; are
> there some undesired side effects of same that you're trying to avoid with this
> change?

[Antonio] Actually I had originally just added a VLOG_ERR before returning NULL like:

     if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
+        VLOG_ERR("Failed to generate a mempool name for \"%s\". "
+                "Hash:0x%x, mtu:%d, mbufs:%u",
+                dmp->if_name, h, dmp->mtu, dmp->mp_size);
         return NULL;
     }

Then this change to use an assert came after the discussion with Darrell and makes sense 
to me because if the mempool name is not generated - for sure it's an unlikely event - we 
cannot call the rte_pktmbuf_pool_create() which uses that name as a unique identifier
for the mempool. That's why we agreed to use an assert. What do you think?

> 
> 
> >     return mp_name;
> > }
> >
> >--
> >2.4.11
> >
> >_______________________________________________
> >dev mailing list
> >dev@openvswitch.org
> >https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Mark Kavanagh Oct. 13, 2017, 4:06 p.m. | #3
>From: Fischetti, Antonio
>Sent: Friday, October 13, 2017 4:47 PM
>To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; dev@openvswitch.org
>Subject: RE: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names.
>
>
>
>> -----Original Message-----
>> From: Kavanagh, Mark B
>> Sent: Friday, October 13, 2017 3:47 PM
>> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
>> Subject: RE: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names.
>>
>> >From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
>bounces@openvswitch.org]
>> >On Behalf Of antonio.fischetti@intel.com
>> >Sent: Wednesday, October 11, 2017 5:01 PM
>> >To: dev@openvswitch.org
>> >Subject: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names.
>> >
>> >Replace if statement with an assert.
>> >
>> >CC: Darrell Ball <dlu998@gmail.com>
>> >CC: Ciara Loftus <ciara.loftus@intel.com>
>> >CC: Kevin Traynor <ktraynor@redhat.com>
>> >CC: Aaron Conole <aconole@redhat.com>
>> >Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
>>
>> Hey Antonio,
>>
>> As per my previous comments, I believe that this patch should not be
>included
>> in the patchset.
>>
>> Other than that, two comments inline below.
>>
>> Thanks,
>> Mark
>>
>>
>> >---
>> > lib/netdev-dpdk.c | 4 +---
>> > 1 file changed, 1 insertion(+), 3 deletions(-)
>> >
>> >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> >index 2aa4a55..c451bc9 100644
>> >--- a/lib/netdev-dpdk.c
>> >+++ b/lib/netdev-dpdk.c
>> >@@ -501,9 +501,7 @@ dpdk_mp_name(struct dpdk_mp *dmp)
>> >     char *mp_name = xcalloc(RTE_MEMPOOL_NAMESIZE, sizeof *mp_name);
>> >     int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
>> >                        h, dmp->socket_id, dmp->mtu, dmp->mp_size);
>> >-    if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
>> >-        return NULL;
>> >-    }
>> >+    ovs_assert(ret >= 0 && ret < RTE_MEMPOOL_NAMESIZE);
>>
>> The behavior of ovs_assert in the event of failure is to abort execution -
>are
>> you sure that's what you want to happen here?
>>
>> In the previous implementation, NULL was returned, and execution continued;
>are
>> there some undesired side effects of same that you're trying to avoid with
>this
>> change?
>
>[Antonio] Actually I had originally just added a VLOG_ERR before returning
>NULL like:
>
>     if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
>+        VLOG_ERR("Failed to generate a mempool name for \"%s\". "
>+                "Hash:0x%x, mtu:%d, mbufs:%u",
>+                dmp->if_name, h, dmp->mtu, dmp->mp_size);
>         return NULL;
>     }
>
>Then this change to use an assert came after the discussion with Darrell and
>makes sense
>to me because if the mempool name is not generated - for sure it's an unlikely
>event - we
>cannot call the rte_pktmbuf_pool_create() which uses that name as a unique
>identifier
>for the mempool. That's why we agreed to use an assert. What do you think?

I'd approach it as follows:
- [dpdk_mp_name]   return NULL if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) ; don't log failure here, as that's handled by netdev_dpdk_mempool_create()
- [dpdk_mp_create] check for a NULL return value from dpdk_mp_name; return same to caller
- [dpdk_mp_get]    return NULL  (no changes needed here)
- [netdev_dpdk_mempool_create] return value of NULL yields an error log and returns rte_errno to the caller (no changes needed here).

At least with this approach, execution can continue, instead of shutting the application abnormally.

Thanks,
Mark    


>
>>
>>
>> >     return mp_name;
>> > }
>> >
>> >--
>> >2.4.11
>> >
>> >_______________________________________________
>> >dev mailing list
>> >dev@openvswitch.org
>> >https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Fischetti, Antonio Oct. 13, 2017, 4:55 p.m. | #4
+ CC Darrell to the loop because we already discussed about this.

> -----Original Message-----
> From: Kavanagh, Mark B
> Sent: Friday, October 13, 2017 5:07 PM
> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
> Subject: RE: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names.
> 
> >From: Fischetti, Antonio
> >Sent: Friday, October 13, 2017 4:47 PM
> >To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; dev@openvswitch.org
> >Subject: RE: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names.
> >
> >
> >
> >> -----Original Message-----
> >> From: Kavanagh, Mark B
> >> Sent: Friday, October 13, 2017 3:47 PM
> >> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
> >> Subject: RE: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names.
> >>
> >> >From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> >bounces@openvswitch.org]
> >> >On Behalf Of antonio.fischetti@intel.com
> >> >Sent: Wednesday, October 11, 2017 5:01 PM
> >> >To: dev@openvswitch.org
> >> >Subject: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names.
> >> >
> >> >Replace if statement with an assert.
> >> >
> >> >CC: Darrell Ball <dlu998@gmail.com>
> >> >CC: Ciara Loftus <ciara.loftus@intel.com>
> >> >CC: Kevin Traynor <ktraynor@redhat.com>
> >> >CC: Aaron Conole <aconole@redhat.com>
> >> >Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> >>
> >> Hey Antonio,
> >>
> >> As per my previous comments, I believe that this patch should not be
> >included
> >> in the patchset.
> >>
> >> Other than that, two comments inline below.
> >>
> >> Thanks,
> >> Mark
> >>
> >>
> >> >---
> >> > lib/netdev-dpdk.c | 4 +---
> >> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >> >
> >> >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> >> >index 2aa4a55..c451bc9 100644
> >> >--- a/lib/netdev-dpdk.c
> >> >+++ b/lib/netdev-dpdk.c
> >> >@@ -501,9 +501,7 @@ dpdk_mp_name(struct dpdk_mp *dmp)
> >> >     char *mp_name = xcalloc(RTE_MEMPOOL_NAMESIZE, sizeof *mp_name);
> >> >     int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
> >> >                        h, dmp->socket_id, dmp->mtu, dmp->mp_size);
> >> >-    if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
> >> >-        return NULL;
> >> >-    }
> >> >+    ovs_assert(ret >= 0 && ret < RTE_MEMPOOL_NAMESIZE);
> >>
> >> The behavior of ovs_assert in the event of failure is to abort execution -
> >are
> >> you sure that's what you want to happen here?
> >>
> >> In the previous implementation, NULL was returned, and execution continued;
> >are
> >> there some undesired side effects of same that you're trying to avoid with
> >this
> >> change?
> >
> >[Antonio] Actually I had originally just added a VLOG_ERR before returning
> >NULL like:
> >
> >     if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
> >+        VLOG_ERR("Failed to generate a mempool name for \"%s\". "
> >+                "Hash:0x%x, mtu:%d, mbufs:%u",
> >+                dmp->if_name, h, dmp->mtu, dmp->mp_size);
> >         return NULL;
> >     }
> >
> >Then this change to use an assert came after the discussion with Darrell and
> >makes sense
> >to me because if the mempool name is not generated - for sure it's an unlikely
> >event - we
> >cannot call the rte_pktmbuf_pool_create() which uses that name as a unique
> >identifier
> >for the mempool. That's why we agreed to use an assert. What do you think?
> 
> I'd approach it as follows:
> - [dpdk_mp_name]   return NULL if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) ;
> don't log failure here, as that's handled by netdev_dpdk_mempool_create()
> - [dpdk_mp_create] check for a NULL return value from dpdk_mp_name; return same
> to caller
> - [dpdk_mp_get]    return NULL  (no changes needed here)
> - [netdev_dpdk_mempool_create] return value of NULL yields an error log and
> returns rte_errno to the caller (no changes needed here).
> 
> At least with this approach, execution can continue, instead of shutting the
> application abnormally.

[Antonio] Yes, makes sense and looks much better, thanks! 
TBH I'd keep the VLOG_ERR msg inside dpdk_mp_name otherwise this particular
failure wouldn't be clearly tracked by netdev_dpdk_mempool_configure().
Then inside dpdk_mp_create() something like:

    do {
        char *mp_name = dpdk_mp_name(dmp);
+       if (!mp_name) {
+	     // to track this particular failure, a log message here or inside dpdk_mp_name?
+           rte_free(dmp);
+           return NULL;
+       }
        VLOG_DBG("Requesting a mempool of %u mbufs for netdev %s "
                 "with %d Rx and %d Tx queues.",


> 
> Thanks,
> Mark
> 
> 
> >
> >>
> >>
> >> >     return mp_name;
> >> > }
> >> >
> >> >--
> >> >2.4.11
> >> >
> >> >_______________________________________________
> >> >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 2aa4a55..c451bc9 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -501,9 +501,7 @@  dpdk_mp_name(struct dpdk_mp *dmp)
     char *mp_name = xcalloc(RTE_MEMPOOL_NAMESIZE, sizeof *mp_name);
     int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
                        h, dmp->socket_id, dmp->mtu, dmp->mp_size);
-    if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
-        return NULL;
-    }
+    ovs_assert(ret >= 0 && ret < RTE_MEMPOOL_NAMESIZE);
     return mp_name;
 }