[v3,2/7] eal: add API to set user default mbuf mempool ops

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

Commit Message

Hemant Agrawal Jan. 18, 2018, 1:26 p.m.
Add new API to set the user defined mbuf mempool ops name
i.e. set the provided ops name to `internal_config.mbuf_pool_ops_name`.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>

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

---
 lib/librte_eal/bsdapp/eal/eal.c         | 6 ++++++
 lib/librte_eal/common/include/rte_eal.h | 9 +++++++++
 lib/librte_eal/linuxapp/eal/eal.c       | 6 ++++++
 lib/librte_eal/rte_eal_version.map      | 1 +
 4 files changed, 22 insertions(+)

-- 
2.7.4

Comments

Olivier Matz Jan. 19, 2018, 10:01 a.m. | #1
On Thu, Jan 18, 2018 at 06:56:27PM +0530, Hemant Agrawal wrote:
> Add new API to set the user defined mbuf mempool ops name

> i.e. set the provided ops name to `internal_config.mbuf_pool_ops_name`.

> 

> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>

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

> ---

>  lib/librte_eal/bsdapp/eal/eal.c         | 6 ++++++

>  lib/librte_eal/common/include/rte_eal.h | 9 +++++++++

>  lib/librte_eal/linuxapp/eal/eal.c       | 6 ++++++

>  lib/librte_eal/rte_eal_version.map      | 1 +

>  4 files changed, 22 insertions(+)

> 

> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c

> index c602d02..64f010a 100644

> --- a/lib/librte_eal/bsdapp/eal/eal.c

> +++ b/lib/librte_eal/bsdapp/eal/eal.c

> @@ -117,6 +117,12 @@ rte_eal_mbuf_default_mempool_ops(void)

>  	return internal_config.user_mbuf_pool_ops_name;

>  }

>  

> +void

> +rte_eal_set_mbuf_user_mempool_ops(const char *ops_name)

> +{

> +	internal_config.user_mbuf_pool_ops_name = ops_name;

> +}

> +


I think we should only have the "set" API in mbuf lib.

What do you think about what I suggested in
http://dpdk.org/ml/archives/dev/2018-January/087419.html ?

"""
The proper way is maybe to keep the parsing in eal, and at librte_mbuf
initialization, query the eal library to get the user pool if any.
After that, all will be managed inside librte_mbuf. So the eal lib will
only do the argument parsing.
"""

In that case, this patch could be dropped. Please refer to my comment
in patch 3 to see what other modifications would be needed.
Hemant Agrawal Jan. 19, 2018, 12:31 p.m. | #2
Hi Olivier,

On 1/19/2018 3:31 PM, Olivier Matz wrote:
> On Thu, Jan 18, 2018 at 06:56:27PM +0530, Hemant Agrawal wrote:

>> Add new API to set the user defined mbuf mempool ops name

>> i.e. set the provided ops name to `internal_config.mbuf_pool_ops_name`.

>>

>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>

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

>> ---

>>  lib/librte_eal/bsdapp/eal/eal.c         | 6 ++++++

>>  lib/librte_eal/common/include/rte_eal.h | 9 +++++++++

>>  lib/librte_eal/linuxapp/eal/eal.c       | 6 ++++++

>>  lib/librte_eal/rte_eal_version.map      | 1 +

>>  4 files changed, 22 insertions(+)

>>

>> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c

>> index c602d02..64f010a 100644

>> --- a/lib/librte_eal/bsdapp/eal/eal.c

>> +++ b/lib/librte_eal/bsdapp/eal/eal.c

>> @@ -117,6 +117,12 @@ rte_eal_mbuf_default_mempool_ops(void)

>>  	return internal_config.user_mbuf_pool_ops_name;

>>  }

>>

>> +void

>> +rte_eal_set_mbuf_user_mempool_ops(const char *ops_name)

>> +{

>> +	internal_config.user_mbuf_pool_ops_name = ops_name;

>> +}

>> +

>

> I think we should only have the "set" API in mbuf lib.

>

> What do you think about what I suggested in

> http://dpdk.org/ml/archives/dev/2018-January/087419.html ?

>

> """

> The proper way is maybe to keep the parsing in eal, and at librte_mbuf

> initialization, query the eal library to get the user pool if any.

> After that, all will be managed inside librte_mbuf. So the eal lib will

> only do the argument parsing.

> """

>


Will you please help me in understanding, how should I do it?

1. The is no standard librte_mbuf initialization routine. RTE_INIT will 
not work, as it will be executed before eal_init. dlopen is for shared 
lib only. Is there any other method?

2. If I call it on the very first call of rte_mbuf_user_mempool_ops_name 
or rte_mbuf_set_user_mempool_ops_name, I will be checking against NULL. 
It can be NULL always. So, no real meaning of maintaining it in 
librte_mbuf as well.
Yes, I can maintain a flag to know, If I have synced with eal parse 
before. But that is not sounding to me clean.


