diff mbox

[dpdk-dev] net/mlx5: remove not needed query if and rd from create qp

Message ID 20170330080645.30973-1-shahafs@mellanox.com (mailing list archive)
State Superseded, archived
Headers

Checks

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

Commit Message

Shahaf Shuler March 30, 2017, 8:06 a.m. UTC 
  Since mlx5 PMD data path is on top of PRM, such verbs calls are
no longer needed.

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 drivers/net/mlx5/mlx5_rxq.c  | 79 +---------------------------------------
 drivers/net/mlx5/mlx5_rxtx.h |  6 ---
 drivers/net/mlx5/mlx5_txq.c  | 87 +-------------------------------------------
 3 files changed, 3 insertions(+), 169 deletions(-)

Comments

Yongseok Koh March 30, 2017, 5:03 p.m. UTC | #1 
Hi,

> On Mar 30, 2017, at 1:06 AM, Shahaf Shuler <shahafs@mellanox.com> wrote:
> 
> Since mlx5 PMD data path is on top of PRM, such verbs calls are
> no longer needed.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>

Acked-by: Yongseok Koh <yskoh@mellanox.com>

I ack the code change but one small suggestion regarding the commit header.
It looks like 'if' means interface, 'rd' means the Resource Domain, and
'qp' means the Queue Pair. which are all mellanox specific terms and
acronyms. My suggestion is:

net/mlx5: remove unnecessary Verbs library calls

And how about moving the details into the message body:

Remove unnecessary interface queries and the Resource Domain when creating
the Queue Pair. Since mlx5 PMD data path is on top of native APIs, such
Verbs library calls are no longer needed.

Thanks,
Yongseok
Ferruh Yigit March 30, 2017, 5:08 p.m. UTC | #2 
On 3/30/2017 6:03 PM, Yongseok Koh wrote:
> 
> Hi,
> 
>> On Mar 30, 2017, at 1:06 AM, Shahaf Shuler <shahafs@mellanox.com> wrote:
>>
>> Since mlx5 PMD data path is on top of PRM, such verbs calls are
>> no longer needed.
>>
>> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
>> Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> 
> Acked-by: Yongseok Koh <yskoh@mellanox.com>
> 
> I ack the code change but one small suggestion regarding the commit header.
> It looks like 'if' means interface, 'rd' means the Resource Domain, and
> 'qp' means the Queue Pair. which are all mellanox specific terms and
> acronyms. My suggestion is:
> 
> net/mlx5: remove unnecessary Verbs library calls

Thanks for the suggestion, I was thinking about asking for clarification
on patch subject :)

> 
> And how about moving the details into the message body:
> 
> Remove unnecessary interface queries and the Resource Domain when creating
> the Queue Pair. Since mlx5 PMD data path is on top of native APIs, such
> Verbs library calls are no longer needed.
> 
> Thanks,
> Yongseok
>
Ferruh Yigit March 31, 2017, 9:23 a.m. UTC | #3 
On 3/30/2017 6:03 PM, Yongseok Koh wrote:
> 
> Hi,
> 
>> On Mar 30, 2017, at 1:06 AM, Shahaf Shuler <shahafs@mellanox.com> wrote:
>>
>> Since mlx5 PMD data path is on top of PRM, such verbs calls are
>> no longer needed.
>>
>> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
>> Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> 
> Acked-by: Yongseok Koh <yskoh@mellanox.com>
> 
> I ack the code change but one small suggestion regarding the commit header.
> It looks like 'if' means interface, 'rd' means the Resource Domain, and
> 'qp' means the Queue Pair. which are all mellanox specific terms and
> acronyms. My suggestion is:
> 
> net/mlx5: remove unnecessary Verbs library calls
> 
> And how about moving the details into the message body:
> 
> Remove unnecessary interface queries and the Resource Domain when creating
> the Queue Pair. Since mlx5 PMD data path is on top of native APIs, such
> Verbs library calls are no longer needed.

Can you please send a new version of the patch with suggested updates?
(Keeping the Ack)

Thanks,
ferruh

> 
> Thanks,
> Yongseok
>
diff mbox

Patch

diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index d5825fdd4..556648292 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -743,51 +743,16 @@  rxq_free_elts(struct rxq_ctrl *rxq_ctrl)
 void
 rxq_cleanup(struct rxq_ctrl *rxq_ctrl)
 {
-	struct ibv_exp_release_intf_params params;
-
 	DEBUG("cleaning up %p", (void *)rxq_ctrl);
 	rxq_free_elts(rxq_ctrl);
 	if (rxq_ctrl->fdir_queue != NULL)
 		priv_fdir_queue_destroy(rxq_ctrl->priv, rxq_ctrl->fdir_queue);
-	if (rxq_ctrl->if_wq != NULL) {
-		assert(rxq_ctrl->priv != NULL);
-		assert(rxq_ctrl->priv->ctx != NULL);
-		assert(rxq_ctrl->wq != NULL);
-		params = (struct ibv_exp_release_intf_params){
-			.comp_mask = 0,
-		};
-		claim_zero(ibv_exp_release_intf(rxq_ctrl->priv->ctx,
-						rxq_ctrl->if_wq,
-						&params));
-	}
-	if (rxq_ctrl->if_cq != NULL) {
-		assert(rxq_ctrl->priv != NULL);
-		assert(rxq_ctrl->priv->ctx != NULL);
-		assert(rxq_ctrl->cq != NULL);
-		params = (struct ibv_exp_release_intf_params){
-			.comp_mask = 0,
-		};
-		claim_zero(ibv_exp_release_intf(rxq_ctrl->priv->ctx,
-						rxq_ctrl->if_cq,
-						&params));
-	}
 	if (rxq_ctrl->wq != NULL)
 		claim_zero(ibv_exp_destroy_wq(rxq_ctrl->wq));
 	if (rxq_ctrl->cq != NULL)
 		claim_zero(ibv_destroy_cq(rxq_ctrl->cq));
 	if (rxq_ctrl->channel != NULL)
 		claim_zero(ibv_destroy_comp_channel(rxq_ctrl->channel));
-	if (rxq_ctrl->rd != NULL) {
-		struct ibv_exp_destroy_res_domain_attr attr = {
-			.comp_mask = 0,
-		};
-
-		assert(rxq_ctrl->priv != NULL);
-		assert(rxq_ctrl->priv->ctx != NULL);
-		claim_zero(ibv_exp_destroy_res_domain(rxq_ctrl->priv->ctx,
-						      rxq_ctrl->rd,
-						      &attr));
-	}
 	if (rxq_ctrl->mr != NULL)
 		claim_zero(ibv_dereg_mr(rxq_ctrl->mr));
 	memset(rxq_ctrl, 0, sizeof(*rxq_ctrl));
@@ -935,13 +900,10 @@  rxq_ctrl_setup(struct rte_eth_dev *dev, struct rxq_ctrl *rxq_ctrl,
 	};
 	struct ibv_exp_wq_attr mod;
 	union {
-		struct ibv_exp_query_intf_params params;
 		struct ibv_exp_cq_init_attr cq;
-		struct ibv_exp_res_domain_init_attr rd;
 		struct ibv_exp_wq_init_attr wq;
 		struct ibv_exp_cq_attr cq_attr;
 	} attr;
-	enum ibv_exp_query_intf_status status;
 	unsigned int mb_len = rte_pktmbuf_data_room_size(mp);
 	unsigned int cqe_n = desc - 1;
 	struct rte_mbuf *(*elts)[desc] = NULL;
@@ -1008,19 +970,6 @@  rxq_ctrl_setup(struct rte_eth_dev *dev, struct rxq_ctrl *rxq_ctrl,
 		      (void *)dev, strerror(ret));
 		goto error;
 	}
