diff mbox

[rdma-core,2/2] mlx5: Disable locking on a QP if it is assigned to a thread domain

Message ID 1524803775-24118-3-git-send-email-rzambre@uci.edu (mailing list archive)
State Changes Requested
Headers show

Commit Message

Rohit Zambre April 27, 2018, 4:36 a.m. UTC
By creating a QP within a thread domain the application is guaranteeing
that the QP will not be accessed concurrently from multiple user threads.
Hence, a lock is not needed for QP that is assigned to a thread domain.

This patch disables locking on the QP if a thread domain is passed during
QP-creation.

Signed-off-by: Rohit Zambre <rzambre@uci.edu>
---
 providers/mlx5/verbs.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Jason Gunthorpe April 30, 2018, 6:38 p.m. UTC | #1
On Fri, Apr 27, 2018 at 04:36:15AM +0000, Rohit Zambre wrote:
> By creating a QP within a thread domain the application is guaranteeing
> that the QP will not be accessed concurrently from multiple user threads.
> Hence, a lock is not needed for QP that is assigned to a thread domain.
> 
> This patch disables locking on the QP if a thread domain is passed during
> QP-creation.
> 
> Signed-off-by: Rohit Zambre <rzambre@uci.edu>
>  providers/mlx5/verbs.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/providers/mlx5/verbs.c b/providers/mlx5/verbs.c
> index 9fd765e..f7132e4 100644
> +++ b/providers/mlx5/verbs.c
> @@ -1607,6 +1607,7 @@ static struct ibv_qp *create_qp(struct ibv_context *context,
>  	struct mlx5_create_qp_ex_resp  resp_ex;
>  	struct mlx5_qp		       *qp;
>  	int				ret;
> +	int				thread_safe;
>  	struct mlx5_context	       *ctx = to_mctx(context);
>  	struct ibv_qp		       *ibqp;
>  	int32_t				usr_idx = 0;
> @@ -1751,8 +1752,14 @@ static struct ibv_qp *create_qp(struct ibv_context *context,
>  
>  	mlx5_init_qp_indices(qp);
>  
> -	if (mlx5_spinlock_init(&qp->sq.lock, !mlx5_single_threaded) ||
> -	    mlx5_spinlock_init(&qp->rq.lock, !mlx5_single_threaded))
> +	mparent_domain = to_mparent_domain(attr->pd);
> +	if (mparent_domain && mparent_domain->mtd)
> +		thread_safe = 1;
> +	else
> +		thread_safe = mlx5_single_threaded;
> +
> +	if (mlx5_spinlock_init(&qp->sq.lock, !thread_safe) ||
> +	    mlx5_spinlock_init(&qp->rq.lock, !thread_safe))
>  		goto err_free_qp_buf;

This would be nicer to have a 'mlx5_spinlock_init_pd' that takes the
object's pd and does the above calcuations.

All the remaining uses should be revised at once to use a pd if it
makes sense during the init..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rohit Zambre May 1, 2018, 9:55 p.m. UTC | #2
On Mon, Apr 30, 2018 at 1:38 PM, Jason Gunthorpe <jgg@mellanox.com> wrote:
>
> On Fri, Apr 27, 2018 at 04:36:15AM +0000, Rohit Zambre wrote:
> > By creating a QP within a thread domain the application is guaranteeing
> > that the QP will not be accessed concurrently from multiple user threads.
> > Hence, a lock is not needed for QP that is assigned to a thread domain.
> >
> > This patch disables locking on the QP if a thread domain is passed during
> > QP-creation.
> >
> > Signed-off-by: Rohit Zambre <rzambre@uci.edu>
> >  providers/mlx5/verbs.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/providers/mlx5/verbs.c b/providers/mlx5/verbs.c
> > index 9fd765e..f7132e4 100644
> > +++ b/providers/mlx5/verbs.c
> > @@ -1607,6 +1607,7 @@ static struct ibv_qp *create_qp(struct ibv_context *context,
> >       struct mlx5_create_qp_ex_resp  resp_ex;
> >       struct mlx5_qp                 *qp;
> >       int                             ret;
> > +     int                             thread_safe;
> >       struct mlx5_context            *ctx = to_mctx(context);
> >       struct ibv_qp                  *ibqp;
> >       int32_t                         usr_idx = 0;
> > @@ -1751,8 +1752,14 @@ static struct ibv_qp *create_qp(struct ibv_context *context,
> >
> >       mlx5_init_qp_indices(qp);
> >
> > -     if (mlx5_spinlock_init(&qp->sq.lock, !mlx5_single_threaded) ||
> > -         mlx5_spinlock_init(&qp->rq.lock, !mlx5_single_threaded))
> > +     mparent_domain = to_mparent_domain(attr->pd);
> > +     if (mparent_domain && mparent_domain->mtd)
> > +             thread_safe = 1;
> > +     else
> > +             thread_safe = mlx5_single_threaded;
> > +
> > +     if (mlx5_spinlock_init(&qp->sq.lock, !thread_safe) ||
> > +         mlx5_spinlock_init(&qp->rq.lock, !thread_safe))
> >               goto err_free_qp_buf;
>
> This would be nicer to have a 'mlx5_spinlock_init_pd' that takes the
> object's pd and does the above calcuations.
>
> All the remaining uses should be revised at once to use a pd if it
> makes sense during the init..

Makes sense. Are you referring to the Parent Domain or the Protection
Domain by PD here?

-Rohit
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe May 1, 2018, 9:56 p.m. UTC | #3
On Tue, May 01, 2018 at 04:55:05PM -0500, Rohit Zambre wrote:
> On Mon, Apr 30, 2018 at 1:38 PM, Jason Gunthorpe <jgg@mellanox.com> wrote:
> >
> > On Fri, Apr 27, 2018 at 04:36:15AM +0000, Rohit Zambre wrote:
> > > By creating a QP within a thread domain the application is guaranteeing
> > > that the QP will not be accessed concurrently from multiple user threads.
> > > Hence, a lock is not needed for QP that is assigned to a thread domain.
> > >
> > > This patch disables locking on the QP if a thread domain is passed during
> > > QP-creation.
> > >
> > > Signed-off-by: Rohit Zambre <rzambre@uci.edu>
> > >  providers/mlx5/verbs.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/providers/mlx5/verbs.c b/providers/mlx5/verbs.c
> > > index 9fd765e..f7132e4 100644
> > > +++ b/providers/mlx5/verbs.c
> > > @@ -1607,6 +1607,7 @@ static struct ibv_qp *create_qp(struct ibv_context *context,
> > >       struct mlx5_create_qp_ex_resp  resp_ex;
> > >       struct mlx5_qp                 *qp;
> > >       int                             ret;
> > > +     int                             thread_safe;
> > >       struct mlx5_context            *ctx = to_mctx(context);
> > >       struct ibv_qp                  *ibqp;
> > >       int32_t                         usr_idx = 0;
> > > @@ -1751,8 +1752,14 @@ static struct ibv_qp *create_qp(struct ibv_context *context,
> > >
> > >       mlx5_init_qp_indices(qp);
> > >
> > > -     if (mlx5_spinlock_init(&qp->sq.lock, !mlx5_single_threaded) ||
> > > -         mlx5_spinlock_init(&qp->rq.lock, !mlx5_single_threaded))
> > > +     mparent_domain = to_mparent_domain(attr->pd);
> > > +     if (mparent_domain && mparent_domain->mtd)
> > > +             thread_safe = 1;
> > > +     else
> > > +             thread_safe = mlx5_single_threaded;
> > > +
> > > +     if (mlx5_spinlock_init(&qp->sq.lock, !thread_safe) ||
> > > +         mlx5_spinlock_init(&qp->rq.lock, !thread_safe))
> > >               goto err_free_qp_buf;
> >
> > This would be nicer to have a 'mlx5_spinlock_init_pd' that takes the
> > object's pd and does the above calcuations.
> >
> > All the remaining uses should be revised at once to use a pd if it
> > makes sense during the init..
> 
> Makes sense. Are you referring to the Parent Domain or the Protection
> Domain by PD here?

If I recall, at this layer they are still interchangable. The
'mlx5_spinlock_init_pd' should determine if the ibv_pd is a parent
domain, then determine if it specifies single threaded operation.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/providers/mlx5/verbs.c b/providers/mlx5/verbs.c
index 9fd765e..f7132e4 100644
--- a/providers/mlx5/verbs.c
+++ b/providers/mlx5/verbs.c
@@ -1607,6 +1607,7 @@  static struct ibv_qp *create_qp(struct ibv_context *context,
 	struct mlx5_create_qp_ex_resp  resp_ex;
 	struct mlx5_qp		       *qp;
 	int				ret;
+	int				thread_safe;
 	struct mlx5_context	       *ctx = to_mctx(context);
 	struct ibv_qp		       *ibqp;
 	int32_t				usr_idx = 0;
@@ -1751,8 +1752,14 @@  static struct ibv_qp *create_qp(struct ibv_context *context,
 
 	mlx5_init_qp_indices(qp);
 
-	if (mlx5_spinlock_init(&qp->sq.lock, !mlx5_single_threaded) ||
-	    mlx5_spinlock_init(&qp->rq.lock, !mlx5_single_threaded))
+	mparent_domain = to_mparent_domain(attr->pd);
+	if (mparent_domain && mparent_domain->mtd)
+		thread_safe = 1;
+	else
+		thread_safe = mlx5_single_threaded;
+
+	if (mlx5_spinlock_init(&qp->sq.lock, !thread_safe) ||
+	    mlx5_spinlock_init(&qp->rq.lock, !thread_safe))
 		goto err_free_qp_buf;
 
 	qp->db = mlx5_alloc_dbrec(ctx);
@@ -1789,7 +1796,6 @@  static struct ibv_qp *create_qp(struct ibv_context *context,
 		cmd.uidx = usr_idx;
 	}
 
-	mparent_domain = to_mparent_domain(attr->pd);
 	if (mparent_domain && mparent_domain->mtd)
 		bf = mparent_domain->mtd->bf;