diff mbox series

[4/9] efa: Move the context intialization out of efa_query_device_ex()

Message ID 4-v1-34e141ddf17e+89-query_device_ex_jgg@nvidia.com (mailing list archive)
State Not Applicable
Headers show
Series Simplify query_device() in libibverbs | expand

Commit Message

Jason Gunthorpe Nov. 16, 2020, 8:23 p.m. UTC 
When the user calls efa_query_device_ex() it should not cause the context
values to be mutated, only the attribute shuld be returned.

Move this code to a dedicated function that is only called during context
setup.

Cc: Gal Pressman <galpress@amazon.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 providers/efa/efa.c   | 14 +------------
 providers/efa/verbs.c | 46 +++++++++++++++++++++++++++++++++++--------
 providers/efa/verbs.h |  1 +
 3 files changed, 40 insertions(+), 21 deletions(-)

Comments

Gal Pressman Nov. 18, 2020, 7:39 a.m. UTC | #1 
On 16/11/2020 22:23, Jason Gunthorpe wrote:
> When the user calls efa_query_device_ex() it should not cause the context
> values to be mutated, only the attribute shuld be returned.
> 
> Move this code to a dedicated function that is only called during context
> setup.
> 
> Cc: Gal Pressman <galpress@amazon.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Didn't get a chance to review yet, but this one breaks EFA.
I'll try to provide more info today/tomorrow.

