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

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

On Thu, Dec 04, 2014 at 03:26:21PM +0100, Jean-Mickael Guerin wrote:
> 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")

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

> ---
>  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);
> +	if (mb_def == NULL) {
> +		PMD_INIT_LOG(ERR, "ixgbe_rxq_vec_setup: could not allocate one mbuf");
> +		return -1;
> +	}
> +	/* nb_segs, refcnt, data_off and buf_len are already set */
> +	mb_def->port = rxq->port_id;
>  
>  	/* prevent compiler reordering: rearm_data covers previous fields */
>  	rte_compiler_barrier();
> -	rxq->mbuf_initializer = *((uint64_t *)&mb_def.rearm_data);
> +	rxq->mbuf_initializer = *((uint64_t *)&mb_def->rearm_data);
> +
> +	rte_pktmbuf_free(mb_def);
> +
>  	return 0;
>  }
>  
> -- 
> 2.1.3
>
  

Hi,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jean-Mickael Guerin
> Sent: Thursday, December 04, 2014 2:26 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> 
> 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.
Plus if rte_pktmbuf_alloc() would fail, we'll leave our rx queue not configured properly.
As I can see ixgbe_dev_rx_queue_setup() doesn't check the return value of >  ixgbe_rxq_vec_setup()
(as it is just not supposed to fail).
So ixgbe_dev_rx_queue_setup() will return OK for unconfigured RX queue.

> +	if (mb_def == NULL) {
> +		PMD_INIT_LOG(ERR, "ixgbe_rxq_vec_setup: could not allocate one mbuf");
> +		return -1;
> +	}
> +	/* nb_segs, refcnt, data_off and buf_len are already set */
> +	mb_def->port = rxq->port_id;
> 
>  	/* prevent compiler reordering: rearm_data covers previous fields */
>  	rte_compiler_barrier();

I don't think we need it here.

> -	rxq->mbuf_initializer = *((uint64_t *)&mb_def.rearm_data);
> +	rxq->mbuf_initializer = *((uint64_t *)&mb_def->rearm_data);
> +
> +	rte_pktmbuf_free(mb_def);
> +
>  	return 0;
>  }
> 
> --
> 2.1.3

Somy vote -  NACK for the whole series.
Konstantin
  

On Thu, Dec 04, 2014 at 02:50:11PM +0000, Ananyev, Konstantin wrote:
> Hi,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jean-Mickael Guerin
> > Sent: Thursday, December 04, 2014 2:26 PM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> > 
> > 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.

> Plus if rte_pktmbuf_alloc() would fail, we'll leave our rx queue not configured properly.
> As I can see ixgbe_dev_rx_queue_setup() doesn't check the return value of >  ixgbe_rxq_vec_setup()
> (as it is just not supposed to fail).
> So ixgbe_dev_rx_queue_setup() will return OK for unconfigured RX queue.

Good catch, that's something that should perhaps be looked at in a V2 patch, I
think.

> 
> > +	if (mb_def == NULL) {
> > +		PMD_INIT_LOG(ERR, "ixgbe_rxq_vec_setup: could not allocate one mbuf");
> > +		return -1;
> > +	}
> > +	/* nb_segs, refcnt, data_off and buf_len are already set */
> > +	mb_def->port = rxq->port_id;
> > 
> >  	/* prevent compiler reordering: rearm_data covers previous fields */
> >  	rte_compiler_barrier();
> 
> I don't think we need it here.

I think we might, as the compiler doesn't know that the rearm data overlaps 
with the previously set fields, so may reorder the reads and writes thinking
they are independent.
> 
> > -	rxq->mbuf_initializer = *((uint64_t *)&mb_def.rearm_data);
> > +	rxq->mbuf_initializer = *((uint64_t *)&mb_def->rearm_data);
> > +
> > +	rte_pktmbuf_free(mb_def);
> > +
> >  	return 0;
> >  }
> > 
> > --
> > 2.1.3
> 
> Somy vote -  NACK for the whole series.
> Konstantin
>
  

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Thursday, December 04, 2014 3:15 PM
> To: Ananyev, Konstantin
> Cc: Jean-Mickael Guerin; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> 
> On Thu, Dec 04, 2014 at 02:50:11PM +0000, Ananyev, Konstantin wrote:
> > Hi,
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jean-Mickael Guerin
> > > Sent: Thursday, December 04, 2014 2:26 PM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> > >
> > > 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.

