Re: [dpdk-dev] [PATCH v2 5/5] net/mlx5: add vectorized Rx/Tx burst for SSE4.1

["Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>]

On Tue, Jul 04, 2017 at 05:38:44PM -0700, Yongseok Koh wrote:
> On Tue, Jul 04, 2017 at 10:58:52AM +0200, Nélio Laranjeiro wrote:
> > Yongseok, some comments in this huge and great work,
> > 
> > On Fri, Jun 30, 2017 at 12:23:33PM -0700, Yongseok Koh wrote:
> > > To make vectorized burst routines enabled, it is required to run on x86_64
> > > architecture which can support at least SSE4.1. If all the conditions are
> > > met, the vectorized burst functions are enabled automatically. The decision
> > > is made individually on RX and TX. There's no PMD option to make a
> > > selection.
> > > 
> > > Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> > > ---
> > >  drivers/net/mlx5/Makefile            |   10 +
> > >  drivers/net/mlx5/mlx5_defs.h         |   18 +
> > >  drivers/net/mlx5/mlx5_ethdev.c       |   28 +-
> > >  drivers/net/mlx5/mlx5_rxq.c          |   55 +-
> > >  drivers/net/mlx5/mlx5_rxtx.c         |  339 ++------
> > >  drivers/net/mlx5/mlx5_rxtx.h         |  283 ++++++-
> > >  drivers/net/mlx5/mlx5_rxtx_vec_sse.c | 1451 ++++++++++++++++++++++++++++++++++
> > >  drivers/net/mlx5/mlx5_txq.c          |    2 +-
> > >  8 files changed, 1909 insertions(+), 277 deletions(-)
> > >  create mode 100644 drivers/net/mlx5/mlx5_rxtx_vec_sse.c
> > > 
> > > diff --git a/drivers/net/mlx5/mlx5_rxtx.h b/drivers/net/mlx5/mlx5_rxtx.h
> > > index 51e258a15..2d0894fcd 100644
> > > --- a/drivers/net/mlx5/mlx5_rxtx.h
> > > +++ b/drivers/net/mlx5/mlx5_rxtx.h
> [...]
> > > +	txq_complete(txq);
> > > +	/* A CQE slot must always be available. */
> > > +	assert((1u << txq->cqe_n) - (txq->cq_pi - txq->cq_ci));
> > 
> > This assert should be moved to the txq_complete(), or it should not be
> > an assert.
> txq_complete() is a common function, so this can't force to spare at least one
> slot in a completion queue. This assert is to force to allocate enough CQE slots
> by accurate calculation as completion is suppressed by MLX5_TX_COMP_THRESH. If
> the CQ size is well defined (e.g. size of Tx ring / MLX5_TX_COMP_THRESH), it
> doesn't need to check deficiency of CQ slot but checking slots in Tx ring
> (max_elts) is sufficient. If you are okay with this, please let me know, then
> I'll send out v3.

Just using your comment...
 /* A CQE slot must always be available. */

This is always true in any Tx function where this assumption should be
true to avoid testing it before posting a completion request and thus
avoid cycles waste and this independently of the computation for the CQ
ring size.

As it is an assert it is only present to help the developer in error
code he developed (or for a user to point an issue in the code), this
can be any-where in the Tx data path.  It becomes useful for any Tx
function using this txq_complete().  

If this assert is only related to your code, it may means it should be
an if which avoids to post a completion request when no slots are
available.

> And I agree on all other comments you made. I'll make changes accordingly in v3.

Ok,

> 
> Thanks for your quick review!
> Yongseok

Thanks,

-- 
Nélio Laranjeiro
6WIND