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

On Wed, Jul 05, 2017 at 10:21:26AM +0200, Nélio Laranjeiro wrote:

On Tue, Jul 04, 2017 at 05:38:44PM -0700, Yongseok Koh wrote:

quoted

On Tue, Jul 04, 2017 at 10:58:52AM +0200, Nélio Laranjeiro wrote:

quoted

Yongseok, some comments in this huge and great work,

On Fri, Jun 30, 2017 at 12:23:33PM -0700, Yongseok Koh wrote:

quoted

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

[...]

quoted

quoted

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

I think I was wrong about the assert. If the Tx ring is full (max_elts == 0),
then there won't be any empty slot left in CQ. I'll remove the two asserts.

Thanks
Yongseok