[FIX-OPTION-1] mbuf: fix the logic of user mempool ops API

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

Commit Message

Hemant Agrawal Feb. 2, 2018, 8:03 a.m.
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>

---
 lib/librte_eal/bsdapp/eal/eal.c         |  7 +++++++
 lib/librte_eal/common/include/rte_eal.h | 12 ++++++++++++
 lib/librte_eal/linuxapp/eal/eal.c       |  7 +++++++
 lib/librte_eal/rte_eal_version.map      |  1 +
 lib/librte_mbuf/rte_mbuf_pool_ops.c     |  2 +-
 5 files changed, 28 insertions(+), 1 deletion(-)

-- 
2.7.4

Comments

Olivier Matz Feb. 2, 2018, 1:40 p.m. | #1
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>
Hemant Agrawal Feb. 2, 2018, 1:47 p.m. | #2
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
santosh Feb. 4, 2018, 6:34 a.m. | #3
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.
Hemant Agrawal Feb. 4, 2018, 10:27 a.m. | #4
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.
Thomas Monjalon Feb. 6, 2018, 12:05 a.m. | #5
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

Patch

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