> In that case, this patch could be dropped. Please refer to my comment

> in patch 3 to see what other modifications would be needed.

>
Olivier Matz Jan. 19, 2018, 12:43 p.m. | #3
On Fri, Jan 19, 2018 at 06:01:55PM +0530, Hemant Agrawal wrote:
> Hi Olivier,

> 

> On 1/19/2018 3:31 PM, Olivier Matz wrote:

> > On Thu, Jan 18, 2018 at 06:56:27PM +0530, Hemant Agrawal wrote:

> > > Add new API to set the user defined mbuf mempool ops name

> > > i.e. set the provided ops name to `internal_config.mbuf_pool_ops_name`.

> > > 

> > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>

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

> > > ---

> > >  lib/librte_eal/bsdapp/eal/eal.c         | 6 ++++++

> > >  lib/librte_eal/common/include/rte_eal.h | 9 +++++++++

> > >  lib/librte_eal/linuxapp/eal/eal.c       | 6 ++++++

> > >  lib/librte_eal/rte_eal_version.map      | 1 +

> > >  4 files changed, 22 insertions(+)

> > > 

> > > diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c

> > > index c602d02..64f010a 100644

> > > --- a/lib/librte_eal/bsdapp/eal/eal.c

> > > +++ b/lib/librte_eal/bsdapp/eal/eal.c

> > > @@ -117,6 +117,12 @@ rte_eal_mbuf_default_mempool_ops(void)

> > >  	return internal_config.user_mbuf_pool_ops_name;

> > >  }

> > > 

> > > +void

> > > +rte_eal_set_mbuf_user_mempool_ops(const char *ops_name)

> > > +{

> > > +	internal_config.user_mbuf_pool_ops_name = ops_name;

> > > +}

> > > +

> > 

> > I think we should only have the "set" API in mbuf lib.

> > 

> > What do you think about what I suggested in

> > http://dpdk.org/ml/archives/dev/2018-January/087419.html ?

> > 

> > """

> > The proper way is maybe to keep the parsing in eal, and at librte_mbuf

> > initialization, query the eal library to get the user pool if any.

> > After that, all will be managed inside librte_mbuf. So the eal lib will

> > only do the argument parsing.

> > """

> > 

> 

> Will you please help me in understanding, how should I do it?

> 

> 1. The is no standard librte_mbuf initialization routine. RTE_INIT will not

> work, as it will be executed before eal_init. dlopen is for shared lib only.

> Is there any other method?

> 

> 2. If I call it on the very first call of rte_mbuf_user_mempool_ops_name or

> rte_mbuf_set_user_mempool_ops_name, I will be checking against NULL. It can

> be NULL always. So, no real meaning of maintaining it in librte_mbuf as

> well.

> Yes, I can maintain a flag to know, If I have synced with eal parse before.

> But that is not sounding to me clean.


My initial idea was to do it at init, but I came to the same conclusion
than yours, it is not possible.

Can you take a look to the comments of patch 3/7 and let me know if the
proposition fits your needs?

Patch

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index c602d02..64f010a 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -117,6 +117,12 @@  rte_eal_mbuf_default_mempool_ops(void)
 	return internal_config.user_mbuf_pool_ops_name;
 }
 
+void
+rte_eal_set_mbuf_user_mempool_ops(const char *ops_name)
+{
+	internal_config.user_mbuf_pool_ops_name = ops_name;
+}
+
 /* Return a pointer to the configuration structure */
 struct rte_config *
 rte_eal_get_configuration(void)
diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
index 2aba2c8..7645b34 100644
--- a/lib/librte_eal/common/include/rte_eal.h
+++ b/lib/librte_eal/common/include/rte_eal.h
@@ -307,6 +307,15 @@  enum rte_iova_mode rte_eal_iova_mode(void);
 const char *
 rte_eal_mbuf_default_mempool_ops(void);
 
+/**
+ * Set user pool ops name for mbuf
+ *
+ * @param ops_name
+ *   mempool ops name that is to be set as user defined.
+ */
+void
+rte_eal_set_mbuf_user_mempool_ops(const char *ops_name);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index e8c7100..46b2bb3 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -127,6 +127,12 @@  rte_eal_mbuf_default_mempool_ops(void)
 	return internal_config.user_mbuf_pool_ops_name;
 }
 
+void
+rte_eal_set_mbuf_user_mempool_ops(const char *ops_name)
+{
+	internal_config.user_mbuf_pool_ops_name = ops_name;
+}
+
 /* Return a pointer to the configuration structure */
 struct rte_config *
 rte_eal_get_configuration(void)
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index 7088b72..3529885 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -203,6 +203,7 @@  DPDK_17.11 {
 DPDK_18.02 {
 	global:
 
+	rte_eal_set_mbuf_user_mempool_ops;
 	rte_hypervisor_get;
 	rte_hypervisor_get_name;
 	rte_vfio_clear_group;