[1/2] mbuf: update default Mempool ops with HW active pool

Message ID 1513333483-4372-2-git-send-email-hemant.agrawal@nxp.com
State New
Headers show
Series
  • Dynamic HW Mempool Detection Support
Related show

Commit Message

Hemant Agrawal Dec. 15, 2017, 10:24 a.m.
With this patch the specific HW mempool are no longer required to be
specified in the config file at compile. A default active hw mempool
can be detected dynamically and published to default mempools ops
config at run time. Only one type of HW mempool can be active default.

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>

---
 lib/librte_mbuf/rte_mbuf.c | 33 ++++++++++++++++++++++++++++++++-
 lib/librte_mbuf/rte_mbuf.h | 13 +++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

-- 
2.7.4

Comments

Stephen Hemminger Dec. 15, 2017, 3:52 p.m. | #1
On Fri, 15 Dec 2017 15:54:42 +0530
Hemant Agrawal <hemant.agrawal@nxp.com> wrote:

> +	if ((strcmp(default_ops, RTE_MBUF_DEFAULT_MEMPOOL_OPS) == 0) &&

> +		(active_mbuf_pool_ops_name != NULL))


Why not have less parenthesis () in conditionals?
Jerin Jacob Dec. 18, 2017, 8:55 a.m. | #2
-----Original Message-----
> Date: Fri, 15 Dec 2017 15:54:42 +0530

> From: Hemant Agrawal <hemant.agrawal@nxp.com>

> To: olivier.matz@6wind.com, santosh.shukla@caviumnetworks.com

> CC: dev@dpdk.org

> Subject: [dpdk-dev] [PATCH 1/2] mbuf: update default Mempool ops with HW

>  active pool

> X-Mailer: git-send-email 2.7.4

> 

> With this patch the specific HW mempool are no longer required to be

> specified in the config file at compile. A default active hw mempool

> can be detected dynamically and published to default mempools ops

> config at run time. Only one type of HW mempool can be active default.


For me, it looks very reasonable approach as it caters the basic use
case without any change in the application nor the additional(--mbuf-pool-ops-name)
EAL command line scheme to select different mempool ops.
Though, this option will not enough cater all the use case. I think, we can have
three options and the following order of precedence to select the mempool ops

1) This patch(update active mempool based on the device probe())
2) Selection of mempool ops though --mbuf-pool-ops-name= EAL commandline argument.
Which can overridden the scheme(1)
3) More sophisticated mempool section based on
a) The ethdev PMD capability exposed through existing rte_eth_dev_pool_ops_supported()
b) Add mempool ops option in rte_pktmbuf_pool_create()
http://dpdk.org/ml/archives/dev/2017-December/083985.html
c) Use (a) and (b) to select the update the mempool ops with
some "weight" based algorithm like
http://dpdk.org/dev/patchwork/patch/32245/

> 

> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>

> ---

>  lib/librte_mbuf/rte_mbuf.c | 33 ++++++++++++++++++++++++++++++++-

>  lib/librte_mbuf/rte_mbuf.h | 13 +++++++++++++

>  2 files changed, 45 insertions(+), 1 deletion(-)

> 

> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c

> index 7543662..e074afa 100644

> --- a/lib/librte_mbuf/rte_mbuf.c

> +++ b/lib/librte_mbuf/rte_mbuf.c

> @@ -148,6 +148,37 @@ rte_pktmbuf_init(struct rte_mempool *mp,

>  	m->next = NULL;

>  }

>  

> +static const char *active_mbuf_pool_ops_name;


Global variable will create issue in multi process case.

> +

> +int

> +rte_pktmbuf_reg_active_mempool_ops(const char *ops_name)

