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

[Fischetti, Antonio]

+ 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 at intel.com>; dev at 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 at intel.com>; dev at 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 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?
> 
> 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 at openvswitch.org
> >> >https://mail.openvswitch.org/mailman/listinfo/ovs-dev