[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