> +{

> +	if (active_mbuf_pool_ops_name == NULL) {

> +		active_mbuf_pool_ops_name = ops_name;


I think, if we could update "internal_config.mbuf_pool_ops_name" value then
we don't need any extra global variable.
Hemant Agrawal Dec. 18, 2017, 9:26 a.m. | #3
On 12/15/2017 9:22 PM, Stephen Hemminger wrote:
> On Fri, 15 Dec 2017 15:54:42 +0530

> Hemant Agrawal <hemant.agrawal@nxp.com> wrote:

>

>> +	if ((strcmp(default_ops, RTE_MBUF_DEFAULT_MEMPOOL_OPS) == 0) &&

>> +		(active_mbuf_pool_ops_name != NULL))

>

> Why not have less parenthesis () in conditionals?

>

I will change it in next version.
Hemant Agrawal Dec. 18, 2017, 9:36 a.m. | #4
On 12/18/2017 2:25 PM, Jerin Jacob wrote:
> -----Original Message-----

>> Date: Fri, 15 Dec 2017 15:54:42 +0530

>> From: Hemant Agrawal <hemant.agrawal@nxp.com>

>> To: olivier.matz@6wind.com, santosh.shukla@caviumnetworks.com

>> CC: dev@dpdk.org

>> Subject: [dpdk-dev] [PATCH 1/2] mbuf: update default Mempool ops with HW

>>  active pool

>> X-Mailer: git-send-email 2.7.4

>>

>> With this patch the specific HW mempool are no longer required to be

>> specified in the config file at compile. A default active hw mempool

>> can be detected dynamically and published to default mempools ops

>> config at run time. Only one type of HW mempool can be active default.

>

> For me, it looks very reasonable approach as it caters the basic use

> case without any change in the application nor the additional(--mbuf-pool-ops-name)

> EAL command line scheme to select different mempool ops.

> Though, this option will not enough cater all the use case. I think, we can have

> three options and the following order of precedence to select the mempool ops

>

> 1) This patch(update active mempool based on the device probe())

> 2) Selection of mempool ops though --mbuf-pool-ops-name= EAL commandline argument.

> Which can overridden the scheme(1)

> 3) More sophisticated mempool section based on

> a) The ethdev PMD capability exposed through existing rte_eth_dev_pool_ops_supported()

> b) Add mempool ops option in rte_pktmbuf_pool_create()

> http://dpdk.org/ml/archives/dev/2017-December/083985.html

> c) Use (a) and (b) to select the update the mempool ops with

> some "weight" based algorithm like

> http://dpdk.org/dev/patchwork/patch/32245/

>


Yes! We need more options to fine tune control over the mempool uses, 
specially when dealing with HW mempools.

Once the above mentioned mechanisms will be in place, it will be much 
easier and flexible.

>>

>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>

>> ---

>>  lib/librte_mbuf/rte_mbuf.c | 33 ++++++++++++++++++++++++++++++++-

>>  lib/librte_mbuf/rte_mbuf.h | 13 +++++++++++++

>>  2 files changed, 45 insertions(+), 1 deletion(-)

>>

>> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c

>> index 7543662..e074afa 100644

>> --- a/lib/librte_mbuf/rte_mbuf.c

>> +++ b/lib/librte_mbuf/rte_mbuf.c

>> @@ -148,6 +148,37 @@ rte_pktmbuf_init(struct rte_mempool *mp,

>>  	m->next = NULL;

>>  }

>>

>> +static const char *active_mbuf_pool_ops_name;

>

> Global variable will create issue in multi process case.

>

>> +

>> +int

>> +rte_pktmbuf_reg_active_mempool_ops(const char *ops_name)

