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