[dpdk-dev] [PATCH v2 7/8] net/mlx4: align Tx descriptors number

[Matan Azrad]

Hi Adrien

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com]
> Sent: Wednesday, December 6, 2017 6:23 PM
> To: Matan Azrad <matan at mellanox.com>
> Cc: dev at dpdk.org
> Subject: Re: [PATCH v2 7/8] net/mlx4: align Tx descriptors number
> 
> On Wed, Dec 06, 2017 at 02:48:12PM +0000, Matan Azrad wrote:
> > Using power of 2 descriptors number makes the ring management easier
> > and allows to use mask operation instead of wraparound conditions.
> >
> > Adjust Tx descriptor number to be power of 2 and change calculation to
> > use mask accordingly.
> >
> > Signed-off-by: Matan Azrad <matan at mellanox.com>
> > ---
> >  drivers/net/mlx4/mlx4_rxtx.c | 28 +++++++++++++---------------
> > drivers/net/mlx4/mlx4_txq.c  | 13 +++++++++----
> >  2 files changed, 22 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/net/mlx4/mlx4_rxtx.c
> > b/drivers/net/mlx4/mlx4_rxtx.c index 8b8d95e..14192fe 100644
> > --- a/drivers/net/mlx4/mlx4_rxtx.c
> > +++ b/drivers/net/mlx4/mlx4_rxtx.c
> > @@ -312,10 +312,14 @@ struct pv {
> >   *
> >   * @param txq
> >   *   Pointer to Tx queue structure.
> > + * @param sq
> > + *   Pointer to the SQ structure.
> > + * @param elts_m
> > + *   Tx elements number mask.
> 
> It's minor however these parameters should be described in the same order
> as they appear in the function prototype, please swap them if you send an
> updated series.
> 
> >   */
> >  static void
> > -mlx4_txq_complete(struct txq *txq, const unsigned int elts_n,
> > -				  struct mlx4_sq *sq)
> > +mlx4_txq_complete(struct txq *txq, const unsigned int elts_m,
> > +		  struct mlx4_sq *sq)
> >  {
> <snip>
> > diff --git a/drivers/net/mlx4/mlx4_txq.c b/drivers/net/mlx4/mlx4_txq.c
> > index 4c7b62a..7eb4b04 100644
> > --- a/drivers/net/mlx4/mlx4_txq.c
> > +++ b/drivers/net/mlx4/mlx4_txq.c
> > @@ -76,17 +76,16 @@
> >  	unsigned int elts_head = txq->elts_head;
> >  	unsigned int elts_tail = txq->elts_tail;
> >  	struct txq_elt (*elts)[txq->elts_n] = txq->elts;
> > +	unsigned int elts_m = txq->elts_n - 1;
> >
> >  	DEBUG("%p: freeing WRs", (void *)txq);
> >  	while (elts_tail != elts_head) {
> > -		struct txq_elt *elt = &(*elts)[elts_tail];
> > +		struct txq_elt *elt = &(*elts)[elts_tail++ & elts_m];
> >
> >  		assert(elt->buf != NULL);
> >  		rte_pktmbuf_free(elt->buf);
> >  		elt->buf = NULL;
> >  		elt->wqe = NULL;
> > -		if (++elts_tail == RTE_DIM(*elts))
> > -			elts_tail = 0;
> >  	}
> >  	txq->elts_tail = txq->elts_head;
> >  }
> > @@ -208,7 +207,7 @@ struct txq_mp2mr_mbuf_check_data {
> >  	struct mlx4dv_obj mlxdv;
> >  	struct mlx4dv_qp dv_qp;
> >  	struct mlx4dv_cq dv_cq;
> > -	struct txq_elt (*elts)[desc];
> > +	struct txq_elt (*elts)[rte_align32pow2(desc)];
> 
> OK, I'm curious about what happened to the magic 0x1000 though? Was it a
> limitation or some leftover debugging code?
> 
Wrong limitation to the max number of descriptors.
Thanks again for the second good review. Will adjust all your comments for v3.
> --
> Adrien Mazarguil
> 6WIND