[dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length

[Ananyev, Konstantin]

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Thursday, December 04, 2014 4:59 PM
> To: Ananyev, Konstantin
> Cc: Thomas Monjalon; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> 
> On Thu, Dec 04, 2014 at 04:18:03PM +0000, Ananyev, Konstantin wrote:
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ananyev, Konstantin
> > > Sent: Thursday, December 04, 2014 4:05 PM
> > > To: Thomas Monjalon
> > > Cc: dev at dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > > > Sent: Thursday, December 04, 2014 3:48 PM
> > > > To: Ananyev, Konstantin
> > > > Cc: dev at dpdk.org; Richardson, Bruce
> > > > Subject: Re: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> > > >
> > > > 2014-12-04 15:29, Ananyev, Konstantin:
> > > > > From: Richardson, Bruce
> > > > > > On Thu, Dec 04, 2014 at 02:50:11PM +0000, Ananyev, Konstantin wrote:
> > > > > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Jean-Mickael Guerin
> > > > > > > > The template mbuf_initializer is hard coded with a buflen which
> > > > > > > > might have been set differently by the application at the time of
> > > > > > > > mbuf pool creation.
> > > > > > > >
> > > > > > > > Switch to a mbuf allocation, to fetch the correct default values.
> > > > > > > > There is no performance impact because this is not a data-plane API.
> > > > > > > >
> > > > > > > > Signed-off-by: Jean-Mickael Guerin <jean-mickael.guerin at 6wind.com>
> > > > > > > > Acked-by: David Marchand <david.marchand at 6wind.com>
> > > > > > > > Fixes: 0ff3324da2 ("ixgbe: rework vector pmd following mbuf changes")
> > > > > > > > ---
> > > > > > > >  lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c | 19 ++++++++++++-------
> > > > > > > >  1 file changed, 12 insertions(+), 7 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
> > > > > > > > index c1b5a78..f7b02f5 100644
> > > > > > > > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
> > > > > > > > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
> > > > > > > > @@ -732,17 +732,22 @@ static struct ixgbe_txq_ops vec_txq_ops = {
> > > > > > > >  int
> > > > > > > >  ixgbe_rxq_vec_setup(struct igb_rx_queue *rxq)
> > > > > > > >  {
> > > > > > > > -	struct rte_mbuf mb_def = { .buf_addr = 0 }; /* zeroed mbuf */
> > > > > > > > +	struct rte_mbuf *mb_def;
> > > > > > > >
> > > > > > > > -	mb_def.nb_segs = 1;
> > > > > > > > -	mb_def.data_off = RTE_PKTMBUF_HEADROOM;
> > > > > > > > -	mb_def.buf_len = rxq->mb_pool->elt_size - sizeof(struct rte_mbuf);
> > > > > > > > -	mb_def.port = rxq->port_id;
> > > > > > > > -	rte_mbuf_refcnt_set(&mb_def, 1);
> > > > > > > > +	mb_def = rte_pktmbuf_alloc(rxq->mb_pool);
> > > > > > >
> > > > > > > Could you explain to me, what is an advantage of using dynamic allocation vs local struct here?
> > > > > > > I don't see any.
> > > > > >
> > > > > > It means that we get an mbuf that is initialized as done by the initialization
> > > > > > function passed to the mempool_create call. The static variable method assumes
> > > > > > that we configure the mbuf using default setting for buf_len etc.
> > > > >
> > > > > I understand that, but  why it can't be done in some other way?
> > > > > Without allocating/freeing?
> > > > > Let say, at mempool_create() store obj_init() and then add ability to call it again?
> > > > > Anyway, it doesn't look to me like a critical problem, that requires an urgent patch for 1.8.
> > > >
> > > > Konstantin, when a bug is seen, it must be fixed ASAP.
> > >
> > > Well, it will be exposed only if someone will use a custom mbufs right?
> > > I.e, the se 2 lines would not be correct:
> > > mb_def.data_off = RTE_PKTMBUF_HEADROOM;
> > > mb_def.buf_len = rxq->mb_pool->elt_size - sizeof(struct rte_mbuf);
> > >
> > > Thoug we setup same data_off  like that in all other PMDs as well.
> > > Something like that:
> > > m->data_off = RTE_PKTMBUF_HEADROOM;
> > > could be seen across all RX functions we have for different PMDs.
> > >
> > > The only difference is buf_len, but in theory even with dynamic allocation,
> > > the fix would be totally correct.
> > > As no one can guarantee, that with custom mbufs, all buffers inside the pool will have the same length.
> >
> > Which makes me think, that we probably shouldn't overwrite buf_len by rxq->mbuf_initializer.
> >
> I believe that it is perfectly safe to do so. All buffers from a mempool are meant
> to be the same size, therefore reading the length of one buffer should tell you
> what size all buffers are.

Yes, objects in the mempool are the same size
But nothing prevents you, in your custom obj_init() to setup mbuf->buf_len to some other value,
that could be smaller, then mempool element size.
Let say you'd like all your mbufs from particular mempool to be at least 2K long and 1K aligned.
So you set RTE_MBUF_REFCNT_ATOMIC=n, and call  rte_mempool_create(...,elst_size=0xc00,...);
Then at you custom obj_init() you do:

struct rte_mbuf *m =  RTE_ALIGN_CEIL(_m, 1024);
buf_len = mp->elt_size - sizeof(struct rte_mbuf) - (m - _m);
...

>From my point, nothing wrong is done here, and we have a mempool where mbufs might have different buf_len.

Another example, is attachment of external buffer to the mbuf.
We are doing it to support zero-copy inside our vhost app.
Right now we don't allow external buffer length be bigger then mbuf buf_len, but again some people may like to allow that.

> If we do hit a scenario where we do need to support
> variable size buffers from a single mempool, we can do that via the older unoptimized
> code paths, I think, since it's a definite edge case.
> 
> /Bruce