>> +{

>> +	if (active_mbuf_pool_ops_name == NULL) {

>> +		active_mbuf_pool_ops_name = ops_name;

>

> I think, if we could update "internal_config.mbuf_pool_ops_name" value then

> we don't need any extra global variable.

>

>

That is a good suggestion.
Also, We may need additional variable in internal config to indicate 
that this is a application provided ops_name - so that it should not be 
overwritten.
Olivier Matz Dec. 22, 2017, 2:41 p.m. | #5
Hi,

On Fri, Dec 15, 2017 at 03:54:42PM +0530, Hemant Agrawal wrote:
> With this patch the specific HW mempool are no longer required to be

> specified in the config file at compile. A default active hw mempool

> can be detected dynamically and published to default mempools ops

> config at run time. Only one type of HW mempool can be active default.

> 

> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>

> ---

>  lib/librte_mbuf/rte_mbuf.c | 33 ++++++++++++++++++++++++++++++++-

>  lib/librte_mbuf/rte_mbuf.h | 13 +++++++++++++

>  2 files changed, 45 insertions(+), 1 deletion(-)

> 

> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c

> index 7543662..e074afa 100644

> --- a/lib/librte_mbuf/rte_mbuf.c

> +++ b/lib/librte_mbuf/rte_mbuf.c

> @@ -148,6 +148,37 @@ rte_pktmbuf_init(struct rte_mempool *mp,

>  	m->next = NULL;

>  }

>  

> +static const char *active_mbuf_pool_ops_name;

> +

> +int

> +rte_pktmbuf_reg_active_mempool_ops(const char *ops_name)


I think active_mempool is not the best name: it is not always active
if the user forces another one.

Since there is only one pool like this, would "platform_mempool" be a
better name?

For naming, I suggest "pktmbuf" can be "mbuf", it's shorter and there is
no need anymore to differentiate with ctrlmbuf, because ctrlmbuf will be
removed soon.  I also think "register" is clearer than "reg".  So, what
about rte_mbuf_register_platform_mempool_ops()?

> +{

> +	if (active_mbuf_pool_ops_name == NULL) {

> +		active_mbuf_pool_ops_name = ops_name;

> +		return 0;

> +	}

> +	RTE_LOG(ERR, MBUF,

> +		"%s is already registered as active pktmbuf pool ops\n",

> +		active_mbuf_pool_ops_name);

> +	return -EACCES;

> +}

> +

> +/* Return mbuf pool ops name */

> +static const char *

> +rte_pktmbuf_active_mempool_ops(void)

> +{

> +	const char *default_ops = rte_eal_mbuf_default_mempool_ops();

> +

> +	/* If mbuf default ops is same as compile time default

> +	 * Just to be sure that no one has updated it by other means.

> +	 */

> +	if ((strcmp(default_ops, RTE_MBUF_DEFAULT_MEMPOOL_OPS) == 0) &&

> +		(active_mbuf_pool_ops_name != NULL))

> +		return active_mbuf_pool_ops_name;

> +	else

> +		return default_ops;

> +}


The name of this function is confusing because it does not really return
the active mempool. If the user selected a pool with
--mbuf-pool-ops-name, it is returned...

...except if --mbuf-pool-ops-name=<name of default ops> was passed,
which I think is also very confusing.
Olivier Matz Dec. 22, 2017, 2:59 p.m. | #6
On Mon, Dec 18, 2017 at 03:06:21PM +0530, Hemant Agrawal wrote:
> On 12/18/2017 2:25 PM, Jerin Jacob wrote:

> > -----Original Message-----

> > > Date: Fri, 15 Dec 2017 15:54:42 +0530

> > > From: Hemant Agrawal <hemant.agrawal@nxp.com>

> > > To: olivier.matz@6wind.com, santosh.shukla@caviumnetworks.com

> > > CC: dev@dpdk.org

> > > Subject: [dpdk-dev] [PATCH 1/2] mbuf: update default Mempool ops with HW

> > >  active pool

> > > X-Mailer: git-send-email 2.7.4

> > > 

> > > With this patch the specific HW mempool are no longer required to be

> > > specified in the config file at compile. A default active hw mempool

> > > can be detected dynamically and published to default mempools ops

> > > config at run time. Only one type of HW mempool can be active default.

> > 

> > For me, it looks very reasonable approach as it caters the basic use

> > case without any change in the application nor the additional(--mbuf-pool-ops-name)

> > EAL command line scheme to select different mempool ops.

> > Though, this option will not enough cater all the use case. I think, we can have

> > three options and the following order of precedence to select the mempool ops

> > 

> > 1) This patch(update active mempool based on the device probe())

> > 2) Selection of mempool ops though --mbuf-pool-ops-name= EAL commandline argument.

> > Which can overridden the scheme(1)

> > 3) More sophisticated mempool section based on

> > a) The ethdev PMD capability exposed through existing rte_eth_dev_pool_ops_supported()

> > b) Add mempool ops option in rte_pktmbuf_pool_create()

> > http://dpdk.org/ml/archives/dev/2017-December/083985.html

> > c) Use (a) and (b) to select the update the mempool ops with

> > some "weight" based algorithm like

> > http://dpdk.org/dev/patchwork/patch/32245/

> > 

> 

> Yes! We need more options to fine tune control over the mempool uses,

> specially when dealing with HW mempools.

> 

> Once the above mentioned mechanisms will be in place, it will be much easier

> and flexible.


I'm inline with this description. It would be great if the same binary can work
on different platforms without configuration.

I just feel it's a bit messy to have:

- rte_eal_mbuf_default_mempool_ops() in eal API
  return user-selected ops if any, or compile-time default

- rte_pktmbuf_active_mempool_ops() in mbuf API
  return platform ops except if a selected user ops != compile default

