DPDK patches and discussions
 help / color / Atom feed
* [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure
@ 2019-11-18 10:02 Shahaf Shuler
  2019-11-18 16:12 ` Stephen Hemminger
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Shahaf Shuler @ 2019-11-18 10:02 UTC (permalink / raw)
  To: olivier.matz, Thomas Monjalon, dev

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure
  2019-11-18 10:02 [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure Shahaf Shuler
@ 2019-11-18 16:12 ` Stephen Hemminger
  2019-11-19  6:35   ` Shahaf Shuler
  2019-11-19  9:32 ` Thomas Monjalon
  2019-11-21 12:28 ` [dpdk-dev] [PATCH v2] " Shahaf Shuler
  2 siblings, 1 reply; 22+ messages in thread
From: Stephen Hemminger @ 2019-11-18 16:12 UTC (permalink / raw)
  To: Shahaf Shuler; +Cc: olivier.matz, Thomas Monjalon, dev

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure
  2019-11-18 16:12 ` Stephen Hemminger
@ 2019-11-19  6:35   ` Shahaf Shuler
  0 siblings, 0 replies; 22+ messages in thread
From: Shahaf Shuler @ 2019-11-19  6:35 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: olivier.matz, Thomas Monjalon, dev

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure
  2019-11-18 10:02 [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure Shahaf Shuler
  2019-11-18 16:12 ` Stephen Hemminger
@ 2019-11-19  9:32 ` Thomas Monjalon
  2019-11-19 15:23   ` Shahaf Shuler
  2019-11-21 12:28 ` [dpdk-dev] [PATCH v2] " Shahaf Shuler
  2 siblings, 1 reply; 22+ messages in thread
From: Thomas Monjalon @ 2019-11-19  9:32 UTC (permalink / raw)
  To: Shahaf Shuler, olivier.matz; +Cc: dev

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?



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure
  2019-11-19  9:32 ` Thomas Monjalon
@ 2019-11-19 15:23   ` Shahaf Shuler
  2019-11-19 16:25     ` Stephen Hemminger
  0 siblings, 1 reply; 22+ messages in thread
From: Shahaf Shuler @ 2019-11-19 15:23 UTC (permalink / raw)
  To: Thomas Monjalon, olivier.matz; +Cc: dev

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?
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure
  2019-11-19 15:23   ` Shahaf Shuler
@ 2019-11-19 16:25     ` Stephen Hemminger
  2019-11-19 22:30       ` Thomas Monjalon
  0 siblings, 1 reply; 22+ messages in thread
From: Stephen Hemminger @ 2019-11-19 16:25 UTC (permalink / raw)
  To: Shahaf Shuler; +Cc: Thomas Monjalon, olivier.matz, dev

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure
  2019-11-19 16:25     ` Stephen Hemminger
@ 2019-11-19 22:30       ` Thomas Monjalon
  2019-11-19 23:50         ` Stephen Hemminger
  0 siblings, 1 reply; 22+ messages in thread
From: Thomas Monjalon @ 2019-11-19 22:30 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Shahaf Shuler, olivier.matz, dev

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?



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure
  2019-11-19 22:30       ` Thomas Monjalon
@ 2019-11-19 23:50         ` Stephen Hemminger
  2019-11-20  7:01           ` Shahaf Shuler
  0 siblings, 1 reply; 22+ messages in thread
From: Stephen Hemminger @ 2019-11-19 23:50 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: Shahaf Shuler, olivier.matz, dev

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).

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure
  2019-11-19 23:50         ` Stephen Hemminger
@ 2019-11-20  7:01           ` Shahaf Shuler
  2019-11-20 15:56             ` Stephen Hemminger
  0 siblings, 1 reply; 22+ messages in thread
From: Shahaf Shuler @ 2019-11-20  7:01 UTC (permalink / raw)
  To: Stephen Hemminger, Thomas Monjalon; +Cc: olivier.matz, dev

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).

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure
  2019-11-20  7:01           ` Shahaf Shuler
@ 2019-11-20 15:56             ` Stephen Hemminger
  2019-11-21 14:15               ` Olivier Matz
  0 siblings, 1 reply; 22+ messages in thread
From: Stephen Hemminger @ 2019-11-20 15:56 UTC (permalink / raw)
  To: Shahaf Shuler; +Cc: Thomas Monjalon, olivier.matz, dev

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. 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v2] mbuf: extend pktmbuf pool private structure
  2019-11-18 10:02 [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure Shahaf Shuler
  2019-11-18 16:12 ` Stephen Hemminger
  2019-11-19  9:32 ` Thomas Monjalon
@ 2019-11-21 12:28 ` " Shahaf Shuler
  2019-11-21 14:31   ` Olivier Matz
  2019-11-24  5:53   ` [dpdk-dev] [PATCH v3] " Shahaf Shuler
  2 siblings, 2 replies; 22+ messages in thread
