mbuf: extend pktmbuf pool private structure
Checks
Commit Message
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(+)
Comments
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
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&data=02%7C01%7Cshahafs%40m
> ellanox.
> >
> com%7C261be5d5bd344538663c08d76c420fcb%7Ca652971c7d2e4d9ba6a4d14
> 9256f4
> >
> 61b%7C0%7C0%7C637096903295807188&sdata=ruzHICpeUCNXbw4XXD
> Xj1ZOP35Z
> > ZoUtgWbSPHYSKDFo%3D&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&data=02%7C0
> 1%7Cshahafs%40mellanox.com%7C261be5d5bd344538663c08d76c420fcb%7C
> a652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637096903295807188&a
> mp;sdata=U8wYu7M85a9F%2BrGxaaxpOce6Pedh4THo4%2Fi03Sj2OV0%3D&
> amp;reserved=0
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?
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?
>
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.
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?
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).
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).
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.
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
@@ -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