>From: Fischetti, Antonio >Sent: Friday, October 13, 2017 4:47 PM >To: Kavanagh, Mark B <[email protected]>; [email protected] >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 <[email protected]>; [email protected] >> Subject: RE: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names. >> >> >From: [email protected] [mailto:ovs-dev- >[email protected]] >> >On Behalf Of [email protected] >> >Sent: Wednesday, October 11, 2017 5:01 PM >> >To: [email protected] >> >Subject: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names. >> > >> >Replace if statement with an assert. >> > >> >CC: Darrell Ball <[email protected]> >> >CC: Ciara Loftus <[email protected]> >> >CC: Kevin Traynor <[email protected]> >> >CC: Aaron Conole <[email protected]> >> >Signed-off-by: Antonio Fischetti <[email protected]> >> >> 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 >> >[email protected] >> >https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