Thomas suggested somewhere (but I don't remember in which thread) to have
rte_eal_mbuf_default_mempool_ops() in mbuf code, and I think he was right.

I think the whole mbuf pool ops selection mechanism should be at the
same place. I could be in a specific file of librte_mbuf.

The API could be:
- get compile time default ops
- get/set platform ops (NULL if none)
- get/set user ops (NULL if none)
- get preferred ops from currently configured PMD

- get best ops: return user, or pmd-prefered, or platform, or default.

rte_pktmbuf_pool_create() will use "get best ops" if no ops (NULL) is
passed as argument.
Hemant Agrawal Dec. 28, 2017, 12:07 p.m. | #7
Hi Olivier,

On 12/22/2017 8:29 PM, Olivier MATZ wrote:
> On Mon, Dec 18, 2017 at 03:06:21PM +0530, Hemant Agrawal wrote:

>> On 12/18/2017 2:25 PM, Jerin Jacob wrote:

>>> -----Original Message-----

>>>> Date: Fri, 15 Dec 2017 15:54:42 +0530

>>>> From: Hemant Agrawal <hemant.agrawal@nxp.com>

>>>> To: olivier.matz@6wind.com, santosh.shukla@caviumnetworks.com

>>>> CC: dev@dpdk.org

>>>> Subject: [dpdk-dev] [PATCH 1/2] mbuf: update default Mempool ops with HW

>>>>  active pool

>>>> X-Mailer: git-send-email 2.7.4

>>>>

>>>> With this patch the specific HW mempool are no longer required to be

>>>> specified in the config file at compile. A default active hw mempool

>>>> can be detected dynamically and published to default mempools ops

>>>> config at run time. Only one type of HW mempool can be active default.

>>>

>>> For me, it looks very reasonable approach as it caters the basic use

>>> case without any change in the application nor the additional(--mbuf-pool-ops-name)

>>> EAL command line scheme to select different mempool ops.

>>> Though, this option will not enough cater all the use case. I think, we can have

>>> three options and the following order of precedence to select the mempool ops

>>>

>>> 1) This patch(update active mempool based on the device probe())

>>> 2) Selection of mempool ops though --mbuf-pool-ops-name= EAL commandline argument.

>>> Which can overridden the scheme(1)

>>> 3) More sophisticated mempool section based on

>>> a) The ethdev PMD capability exposed through existing rte_eth_dev_pool_ops_supported()

>>> b) Add mempool ops option in rte_pktmbuf_pool_create()

>>> http://dpdk.org/ml/archives/dev/2017-December/083985.html

>>> c) Use (a) and (b) to select the update the mempool ops with

>>> some "weight" based algorithm like

>>> http://dpdk.org/dev/patchwork/patch/32245/

>>>

>>

>> Yes! We need more options to fine tune control over the mempool uses,

>> specially when dealing with HW mempools.

>>

>> Once the above mentioned mechanisms will be in place, it will be much easier

>> and flexible.

>

> I'm inline with this description. It would be great if the same binary can work

> on different platforms without configuration.

>

> I just feel it's a bit messy to have:

>

> - rte_eal_mbuf_default_mempool_ops() in eal API

>   return user-selected ops if any, or compile-time default

>

> - rte_pktmbuf_active_mempool_ops() in mbuf API

>   return platform ops except if a selected user ops != compile default

>

