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

Kavanagh, Mark B mark.b.kavanagh at intel.com
Fri Oct 13 16:06:45 UTC 2017


>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.

Thanks,
Mark    


>
>>
>>
>> >     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