> > Plus if rte_pktmbuf_alloc() would fail, we'll leave our rx queue not configured properly.
> > As I can see ixgbe_dev_rx_queue_setup() doesn't check the return value of >  ixgbe_rxq_vec_setup()
> > (as it is just not supposed to fail).
> > So ixgbe_dev_rx_queue_setup() will return OK for unconfigured RX queue.
> 
> Good catch, that's something that should perhaps be looked at in a V2 patch, I
> think.
> 
> >
> > > +	if (mb_def == NULL) {
> > > +		PMD_INIT_LOG(ERR, "ixgbe_rxq_vec_setup: could not allocate one mbuf");
> > > +		return -1;
> > > +	}
> > > +	/* nb_segs, refcnt, data_off and buf_len are already set */
> > > +	mb_def->port = rxq->port_id;
> > >
> > >  	/* prevent compiler reordering: rearm_data covers previous fields */
> > >  	rte_compiler_barrier();
> >
> > I don't think we need it here.
> 
> I think we might, as the compiler doesn't know that the rearm data overlaps
> with the previously set fields, so may reorder the reads and writes thinking
> they are independent.

Why it doesn't?
I suppose compiler has all the knowledge of the mbuf structure layout at that point.
Or there is a some sort of bug in some version of the compiler?

> >
> > > -	rxq->mbuf_initializer = *((uint64_t *)&mb_def.rearm_data);
> > > +	rxq->mbuf_initializer = *((uint64_t *)&mb_def->rearm_data);
> > > +
> > > +	rte_pktmbuf_free(mb_def);
> > > +
> > >  	return 0;
> > >  }
> > >
> > > --
> > > 2.1.3
> >
> > Somy vote -  NACK for the whole series.
> > Konstantin
> >
  

On Thu, Dec 04, 2014 at 03:29:04PM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Thursday, December 04, 2014 3:15 PM
> > To: Ananyev, Konstantin
> > Cc: Jean-Mickael Guerin; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> > 
> > On Thu, Dec 04, 2014 at 02:50:11PM +0000, Ananyev, Konstantin wrote:
> > > Hi,
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jean-Mickael Guerin
> > > > Sent: Thursday, December 04, 2014 2:26 PM
> > > > To: dev@dpdk.org
> > > > Subject: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> > > >
> > > > 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.
> 
> > > Plus if rte_pktmbuf_alloc() would fail, we'll leave our rx queue not configured properly.
> > > As I can see ixgbe_dev_rx_queue_setup() doesn't check the return value of >  ixgbe_rxq_vec_setup()
> > > (as it is just not supposed to fail).
> > > So ixgbe_dev_rx_queue_setup() will return OK for unconfigured RX queue.
> > 
> > Good catch, that's something that should perhaps be looked at in a V2 patch, I
> > think.
> > 
> > >
> > > > +	if (mb_def == NULL) {
> > > > +		PMD_INIT_LOG(ERR, "ixgbe_rxq_vec_setup: could not allocate one mbuf");
> > > > +		return -1;
> > > > +	}
> > > > +	/* nb_segs, refcnt, data_off and buf_len are already set */
> > > > +	mb_def->port = rxq->port_id;
> > > >
> > > >  	/* prevent compiler reordering: rearm_data covers previous fields */
> > > >  	rte_compiler_barrier();
> > >
> > > I don't think we need it here.
> > 
> > I think we might, as the compiler doesn't know that the rearm data overlaps
> > with the previously set fields, so may reorder the reads and writes thinking
> > they are independent.
> 
> Why it doesn't?
> I suppose compiler has all the knowledge of the mbuf structure layout at that point.
> Or there is a some sort of bug in some version of the compiler?
> 

No, we're just violating the layout here by dereferencing past the end of the array :-)

/Bruce

> > >
> > > > -	rxq->mbuf_initializer = *((uint64_t *)&mb_def.rearm_data);
> > > > +	rxq->mbuf_initializer = *((uint64_t *)&mb_def->rearm_data);
> > > > +
> > > > +	rte_pktmbuf_free(mb_def);
> > > > +
> > > >  	return 0;
> > > >  }
> > > >
> > > > --
> > > > 2.1.3
> > >
> > > Somy vote -  NACK for the whole series.
> > > Konstantin
> > >
  

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.
  

On 04/12/2014 16:32, Bruce Richardson wrote:
> On Thu, Dec 04, 2014 at 03:29:04PM +0000, Ananyev, Konstantin wrote:
>>
>>
>>> -----Original Message-----
>>> From: Richardson, Bruce
>>> Sent: Thursday, December 04, 2014 3:15 PM
>>> To: Ananyev, Konstantin
>>> Cc: Jean-Mickael Guerin; dev@dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
>>>
>>> On Thu, Dec 04, 2014 at 02:50:11PM +0000, Ananyev, Konstantin wrote:
>>>> Hi,
>>>>
>>>>> -----Original Message-----
>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jean-Mickael Guerin
>>>>> Sent: Thursday, December 04, 2014 2:26 PM
>>>>> To: dev@dpdk.org
>>>>> Subject: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
>>>>>
>>>>> 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?