> Thomas suggested somewhere (but I don't remember in which thread) to have

> rte_eal_mbuf_default_mempool_ops() in mbuf code, and I think he was right.

>


The idea is good. It will break ABI, but we can move around in 
systematic way.

> I think the whole mbuf pool ops selection mechanism should be at the

> same place. I could be in a specific file of librte_mbuf.

>

> The API could be:

> - get compile time default ops

We can get them from "RTE_MBUF_DEFAULT_MEMPOOL_OPS"
or "
const char *rte_get_mbuf_config_mempool_ops(void)

> - get/set platform ops (NULL if none)


const char *rte_get_mbuf_platform_mempool_ops(void);
int rte_set_mbuf_platform_mempool_ops(const char *ops_name);

> - get/set user ops (NULL if none)

internal_config will only store the command line argument or NULL.

rename : rte_eal_mbuf_default_mempool_ops(void)
to  const char *rte_get_mbuf_user_mempool_ops(void)

> - get preferred ops from currently configured PMD

>


The following proposal is updating the mempool_ops name in 
internal_config, I thing that is not the best solution. We shall not 
update the internal config.

eal: add API to set default mbuf mempool ops
http://dpdk.org/ml/archives/dev/2017-December/083849.html
rte_eth_dev_get_preferred_pool_name()

> - get best ops: return user, or pmd-prefered, or platform, or default.

>


pktmbuf_pool_create is currently calling, *rte_eal_mbuf_default_mempool_ops*

This should be changed to call *rte_get_mbuf_best_mempool_ops* in the 
following order
	1. rte_get_mbuf_user_mempool_ops
         2. rte_eth_dev_get_preferred_pool_name (whenever this API comes)
         3. rte_get_mbuf_platform_mempool_ops
         4. rte_get_mbuf_config_mempool_ops

> rte_pktmbuf_pool_create() will use "get best ops" if no ops (NULL) is

> passed as argument.

>


However, we shall still provide a additional API, 
*rte_pktmbuf_pool_create_specific* if user still wants fine control or 
don't want to use the default best logic.
Hemant Agrawal Jan. 10, 2018, 12:49 p.m. | #8
Hi Olivier,

>> I just feel it's a bit messy to have:

>>

>> - rte_eal_mbuf_default_mempool_ops() in eal API

>>   return user-selected ops if any, or compile-time default

>>

>> - rte_pktmbuf_active_mempool_ops() in mbuf API

>>   return platform ops except if a selected user ops != compile default

>>

>> Thomas suggested somewhere (but I don't remember in which thread) to have

>> rte_eal_mbuf_default_mempool_ops() in mbuf code, and I think he was

>> right.

>>

>

> The idea is good. It will break ABI, but we can move around in

> systematic way.

>

>> I think the whole mbuf pool ops selection mechanism should be at the

>> same place. I could be in a specific file of librte_mbuf.

>>

I have just tried to implement your suggestions.  I need one clarification.

Eal based internal config is being used to store the command line 
mempool_ops_name.

If we want it to be a mbuf based API (instead of eal_mbuf), we need to 
export internal_config via map file for shared build.
Are you fine with that?

If not, we have to live with eal_mbuf APIs only.

Regards,
Hemant

Patch

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 7543662..e074afa 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -148,6 +148,37 @@  rte_pktmbuf_init(struct rte_mempool *mp,
 	m->next = NULL;
 }
 
+static const char *active_mbuf_pool_ops_name;
+
+int
+rte_pktmbuf_reg_active_mempool_ops(const char *ops_name)
+{
+	if (active_mbuf_pool_ops_name == NULL) {
+		active_mbuf_pool_ops_name = ops_name;
+		return 0;
+	}
+	RTE_LOG(ERR, MBUF,
+		"%s is already registered as active pktmbuf pool ops\n",
+		active_mbuf_pool_ops_name);
+	return -EACCES;
+}
+
+/* Return mbuf pool ops name */
+static const char *
+rte_pktmbuf_active_mempool_ops(void)
+{
+	const char *default_ops = rte_eal_mbuf_default_mempool_ops();
+
+	/* If mbuf default ops is same as compile time default
+	 * Just to be sure that no one has updated it by other means.
+	 */
+	if ((strcmp(default_ops, RTE_MBUF_DEFAULT_MEMPOOL_OPS) == 0) &&
+		(active_mbuf_pool_ops_name != NULL))
+		return active_mbuf_pool_ops_name;
+	else
+		return default_ops;
+}
+
 /* helper to create a mbuf pool */
 struct rte_mempool *
 rte_pktmbuf_pool_create(const char *name, unsigned n,
@@ -176,7 +207,7 @@  rte_pktmbuf_pool_create(const char *name, unsigned n,
 	if (mp == NULL)
 		return NULL;
 
-	mp_ops_name = rte_eal_mbuf_default_mempool_ops();
+	mp_ops_name = rte_pktmbuf_active_mempool_ops();
 	ret = rte_mempool_set_ops_byname(mp, mp_ops_name, NULL);
 	if (ret != 0) {
 		RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index ce8a05d..3140a0b 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1081,6 +1081,19 @@  rte_pktmbuf_pool_create(const char *name, unsigned n,
 	int socket_id);
 
 /**
+ * Register the active HW pkt mbuf pool
+ *
+ * Register the active pktmbuf HW pool to overwrite the default pool
+ *
+ * @param pool ops name
+ * @return
+ *   - 0: Success
+ *   - -EACCES: Active mempool is already registered.
+ */
+int
+rte_pktmbuf_reg_active_mempool_ops(const char *ops_name);
+
+/**
  * Get the data room size of mbufs stored in a pktmbuf_pool
  *
  * The data room size is the amount of data that can be stored in a