| Message ID | 1507737656-31627-5-git-send-email-antonio.fischetti@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | netdev-dpdk: Fix management of pre-existing mempools. | expand |
>From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] >On Behalf Of antonio.fischetti@intel.com >Sent: Wednesday, October 11, 2017 5:01 PM >To: dev@openvswitch.org >Subject: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names. > >Replace if statement with an assert. > >CC: Darrell Ball <dlu998@gmail.com> >CC: Ciara Loftus <ciara.loftus@intel.com> >CC: Kevin Traynor <ktraynor@redhat.com> >CC: Aaron Conole <aconole@redhat.com> >Signed-off-by: Antonio Fischetti <antonio.fischetti@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? > return mp_name; > } > >-- >2.4.11 > >_______________________________________________ >dev mailing list >dev@openvswitch.org >https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> -----Original Message----- > From: Kavanagh, Mark B > Sent: Friday, October 13, 2017 3:47 PM > To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org > Subject: RE: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names. > > >From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] > >On Behalf Of antonio.fischetti@intel.com > >Sent: Wednesday, October 11, 2017 5:01 PM > >To: dev@openvswitch.org > >Subject: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names. > > > >Replace if statement with an assert. > > > >CC: Darrell Ball <dlu998@gmail.com> > >CC: Ciara Loftus <ciara.loftus@intel.com> > >CC: Kevin Traynor <ktraynor@redhat.com> > >CC: Aaron Conole <aconole@redhat.com> > >Signed-off-by: Antonio Fischetti <antonio.fischetti@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@openvswitch.org > >https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>From: Fischetti, Antonio >Sent: Friday, October 13, 2017 4:47 PM >To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; dev@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@intel.com>; dev@openvswitch.org >> Subject: RE: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names. >> >> >From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- >bounces@openvswitch.org] >> >On Behalf Of antonio.fischetti@intel.com >> >Sent: Wednesday, October 11, 2017 5:01 PM >> >To: dev@openvswitch.org >> >Subject: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names. >> > >> >Replace if statement with an assert. >> > >> >CC: Darrell Ball <dlu998@gmail.com> >> >CC: Ciara Loftus <ciara.loftus@intel.com> >> >CC: Kevin Traynor <ktraynor@redhat.com> >> >CC: Aaron Conole <aconole@redhat.com> >> >Signed-off-by: Antonio Fischetti <antonio.fischetti@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@openvswitch.org >> >https://mail.openvswitch.org/mailman/listinfo/ovs-dev
+ 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@intel.com>; dev@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@intel.com>; dev@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@intel.com>; dev@openvswitch.org > >> Subject: RE: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names. > >> > >> >From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > >bounces@openvswitch.org] > >> >On Behalf Of antonio.fischetti@intel.com > >> >Sent: Wednesday, October 11, 2017 5:01 PM > >> >To: dev@openvswitch.org > >> >Subject: [ovs-dev] [PATCH v5 4/6] netdev-dpdk: assert mempool names. > >> > > >> >Replace if statement with an assert. > >> > > >> >CC: Darrell Ball <dlu998@gmail.com> > >> >CC: Ciara Loftus <ciara.loftus@intel.com> > >> >CC: Kevin Traynor <ktraynor@redhat.com> > >> >CC: Aaron Conole <aconole@redhat.com> > >> >Signed-off-by: Antonio Fischetti <antonio.fischetti@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@openvswitch.org > >> >https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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); return mp_name; }
Replace if statement with an assert. CC: Darrell Ball <dlu998@gmail.com> CC: Ciara Loftus <ciara.loftus@intel.com> CC: Kevin Traynor <ktraynor@redhat.com> CC: Aaron Conole <aconole@redhat.com> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com> --- lib/netdev-dpdk.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)