From: Shahaf Shuler @ 2019-11-21 12:28 UTC (permalink / raw)
  To: olivier.matz, Thomas Monjalon, dev

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure
  2019-11-20 15:56             ` Stephen Hemminger
@ 2019-11-21 14:15               ` Olivier Matz
  0 siblings, 0 replies; 22+ messages in thread
From: Olivier Matz @ 2019-11-21 14:15 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Shahaf Shuler, Thomas Monjalon, dev

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v2] mbuf: extend pktmbuf pool private structure
  2019-11-21 12:28 ` [dpdk-dev] [PATCH v2] " Shahaf Shuler
@ 2019-11-21 14:31   ` Olivier Matz
  2019-11-24  5:52     ` Shahaf Shuler
  2019-11-24  5:53   ` [dpdk-dev] [PATCH v3] " Shahaf Shuler
  1 sibling, 1 reply; 22+ messages in thread
From: Olivier Matz @ 2019-11-21 14:31 UTC (permalink / raw)
  To: Shahaf Shuler; +Cc: Thomas Monjalon, dev

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v2] mbuf: extend pktmbuf pool private structure
  2019-11-21 14:31   ` Olivier Matz
@ 2019-11-24  5:52     ` Shahaf Shuler
  0 siblings, 0 replies; 22+ messages in thread
From: Shahaf Shuler @ 2019-11-24  5:52 UTC (permalink / raw)
  To: Olivier Matz; +Cc: Thomas Monjalon, dev

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v3] mbuf: extend pktmbuf pool private structure
  2019-11-21 12:28 ` [dpdk-dev] [PATCH v2] " Shahaf Shuler
  2019-11-21 14:31   ` Olivier Matz
@ 2019-11-24  5:53   ` " Shahaf Shuler
  2019-11-24 17:50     ` Stephen Hemminger
                       ` (2 more replies)
  1 sibling, 3 replies; 22+ messages in thread
From: Shahaf Shuler @ 2019-11-24  5:53 UTC (permalink / raw)
  To: dev, Thomas Monjalon, olivier.matz

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v3] mbuf: extend pktmbuf pool private structure
  2019-11-24  5:53   ` [dpdk-dev] [PATCH v3] " Shahaf Shuler
@ 2019-11-24 17:50     ` Stephen Hemminger
  2019-11-24 18:05       ` Thomas Monjalon
  2019-11-25  8:12     ` Olivier Matz
  2019-11-25 10:21     ` [dpdk-dev] [PATCH v4] " Shahaf Shuler
  2 siblings, 1 reply; 22+ messages in thread
From: Stephen Hemminger @ 2019-11-24 17:50 UTC (permalink / raw)
  To: Shahaf Shuler; +Cc: dev, Thomas Monjalon, olivier.matz

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v3] mbuf: extend pktmbuf pool private structure
  2019-11-24 17:50     ` Stephen Hemminger
@ 2019-11-24 18:05       ` Thomas Monjalon
  2019-11-24 18:10         ` Shahaf Shuler
  0 siblings, 1 reply; 22+ messages in thread
From: Thomas Monjalon @ 2019-11-24 18:05 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Shahaf Shuler, dev, olivier.matz

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



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v3] mbuf: extend pktmbuf pool private structure
  2019-11-24 18:05       ` Thomas Monjalon