-	attr.rd = (struct ibv_exp_res_domain_init_attr){
-		.comp_mask = (IBV_EXP_RES_DOMAIN_THREAD_MODEL |
-			      IBV_EXP_RES_DOMAIN_MSG_MODEL),
-		.thread_model = IBV_EXP_THREAD_SINGLE,
-		.msg_model = IBV_EXP_MSG_HIGH_BW,
-	};
-	tmpl.rd = ibv_exp_create_res_domain(priv->ctx, &attr.rd);
-	if (tmpl.rd == NULL) {
-		ret = ENOMEM;
-		ERROR("%p: RD creation failure: %s",
-		      (void *)dev, strerror(ret));
-		goto error;
-	}
 	if (dev->data->dev_conf.intr_conf.rxq) {
 		tmpl.channel = ibv_create_comp_channel(priv->ctx);
 		if (tmpl.channel == NULL) {
@@ -1032,8 +981,7 @@  rxq_ctrl_setup(struct rte_eth_dev *dev, struct rxq_ctrl *rxq_ctrl,
 		}
 	}
 	attr.cq = (struct ibv_exp_cq_init_attr){
-		.comp_mask = IBV_EXP_CQ_INIT_ATTR_RES_DOMAIN,
-		.res_domain = tmpl.rd,
+		.comp_mask = 0,
 	};
 	if (priv->cqe_comp) {
 		attr.cq.comp_mask |= IBV_EXP_CQ_INIT_ATTR_FLAGS;
@@ -1065,10 +1013,8 @@  rxq_ctrl_setup(struct rte_eth_dev *dev, struct rxq_ctrl *rxq_ctrl,
 		.pd = priv->pd,
 		.cq = tmpl.cq,
 		.comp_mask =
-			IBV_EXP_CREATE_WQ_RES_DOMAIN |
 			IBV_EXP_CREATE_WQ_VLAN_OFFLOADS |
 			0,
-		.res_domain = tmpl.rd,
 		.vlan_offloads = (tmpl.rxq.vlan_strip ?
 				  IBV_EXP_RECEIVE_WQ_CVLAN_STRIP :
 				  0),
@@ -1129,29 +1075,6 @@  rxq_ctrl_setup(struct rte_eth_dev *dev, struct rxq_ctrl *rxq_ctrl,
 	/* Save port ID. */
 	tmpl.rxq.port_id = dev->data->port_id;
 	DEBUG("%p: RTE port ID: %u", (void *)rxq_ctrl, tmpl.rxq.port_id);
-	attr.params = (struct ibv_exp_query_intf_params){
-		.intf_scope = IBV_EXP_INTF_GLOBAL,
-		.intf_version = 1,
-		.intf = IBV_EXP_INTF_CQ,
-		.obj = tmpl.cq,
-	};
-	tmpl.if_cq = ibv_exp_query_intf(priv->ctx, &attr.params, &status);
-	if (tmpl.if_cq == NULL) {
-		ERROR("%p: CQ interface family query failed with status %d",
-		      (void *)dev, status);
-		goto error;
-	}
-	attr.params = (struct ibv_exp_query_intf_params){
-		.intf_scope = IBV_EXP_INTF_GLOBAL,
-		.intf = IBV_EXP_INTF_WQ,
-		.obj = tmpl.wq,
-	};
-	tmpl.if_wq = ibv_exp_query_intf(priv->ctx, &attr.params, &status);
-	if (tmpl.if_wq == NULL) {
-		ERROR("%p: WQ interface family query failed with status %d",
-		      (void *)dev, status);
-		goto error;
-	}
 	/* Change queue state to ready. */
 	mod = (struct ibv_exp_wq_attr){
 		.attr_mask = IBV_EXP_WQ_ATTR_STATE,
diff --git a/drivers/net/mlx5/mlx5_rxtx.h b/drivers/net/mlx5/mlx5_rxtx.h
index 4a4bd8402..100c149aa 100644
--- a/drivers/net/mlx5/mlx5_rxtx.h
+++ b/drivers/net/mlx5/mlx5_rxtx.h
@@ -133,11 +133,8 @@  struct rxq_ctrl {
 	struct priv *priv; /* Back pointer to private data. */
 	struct ibv_cq *cq; /* Completion Queue. */
 	struct ibv_exp_wq *wq; /* Work Queue. */
-	struct ibv_exp_res_domain *rd; /* Resource Domain. */
 	struct fdir_queue *fdir_queue; /* Flow director queue. */
 	struct ibv_mr *mr; /* Memory Region (for mp). */
-	struct ibv_exp_wq_family *if_wq; /* WQ burst interface. */
-	struct ibv_exp_cq_family_v1 *if_cq; /* CQ interface. */
 	struct ibv_comp_channel *channel;
 	unsigned int socket; /* CPU socket ID for allocations. */
 	struct rxq rxq; /* Data path structure. */
@@ -283,9 +280,6 @@  struct txq_ctrl {
 	struct priv *priv; /* Back pointer to private data. */
 	struct ibv_cq *cq; /* Completion Queue. */
 	struct ibv_qp *qp; /* Queue Pair. */
-	struct ibv_exp_qp_burst_family *if_qp; /* QP burst interface. */
-	struct ibv_exp_cq_family *if_cq; /* CQ interface. */
-	struct ibv_exp_res_domain *rd; /* Resource Domain. */
 	unsigned int socket; /* CPU socket ID for allocations. */
 	struct txq txq; /* Data path structure. */
 };
diff --git a/drivers/net/mlx5/mlx5_txq.c b/drivers/net/mlx5/mlx5_txq.c
index e9b837d10..f80740a13 100644
--- a/drivers/net/mlx5/mlx5_txq.c
+++ b/drivers/net/mlx5/mlx5_txq.c
@@ -140,48 +140,14 @@  txq_free_elts(struct txq_ctrl *txq_ctrl)
 void
 txq_cleanup(struct txq_ctrl *txq_ctrl)
 {
-	struct ibv_exp_release_intf_params params;
 	size_t i;
 
 	DEBUG("cleaning up %p", (void *)txq_ctrl);
 	txq_free_elts(txq_ctrl);
-	if (txq_ctrl->if_qp != NULL) {
-		assert(txq_ctrl->priv != NULL);
-		assert(txq_ctrl->priv->ctx != NULL);
-		assert(txq_ctrl->qp != NULL);
-		params = (struct ibv_exp_release_intf_params){
-			.comp_mask = 0,
-		};
-		claim_zero(ibv_exp_release_intf(txq_ctrl->priv->ctx,
-						txq_ctrl->if_qp,
-						&params));
-	}
-	if (txq_ctrl->if_cq != NULL) {
-		assert(txq_ctrl->priv != NULL);
-		assert(txq_ctrl->priv->ctx != NULL);
-		assert(txq_ctrl->cq != NULL);
-		params = (struct ibv_exp_release_intf_params){
-			.comp_mask = 0,
-		};
-		claim_zero(ibv_exp_release_intf(txq_ctrl->priv->ctx,
-						txq_ctrl->if_cq,
-						&params));
-	}
 	if (txq_ctrl->qp != NULL)
 		claim_zero(ibv_destroy_qp(txq_ctrl->qp));
 	if (txq_ctrl->cq != NULL)
 		claim_zero(ibv_destroy_cq(txq_ctrl->cq));
-	if (txq_ctrl->rd != NULL) {
-		struct ibv_exp_destroy_res_domain_attr attr = {
-			.comp_mask = 0,
-		};
-
-		assert(txq_ctrl->priv != NULL);
-		assert(txq_ctrl->priv->ctx != NULL);
-		claim_zero(ibv_exp_destroy_res_domain(txq_ctrl->priv->ctx,
-						      txq_ctrl->rd,
-						      &attr));
-	}
 	for (i = 0; (i != RTE_DIM(txq_ctrl->txq.mp2mr)); ++i) {
 		if (txq_ctrl->txq.mp2mr[i].mp == NULL)
 			break;
@@ -258,14 +224,11 @@  txq_ctrl_setup(struct rte_eth_dev *dev, struct txq_ctrl *txq_ctrl,
 		.socket = socket,
 	};
 	union {
-		struct ibv_exp_query_intf_params params;
 		struct ibv_exp_qp_init_attr init;
-		struct ibv_exp_res_domain_init_attr rd;
 		struct ibv_exp_cq_init_attr cq;
 		struct ibv_exp_qp_attr mod;
 		struct ibv_exp_cq_attr cq_attr;
 	} attr;
-	enum ibv_exp_query_intf_status status;
 	unsigned int cqe_n;
 	int ret = 0;
 
@@ -280,22 +243,8 @@  txq_ctrl_setup(struct rte_eth_dev *dev, struct txq_ctrl *txq_ctrl,
 	if (priv->mps == MLX5_MPW_ENHANCED)
 		tmpl.txq.mpw_hdr_dseg = priv->mpw_hdr_dseg;
 	/* MRs will be registered in mp2mr[] later. */
-	attr.rd = (struct ibv_exp_res_domain_init_attr){
-		.comp_mask = (IBV_EXP_RES_DOMAIN_THREAD_MODEL |
-			      IBV_EXP_RES_DOMAIN_MSG_MODEL),
-		.thread_model = IBV_EXP_THREAD_SINGLE,
-		.msg_model = IBV_EXP_MSG_HIGH_BW,
-	};
-	tmpl.rd = ibv_exp_create_res_domain(priv->ctx, &attr.rd);
-	if (tmpl.rd == NULL) {
-		ret = ENOMEM;
-		ERROR("%p: RD creation failure: %s",
-		      (void *)dev, strerror(ret));
-		goto error;
-	}
 	attr.cq = (struct ibv_exp_cq_init_attr){
-		.comp_mask = IBV_EXP_CQ_INIT_ATTR_RES_DOMAIN,
-		.res_domain = tmpl.rd,
+		.comp_mask = 0,
 	};
 	cqe_n = ((desc / MLX5_TX_COMP_THRESH) - 1) ?
 		((desc / MLX5_TX_COMP_THRESH) - 1) : 1;
@@ -338,9 +287,7 @@  txq_ctrl_setup(struct rte_eth_dev *dev, struct txq_ctrl *txq_ctrl,
 		 * TX burst. */
 		.sq_sig_all = 0,
 		.pd = priv->pd,
-		.res_domain = tmpl.rd,
-		.comp_mask = (IBV_EXP_QP_INIT_ATTR_PD |
-			      IBV_EXP_QP_INIT_ATTR_RES_DOMAIN),
+		.comp_mask = IBV_EXP_QP_INIT_ATTR_PD,
 	};
 	if (priv->txq_inline && (priv->txqs_n >= priv->txqs_inline)) {
 		tmpl.txq.max_inline =
@@ -427,36 +374,6 @@  txq_ctrl_setup(struct rte_eth_dev *dev, struct txq_ctrl *txq_ctrl,
 		      (void *)dev, strerror(ret));
 		goto error;
 	}
-	attr.params = (struct ibv_exp_query_intf_params){
-		.intf_scope = IBV_EXP_INTF_GLOBAL,
-		.intf = IBV_EXP_INTF_CQ,
-		.obj = tmpl.cq,
-	};
-	tmpl.if_cq = ibv_exp_query_intf(priv->ctx, &attr.params, &status);
-	if (tmpl.if_cq == NULL) {
-		ret = EINVAL;
-		ERROR("%p: CQ interface family query failed with status %d",
-		      (void *)dev, status);
-		goto error;
-	}
-	attr.params = (struct ibv_exp_query_intf_params){
-		.intf_scope = IBV_EXP_INTF_GLOBAL,
-		.intf = IBV_EXP_INTF_QP_BURST,
-		.intf_version = 1,
-		.obj = tmpl.qp,
-		/* Enable multi-packet send if supported. */
-		.family_flags =
-			(priv->mps ?
-			 IBV_EXP_QP_BURST_CREATE_ENABLE_MULTI_PACKET_SEND_WR :
-			 0),
-	};
-	tmpl.if_qp = ibv_exp_query_intf(priv->ctx, &attr.params, &status);
-	if (tmpl.if_qp == NULL) {
-		ret = EINVAL;
-		ERROR("%p: QP interface family query failed with status %d",
-		      (void *)dev, status);
-		goto error;
-	}
 	/* Clean up txq in case we're reinitializing it. */
 	DEBUG("%p: cleaning-up old txq just in case", (void *)txq_ctrl);
 	txq_cleanup(txq_ctrl);