[v3] 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>
---
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(-)
Comments
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 = { };
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
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.
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>
@@ -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);
@@ -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;
@@ -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