@ 2019-11-24 18:10         ` Shahaf Shuler
  0 siblings, 0 replies; 22+ messages in thread
From: Shahaf Shuler @ 2019-11-24 18:10 UTC (permalink / raw)
  To: Thomas Monjalon, Stephen Hemminger; +Cc: dev, olivier.matz

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. 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v3] mbuf: extend pktmbuf pool private structure
  2019-11-24  5:53   ` [dpdk-dev] [PATCH v3] " Shahaf Shuler
  2019-11-24 17:50     ` Stephen Hemminger
@ 2019-11-25  8:12     ` Olivier Matz
  2019-11-25 10:21     ` [dpdk-dev] [PATCH v4] " Shahaf Shuler
  2 siblings, 0 replies; 22+ messages in thread
From: Olivier Matz @ 2019-11-25  8:12 UTC (permalink / raw)
  To: Shahaf Shuler; +Cc: dev, Thomas Monjalon

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>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v4] mbuf: extend pktmbuf pool private structure
  2019-11-24  5:53   ` [dpdk-dev] [PATCH v3] " Shahaf Shuler
  2019-11-24 17:50     ` Stephen Hemminger
  2019-11-25  8:12     ` Olivier Matz
@ 2019-11-25 10:21     ` " Shahaf Shuler
  2019-11-25 10:27       ` Olivier Matz
  2 siblings, 1 reply; 22+ messages in thread
From: Shahaf Shuler @ 2019-11-25 10:21 UTC (permalink / raw)
  To: dev, Thomas Monjalon, olivier.matz

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v4] mbuf: extend pktmbuf pool private structure
  2019-11-25 10:21     ` [dpdk-dev] [PATCH v4] " Shahaf Shuler
@ 2019-11-25 10:27       ` Olivier Matz
  2019-11-25 21:42         ` Thomas Monjalon
  0 siblings, 1 reply; 22+ messages in thread
From: Olivier Matz @ 2019-11-25 10:27 UTC (permalink / raw)
  To: Shahaf Shuler; +Cc: dev, Thomas Monjalon

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>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v4] mbuf: extend pktmbuf pool private structure
  2019-11-25 10:27       ` Olivier Matz
@ 2019-11-25 21:42         ` Thomas Monjalon
  0 siblings, 0 replies; 22+ messages in thread
From: Thomas Monjalon @ 2019-11-25 21:42 UTC (permalink / raw)
  To: Shahaf Shuler; +Cc: dev, Olivier Matz

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



^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, back to index

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-18 10:02 [dpdk-dev] [PATCH] mbuf: extend pktmbuf pool private structure Shahaf Shuler
2019-11-18 16:12 ` Stephen Hemminger
2019-11-19  6:35   ` Shahaf Shuler
2019-11-19  9:32 ` Thomas Monjalon
2019-11-19 15:23   ` Shahaf Shuler
2019-11-19 16:25     ` Stephen Hemminger
2019-11-19 22:30       ` Thomas Monjalon
2019-11-19 23:50         ` Stephen Hemminger
2019-11-20  7:01           ` Shahaf Shuler
2019-11-20 15:56             ` Stephen Hemminger
2019-11-21 14:15               ` Olivier Matz
2019-11-21 12:28 ` [dpdk-dev] [PATCH v2] " Shahaf Shuler
2019-11-21 14:31   ` Olivier Matz
2019-11-24  5:52     ` Shahaf Shuler
2019-11-24  5:53   ` [dpdk-dev] [PATCH v3] " Shahaf Shuler
2019-11-24 17:50     ` Stephen Hemminger
2019-11-24 18:05       ` Thomas Monjalon
2019-11-24 18:10         ` Shahaf Shuler
2019-11-25  8:12     ` Olivier Matz
2019-11-25 10:21     ` [dpdk-dev] [PATCH v4] " Shahaf Shuler
2019-11-25 10:27       ` Olivier Matz
2019-11-25 21:42         ` Thomas Monjalon

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox