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

Fischetti, Antonio antonio.fischetti at intel.com
Fri Oct 13 15:47:09 UTC 2017



> -----Original Message-----
> From: Kavanagh, Mark B
> Sent: Friday, October 13, 2017 3:47 PM
> To: Fischetti, Antonio <antonio.fischetti at intel.com>; dev at openvswitch.org
> Subject: RE: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names.
> 
> >From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-bounces at openvswitch.org]
> >On Behalf Of antonio.fischetti at intel.com
> >Sent: Wednesday, October 11, 2017 5:01 PM
> >To: dev at openvswitch.org
> >Subject: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names.
> >
> >Replace if statement with an assert.
> >
> >CC: Darrell Ball <dlu998 at gmail.com>
> >CC: Ciara Loftus <ciara.loftus at intel.com>
> >CC: Kevin Traynor <ktraynor at redhat.com>
> >CC: Aaron Conole <aconole at redhat.com>
> >Signed-off-by: Antonio Fischetti <antonio.fischetti at 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 at openvswitch.org
> >https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list