[dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure

[dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure

[Shahaf Shuler]

With the API and ABI freeze ahead, it will be good to reserve
some bits on the private structure for future use.

Otherwise we will potentially need to maintain two different
private structure during 2020 period.

There is already one use case for those reserved bits[1]

The reserved field should be set to 0 by the user.

[1]
https://patches.dpdk.org/patch/63077/

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---

Note - am aware no proper RFC was sent before the proposal
deadline of 19.11. However i hope this small change can be
accepeted for the sake of simpler maintainance in the future.

---
 lib/librte_mbuf/rte_mbuf.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 92d81972ab..6912594d57 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -303,6 +303,7 @@ rte_mbuf_to_priv(struct rte_mbuf *m)
 struct rte_pktmbuf_pool_private {
 	uint16_t mbuf_data_room_size; /**< Size of data space in each mbuf. */
 	uint16_t mbuf_priv_size;      /**< Size of private area in each mbuf. */
+	uint32_t reserved; /**< reserved for future use. */
 };
 
 #ifdef RTE_LIBRTE_MBUF_DEBUG
-- 
2.12.0

Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure

[Stephen Hemminger]

On Mon, 18 Nov 2019 10:02:55 +0000
Shahaf Shuler <shahafs@mellanox.com> wrote:

> With the API and ABI freeze ahead, it will be good to reserve
> some bits on the private structure for future use.
> 
> Otherwise we will potentially need to maintain two different
> private structure during 2020 period.
> 
> There is already one use case for those reserved bits[1]
> 
> The reserved field should be set to 0 by the user.
> 
> [1]
> https://patches.dpdk.org/patch/63077/
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---

Understand the motivation but in my experience until you know
what it is for, this doesn't work. And it creates lots of dead ends.

https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it

Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure

[Shahaf Shuler]

Monday, November 18, 2019 6:12 PM, Stephen Hemminger:
> Subject: Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private
> structure
> 
> On Mon, 18 Nov 2019 10:02:55 +0000
> Shahaf Shuler <shahafs@mellanox.com> wrote:
> 
> > With the API and ABI freeze ahead, it will be good to reserve some
> > bits on the private structure for future use.
> >
> > Otherwise we will potentially need to maintain two different private
> > structure during 2020 period.
> >
> > There is already one use case for those reserved bits[1]
> >
> > The reserved field should be set to 0 by the user.
> >
> > [1]
> >
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> >
> hes.dpdk.org%2Fpatch%2F63077%2F&amp;data=02%7C01%7Cshahafs%40m
> ellanox.
> >
> com%7C261be5d5bd344538663c08d76c420fcb%7Ca652971c7d2e4d9ba6a4d14
> 9256f4
> >
> 61b%7C0%7C0%7C637096903295807188&amp;sdata=ruzHICpeUCNXbw4XXD
> Xj1ZOP35Z
> > ZoUtgWbSPHYSKDFo%3D&amp;reserved=0
> >
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > ---
> 
> Understand the motivation but in my experience until you know what it is
> for, this doesn't work. And it creates lots of dead ends.

See cross reference [1]. I already have use case for it and understand how it is going to be used.
This is exactly why I am pushing this patch for 19.11. 

> 
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wi
> kipedia.org%2Fwiki%2FYou_aren%2527t_gonna_need_it&amp;data=02%7C0
> 1%7Cshahafs%40mellanox.com%7C261be5d5bd344538663c08d76c420fcb%7C
> a652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637096903295807188&a
> mp;sdata=U8wYu7M85a9F%2BrGxaaxpOce6Pedh4THo4%2Fi03Sj2OV0%3D&
> amp;reserved=0

Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure

[Thomas Monjalon]

18/11/2019 11:02, Shahaf Shuler:
>  struct rte_pktmbuf_pool_private {
>  	uint16_t mbuf_data_room_size; /**< Size of data space in each mbuf. */
>  	uint16_t mbuf_priv_size;      /**< Size of private area in each mbuf. */
> +	uint32_t reserved; /**< reserved for future use. */

Maybe simpler to give the future name "flags" and keep the comment
"reserved for future use".

Olivier, what do you think?

Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure

[Shahaf Shuler]

Tuesday, November 19, 2019 11:33 AM, Thomas Monjalon:
> Subject: Re: [PATCH] mbuf: extend pktmbuf pool private structure
> 
> 18/11/2019 11:02, Shahaf Shuler:
> >  struct rte_pktmbuf_pool_private {
> >  	uint16_t mbuf_data_room_size; /**< Size of data space in each
> mbuf. */
> >  	uint16_t mbuf_priv_size;      /**< Size of private area in each mbuf.
> */
> > +	uint32_t reserved; /**< reserved for future use. */
> 
> Maybe simpler to give the future name "flags" and keep the comment
> "reserved for future use".

I'm am OK w/ changing to flags.
If Olivier accepts maybe you can change while applying? 

> 
> Olivier, what do you think?
> 

Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure

[Stephen Hemminger]

On Tue, 19 Nov 2019 15:23:50 +0000
Shahaf Shuler <shahafs@mellanox.com> wrote:

> Tuesday, November 19, 2019 11:33 AM, Thomas Monjalon:
> > Subject: Re: [PATCH] mbuf: extend pktmbuf pool private structure
> > 
> > 18/11/2019 11:02, Shahaf Shuler:  
> > >  struct rte_pktmbuf_pool_private {
> > >  	uint16_t mbuf_data_room_size; /**< Size of data space in each  
> > mbuf. */  
> > >  	uint16_t mbuf_priv_size;      /**< Size of private area in each mbuf.  
> > */  
> > > +	uint32_t reserved; /**< reserved for future use. */  
> > 
> > Maybe simpler to give the future name "flags" and keep the comment
> > "reserved for future use".  
> 
> I'm am OK w/ changing to flags.
> If Olivier accepts maybe you can change while applying? 

After the Linux openat experience if you want to add flags.
Then all usage of API needs to validate that flags is 0.

Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure

[Thomas Monjalon]

19/11/2019 17:25, Stephen Hemminger:
> On Tue, 19 Nov 2019 15:23:50 +0000
> Shahaf Shuler <shahafs@mellanox.com> wrote:
> 
> > Tuesday, November 19, 2019 11:33 AM, Thomas Monjalon:
> > > Subject: Re: [PATCH] mbuf: extend pktmbuf pool private structure
> > > 
> > > 18/11/2019 11:02, Shahaf Shuler:  
> > > >  struct rte_pktmbuf_pool_private {
> > > >  	uint16_t mbuf_data_room_size; /**< Size of data space in each  
> > > mbuf. */  
> > > >  	uint16_t mbuf_priv_size;      /**< Size of private area in each mbuf.  
> > > */  
> > > > +	uint32_t reserved; /**< reserved for future use. */  
> > > 
> > > Maybe simpler to give the future name "flags" and keep the comment
> > > "reserved for future use".  
> > 
> > I'm am OK w/ changing to flags.
> > If Olivier accepts maybe you can change while applying? 
> 
> After the Linux openat experience if you want to add flags.
> Then all usage of API needs to validate that flags is 0.

Sorry Stephen, I don't understand what you mean.
Please could you explain?

Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure

[Stephen Hemminger]

On Tue, 19 Nov 2019 23:30:15 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:

> 19/11/2019 17:25, Stephen Hemminger:
> > On Tue, 19 Nov 2019 15:23:50 +0000
> > Shahaf Shuler <shahafs@mellanox.com> wrote:
> >   
> > > Tuesday, November 19, 2019 11:33 AM, Thomas Monjalon:  
> > > > Subject: Re: [PATCH] mbuf: extend pktmbuf pool private structure
> > > > 
> > > > 18/11/2019 11:02, Shahaf Shuler:    
> > > > >  struct rte_pktmbuf_pool_private {
> > > > >  	uint16_t mbuf_data_room_size; /**< Size of data space in each    
> > > > mbuf. */    
> > > > >  	uint16_t mbuf_priv_size;      /**< Size of private area in each mbuf.    
> > > > */    
> > > > > +	uint32_t reserved; /**< reserved for future use. */    
> > > > 
> > > > Maybe simpler to give the future name "flags" and keep the comment
> > > > "reserved for future use".    
> > > 
> > > I'm am OK w/ changing to flags.
> > > If Olivier accepts maybe you can change while applying?   
> > 
> > After the Linux openat experience if you want to add flags.
> > Then all usage of API needs to validate that flags is 0.  
> 
> Sorry Stephen, I don't understand what you mean.
> Please could you explain?
> 
> 

Any time a new field is added that maybe used later you can
not guarantee that existing code correctly initializes the value to
zero. What happened with openat() was that there was a flag value
that was originally unused, but since kernel did not enforce that
it was zero; it could not later be used for extensions.

You need to make sure that all reserved fields are initialized.
That means when a private pool is created it is zeroed. And if
a flag is new argument to an API, check for zero at create time.

An example of how DPDK failed at this is the filter field in
rte_pdump. Since it is not checked for NULL, it can't safely
be used now (and still claim API/ABI compatiablity).

Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure

[Shahaf Shuler]

Wednesday, November 20, 2019 1:51 AM, Stephen Hemminger:
> Subject: Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private
> structure
> 
> On Tue, 19 Nov 2019 23:30:15 +0100
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 19/11/2019 17:25, Stephen Hemminger:
> > > On Tue, 19 Nov 2019 15:23:50 +0000
> > > Shahaf Shuler <shahafs@mellanox.com> wrote:
> > >
> > > > Tuesday, November 19, 2019 11:33 AM, Thomas Monjalon:
> > > > > Subject: Re: [PATCH] mbuf: extend pktmbuf pool private structure
> > > > >
> > > > > 18/11/2019 11:02, Shahaf Shuler:
> > > > > >  struct rte_pktmbuf_pool_private {
> > > > > >  	uint16_t mbuf_data_room_size; /**< Size of data space in
> each
> > > > > mbuf. */
> > > > > >  	uint16_t mbuf_priv_size;      /**< Size of private area in each
> mbuf.
> > > > > */
> > > > > > +	uint32_t reserved; /**< reserved for future use. */
> > > > >
> > > > > Maybe simpler to give the future name "flags" and keep the
> comment
> > > > > "reserved for future use".
> > > >
> > > > I'm am OK w/ changing to flags.
> > > > If Olivier accepts maybe you can change while applying?
> > >
> > > After the Linux openat experience if you want to add flags.
> > > Then all usage of API needs to validate that flags is 0.
> >
> > Sorry Stephen, I don't understand what you mean.
> > Please could you explain?
> >
> >
> 
> Any time a new field is added that maybe used later you can not guarantee
> that existing code correctly initializes the value to zero. What happened with
> openat() was that there was a flag value that was originally unused, but since
> kernel did not enforce that it was zero; it could not later be used for
> extensions.
> 
> You need to make sure that all reserved fields are initialized.
> That means when a private pool is created it is zeroed. And if a flag is new
> argument to an API, check for zero at create time.

I guess we can hard code the value for 0 on the rte_pktmbuf_pool_create function and have some assert on the rte_pktmbuf_pool_init callback (we cannot fail as this function returns void).
Any other places you find problematic? 

> 
> An example of how DPDK failed at this is the filter field in rte_pdump. Since it
> is not checked for NULL, it can't safely be used now (and still claim API/ABI
> compatiablity).

Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure

[Stephen Hemminger]

On Wed, 20 Nov 2019 07:01:26 +0000
Shahaf Shuler <shahafs@mellanox.com> wrote:

> Wednesday, November 20, 2019 1:51 AM, Stephen Hemminger:
> > Subject: Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private
> > structure
> > 
> > On Tue, 19 Nov 2019 23:30:15 +0100
> > Thomas Monjalon <thomas@monjalon.net> wrote:
> >   
> > > 19/11/2019 17:25, Stephen Hemminger:  
> > > > On Tue, 19 Nov 2019 15:23:50 +0000
> > > > Shahaf Shuler <shahafs@mellanox.com> wrote:
> > > >  
> > > > > Tuesday, November 19, 2019 11:33 AM, Thomas Monjalon:  
> > > > > > Subject: Re: [PATCH] mbuf: extend pktmbuf pool private structure
> > > > > >
> > > > > > 18/11/2019 11:02, Shahaf Shuler:  
> > > > > > >  struct rte_pktmbuf_pool_private {
> > > > > > >  	uint16_t mbuf_data_room_size; /**< Size of data space in  
> > each  
> > > > > > mbuf. */  
> > > > > > >  	uint16_t mbuf_priv_size;      /**< Size of private area in each  
> > mbuf.  
> > > > > > */  
> > > > > > > +	uint32_t reserved; /**< reserved for future use. */  
> > > > > >
> > > > > > Maybe simpler to give the future name "flags" and keep the  
> > comment  
> > > > > > "reserved for future use".  
> > > > >
> > > > > I'm am OK w/ changing to flags.
> > > > > If Olivier accepts maybe you can change while applying?  
> > > >
> > > > After the Linux openat experience if you want to add flags.
> > > > Then all usage of API needs to validate that flags is 0.  
> > >
> > > Sorry Stephen, I don't understand what you mean.
> > > Please could you explain?
> > >
> > >  
> > 
> > Any time a new field is added that maybe used later you can not guarantee
> > that existing code correctly initializes the value to zero. What happened with
> > openat() was that there was a flag value that was originally unused, but since
> > kernel did not enforce that it was zero; it could not later be used for
> > extensions.
> > 
> > You need to make sure that all reserved fields are initialized.
> > That means when a private pool is created it is zeroed. And if a flag is new
> > argument to an API, check for zero at create time.  
> 
> I guess we can hard code the value for 0 on the rte_pktmbuf_pool_create function and have some assert on the rte_pktmbuf_pool_init callback (we cannot fail as this function returns void).
> Any other places you find problematic? 

No. that should be good. 

[dpdk-dev] [PATCH v2] mbuf: extend pktmbuf pool private structure

[Shahaf Shuler]

With the API and ABI freeze ahead, it will be good to reserve
some bits on the private structure for future use.

Otherwise we will potentially need to maintain two different
private structure during 2020 period.

There is already one use case for those reserved bits[1]

The reserved field should be set to 0 by the user.

[1]
https://patches.dpdk.org/patch/63077/

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
On v2:
 - rename new field to flags
 - add extra validation in code that flags = 0

---
 lib/librte_mbuf/rte_mbuf.c | 10 +++++++---
 lib/librte_mbuf/rte_mbuf.h |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 35df1c4c38..bd64c55fe6 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -50,6 +50,7 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp, void *opaque_arg)
 	user_mbp_priv = opaque_arg;
 	if (user_mbp_priv == NULL) {
 		default_mbp_priv.mbuf_priv_size = 0;
+		default_mbp_priv.flags = 0;
 		if (mp->elt_size > sizeof(struct rte_mbuf))
 			roomsz = mp->elt_size - sizeof(struct rte_mbuf);
 		else
@@ -61,6 +62,7 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp, void *opaque_arg)
 	RTE_ASSERT(mp->elt_size >= sizeof(struct rte_mbuf) +
 		user_mbp_priv->mbuf_data_room_size +
 		user_mbp_priv->mbuf_priv_size);
+	RTE_ASSERT(user_mbp_priv->flags == 0);
 
 	mbp_priv = rte_mempool_get_priv(mp);
 	memcpy(mbp_priv, user_mbp_priv, sizeof(*mbp_priv));
@@ -113,7 +115,11 @@ rte_pktmbuf_pool_create_by_ops(const char *name, unsigned int n,
 	int socket_id, const char *ops_name)
 {
 	struct rte_mempool *mp;
-	struct rte_pktmbuf_pool_private mbp_priv;
+	struct rte_pktmbuf_pool_private mbp_priv = {
+		.mbuf_data_room_size = data_room_size,
+		.mbuf_priv_size = priv_size,
+		.flags = 0,
+	};
 	const char *mp_ops_name = ops_name;
 	unsigned elt_size;
 	int ret;
@@ -126,8 +132,6 @@ rte_pktmbuf_pool_create_by_ops(const char *name, unsigned int n,
 	}
 	elt_size = sizeof(struct rte_mbuf) + (unsigned)priv_size +
 		(unsigned)data_room_size;
-	mbp_priv.mbuf_data_room_size = data_room_size;
-	mbp_priv.mbuf_priv_size = priv_size;
 
 	mp = rte_mempool_create_empty(name, n, elt_size, cache_size,
 		 sizeof(struct rte_pktmbuf_pool_private), socket_id, 0);
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 92d81972ab..219b110b76 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -303,6 +303,7 @@ rte_mbuf_to_priv(struct rte_mbuf *m)
 struct rte_pktmbuf_pool_private {
 	uint16_t mbuf_data_room_size; /**< Size of data space in each mbuf. */
 	uint16_t mbuf_priv_size;      /**< Size of private area in each mbuf. */
+	uint32_t flags; /**< reserved for future use. */
 };
 
 #ifdef RTE_LIBRTE_MBUF_DEBUG
-- 
2.12.0

Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure

[Olivier Matz]

Hi,

On Wed, Nov 20, 2019 at 07:56:14AM -0800, Stephen Hemminger wrote:
> On Wed, 20 Nov 2019 07:01:26 +0000
> Shahaf Shuler <shahafs@mellanox.com> wrote:
> 
> > Wednesday, November 20, 2019 1:51 AM, Stephen Hemminger:
> > > Subject: Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private
> > > structure
> > > 
> > > On Tue, 19 Nov 2019 23:30:15 +0100
> > > Thomas Monjalon <thomas@monjalon.net> wrote:
> > >   
> > > > 19/11/2019 17:25, Stephen Hemminger:  
> > > > > On Tue, 19 Nov 2019 15:23:50 +0000
> > > > > Shahaf Shuler <shahafs@mellanox.com> wrote:
> > > > >  
> > > > > > Tuesday, November 19, 2019 11:33 AM, Thomas Monjalon:  
> > > > > > > Subject: Re: [PATCH] mbuf: extend pktmbuf pool private structure
> > > > > > >
> > > > > > > 18/11/2019 11:02, Shahaf Shuler:  
> > > > > > > >  struct rte_pktmbuf_pool_private {
> > > > > > > >  	uint16_t mbuf_data_room_size; /**< Size of data space in  
> > > each  
> > > > > > > mbuf. */  
> > > > > > > >  	uint16_t mbuf_priv_size;      /**< Size of private area in each  
> > > mbuf.  
> > > > > > > */  
> > > > > > > > +	uint32_t reserved; /**< reserved for future use. */  
> > > > > > >
> > > > > > > Maybe simpler to give the future name "flags" and keep the  
> > > comment  
> > > > > > > "reserved for future use".  
> > > > > >
> > > > > > I'm am OK w/ changing to flags.
> > > > > > If Olivier accepts maybe you can change while applying?  

OK for flags.

> > > > >
> > > > > After the Linux openat experience if you want to add flags.
> > > > > Then all usage of API needs to validate that flags is 0.  
> > > >
> > > > Sorry Stephen, I don't understand what you mean.
> > > > Please could you explain?
> > > >
> > > >  
> > > 
> > > Any time a new field is added that maybe used later you can not guarantee
> > > that existing code correctly initializes the value to zero. What happened with
> > > openat() was that there was a flag value that was originally unused, but since
> > > kernel did not enforce that it was zero; it could not later be used for
> > > extensions.
> > > 
> > > You need to make sure that all reserved fields are initialized.
> > > That means when a private pool is created it is zeroed. And if a flag is new
> > > argument to an API, check for zero at create time.  

+1, this is a good point

> > 
> > I guess we can hard code the value for 0 on the rte_pktmbuf_pool_create function and have some assert on the rte_pktmbuf_pool_init callback (we cannot fail as this function returns void).
> > Any other places you find problematic? 
> 
> No. that should be good. 

Adding an assertion in rte_pktmbuf_pool_init() to check that flag is 0
is a good idea.

In addition, we must ensure that mbp_priv->flags is set to 0 by calling
memset(&mbp_priv, 0, sizeof(mbp_priv)) at several places:

- in rte_pktmbuf_pool_init() when user_mbp_priv == NULL
- in rte_pktmbuf_pool_create_by_ops()
- in examples/ntb/ntb_fwd.c:ntb_mbuf_pool_create()

I think an entry in the release note could be added too.

Olivier

Re: [dpdk-dev] [PATCH v2] mbuf: extend pktmbuf pool private structure

[Olivier Matz]

On Thu, Nov 21, 2019 at 12:28:18PM +0000, Shahaf Shuler wrote:
> With the API and ABI freeze ahead, it will be good to reserve
> some bits on the private structure for future use.
> 
> Otherwise we will potentially need to maintain two different
> private structure during 2020 period.
> 
> There is already one use case for those reserved bits[1]
> 
> The reserved field should be set to 0 by the user.
> 
> [1]
> https://patches.dpdk.org/patch/63077/
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
> On v2:
>  - rename new field to flags
>  - add extra validation in code that flags = 0
> 
> ---
>  lib/librte_mbuf/rte_mbuf.c | 10 +++++++---
>  lib/librte_mbuf/rte_mbuf.h |  1 +
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 35df1c4c38..bd64c55fe6 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -50,6 +50,7 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp, void *opaque_arg)
>  	user_mbp_priv = opaque_arg;
>  	if (user_mbp_priv == NULL) {
>  		default_mbp_priv.mbuf_priv_size = 0;
> +		default_mbp_priv.flags = 0;
>  		if (mp->elt_size > sizeof(struct rte_mbuf))
>  			roomsz = mp->elt_size - sizeof(struct rte_mbuf);
>  		else
> @@ -61,6 +62,7 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp, void *opaque_arg)
>  	RTE_ASSERT(mp->elt_size >= sizeof(struct rte_mbuf) +
>  		user_mbp_priv->mbuf_data_room_size +
>  		user_mbp_priv->mbuf_priv_size);
> +	RTE_ASSERT(user_mbp_priv->flags == 0);
>  
>  	mbp_priv = rte_mempool_get_priv(mp);
>  	memcpy(mbp_priv, user_mbp_priv, sizeof(*mbp_priv));
> @@ -113,7 +115,11 @@ rte_pktmbuf_pool_create_by_ops(const char *name, unsigned int n,
>  	int socket_id, const char *ops_name)
>  {
>  	struct rte_mempool *mp;
> -	struct rte_pktmbuf_pool_private mbp_priv;
> +	struct rte_pktmbuf_pool_private mbp_priv = {
> +		.mbuf_data_room_size = data_room_size,
> +		.mbuf_priv_size = priv_size,
> +		.flags = 0,
> +	};
>  	const char *mp_ops_name = ops_name;
>  	unsigned elt_size;
>  	int ret;
> @@ -126,8 +132,6 @@ rte_pktmbuf_pool_create_by_ops(const char *name, unsigned int n,
>  	}
>  	elt_size = sizeof(struct rte_mbuf) + (unsigned)priv_size +
>  		(unsigned)data_room_size;
> -	mbp_priv.mbuf_data_room_size = data_room_size;
> -	mbp_priv.mbuf_priv_size = priv_size;
>  
>  	mp = rte_mempool_create_empty(name, n, elt_size, cache_size,
>  		 sizeof(struct rte_pktmbuf_pool_private), socket_id, 0);
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 92d81972ab..219b110b76 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -303,6 +303,7 @@ rte_mbuf_to_priv(struct rte_mbuf *m)
>  struct rte_pktmbuf_pool_private {
>  	uint16_t mbuf_data_room_size; /**< Size of data space in each mbuf. */
>  	uint16_t mbuf_priv_size;      /**< Size of private area in each mbuf. */
> +	uint32_t flags; /**< reserved for future use. */
>  };
>  
>  #ifdef RTE_LIBRTE_MBUF_DEBUG
> -- 
> 2.12.0
> 

Sorry I missed the last version in my previous mail.

I think in examples/ntb/ntb_fwd.c:ntb_mbuf_pool_create() should be
updated too. To me, it looks slightly better to not name "flags"
explicitly (ie. use a memset instead), to avoid doing the same
change again in the future.

Thanks,
Olivier

Re: [dpdk-dev] [PATCH v2] mbuf: extend pktmbuf pool private structure

[Shahaf Shuler]

Thursday, November 21, 2019 4:32 PM, Olivier Matz:
> Subject: Re: [PATCH v2] mbuf: extend pktmbuf pool private structure
> 
> On Thu, Nov 21, 2019 at 12:28:18PM +0000, Shahaf Shuler wrote:

[...[

> >
> 
> Sorry I missed the last version in my previous mail.
> 
> I think in examples/ntb/ntb_fwd.c:ntb_mbuf_pool_create() should be
> updated too. To me, it looks slightly better to not name "flags"
> explicitly (ie. use a memset instead), to avoid doing the same
> change again in the future.

Done,
Checkout the v3. 

> 
> Thanks,
> Olivier

[dpdk-dev] [PATCH v3] mbuf: extend pktmbuf pool private structure

[Shahaf Shuler]

With the API and ABI freeze ahead, it will be good to reserve
some bits on the private structure for future use.

Otherwise we will potentially need to maintain two different
private structure during 2020 period.

There is already one use case for those reserved bits[1]

The reserved field should be set to 0 by the user.

[1]
https://patches.dpdk.org/patch/63077/

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
On v3:
 - use memset to zero the entire private struct
 - validate flags is set to 0 also on ntb example

On v2:
 - rename new field to flags
 - add extra validation in code that flags = 0
---
 examples/ntb/ntb_fwd.c     | 1 +
 lib/librte_mbuf/rte_mbuf.c | 4 +++-
 lib/librte_mbuf/rte_mbuf.h | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/examples/ntb/ntb_fwd.c b/examples/ntb/ntb_fwd.c
index edce77ecdc..c914256dd4 100644
--- a/examples/ntb/ntb_fwd.c
+++ b/examples/ntb/ntb_fwd.c
@@ -1256,6 +1256,7 @@ ntb_mbuf_pool_create(uint16_t mbuf_seg_size, uint32_t nb_mbuf,
 	if (mp == NULL)
 		return NULL;
 
+	memset(&mbp_priv, 0, sizeof(mbp_priv));
 	mbp_priv.mbuf_data_room_size = mbuf_seg_size;
 	mbp_priv.mbuf_priv_size = 0;
 	rte_pktmbuf_pool_init(mp, &mbp_priv);
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 35df1c4c38..8fa7f49645 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -49,7 +49,7 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp, void *opaque_arg)
 	/* if no structure is provided, assume no mbuf private area */
 	user_mbp_priv = opaque_arg;
 	if (user_mbp_priv == NULL) {
-		default_mbp_priv.mbuf_priv_size = 0;
+		memset(&default_mbp_priv, 0, sizeof(default_mbp_priv));
 		if (mp->elt_size > sizeof(struct rte_mbuf))
 			roomsz = mp->elt_size - sizeof(struct rte_mbuf);
 		else
@@ -61,6 +61,7 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp, void *opaque_arg)
 	RTE_ASSERT(mp->elt_size >= sizeof(struct rte_mbuf) +
 		user_mbp_priv->mbuf_data_room_size +
 		user_mbp_priv->mbuf_priv_size);
+	RTE_ASSERT(user_mbp_priv->flags == 0);
 
 	mbp_priv = rte_mempool_get_priv(mp);
 	memcpy(mbp_priv, user_mbp_priv, sizeof(*mbp_priv));
@@ -126,6 +127,7 @@ rte_pktmbuf_pool_create_by_ops(const char *name, unsigned int n,
 	}
 	elt_size = sizeof(struct rte_mbuf) + (unsigned)priv_size +
 		(unsigned)data_room_size;
+	memset(&mbp_priv, 0, sizeof(mbp_priv));
 	mbp_priv.mbuf_data_room_size = data_room_size;
 	mbp_priv.mbuf_priv_size = priv_size;
 
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 92d81972ab..219b110b76 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -303,6 +303,7 @@ rte_mbuf_to_priv(struct rte_mbuf *m)
 struct rte_pktmbuf_pool_private {
 	uint16_t mbuf_data_room_size; /**< Size of data space in each mbuf. */
 	uint16_t mbuf_priv_size;      /**< Size of private area in each mbuf. */
+	uint32_t flags; /**< reserved for future use. */
 };
 
 #ifdef RTE_LIBRTE_MBUF_DEBUG
-- 
2.12.0

Re: [dpdk-dev] [PATCH v3] mbuf: extend pktmbuf pool private structure

[Stephen Hemminger]

On Sun, 24 Nov 2019 05:53:46 +0000
Shahaf Shuler <shahafs@mellanox.com> wrote:

> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 35df1c4c38..8fa7f49645 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -49,7 +49,7 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp, void *opaque_arg)
>  	/* if no structure is provided, assume no mbuf private area */
>  	user_mbp_priv = opaque_arg;
>  	if (user_mbp_priv == NULL) {
> -		default_mbp_priv.mbuf_priv_size = 0;
> +		memset(&default_mbp_priv, 0, sizeof(default_mbp_priv));

An alternative would be to use structure initialization.

	struct rte_pktmbuf_pool_private default_mbp_priv = { };

Re: [dpdk-dev] [PATCH v3] mbuf: extend pktmbuf pool private structure

[Thomas Monjalon]

24/11/2019 18:50, Stephen Hemminger:
> On Sun, 24 Nov 2019 05:53:46 +0000
> Shahaf Shuler <shahafs@mellanox.com> wrote:
> 
> > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > index 35df1c4c38..8fa7f49645 100644
> > --- a/lib/librte_mbuf/rte_mbuf.c
> > +++ b/lib/librte_mbuf/rte_mbuf.c
> > @@ -49,7 +49,7 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp, void *opaque_arg)
> >  	/* if no structure is provided, assume no mbuf private area */
> >  	user_mbp_priv = opaque_arg;
> >  	if (user_mbp_priv == NULL) {
> > -		default_mbp_priv.mbuf_priv_size = 0;
> > +		memset(&default_mbp_priv, 0, sizeof(default_mbp_priv));
> 
> An alternative would be to use structure initialization.
> 
> 	struct rte_pktmbuf_pool_private default_mbp_priv = { };

I think we used to have issues with such structure initialization.
If I remember well, icc was not always happy with such construct...
memset is safe

Re: [dpdk-dev] [PATCH v3] mbuf: extend pktmbuf pool private structure

[Shahaf Shuler]

Sunday, November 24, 2019 8:05 PM, Thomas Monjalon:
> Subject: Re: [dpdk-dev] [PATCH v3] mbuf: extend pktmbuf pool private
> structure
> 
> 24/11/2019 18:50, Stephen Hemminger:
> > On Sun, 24 Nov 2019 05:53:46 +0000
> > Shahaf Shuler <shahafs@mellanox.com> wrote:
> >
> > > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > > index 35df1c4c38..8fa7f49645 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.c
> > > +++ b/lib/librte_mbuf/rte_mbuf.c
> > > @@ -49,7 +49,7 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp,
> void *opaque_arg)
> > >  	/* if no structure is provided, assume no mbuf private area */
> > >  	user_mbp_priv = opaque_arg;
> > >  	if (user_mbp_priv == NULL) {
> > > -		default_mbp_priv.mbuf_priv_size = 0;
> > > +		memset(&default_mbp_priv, 0, sizeof(default_mbp_priv));
> >
> > An alternative would be to use structure initialization.
> >
> > 	struct rte_pktmbuf_pool_private default_mbp_priv = { };
> 
> I think we used to have issues with such structure initialization.
> If I remember well, icc was not always happy with such construct...

Yes. Some versions of clang also didn't like it. 

> memset is safe
> 

+1. 

Re: [dpdk-dev] [PATCH v3] mbuf: extend pktmbuf pool private structure

[Olivier Matz]

On Sun, Nov 24, 2019 at 05:53:46AM +0000, Shahaf Shuler wrote:
> With the API and ABI freeze ahead, it will be good to reserve
> some bits on the private structure for future use.
> 
> Otherwise we will potentially need to maintain two different
> private structure during 2020 period.
> 
> There is already one use case for those reserved bits[1]
> 
> The reserved field should be set to 0 by the user.
> 
> [1]
> https://patches.dpdk.org/patch/63077/
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>

An entry in the release note explaining the structure extension may help
the users to anticipate, so they are notified before triggering the
assert.

Then,
Acked-by: Olivier Matz <olivier.matz@6wind.com>

[dpdk-dev] [PATCH v4] mbuf: extend pktmbuf pool private structure

[Shahaf Shuler]

With the API and ABI freeze ahead, it will be good to reserve
some bits on the private structure for future use.

Otherwise we will potentially need to maintain two different
private structure during 2020 period.

There is already one use case for those reserved bits[1]

The reserved field should be set to 0 by the user.

[1]
https://patches.dpdk.org/patch/63077/

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
On v4:
 - added note to release notes

On v3:
 - use memset to zero the entire private struct
 - validate flags is set to 0 also on ntb example

On v2:
 - rename new field to flags
 - add extra validation in code that flags = 0
---
 doc/guides/rel_notes/release_19_11.rst | 7 +++++++
 examples/ntb/ntb_fwd.c                 | 1 +
 lib/librte_mbuf/rte_mbuf.c             | 4 +++-
 lib/librte_mbuf/rte_mbuf.h             | 1 +
 4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/doc/guides/rel_notes/release_19_11.rst b/doc/guides/rel_notes/release_19_11.rst
index c0045a91ff..b8c4d35b4d 100644
--- a/doc/guides/rel_notes/release_19_11.rst
+++ b/doc/guides/rel_notes/release_19_11.rst
@@ -65,6 +65,13 @@ New Features
 
   The lock-free stack implementation is enabled for aarch64 platforms.
 
+* **Extended pktmbuf mempool private structure.**
+
+  rte_pktmbuf_pool_private structure was extended to include flags field
+  for future compatibility.
+  As per 19.11 release this field is reserved and should be set to 0
+  by the user.
+
 * **Changed mempool allocation behaviour.**
 
   Objects are no longer across pages by default.
diff --git a/examples/ntb/ntb_fwd.c b/examples/ntb/ntb_fwd.c
index edce77ecdc..c914256dd4 100644
--- a/examples/ntb/ntb_fwd.c
+++ b/examples/ntb/ntb_fwd.c
@@ -1256,6 +1256,7 @@ ntb_mbuf_pool_create(uint16_t mbuf_seg_size, uint32_t nb_mbuf,
 	if (mp == NULL)
 		return NULL;
 
+	memset(&mbp_priv, 0, sizeof(mbp_priv));
 	mbp_priv.mbuf_data_room_size = mbuf_seg_size;
 	mbp_priv.mbuf_priv_size = 0;
 	rte_pktmbuf_pool_init(mp, &mbp_priv);
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 35df1c4c38..8fa7f49645 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -49,7 +49,7 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp, void *opaque_arg)
 	/* if no structure is provided, assume no mbuf private area */
 	user_mbp_priv = opaque_arg;
 	if (user_mbp_priv == NULL) {
-		default_mbp_priv.mbuf_priv_size = 0;
+		memset(&default_mbp_priv, 0, sizeof(default_mbp_priv));
 		if (mp->elt_size > sizeof(struct rte_mbuf))
 			roomsz = mp->elt_size - sizeof(struct rte_mbuf);
 		else
@@ -61,6 +61,7 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp, void *opaque_arg)
 	RTE_ASSERT(mp->elt_size >= sizeof(struct rte_mbuf) +
 		user_mbp_priv->mbuf_data_room_size +
 		user_mbp_priv->mbuf_priv_size);
+	RTE_ASSERT(user_mbp_priv->flags == 0);
 
 	mbp_priv = rte_mempool_get_priv(mp);
 	memcpy(mbp_priv, user_mbp_priv, sizeof(*mbp_priv));
@@ -126,6 +127,7 @@ rte_pktmbuf_pool_create_by_ops(const char *name, unsigned int n,
 	}
 	elt_size = sizeof(struct rte_mbuf) + (unsigned)priv_size +
 		(unsigned)data_room_size;
+	memset(&mbp_priv, 0, sizeof(mbp_priv));
 	mbp_priv.mbuf_data_room_size = data_room_size;
 	mbp_priv.mbuf_priv_size = priv_size;
 
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 92d81972ab..219b110b76 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -303,6 +303,7 @@ rte_mbuf_to_priv(struct rte_mbuf *m)
 struct rte_pktmbuf_pool_private {
 	uint16_t mbuf_data_room_size; /**< Size of data space in each mbuf. */
 	uint16_t mbuf_priv_size;      /**< Size of private area in each mbuf. */
+	uint32_t flags; /**< reserved for future use. */
 };
 
 #ifdef RTE_LIBRTE_MBUF_DEBUG
-- 
2.12.0

Re: [dpdk-dev] [PATCH v4] mbuf: extend pktmbuf pool private structure

[Olivier Matz]

On Mon, Nov 25, 2019 at 10:21:32AM +0000, Shahaf Shuler wrote:
> With the API and ABI freeze ahead, it will be good to reserve
> some bits on the private structure for future use.
> 
> Otherwise we will potentially need to maintain two different
> private structure during 2020 period.
> 
> There is already one use case for those reserved bits[1]
> 
> The reserved field should be set to 0 by the user.
> 
> [1]
> https://patches.dpdk.org/patch/63077/
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH v4] mbuf: extend pktmbuf pool private structure

[Thomas Monjalon]

25/11/2019 11:27, Olivier Matz:
> On Mon, Nov 25, 2019 at 10:21:32AM +0000, Shahaf Shuler wrote:
> > With the API and ABI freeze ahead, it will be good to reserve
> > some bits on the private structure for future use.
> > 
> > Otherwise we will potentially need to maintain two different
> > private structure during 2020 period.
> > 
> > There is already one use case for those reserved bits[1]
> > 
> > The reserved field should be set to 0 by the user.
> > 
> > [1]
> > https://patches.dpdk.org/patch/63077/
> > 
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied, thanks