This is a good idea, useful in other places of mbuf API.

>> Anyway, it doesn't look to me like a critical problem, that requires an urgent patch for 1.8.
>>

This is about data corruption - a simple function like 
rte_pktmbuf_tailroom() returns an incorrect value...

Let me try obj_init() variant and we will see if it is acceptable in 1.8 
- it does not look a big change after all.

>>>> Plus if rte_pktmbuf_alloc() would fail, we'll leave our rx queue not configured properly.
>>>> As I can see ixgbe_dev_rx_queue_setup() doesn't check the return value of >  ixgbe_rxq_vec_setup()
>>>> (as it is just not supposed to fail).
>>>> So ixgbe_dev_rx_queue_setup() will return OK for unconfigured RX queue.
>>>
>>> Good catch, that's something that should perhaps be looked at in a V2 patch, I
>>> think.
>>>
>>>>
>>>>> +	if (mb_def == NULL) {
>>>>> +		PMD_INIT_LOG(ERR, "ixgbe_rxq_vec_setup: could not allocate one mbuf");
>>>>> +		return -1;
>>>>> +	}
>>>>> +	/* nb_segs, refcnt, data_off and buf_len are already set */
>>>>> +	mb_def->port = rxq->port_id;
>>>>>
>>>>>   	/* prevent compiler reordering: rearm_data covers previous fields */
>>>>>   	rte_compiler_barrier();
>>>>
>>>> I don't think we need it here.
>>>
>>> I think we might, as the compiler doesn't know that the rearm data overlaps
>>> with the previously set fields, so may reorder the reads and writes thinking
>>> they are independent.
>>
>> Why it doesn't?
>> I suppose compiler has all the knowledge of the mbuf structure layout at that point.
>> Or there is a some sort of bug in some version of the compiler?
>>
>
> No, we're just violating the layout here by dereferencing past the end of the array :-)
>
> /Bruce
>
>>>>
>>>>> -	rxq->mbuf_initializer = *((uint64_t *)&mb_def.rearm_data);
>>>>> +	rxq->mbuf_initializer = *((uint64_t *)&mb_def->rearm_data);
>>>>> +
>>>>> +	rte_pktmbuf_free(mb_def);
>>>>> +
>>>>>   	return 0;
>>>>>   }
>>>>>
>>>>> --
>>>>> 2.1.3
>>>>
>>>> Somy vote -  NACK for the whole series.
>>>> Konstantin
>>>>
  

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

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin
> Sent: Thursday, December 04, 2014 4:05 PM
> To: Thomas Monjalon
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> 
> 
> 
> > -----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.

Which makes me think, that we probably shouldn't overwrite buf_len by rxq->mbuf_initializer.

> 
> Konstantin
> 
> >
> > --
> > Thomas
  

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Thursday, December 04, 2014 3:32 PM
> To: Ananyev, Konstantin
> Cc: Jean-Mickael Guerin; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> 
> On Thu, Dec 04, 2014 at 03:29:04PM +0000, Ananyev, Konstantin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Richardson, Bruce
> > > Sent: Thursday, December 04, 2014 3:15 PM
> > > To: Ananyev, Konstantin
> > > Cc: Jean-Mickael Guerin; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> > >
> > > On Thu, Dec 04, 2014 at 02:50:11PM +0000, Ananyev, Konstantin wrote:
> > > > Hi,
> > > >
> > > > > -----Original Message-----
> > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jean-Mickael Guerin
> > > > > Sent: Thursday, December 04, 2014 2:26 PM
> > > > > To: dev@dpdk.org
> > > > > Subject: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> > > > >
> > > > > 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.
> >
> > > > Plus if rte_pktmbuf_alloc() would fail, we'll leave our rx queue not configured properly.
> > > > As I can see ixgbe_dev_rx_queue_setup() doesn't check the return value of >  ixgbe_rxq_vec_setup()
> > > > (as it is just not supposed to fail).
> > > > So ixgbe_dev_rx_queue_setup() will return OK for unconfigured RX queue.
> > >
> > > Good catch, that's something that should perhaps be looked at in a V2 patch, I
> > > think.
> > >
> > > >
> > > > > +	if (mb_def == NULL) {
> > > > > +		PMD_INIT_LOG(ERR, "ixgbe_rxq_vec_setup: could not allocate one mbuf");
> > > > > +		return -1;
> > > > > +	}
> > > > > +	/* nb_segs, refcnt, data_off and buf_len are already set */
> > > > > +	mb_def->port = rxq->port_id;
> > > > >
> > > > >  	/* prevent compiler reordering: rearm_data covers previous fields */
> > > > >  	rte_compiler_barrier();
> > > >
> > > > I don't think we need it here.
> > >
> > > I think we might, as the compiler doesn't know that the rearm data overlaps
> > > with the previously set fields, so may reorder the reads and writes thinking
> > > they are independent.
> >
> > Why it doesn't?
> > I suppose compiler has all the knowledge of the mbuf structure layout at that point.
> > Or there is a some sort of bug in some version of the compiler?
> >
> 
> No, we're just violating the layout here by dereferencing past the end of the array :-)

