diff mbox

[dpdk-dev] net/mlx5: implement drop action in hardware classifier

Message ID 1495954160-983-2-git-send-email-shacharbe@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Shachar Beiser May 28, 2017, 6:49 a.m. UTC 
  The current drop action is implemented as a queue tail drop,
requiring to instantiate multiple WQs to maintain high drop rate.
This commit, implements the drop action in hardware classifier.
This enables to reduce the amount of contexts needed for the drop,
without affecting the drop rate.

Signed-off-by: Shachar Beiser <shacharbe@mellanox.com>
---
 drivers/net/mlx5/Makefile    |  5 +++++
 drivers/net/mlx5/mlx5_flow.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

Comments

Nélio Laranjeiro May 29, 2017, 1:07 p.m. UTC | #1 
On Sun, May 28, 2017 at 06:49:20AM +0000, Shachar Beiser wrote:
> The current drop action is implemented as a queue tail drop,
> requiring to instantiate multiple WQs to maintain high drop rate.
> This commit, implements the drop action in hardware classifier.
> This enables to reduce the amount of contexts needed for the drop,
> without affecting the drop rate.
> 
> Signed-off-by: Shachar Beiser <shacharbe@mellanox.com>
> ---
>  drivers/net/mlx5/Makefile    |  5 +++++
>  drivers/net/mlx5/mlx5_flow.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/net/mlx5/Makefile b/drivers/net/mlx5/Makefile
> index c079959..daf8013 100644
> --- a/drivers/net/mlx5/Makefile
> +++ b/drivers/net/mlx5/Makefile
> @@ -101,6 +101,11 @@ mlx5_autoconf.h.new: FORCE
>  mlx5_autoconf.h.new: $(RTE_SDK)/buildtools/auto-config-h.sh
>  	$Q $(RM) -f -- '$@'
>  	$Q sh -- '$<' '$@' \
> +		HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP \
> +		infiniband/verbs_exp.h \
> +		enum IBV_EXP_FLOW_SPEC_ACTION_DROP \
> +		$(AUTOCONF_OUTPUT)
> +	$Q sh -- '$<' '$@' \
>  		HAVE_VERBS_IBV_EXP_CQ_COMPRESSED_CQE \
>  		infiniband/verbs_exp.h \
>  		enum IBV_EXP_CQ_COMPRESSED_CQE \
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index adcbe3f..e243d39 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -994,6 +994,11 @@ struct mlx5_flow_action {
>  {
>  	struct rte_flow *rte_flow;
>  
> +#ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
> +	struct ibv_exp_flow_spec_action_drop *drop;
> +	unsigned int size = sizeof(struct ibv_exp_flow_spec_action_drop);
> +#endif

Extra empty line before the #ifdef

> +
>  	assert(priv->pd);
>  	assert(priv->ctx);
>  	rte_flow = rte_calloc(__func__, 1, sizeof(*rte_flow), 0);
> @@ -1007,6 +1012,15 @@ struct mlx5_flow_action {
>  	rte_flow->qp = priv->flow_drop_queue->qp;
>  	if (!priv->started)
>  		return rte_flow;
> +#ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
> +	drop = (void *)((uintptr_t)flow->ibv_attr + flow->offset);
> +	*drop = (struct ibv_exp_flow_spec_action_drop){
> +			.type = IBV_EXP_FLOW_SPEC_ACTION_DROP,
> +			.size = size,
> +	};
> +	++flow->ibv_attr->num_of_specs;
> +	flow->offset += sizeof(struct ibv_exp_flow_spec_action_drop);
> +#endif
>  	rte_flow->ibv_flow = ibv_exp_create_flow(rte_flow->qp,
>  						 rte_flow->ibv_attr);
>  	if (!rte_flow->ibv_flow) {
>[...]

From what I see by just changing the value of MLX5_DROP_WQ_N when
HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP (in the same source file)
limits the patch to this point.

Am I missing something?

Regards,
Shachar Beiser May 29, 2017, 1:35 p.m. UTC | #2 
The current firmware version does not implement queue tail drop therefore queue allocation is not required by firmware. Unfortunately, the ibverbs APIs requires to allocate WQ & CQ  .In the future this allocation will be saved . Right now, you are right. 

-----Original Message-----
From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com] 
Sent: Monday, May 29, 2017 4:08 PM
To: Shachar Beiser <shacharbe@mellanox.com>
Cc: dev@dpdk.org; Adrien Mazarguil <adrien.mazarguil@6wind.com>
Subject: Re: [PATCH] net/mlx5: implement drop action in hardware classifier

On Sun, May 28, 2017 at 06:49:20AM +0000, Shachar Beiser wrote:
> The current drop action is implemented as a queue tail drop,
> requiring to instantiate multiple WQs to maintain high drop rate.
> This commit, implements the drop action in hardware classifier.
> This enables to reduce the amount of contexts needed for the drop,
> without affecting the drop rate.
> 
> Signed-off-by: Shachar Beiser <shacharbe@mellanox.com>
> ---
>  drivers/net/mlx5/Makefile    |  5 +++++
>  drivers/net/mlx5/mlx5_flow.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/net/mlx5/Makefile b/drivers/net/mlx5/Makefile
> index c079959..daf8013 100644
> --- a/drivers/net/mlx5/Makefile
> +++ b/drivers/net/mlx5/Makefile
> @@ -101,6 +101,11 @@ mlx5_autoconf.h.new: FORCE
>  mlx5_autoconf.h.new: $(RTE_SDK)/buildtools/auto-config-h.sh
>  	$Q $(RM) -f -- '$@'
>  	$Q sh -- '$<' '$@' \
> +		HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP \
> +		infiniband/verbs_exp.h \
> +		enum IBV_EXP_FLOW_SPEC_ACTION_DROP \
> +		$(AUTOCONF_OUTPUT)
> +	$Q sh -- '$<' '$@' \
>  		HAVE_VERBS_IBV_EXP_CQ_COMPRESSED_CQE \
>  		infiniband/verbs_exp.h \
>  		enum IBV_EXP_CQ_COMPRESSED_CQE \
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index adcbe3f..e243d39 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -994,6 +994,11 @@ struct mlx5_flow_action {
>  {
>  	struct rte_flow *rte_flow;
>  
> +#ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
> +	struct ibv_exp_flow_spec_action_drop *drop;
> +	unsigned int size = sizeof(struct ibv_exp_flow_spec_action_drop);
> +#endif

Extra empty line before the #ifdef

> +
>  	assert(priv->pd);
>  	assert(priv->ctx);
>  	rte_flow = rte_calloc(__func__, 1, sizeof(*rte_flow), 0);
> @@ -1007,6 +1012,15 @@ struct mlx5_flow_action {
>  	rte_flow->qp = priv->flow_drop_queue->qp;
>  	if (!priv->started)
>  		return rte_flow;
> +#ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
> +	drop = (void *)((uintptr_t)flow->ibv_attr + flow->offset);
> +	*drop = (struct ibv_exp_flow_spec_action_drop){
> +			.type = IBV_EXP_FLOW_SPEC_ACTION_DROP,
> +			.size = size,
> +	};
> +	++flow->ibv_attr->num_of_specs;
> +	flow->offset += sizeof(struct ibv_exp_flow_spec_action_drop);
> +#endif
>  	rte_flow->ibv_flow = ibv_exp_create_flow(rte_flow->qp,
>  						 rte_flow->ibv_attr);
>  	if (!rte_flow->ibv_flow) {
>[...]

From what I see by just changing the value of MLX5_DROP_WQ_N when
HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP (in the same source file)
limits the patch to this point.

Am I missing something?

Regards,
Nélio Laranjeiro May 29, 2017, 2:05 p.m. UTC | #3 
>> From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com] 
>> Sent: Monday, May 29, 2017 4:08 PM
>> To: Shachar Beiser <shacharbe@mellanox.com>
>> Cc: dev@dpdk.org; Adrien Mazarguil <adrien.mazarguil@6wind.com>
>> Subject: Re: [PATCH] net/mlx5: implement drop action in hardware classifier
>> 
>> On Sun, May 28, 2017 at 06:49:20AM +0000, Shachar Beiser wrote:
>> > The current drop action is implemented as a queue tail drop,
>> > requiring to instantiate multiple WQs to maintain high drop rate.
>> > This commit, implements the drop action in hardware classifier.
>> > This enables to reduce the amount of contexts needed for the drop,
>> > without affecting the drop rate.
>> > 
>> > Signed-off-by: Shachar Beiser <shacharbe@mellanox.com>
>> > ---
>> >  drivers/net/mlx5/Makefile    |  5 +++++
>> >  drivers/net/mlx5/mlx5_flow.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 48 insertions(+)
>> > 
>> > diff --git a/drivers/net/mlx5/Makefile b/drivers/net/mlx5/Makefile
>> > index c079959..daf8013 100644
>> > --- a/drivers/net/mlx5/Makefile
>> > +++ b/drivers/net/mlx5/Makefile
>> > @@ -101,6 +101,11 @@ mlx5_autoconf.h.new: FORCE
>> >  mlx5_autoconf.h.new: $(RTE_SDK)/buildtools/auto-config-h.sh
>> >  	$Q $(RM) -f -- '$@'
>> >  	$Q sh -- '$<' '$@' \
>> > +		HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP \
>> > +		infiniband/verbs_exp.h \
>> > +		enum IBV_EXP_FLOW_SPEC_ACTION_DROP \
>> > +		$(AUTOCONF_OUTPUT)
>> > +	$Q sh -- '$<' '$@' \
>> >  		HAVE_VERBS_IBV_EXP_CQ_COMPRESSED_CQE \
>> >  		infiniband/verbs_exp.h \
>> >  		enum IBV_EXP_CQ_COMPRESSED_CQE \
>> > diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
>> > index adcbe3f..e243d39 100644
>> > --- a/drivers/net/mlx5/mlx5_flow.c
>> > +++ b/drivers/net/mlx5/mlx5_flow.c
>> > @@ -994,6 +994,11 @@ struct mlx5_flow_action {
>> >  {
>> >  	struct rte_flow *rte_flow;
>> >  
>> > +#ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
>> > +	struct ibv_exp_flow_spec_action_drop *drop;
>> > +	unsigned int size = sizeof(struct ibv_exp_flow_spec_action_drop);
>> > +#endif
>> 
>> Extra empty line before the #ifdef
>> 
>> > +
>> >  	assert(priv->pd);
>> >  	assert(priv->ctx);
>> >  	rte_flow = rte_calloc(__func__, 1, sizeof(*rte_flow), 0);
>> > @@ -1007,6 +1012,15 @@ struct mlx5_flow_action {
>> >  	rte_flow->qp = priv->flow_drop_queue->qp;
>> >  	if (!priv->started)
>> >  		return rte_flow;
>> > +#ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
>> > +	drop = (void *)((uintptr_t)flow->ibv_attr + flow->offset);
>> > +	*drop = (struct ibv_exp_flow_spec_action_drop){
>> > +			.type = IBV_EXP_FLOW_SPEC_ACTION_DROP,
>> > +			.size = size,
>> > +	};
>> > +	++flow->ibv_attr->num_of_specs;
>> > +	flow->offset += sizeof(struct ibv_exp_flow_spec_action_drop);
>> > +#endif
>> >  	rte_flow->ibv_flow = ibv_exp_create_flow(rte_flow->qp,
>> >  						 rte_flow->ibv_attr);
>> >  	if (!rte_flow->ibv_flow) {
>> >[...]
>> 
>> From what I see by just changing the value of MLX5_DROP_WQ_N when
>> HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP (in the same source file)
>> limits the patch to this point.
>> 
>> Am I missing something?
>> 
>> Regards,
> On Mon, May 29, 2017 at 01:35:57PM +0000, Shachar Beiser wrote:
> The current firmware version does not implement queue tail drop
> therefore queue allocation is not required by firmware. Unfortunately,
> the ibverbs APIs requires to allocate WQ & CQ  .In the future this
> allocation will be saved . Right now, you are right. 

What I mean is, in your patch, what you need is the snippet I let above,
and at compilation time you can update the MLX5_DROP_WQ_N to 1 (defined
at 4 in the same file), this way the remaining code will only create a
single CQ/WQ as you expect.

This small snippet above plus 3 lines handles the job.

Regards,

PS: Avoid top answering on this mailing list.
diff mbox

Patch

diff --git a/drivers/net/mlx5/Makefile b/drivers/net/mlx5/Makefile
index c079959..daf8013 100644
--- a/drivers/net/mlx5/Makefile
+++ b/drivers/net/mlx5/Makefile
@@ -101,6 +101,11 @@  mlx5_autoconf.h.new: FORCE
 mlx5_autoconf.h.new: $(RTE_SDK)/buildtools/auto-config-h.sh
 	$Q $(RM) -f -- '$@'
 	$Q sh -- '$<' '$@' \
+		HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP \
+		infiniband/verbs_exp.h \
+		enum IBV_EXP_FLOW_SPEC_ACTION_DROP \
+		$(AUTOCONF_OUTPUT)
+	$Q sh -- '$<' '$@' \
 		HAVE_VERBS_IBV_EXP_CQ_COMPRESSED_CQE \
 		infiniband/verbs_exp.h \
 		enum IBV_EXP_CQ_COMPRESSED_CQE \
diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index adcbe3f..e243d39 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -994,6 +994,11 @@  struct mlx5_flow_action {
 {
 	struct rte_flow *rte_flow;
 
+#ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
+	struct ibv_exp_flow_spec_action_drop *drop;
+	unsigned int size = sizeof(struct ibv_exp_flow_spec_action_drop);
+#endif
+
 	assert(priv->pd);
 	assert(priv->ctx);
 	rte_flow = rte_calloc(__func__, 1, sizeof(*rte_flow), 0);
@@ -1007,6 +1012,15 @@  struct mlx5_flow_action {
 	rte_flow->qp = priv->flow_drop_queue->qp;
 	if (!priv->started)
 		return rte_flow;
+#ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
+	drop = (void *)((uintptr_t)flow->ibv_attr + flow->offset);
+	*drop = (struct ibv_exp_flow_spec_action_drop){
+			.type = IBV_EXP_FLOW_SPEC_ACTION_DROP,
+			.size = size,
+	};
+	++flow->ibv_attr->num_of_specs;
+	flow->offset += sizeof(struct ibv_exp_flow_spec_action_drop);
+#endif
 	rte_flow->ibv_flow = ibv_exp_create_flow(rte_flow->qp,
 						 rte_flow->ibv_attr);
 	if (!rte_flow->ibv_flow) {
@@ -1370,7 +1384,9 @@  struct rte_flow *
 priv_flow_create_drop_queue(struct priv *priv)
 {
 	struct rte_flow_drop *fdq = NULL;
+#ifndef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
 	unsigned int i;
+#endif
 
 	assert(priv->pd);
 	assert(priv->ctx);
@@ -1387,6 +1403,7 @@  struct rte_flow *
 		WARN("cannot allocate CQ for drop queue");
 		goto error;
 	}
+#ifndef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
 	for (i = 0; i != MLX5_DROP_WQ_N; ++i) {
 		fdq->wqs[i] = ibv_exp_create_wq(priv->ctx,
 				&(struct ibv_exp_wq_init_attr){
@@ -1401,6 +1418,20 @@  struct rte_flow *
 			goto error;
 		}
 	}
+#else
+	fdq->wqs[0] = ibv_exp_create_wq(priv->ctx,
+			&(struct ibv_exp_wq_init_attr){
+			.wq_type = IBV_EXP_WQT_RQ,
+			.max_recv_wr = 1,
+			.max_recv_sge = 1,
+			.pd = priv->pd,
+			.cq = fdq->cq,
+			});
+	if (!fdq->wqs[0]) {
+		WARN("cannot allocate WQ for drop queue");
+		goto error;
+	}
+#endif
 	fdq->ind_table = ibv_exp_create_rwq_ind_table(priv->ctx,
 			&(struct ibv_exp_rwq_ind_table_init_attr){
 			.pd = priv->pd,
@@ -1441,10 +1472,15 @@  struct rte_flow *
 		claim_zero(ibv_destroy_qp(fdq->qp));
 	if (fdq->ind_table)
 		claim_zero(ibv_exp_destroy_rwq_ind_table(fdq->ind_table));
+#ifndef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
 	for (i = 0; i != MLX5_DROP_WQ_N; ++i) {
 		if (fdq->wqs[i])
 			claim_zero(ibv_exp_destroy_wq(fdq->wqs[i]));
 	}
+#else
+	if (fdq->wqs[0])
+		claim_zero(ibv_exp_destroy_wq(fdq->wqs[0]));
+#endif
 	if (fdq->cq)
 		claim_zero(ibv_destroy_cq(fdq->cq));
 	if (fdq)
@@ -1463,7 +1499,9 @@  struct rte_flow *
 priv_flow_delete_drop_queue(struct priv *priv)
 {
 	struct rte_flow_drop *fdq = priv->flow_drop_queue;
+#ifndef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
 	unsigned int i;
+#endif
 
 	if (!fdq)
 		return;
@@ -1471,10 +1509,15 @@  struct rte_flow *
 		claim_zero(ibv_destroy_qp(fdq->qp));
 	if (fdq->ind_table)
 		claim_zero(ibv_exp_destroy_rwq_ind_table(fdq->ind_table));
+#ifndef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
 	for (i = 0; i != MLX5_DROP_WQ_N; ++i) {
 		if (fdq->wqs[i])
 			claim_zero(ibv_exp_destroy_wq(fdq->wqs[i]));
 	}
+#else
+	if (fdq->wqs[0])
+		claim_zero(ibv_exp_destroy_wq(fdq->wqs[0]));
+#endif
 	if (fdq->cq)
 		claim_zero(ibv_destroy_cq(fdq->cq));
 	rte_free(fdq);