| Message ID | 1517558582-27108-2-git-send-email-hemant.agrawal@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FIX-OPTION-1] mbuf: fix the logic of user mempool ops API | expand |
On Fri, Feb 02, 2018 at 01:33:01PM +0530, Hemant Agrawal wrote: > From: Nipun Gupta <nipun.gupta@nxp.com> > > The existing rte_eal_mbuf_default mempool ops can return the compile time > default ops name if the user has not provided command line inputs for > mempool ops name. It will break the logic of best mempool ops as it will > never return platform hw mempool ops. > > This patch introduces a new API to just return the user mempool ops only. > > Fixes: 8b0f7f434132 ("mbuf: maintain user and compile time mempool ops name") > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com> This option is fine for me. I think we may also consider deprecating rte_eal_mbuf_default_mempool_ops(), as it is done in option 2. Acked-by: Olivier Matz <olivier.matz@6wind.com>
Hi Olivier, > On Fri, Feb 02, 2018 at 01:33:01PM +0530, Hemant Agrawal wrote: > > From: Nipun Gupta <nipun.gupta@nxp.com> > > > > The existing rte_eal_mbuf_default mempool ops can return the compile > > time default ops name if the user has not provided command line inputs > > for mempool ops name. It will break the logic of best mempool ops as > > it will never return platform hw mempool ops. > > > > This patch introduces a new API to just return the user mempool ops only. > > > > Fixes: 8b0f7f434132 ("mbuf: maintain user and compile time mempool ops > > name") > > > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com> > > This option is fine for me. I think we may also consider deprecating > rte_eal_mbuf_default_mempool_ops(), as it is done in option 2. [Hemant] Ok. Please also ack following. I will update patchwork for remaining patches accordingly. [PATCH v2] doc: remove eal API for default mempool ops name > Acked-by: Olivier Matz <olivier.matz@6wind.com> Thanks Hemant
Hi Hemant, On Sunday 04 February 2018 11:52 AM, Shukla, Santosh wrote: > The existing rte_eal_mbuf_default mempool ops can return the compile time > default ops name if the user has not provided command line inputs for > mempool ops name. It will break the logic of best mempool ops as it will > never return platform hw mempool ops. > > This patch introduces a new API to just return the user mempool ops only. > > Fixes: 8b0f7f434132 ("mbuf: maintain user and compile time mempool ops name") > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com> > --- This patch introduces regression for octeontx platform. Fails with: --------- testpmd: create a new mbuf pool <mbuf_pool_socket_0>: n=163456, size=2176, socket=0 testpmd: preferred mempool ops selected: dpaa dpaa_mbuf_create_pool(): bman_new_pool() failed EAL: Error - exiting with code: 1 ---------- Command: ./testpmd -c 0xe00000 -n 4 -- --portmask 0x1 --nb-cores=2 --port-topology=loop --rxq=2 --txq=2 --rss-ip --no-flush-rx ------------- It's because rte_dpaa_bus_probe() sets platform mempool ops i.e. rte_mbuf_set_platform_mempool_ops(DPAA_MEMPOOL_OPS_NAME); therefore `rte_mbuf_best_mempool_ops` returns with `dpaa` ops which is incorrect mempool_ops choice for octeontx platform. We can fix such regression with following ways: # option 1) unset LIBRTE_DPAA_MEMPOOL=y from common_armv8a_linuxapp config and set at dpaa/dpaa2 specific configs commit: 1ee9569576f6 ("config: enable dpaaX drivers for generic ARMv8") # option 2) Don;t set platform mempool ops in dpaa bus layer instead defer it to driver layer. Thanks.
Thanks we also identified it. The fix is on the way. Sent from my Android phone using TouchDown (www.symantec.com) -----Original Message----- From: santosh [santosh.shukla@caviumnetworks.com] Received: Sunday, 04 Feb 2018, 12:05PM To: Hemant Agrawal [hemant.agrawal@nxp.com]; dev [dev@dpdk.org] CC: Olivier MATZ [olivier.matz@6wind.com]; Jerin Jacob [jerin.jacob@caviumnetworks.com]; Nipun Gupta [nipun.gupta@nxp.com] Subject: Re: Fw: [PATCH FIX-OPTION-1] mbuf: fix the logic of user mempool ops API Hi Hemant, On Sunday 04 February 2018 11:52 AM, Shukla, Santosh wrote: > The existing rte_eal_mbuf_default mempool ops can return the compile time > default ops name if the user has not provided command line inputs for > mempool ops name. It will break the logic of best mempool ops as it will > never return platform hw mempool ops. > > This patch introduces a new API to just return the user mempool ops only. > > Fixes: 8b0f7f434132 ("mbuf: maintain user and compile time mempool ops name") > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com> > --- This patch introduces regression for octeontx platform. Fails with: --------- testpmd: create a new mbuf pool <mbuf_pool_socket_0>: n=163456, size=2176, socket=0 testpmd: preferred mempool ops selected: dpaa dpaa_mbuf_create_pool(): bman_new_pool() failed EAL: Error - exiting with code: 1 ---------- Command: ./testpmd -c 0xe00000 -n 4 -- --portmask 0x1 --nb-cores=2 --port-topology=loop --rxq=2 --txq=2 --rss-ip --no-flush-rx ------------- It's because rte_dpaa_bus_probe() sets platform mempool ops i.e. rte_mbuf_set_platform_mempool_ops(DPAA_MEMPOOL_OPS_NAME); therefore `rte_mbuf_best_mempool_ops` returns with `dpaa` ops which is incorrect mempool_ops choice for octeontx platform. We can fix such regression with following ways: # option 1) unset LIBRTE_DPAA_MEMPOOL=y from common_armv8a_linuxapp config and set at dpaa/dpaa2 specific configs commit: 1ee9569576f6 ("config: enable dpaaX drivers for generic ARMv8") # option 2) Don;t set platform mempool ops in dpaa bus layer instead defer it to driver layer. Thanks.
02/02/2018 14:40, Olivier Matz: > On Fri, Feb 02, 2018 at 01:33:01PM +0530, Hemant Agrawal wrote: > > From: Nipun Gupta <nipun.gupta@nxp.com> > > > > The existing rte_eal_mbuf_default mempool ops can return the compile time > > default ops name if the user has not provided command line inputs for > > mempool ops name. It will break the logic of best mempool ops as it will > > never return platform hw mempool ops. > > > > This patch introduces a new API to just return the user mempool ops only. > > > > Fixes: 8b0f7f434132 ("mbuf: maintain user and compile time mempool ops name") > > > > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com> > > This option is fine for me. I think we may also consider deprecating > rte_eal_mbuf_default_mempool_ops(), as it is done in option 2. > > Acked-by: Olivier Matz <olivier.matz@6wind.com> Applied, thanks
diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c index 1622a41..4eafcb5 100644 --- a/lib/librte_eal/bsdapp/eal/eal.c +++ b/lib/librte_eal/bsdapp/eal/eal.c @@ -82,6 +82,13 @@ struct internal_config internal_config; /* used by rte_rdtsc() */ int rte_cycles_vmware_tsc_map; +/* Return user provided mbuf pool ops name */ +const char * __rte_experimental +rte_eal_mbuf_user_pool_ops(void) +{ + return internal_config.user_mbuf_pool_ops_name; +} + /* Return mbuf pool ops name */ const char * rte_eal_mbuf_default_mempool_ops(void) diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h index 08c6637..044474e 100644 --- a/lib/librte_eal/common/include/rte_eal.h +++ b/lib/librte_eal/common/include/rte_eal.h @@ -452,6 +452,18 @@ static inline int rte_gettid(void) enum rte_iova_mode rte_eal_iova_mode(void); /** + * @warning + * @b EXPERIMENTAL: this API may change without prior notice + * + * Get user provided pool ops name for mbuf + * + * @return + * returns user provided pool ops name. + */ +const char * __rte_experimental +rte_eal_mbuf_user_pool_ops(void); + +/** * Get default pool ops name for mbuf * * @return diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c index 451fdaf..38306bf 100644 --- a/lib/librte_eal/linuxapp/eal/eal.c +++ b/lib/librte_eal/linuxapp/eal/eal.c @@ -92,6 +92,13 @@ struct internal_config internal_config; /* used by rte_rdtsc() */ int rte_cycles_vmware_tsc_map; +/* Return user provided mbuf pool ops name */ +const char * __rte_experimental +rte_eal_mbuf_user_pool_ops(void) +{ + return internal_config.user_mbuf_pool_ops_name; +} + /* Return mbuf pool ops name */ const char * rte_eal_mbuf_default_mempool_ops(void) diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map index 4146907..2e6cbe9 100644 --- a/lib/librte_eal/rte_eal_version.map +++ b/lib/librte_eal/rte_eal_version.map @@ -220,6 +220,7 @@ EXPERIMENTAL { rte_eal_devargs_remove; rte_eal_hotplug_add; rte_eal_hotplug_remove; + rte_eal_mbuf_user_pool_ops; rte_mp_action_register; rte_mp_action_unregister; rte_mp_sendmsg; diff --git a/lib/librte_mbuf/rte_mbuf_pool_ops.c b/lib/librte_mbuf/rte_mbuf_pool_ops.c index 385fc43..48cc342 100644 --- a/lib/librte_mbuf/rte_mbuf_pool_ops.c +++ b/lib/librte_mbuf/rte_mbuf_pool_ops.c @@ -74,7 +74,7 @@ rte_mbuf_user_mempool_ops(void) mz = rte_memzone_lookup("mbuf_user_pool_ops"); if (mz == NULL) - return rte_eal_mbuf_default_mempool_ops(); + return rte_eal_mbuf_user_pool_ops(); return mz->addr; }