So are  you saying, that any code like:

extern uint64_t *p;
struct {uint32_t a[1], b;} x;
 x.b = 2;
 L1:    
 *p  = (uint64_t *)&x.a; 

Would require a compiler barrier at L1?

> 
> /Bruce
> 
> > > >
> > > > > -	rxq->mbuf_initializer = *((uint64_t *)&mb_def.rearm_data);
> > > > > +	rxq->mbuf_initializer = *((uint64_t *)&mb_def->rearm_data);
> > > > > +
> > > > > +	rte_pktmbuf_free(mb_def);
> > > > > +
> > > > >  	return 0;
> > > > >  }
> > > > >
> > > > > --
> > > > > 2.1.3
> > > >
> > > > Somy vote -  NACK for the whole series.
> > > > Konstantin
> > > >
  

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Thursday, December 04, 2014 4:18 PM
> To: Ananyev, Konstantin; Thomas Monjalon
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin
> > Sent: Thursday, December 04, 2014 4:05 PM
> > To: Thomas Monjalon
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> >
> >
> >
> > > -----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.
> 
> Which makes me think, that we probably shouldn't overwrite buf_len by rxq->mbuf_initializer.

I meant something like:
at ixgbe_rxq_vec_setup() set mb_def.buf_len = 0;

and then, at ixgbe_rxq_rearm():
mb0->rearm_data[0] &= (uint64_t)UINT16_MAX;
mb0->rearm_data[0] |= rxq->mbuf_initializer;

Though, don't know would it cause any performance drop.
Konstantin

> 
> >
> > Konstantin
> >
> > >
> > > --
> > > Thomas
  

On Thu, Dec 04, 2014 at 04:18:03PM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin
> > Sent: Thursday, December 04, 2014 4:05 PM
> > To: Thomas Monjalon
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> > 
> > 
> > 
> > > -----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.
> 
> 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. 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
  

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

I agree, and there is a place to store some values unique for all mbufs 
in a pool:

struct rte_pktmbuf_pool_private {
         uint16_t mbuf_data_room_size; /**< Size of data space in each 
mbuf.*/
};

We could add a new field mbuf_buf_len here, it looks definitely better 
than new callbacks in rte_mempool.

What do you think?
  

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Thursday, December 04, 2014 4:59 PM
> To: Ananyev, Konstantin
> Cc: Thomas Monjalon; dev@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@dpdk.org] On Behalf Of Ananyev, Konstantin
> > > Sent: Thursday, December 04, 2014 4:05 PM
> > > To: Thomas Monjalon
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> > >
> > >
> > >
> > > > -----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.
> >
> > 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
  

On Thu, Dec 04, 2014 at 06:11:41PM +0100, Jean-Mickael Guerin wrote:
> >>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. 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.
> >
> 
> I agree, and there is a place to store some values unique for all mbufs in a
> pool:
> 
> struct rte_pktmbuf_pool_private {
>         uint16_t mbuf_data_room_size; /**< Size of data space in each
> mbuf.*/
> };
> 
> We could add a new field mbuf_buf_len here, it looks definitely better than
> new callbacks in rte_mempool.
> 
> What do you think?

I think it's overkill.
I like the original suggest to allocate a buffer and pull the length settings from
there. Just add the checking so that if the allocation fails the whole setup fails.
If we can't allocate one mbuf from a pool, it's a pretty catastrophic error that
needs to be flagged ASAP. I wouldn't look to do anything up and above that.

/Bruce
  

On Thu, Dec 04, 2014 at 05:17:16PM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Thursday, December 04, 2014 4:59 PM
> > To: Ananyev, Konstantin
> > Cc: Thomas Monjalon; dev@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@dpdk.org] On Behalf Of Ananyev, Konstantin
> > > > Sent: Thursday, December 04, 2014 4:05 PM
> > > > To: Thomas Monjalon
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH 2/2] ixgbe: don't override mbuf buffer length
> > > >
> > > >
> > > >
> > > > > -----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.
> > >
> > > 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.
> 
Would the originally proposed solution not work in all these cases - assuming it's
enhanced to catch and properly handle failure cases?

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