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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, December 04, 2014 3:48 PM
> To: Ananyev, Konstantin
> Cc: dev@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@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@6wind.com>
> > > > > Acked-by: David Marchand <david.marchand@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. 

Konstantin

> 
> --
> Thomas