(BTW: typo in the subject line "intialization", and "shuld" in the commit message)
Gal Pressman Nov. 18, 2020, 8:07 a.m. UTC | #2 
On 16/11/2020 22:23, Jason Gunthorpe wrote:
> When the user calls efa_query_device_ex() it should not cause the context
> values to be mutated, only the attribute shuld be returned.
> 
> Move this code to a dedicated function that is only called during context
> setup.
> 
> Cc: Gal Pressman <galpress@amazon.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  providers/efa/efa.c   | 14 +------------
>  providers/efa/verbs.c | 46 +++++++++++++++++++++++++++++++++++--------
>  providers/efa/verbs.h |  1 +
>  3 files changed, 40 insertions(+), 21 deletions(-)
> 
> diff --git a/providers/efa/efa.c b/providers/efa/efa.c
> index 35f9b246a711ec..b24c14f7fa1fe1 100644
> --- a/providers/efa/efa.c
> +++ b/providers/efa/efa.c
> @@ -54,10 +54,7 @@ static struct verbs_context *efa_alloc_context(struct ibv_device *vdev,
>  {
>  	struct efa_alloc_ucontext_resp resp = {};
>  	struct efa_alloc_ucontext cmd = {};
> -	struct ibv_device_attr_ex attr;
> -	unsigned int qp_table_sz;
>  	struct efa_context *ctx;
> -	int err;
>  
>  	cmd.comp_mask |= EFA_ALLOC_UCONTEXT_CMD_COMP_TX_BATCH;
>  	cmd.comp_mask |= EFA_ALLOC_UCONTEXT_CMD_COMP_MIN_SQ_WR;
> @@ -86,17 +83,8 @@ static struct verbs_context *efa_alloc_context(struct ibv_device *vdev,
>  
>  	verbs_set_ops(&ctx->ibvctx, &efa_ctx_ops);
>  
> -	err = efa_query_device_ex(&ctx->ibvctx.context, NULL, &attr,
> -				  sizeof(attr));
> -	if (err)
> +	if (!efa_query_device_ctx(ctx))
>  		goto err_free_spinlock;

This return error on success seems to be the issue, will verify.
Gal Pressman Nov. 18, 2020, 12:45 p.m. UTC | #3 
On 16/11/2020 22:23, Jason Gunthorpe wrote:
> When the user calls efa_query_device_ex() it should not cause the context
> values to be mutated, only the attribute shuld be returned.
> 
> Move this code to a dedicated function that is only called during context
> setup.
> 
> Cc: Gal Pressman <galpress@amazon.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  providers/efa/efa.c   | 14 +------------
>  providers/efa/verbs.c | 46 +++++++++++++++++++++++++++++++++++--------
>  providers/efa/verbs.h |  1 +
>  3 files changed, 40 insertions(+), 21 deletions(-)
> 
> diff --git a/providers/efa/efa.c b/providers/efa/efa.c
> index 35f9b246a711ec..b24c14f7fa1fe1 100644
> --- a/providers/efa/efa.c
> +++ b/providers/efa/efa.c
> @@ -54,10 +54,7 @@ static struct verbs_context *efa_alloc_context(struct ibv_device *vdev,
>  {
>  	struct efa_alloc_ucontext_resp resp = {};
>  	struct efa_alloc_ucontext cmd = {};
> -	struct ibv_device_attr_ex attr;
> -	unsigned int qp_table_sz;
>  	struct efa_context *ctx;
> -	int err;
>  
>  	cmd.comp_mask |= EFA_ALLOC_UCONTEXT_CMD_COMP_TX_BATCH;
>  	cmd.comp_mask |= EFA_ALLOC_UCONTEXT_CMD_COMP_MIN_SQ_WR;
> @@ -86,17 +83,8 @@ static struct verbs_context *efa_alloc_context(struct ibv_device *vdev,
>  
>  	verbs_set_ops(&ctx->ibvctx, &efa_ctx_ops);
>  
> -	err = efa_query_device_ex(&ctx->ibvctx.context, NULL, &attr,
> -				  sizeof(attr));
> -	if (err)
> +	if (!efa_query_device_ctx(ctx))

Remove the not.

>  		goto err_free_spinlock;
> -
> -	qp_table_sz = roundup_pow_of_two(attr.orig_attr.max_qp);
> -	ctx->qp_table_sz_m1 = qp_table_sz - 1;
> -	ctx->qp_table = calloc(qp_table_sz, sizeof(*ctx->qp_table));
> -	if (!ctx->qp_table)
> -		goto err_free_spinlock;
> -
>  	return &ctx->ibvctx;
>  
>  err_free_spinlock:
> diff --git a/providers/efa/verbs.c b/providers/efa/verbs.c
> index 1a9633155c62f8..52d6285f1f409c 100644
> --- a/providers/efa/verbs.c
> +++ b/providers/efa/verbs.c
> @@ -106,14 +106,6 @@ int efa_query_device_ex(struct ibv_context *context,
>  	if (err)
>  		return err;
>  
> -	ctx->device_caps = resp.device_caps;
> -	ctx->max_sq_wr = resp.max_sq_wr;
> -	ctx->max_rq_wr = resp.max_rq_wr;
> -	ctx->max_sq_sge = resp.max_sq_sge;
> -	ctx->max_rq_sge = resp.max_rq_sge;
> -	ctx->max_rdma_size = resp.max_rdma_size;
> -	ctx->max_wr_rdma_sge = a->max_sge_rd;
> -
>  	a->max_qp_wr = min_t(int, a->max_qp_wr,
>  			     ctx->max_llq_size / sizeof(struct efa_io_tx_wqe));
>  	snprintf(a->fw_ver, sizeof(a->fw_ver), "%u.%u.%u.%u",
> @@ -122,6 +114,44 @@ int efa_query_device_ex(struct ibv_context *context,
>  	return 0;
>  }
>  
> +int efa_query_device_ctx(struct efa_context *ctx)
> +{
> +	struct ibv_device_attr_ex attr;
> +	struct efa_query_device_ex_resp resp;

Preferably I would put this first.

> +	size_t resp_size = sizeof(resp);
> +	unsigned int qp_table_sz;
> +	int err;
> +
> +	if (ctx->cmds_supp_udata_mask & EFA_USER_CMDS_SUPP_UDATA_QUERY_DEVICE) {
> +		err = ibv_cmd_query_device_any(&ctx->ibvctx.context, NULL,
> +					       &attr, sizeof(attr),
> +					       &resp.ibv_resp, &resp_size);
> +		if (err)
> +			return err;
> +
> +		ctx->device_caps = resp.device_caps;
> +		ctx->max_sq_wr = resp.max_sq_wr;
> +		ctx->max_rq_wr = resp.max_rq_wr;
> +		ctx->max_sq_sge = resp.max_sq_sge;
> +		ctx->max_rq_sge = resp.max_rq_sge;
> +		ctx->max_rdma_size = resp.max_rdma_size;
> +		ctx->max_wr_rdma_sge = attr.orig_attr.max_sge_rd;

max_wr_rdma_sge assignment can be done in the else clause as well.

> +	} else {
> +		err = ibv_cmd_query_device_any(&ctx->ibvctx.context, NULL,
> +					       &attr, sizeof(attr.orig_attr),
> +					       NULL, NULL);
> +		if (err)
> +			return err;
> +	}
> +
> +	qp_table_sz = roundup_pow_of_two(attr.orig_attr.max_qp);
> +	ctx->qp_table_sz_m1 = qp_table_sz - 1;
> +	ctx->qp_table = calloc(qp_table_sz, sizeof(*ctx->qp_table));
> +	if (!ctx->qp_table)
> +		return ENOMEM;
> +	return 0;
> +}
Jason Gunthorpe Nov. 18, 2020, 8:59 p.m. UTC | #4 
On Wed, Nov 18, 2020 at 02:45:42PM +0200, Gal Pressman wrote:

> > +	size_t resp_size = sizeof(resp);
> > +	unsigned int qp_table_sz;
> > +	int err;
> > +
> > +	if (ctx->cmds_supp_udata_mask & EFA_USER_CMDS_SUPP_UDATA_QUERY_DEVICE) {
> > +		err = ibv_cmd_query_device_any(&ctx->ibvctx.context, NULL,
> > +					       &attr, sizeof(attr),
> > +					       &resp.ibv_resp, &resp_size);
> > +		if (err)
> > +			return err;
> > +
> > +		ctx->device_caps = resp.device_caps;
> > +		ctx->max_sq_wr = resp.max_sq_wr;
> > +		ctx->max_rq_wr = resp.max_rq_wr;
> > +		ctx->max_sq_sge = resp.max_sq_sge;
> > +		ctx->max_rq_sge = resp.max_rq_sge;
> > +		ctx->max_rdma_size = resp.max_rdma_size;
> > +		ctx->max_wr_rdma_sge = attr.orig_attr.max_sge_rd;
> 
> max_wr_rdma_sge assignment can be done in the else clause as well.

Yes, it is the same mistake as I did in mlx5

I updated everything, thanks

Jason
diff mbox series

Patch

diff --git a/providers/efa/efa.c b/providers/efa/efa.c
index 35f9b246a711ec..b24c14f7fa1fe1 100644
--- a/providers/efa/efa.c
+++ b/providers/efa/efa.c
@@ -54,10 +54,7 @@  static struct verbs_context *efa_alloc_context(struct ibv_device *vdev,
 {
 	struct efa_alloc_ucontext_resp resp = {};
 	struct efa_alloc_ucontext cmd = {};
-	struct ibv_device_attr_ex attr;
-	unsigned int qp_table_sz;
 	struct efa_context *ctx;
-	int err;
 
 	cmd.comp_mask |= EFA_ALLOC_UCONTEXT_CMD_COMP_TX_BATCH;
 	cmd.comp_mask |= EFA_ALLOC_UCONTEXT_CMD_COMP_MIN_SQ_WR;
@@ -86,17 +83,8 @@  static struct verbs_context *efa_alloc_context(struct ibv_device *vdev,
 
 	verbs_set_ops(&ctx->ibvctx, &efa_ctx_ops);
 
-	err = efa_query_device_ex(&ctx->ibvctx.context, NULL, &attr,
-				  sizeof(attr));
-	if (err)
+	if (!efa_query_device_ctx(ctx))
 		goto err_free_spinlock;
-
-	qp_table_sz = roundup_pow_of_two(attr.orig_attr.max_qp);
-	ctx->qp_table_sz_m1 = qp_table_sz - 1;
-	ctx->qp_table = calloc(qp_table_sz, sizeof(*ctx->qp_table));
-	if (!ctx->qp_table)
-		goto err_free_spinlock;
-
 	return &ctx->ibvctx;
 
 err_free_spinlock:
diff --git a/providers/efa/verbs.c b/providers/efa/verbs.c
index 1a9633155c62f8..52d6285f1f409c 100644
--- a/providers/efa/verbs.c
+++ b/providers/efa/verbs.c
@@ -106,14 +106,6 @@  int efa_query_device_ex(struct ibv_context *context,
 	if (err)
 		return err;
 
-	ctx->device_caps = resp.device_caps;
-	ctx->max_sq_wr = resp.max_sq_wr;
-	ctx->max_rq_wr = resp.max_rq_wr;
-	ctx->max_sq_sge = resp.max_sq_sge;
-	ctx->max_rq_sge = resp.max_rq_sge;
-	ctx->max_rdma_size = resp.max_rdma_size;
-	ctx->max_wr_rdma_sge = a->max_sge_rd;
-
 	a->max_qp_wr = min_t(int, a->max_qp_wr,
 			     ctx->max_llq_size / sizeof(struct efa_io_tx_wqe));
 	snprintf(a->fw_ver, sizeof(a->fw_ver), "%u.%u.%u.%u",
@@ -122,6 +114,44 @@  int efa_query_device_ex(struct ibv_context *context,
 	return 0;
 }
 
+int efa_query_device_ctx(struct efa_context *ctx)
+{
+	struct ibv_device_attr_ex attr;
+	struct efa_query_device_ex_resp resp;
+	size_t resp_size = sizeof(resp);
+	unsigned int qp_table_sz;
+	int err;
+
+	if (ctx->cmds_supp_udata_mask & EFA_USER_CMDS_SUPP_UDATA_QUERY_DEVICE) {
+		err = ibv_cmd_query_device_any(&ctx->ibvctx.context, NULL,
+					       &attr, sizeof(attr),
+					       &resp.ibv_resp, &resp_size);
+		if (err)
+			return err;
+
+		ctx->device_caps = resp.device_caps;
+		ctx->max_sq_wr = resp.max_sq_wr;
+		ctx->max_rq_wr = resp.max_rq_wr;
+		ctx->max_sq_sge = resp.max_sq_sge;
+		ctx->max_rq_sge = resp.max_rq_sge;
+		ctx->max_rdma_size = resp.max_rdma_size;
+		ctx->max_wr_rdma_sge = attr.orig_attr.max_sge_rd;
+	} else {
+		err = ibv_cmd_query_device_any(&ctx->ibvctx.context, NULL,
+					       &attr, sizeof(attr.orig_attr),
+					       NULL, NULL);
+		if (err)
+			return err;
+	}
+
+	qp_table_sz = roundup_pow_of_two(attr.orig_attr.max_qp);
+	ctx->qp_table_sz_m1 = qp_table_sz - 1;
+	ctx->qp_table = calloc(qp_table_sz, sizeof(*ctx->qp_table));
+	if (!ctx->qp_table)
+		return ENOMEM;
+	return 0;
+}
+
 int efadv_query_device(struct ibv_context *ibvctx,
 		       struct efadv_device_attr *attr,
 		       uint32_t inlen)
diff --git a/providers/efa/verbs.h b/providers/efa/verbs.h
index da022e615af064..3b0e4e0d498761 100644
--- a/providers/efa/verbs.h
+++ b/providers/efa/verbs.h
@@ -9,6 +9,7 @@ 
 #include <infiniband/driver.h>
 #include <infiniband/verbs.h>
 
+int efa_query_device_ctx(struct efa_context *ctx);
 int efa_query_device(struct ibv_context *uctx, struct ibv_device_attr *attr);
 int efa_query_port(struct ibv_context *uctx, uint8_t port,
 		   struct ibv_port_attr *attr);