linux-rdma.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH rdma-next 0/2] Convert XRC to use xarray
@ 2020-06-21 10:41 Leon Romanovsky
  2020-06-21 10:41 ` [PATCH rdma-next 1/2] RDMA: Clean ib_alloc_xrcd() and reuse it to allocate XRC domain Leon Romanovsky
  2020-06-21 10:41 ` [PATCH rdma-next 2/2] RDMA/core: Optimize XRC target lookup Leon Romanovsky
  0 siblings, 2 replies; 8+ messages in thread
From: Leon Romanovsky @ 2020-06-21 10:41 UTC (permalink / raw)
  To: Doug Ledford, Jason Gunthorpe
  Cc: Leon Romanovsky, linux-kernel, linux-rdma, Maor Gottlieb

From: Leon Romanovsky <leonro@mellanox.com>

Two small patches to simplify and improve XRC logic.

Maor Gottlieb (2):
  RDMA: Clean ib_alloc_xrcd() and reuse it to allocate XRC domain
  RDMA/core: Optimize XRC target lookup

 drivers/infiniband/core/uverbs_cmd.c | 12 ++---
 drivers/infiniband/core/verbs.c      | 69 ++++++++++++++--------------
 drivers/infiniband/hw/mlx5/main.c    | 24 ++++------
 include/rdma/ib_verbs.h              | 27 +++++------
 4 files changed, 60 insertions(+), 72 deletions(-)

--
2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH rdma-next 1/2] RDMA: Clean ib_alloc_xrcd() and reuse it to allocate XRC domain
  2020-06-21 10:41 [PATCH rdma-next 0/2] Convert XRC to use xarray Leon Romanovsky
@ 2020-06-21 10:41 ` Leon Romanovsky
  2020-06-21 10:41 ` [PATCH rdma-next 2/2] RDMA/core: Optimize XRC target lookup Leon Romanovsky
  1 sibling, 0 replies; 8+ messages in thread
From: Leon Romanovsky @ 2020-06-21 10:41 UTC (permalink / raw)
  To: Doug Ledford, Jason Gunthorpe; +Cc: Maor Gottlieb, linux-rdma

From: Maor Gottlieb <maorg@mellanox.com>

ib_alloc_xrcd already does the required initialization, so move
the mlx5 driver and uverbs to call it and save some code duplication,
while cleaning the function argument lists of that function.

Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/uverbs_cmd.c | 12 +++---------
 drivers/infiniband/core/verbs.c      | 19 +++++++++++++------
 drivers/infiniband/hw/mlx5/main.c    | 24 ++++++++----------------
 include/rdma/ib_verbs.h              | 22 ++++++++++++----------
 4 files changed, 36 insertions(+), 41 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 557644dcc923..68c9a0210220 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -614,17 +614,11 @@ static int ib_uverbs_open_xrcd(struct uverbs_attr_bundle *attrs)
 	}
 
 	if (!xrcd) {
-		xrcd = ib_dev->ops.alloc_xrcd(ib_dev, &attrs->driver_udata);
+		xrcd = ib_alloc_xrcd_user(ib_dev, inode, &attrs->driver_udata);
 		if (IS_ERR(xrcd)) {
 			ret = PTR_ERR(xrcd);
 			goto err;
 		}
-
-		xrcd->inode   = inode;
-		xrcd->device  = ib_dev;
-		atomic_set(&xrcd->usecnt, 0);
-		mutex_init(&xrcd->tgt_qp_mutex);
-		INIT_LIST_HEAD(&xrcd->tgt_qp_list);
 		new_xrcd = 1;
 	}
 
@@ -663,7 +657,7 @@ static int ib_uverbs_open_xrcd(struct uverbs_attr_bundle *attrs)
 	}
 
 err_dealloc_xrcd:
-	ib_dealloc_xrcd(xrcd, uverbs_get_cleared_udata(attrs));
+	ib_dealloc_xrcd_user(xrcd, uverbs_get_cleared_udata(attrs));
 
 err:
 	uobj_alloc_abort(&obj->uobject, attrs);
@@ -701,7 +695,7 @@ int ib_uverbs_dealloc_xrcd(struct ib_uobject *uobject, struct ib_xrcd *xrcd,
 	if (inode && !atomic_dec_and_test(&xrcd->usecnt))
 		return 0;
 
-	ret = ib_dealloc_xrcd(xrcd, &attrs->driver_udata);
+	ret = ib_dealloc_xrcd_user(xrcd, &attrs->driver_udata);
 
 	if (ib_is_destroy_retryable(ret, why, uobject)) {
 		atomic_inc(&xrcd->usecnt);
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index d70771caf534..d66a0ad62077 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -2289,17 +2289,24 @@ int ib_detach_mcast(struct ib_qp *qp, union ib_gid *gid, u16 lid)
 }
 EXPORT_SYMBOL(ib_detach_mcast);
 
-struct ib_xrcd *__ib_alloc_xrcd(struct ib_device *device, const char *caller)
+/**
+ * ib_alloc_xrcd_user - Allocates an XRC domain.
+ * @device: The device on which to allocate the XRC domain.
+ * @inode: inode to connect XRCD
+ * @udata: Valid user data or NULL for kernel object
+ */
+struct ib_xrcd *ib_alloc_xrcd_user(struct ib_device *device,
+				   struct inode *inode, struct ib_udata *udata)
 {
 	struct ib_xrcd *xrcd;
 
 	if (!device->ops.alloc_xrcd)
 		return ERR_PTR(-EOPNOTSUPP);
 
-	xrcd = device->ops.alloc_xrcd(device, NULL);
+	xrcd = device->ops.alloc_xrcd(device, udata);
 	if (!IS_ERR(xrcd)) {
 		xrcd->device = device;
-		xrcd->inode = NULL;
+		xrcd->inode = inode;
 		atomic_set(&xrcd->usecnt, 0);
 		mutex_init(&xrcd->tgt_qp_mutex);
 		INIT_LIST_HEAD(&xrcd->tgt_qp_list);
@@ -2307,9 +2314,9 @@ struct ib_xrcd *__ib_alloc_xrcd(struct ib_device *device, const char *caller)
 
 	return xrcd;
 }
-EXPORT_SYMBOL(__ib_alloc_xrcd);
+EXPORT_SYMBOL(ib_alloc_xrcd_user);
 
-int ib_dealloc_xrcd(struct ib_xrcd *xrcd, struct ib_udata *udata)
+int ib_dealloc_xrcd_user(struct ib_xrcd *xrcd, struct ib_udata *udata)
 {
 	struct ib_qp *qp;
 	int ret;
@@ -2327,7 +2334,7 @@ int ib_dealloc_xrcd(struct ib_xrcd *xrcd, struct ib_udata *udata)
 
 	return xrcd->device->ops.dealloc_xrcd(xrcd, udata);
 }
-EXPORT_SYMBOL(ib_dealloc_xrcd);
+EXPORT_SYMBOL(ib_dealloc_xrcd_user);
 
 /**
  * ib_create_wq - Creates a WQ associated with the specified protection
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 47a0c091eea5..46c596a855e7 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -5043,27 +5043,17 @@ static int create_dev_resources(struct mlx5_ib_resources *devr)
 	if (ret)
 		goto err_create_cq;
 
-	devr->x0 = mlx5_ib_alloc_xrcd(&dev->ib_dev, NULL);
+	devr->x0 = ib_alloc_xrcd(&dev->ib_dev);
 	if (IS_ERR(devr->x0)) {
 		ret = PTR_ERR(devr->x0);
 		goto error2;
 	}
-	devr->x0->device = &dev->ib_dev;
-	devr->x0->inode = NULL;
-	atomic_set(&devr->x0->usecnt, 0);
-	mutex_init(&devr->x0->tgt_qp_mutex);
-	INIT_LIST_HEAD(&devr->x0->tgt_qp_list);
 
-	devr->x1 = mlx5_ib_alloc_xrcd(&dev->ib_dev, NULL);
+	devr->x1 = ib_alloc_xrcd(&dev->ib_dev);
 	if (IS_ERR(devr->x1)) {
 		ret = PTR_ERR(devr->x1);
 		goto error3;
 	}
-	devr->x1->device = &dev->ib_dev;
-	devr->x1->inode = NULL;
-	atomic_set(&devr->x1->usecnt, 0);
-	mutex_init(&devr->x1->tgt_qp_mutex);
-	INIT_LIST_HEAD(&devr->x1->tgt_qp_list);
 
 	memset(&attr, 0, sizeof(attr));
 	attr.attr.max_sge = 1;
@@ -5125,13 +5115,14 @@ static int create_dev_resources(struct mlx5_ib_resources *devr)
 error6:
 	kfree(devr->s1);
 error5:
+	atomic_dec(&devr->s0->ext.xrc.xrcd->usecnt);
 	mlx5_ib_destroy_srq(devr->s0, NULL);
 err_create:
 	kfree(devr->s0);
 error4:
-	mlx5_ib_dealloc_xrcd(devr->x1, NULL);
+	ib_dealloc_xrcd(devr->x1);
 error3:
-	mlx5_ib_dealloc_xrcd(devr->x0, NULL);
+	ib_dealloc_xrcd(devr->x0);
 error2:
 	mlx5_ib_destroy_cq(devr->c0, NULL);
 err_create_cq:
@@ -5149,10 +5140,11 @@ static void destroy_dev_resources(struct mlx5_ib_resources *devr)
 
 	mlx5_ib_destroy_srq(devr->s1, NULL);
 	kfree(devr->s1);
+	atomic_dec(&devr->s0->ext.xrc.xrcd->usecnt);
 	mlx5_ib_destroy_srq(devr->s0, NULL);
 	kfree(devr->s0);
-	mlx5_ib_dealloc_xrcd(devr->x0, NULL);
-	mlx5_ib_dealloc_xrcd(devr->x1, NULL);
+	ib_dealloc_xrcd(devr->x0);
+	ib_dealloc_xrcd(devr->x1);
 	mlx5_ib_destroy_cq(devr->c0, NULL);
 	kfree(devr->c0);
 	mlx5_ib_dealloc_pd(devr->p0, NULL);
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index f1e8afe1dd75..f785a4f1e58b 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -4331,21 +4331,23 @@ int ib_attach_mcast(struct ib_qp *qp, union ib_gid *gid, u16 lid);
  */
 int ib_detach_mcast(struct ib_qp *qp, union ib_gid *gid, u16 lid);
 
-/**
- * ib_alloc_xrcd - Allocates an XRC domain.
- * @device: The device on which to allocate the XRC domain.
- * @caller: Module name for kernel consumers
- */
-struct ib_xrcd *__ib_alloc_xrcd(struct ib_device *device, const char *caller);
-#define ib_alloc_xrcd(device) \
-	__ib_alloc_xrcd((device), KBUILD_MODNAME)
+struct ib_xrcd *ib_alloc_xrcd_user(struct ib_device *device,
+				   struct inode *inode, struct ib_udata *udata);
+static inline struct ib_xrcd *ib_alloc_xrcd(struct ib_device *device)
+{
+	return ib_alloc_xrcd_user(device, NULL, NULL);
+}
 
 /**
- * ib_dealloc_xrcd - Deallocates an XRC domain.
+ * ib_dealloc_xrcd_user - Deallocates an XRC domain.
  * @xrcd: The XRC domain to deallocate.
  * @udata: Valid user data or NULL for kernel object
  */
-int ib_dealloc_xrcd(struct ib_xrcd *xrcd, struct ib_udata *udata);
+int ib_dealloc_xrcd_user(struct ib_xrcd *xrcd, struct ib_udata *udata);
+static inline int ib_dealloc_xrcd(struct ib_xrcd *xrcd)
+{
+	return ib_dealloc_xrcd_user(xrcd, NULL);
+}
 
 static inline int ib_check_mr_access(int flags)
 {
-- 
2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH rdma-next 2/2] RDMA/core: Optimize XRC target lookup
  2020-06-21 10:41 [PATCH rdma-next 0/2] Convert XRC to use xarray Leon Romanovsky
  2020-06-21 10:41 ` [PATCH rdma-next 1/2] RDMA: Clean ib_alloc_xrcd() and reuse it to allocate XRC domain Leon Romanovsky
@ 2020-06-21 10:41 ` Leon Romanovsky
       [not found]   ` <CAD+HZHUnW53ni=16=XL6hY1AHoNtsa88_V5P+XOHb55Fm83zZQ@mail.gmail.com>
  2020-06-22 12:29   ` Jason Gunthorpe
  1 sibling, 2 replies; 8+ messages in thread
From: Leon Romanovsky @ 2020-06-21 10:41 UTC (permalink / raw)
  To: Doug Ledford, Jason Gunthorpe; +Cc: Maor Gottlieb, linux-rdma

From: Maor Gottlieb <maorg@mellanox.com>

Replace the mutex with read write semaphore and use xarray instead
of linked list for XRC target QPs. This will give faster XRC target
lookup. In addition, when QP is closed, don't insert it back to the
xarray if the destroy command failed.

Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/verbs.c | 50 +++++++++++++++------------------
 include/rdma/ib_verbs.h         |  5 ++--
 2 files changed, 24 insertions(+), 31 deletions(-)

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index d66a0ad62077..ef980124f7e6 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -1090,13 +1090,6 @@ static void __ib_shared_qp_event_handler(struct ib_event *event, void *context)
 	spin_unlock_irqrestore(&qp->device->qp_open_list_lock, flags);
 }
 
-static void __ib_insert_xrcd_qp(struct ib_xrcd *xrcd, struct ib_qp *qp)
-{
-	mutex_lock(&xrcd->tgt_qp_mutex);
-	list_add(&qp->xrcd_list, &xrcd->tgt_qp_list);
-	mutex_unlock(&xrcd->tgt_qp_mutex);
-}
-
 static struct ib_qp *__ib_open_qp(struct ib_qp *real_qp,
 				  void (*event_handler)(struct ib_event *, void *),
 				  void *qp_context)
@@ -1139,16 +1132,15 @@ struct ib_qp *ib_open_qp(struct ib_xrcd *xrcd,
 	if (qp_open_attr->qp_type != IB_QPT_XRC_TGT)
 		return ERR_PTR(-EINVAL);
 
-	qp = ERR_PTR(-EINVAL);
-	mutex_lock(&xrcd->tgt_qp_mutex);
-	list_for_each_entry(real_qp, &xrcd->tgt_qp_list, xrcd_list) {
-		if (real_qp->qp_num == qp_open_attr->qp_num) {
-			qp = __ib_open_qp(real_qp, qp_open_attr->event_handler,
-					  qp_open_attr->qp_context);
-			break;
-		}
+	down_read(&xrcd->tgt_qps_rwsem);
+	real_qp = xa_load(&xrcd->tgt_qps, qp_open_attr->qp_num);
+	if (!real_qp) {
+		up_read(&xrcd->tgt_qps_rwsem);
+		return ERR_PTR(-EINVAL);
 	}
-	mutex_unlock(&xrcd->tgt_qp_mutex);
+	qp = __ib_open_qp(real_qp, qp_open_attr->event_handler,
+			  qp_open_attr->qp_context);
+	up_read(&xrcd->tgt_qps_rwsem);
 	return qp;
 }
 EXPORT_SYMBOL(ib_open_qp);
@@ -1157,6 +1149,7 @@ static struct ib_qp *create_xrc_qp_user(struct ib_qp *qp,
 					struct ib_qp_init_attr *qp_init_attr)
 {
 	struct ib_qp *real_qp = qp;
+	int err;
 
 	qp->event_handler = __ib_shared_qp_event_handler;
 	qp->qp_context = qp;
@@ -1172,7 +1165,12 @@ static struct ib_qp *create_xrc_qp_user(struct ib_qp *qp,
 	if (IS_ERR(qp))
 		return qp;
 
-	__ib_insert_xrcd_qp(qp_init_attr->xrcd, real_qp);
+	err = xa_err(xa_store(&qp_init_attr->xrcd->tgt_qps, real_qp->qp_num,
+			      real_qp, GFP_KERNEL));
+	if (err) {
+		ib_close_qp(qp);
+		return ERR_PTR(err);
+	}
 	return qp;
 }
 
@@ -1888,21 +1886,18 @@ static int __ib_destroy_shared_qp(struct ib_qp *qp)
 
 	real_qp = qp->real_qp;
 	xrcd = real_qp->xrcd;
-
-	mutex_lock(&xrcd->tgt_qp_mutex);
+	down_write(&xrcd->tgt_qps_rwsem);
 	ib_close_qp(qp);
 	if (atomic_read(&real_qp->usecnt) == 0)
-		list_del(&real_qp->xrcd_list);
+		xa_erase(&xrcd->tgt_qps, real_qp->qp_num);
 	else
 		real_qp = NULL;
-	mutex_unlock(&xrcd->tgt_qp_mutex);
+	up_write(&xrcd->tgt_qps_rwsem);
 
 	if (real_qp) {
 		ret = ib_destroy_qp(real_qp);
 		if (!ret)
 			atomic_dec(&xrcd->usecnt);
-		else
-			__ib_insert_xrcd_qp(xrcd, real_qp);
 	}
 
 	return 0;
@@ -2308,8 +2303,8 @@ struct ib_xrcd *ib_alloc_xrcd_user(struct ib_device *device,
 		xrcd->device = device;
 		xrcd->inode = inode;
 		atomic_set(&xrcd->usecnt, 0);
-		mutex_init(&xrcd->tgt_qp_mutex);
-		INIT_LIST_HEAD(&xrcd->tgt_qp_list);
+		init_rwsem(&xrcd->tgt_qps_rwsem);
+		xa_init(&xrcd->tgt_qps);
 	}
 
 	return xrcd;
@@ -2318,19 +2313,18 @@ EXPORT_SYMBOL(ib_alloc_xrcd_user);
 
 int ib_dealloc_xrcd_user(struct ib_xrcd *xrcd, struct ib_udata *udata)
 {
+	unsigned long index;
 	struct ib_qp *qp;
 	int ret;
 
 	if (atomic_read(&xrcd->usecnt))
 		return -EBUSY;
 
-	while (!list_empty(&xrcd->tgt_qp_list)) {
-		qp = list_entry(xrcd->tgt_qp_list.next, struct ib_qp, xrcd_list);
+	xa_for_each(&xrcd->tgt_qps, index, qp) {
 		ret = ib_destroy_qp(qp);
 		if (ret)
 			return ret;
 	}
-	mutex_destroy(&xrcd->tgt_qp_mutex);
 
 	return xrcd->device->ops.dealloc_xrcd(xrcd, udata);
 }
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index f785a4f1e58b..9b973b3b6f4c 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1568,9 +1568,8 @@ struct ib_xrcd {
 	struct ib_device       *device;
 	atomic_t		usecnt; /* count all exposed resources */
 	struct inode	       *inode;
-
-	struct mutex		tgt_qp_mutex;
-	struct list_head	tgt_qp_list;
+	struct rw_semaphore	tgt_qps_rwsem;
+	struct xarray		tgt_qps;
 };
 
 struct ib_ah {
-- 
2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH rdma-next 2/2] RDMA/core: Optimize XRC target lookup
       [not found]   ` <CAD+HZHUnW53ni=16=XL6hY1AHoNtsa88_V5P+XOHb55Fm83zZQ@mail.gmail.com>
@ 2020-06-21 14:41     ` Maor Gottlieb
  0 siblings, 0 replies; 8+ messages in thread
From: Maor Gottlieb @ 2020-06-21 14:41 UTC (permalink / raw)
  To: Jack Wang, Leon Romanovsky; +Cc: Doug Ledford, Jason Gunthorpe, linux-rdma


On 6/21/2020 2:34 PM, Jack Wang wrote:
>
> Hi
>
>
> Leon Romanovsky <leon@kernel.org <mailto:leon@kernel.org>>于2020年6月21日 
> 周日12:42写道:
>
>     From: Maor Gottlieb <maorg@mellanox.com <mailto:maorg@mellanox.com>>
>
>     Replace the mutex with read write semaphore and use xarray instead
>     of linked list for XRC target QPs. This will give faster XRC target
>     lookup. In addition, when QP is closed, don't insert it back to the
>     xarray if the destroy command failed
>
> Just curious, why not use RCU,xarray is RCU friendly?
>
> Thanks

The lock protects against parallel close and open of the same XRC target 
QP and not the access to the xarray. In addition RCU can't be taken 
since there is a sleepable function that called under the lock, using 
SRCU locking shceme looks overkill for me in this case.
>
>
>
>     Signed-off-by: Maor Gottlieb <maorg@mellanox.com
>     <mailto:maorg@mellanox.com>>
>     Signed-off-by: Leon Romanovsky <leonro@mellanox.com
>     <mailto:leonro@mellanox.com>>
>     ---
>      drivers/infiniband/core/verbs.c | 50
>     +++++++++++++++------------------
>      include/rdma/ib_verbs.h         |  5 ++--
>      2 files changed, 24 insertions(+), 31 deletions(-)
>
>     diff --git a/drivers/infiniband/core/verbs.c
>     b/drivers/infiniband/core/verbs.c
>     index d66a0ad62077..ef980124f7e6 100644
>     --- a/drivers/infiniband/core/verbs.c
>     +++ b/drivers/infiniband/core/verbs.c
>     @@ -1090,13 +1090,6 @@ static void
>     __ib_shared_qp_event_handler(struct ib_event *event, void *context)
>     spin_unlock_irqrestore(&qp->device->qp_open_list_lock, flags);
>      }
>
>     -static void __ib_insert_xrcd_qp(struct ib_xrcd *xrcd, struct
>     ib_qp *qp)
>     -{
>     -       mutex_lock(&xrcd->tgt_qp_mutex);
>     -       list_add(&qp->xrcd_list, &xrcd->tgt_qp_list);
>     -       mutex_unlock(&xrcd->tgt_qp_mutex);
>     -}
>     -
>      static struct ib_qp *__ib_open_qp(struct ib_qp *real_qp,
>                                       void (*event_handler)(struct
>     ib_event *, void *),
>                                       void *qp_context)
>     @@ -1139,16 +1132,15 @@ struct ib_qp *ib_open_qp(struct ib_xrcd *xrcd,
>             if (qp_open_attr->qp_type != IB_QPT_XRC_TGT)
>                     return ERR_PTR(-EINVAL);
>
>     -       qp = ERR_PTR(-EINVAL);
>     -       mutex_lock(&xrcd->tgt_qp_mutex);
>     -       list_for_each_entry(real_qp, &xrcd->tgt_qp_list, xrcd_list) {
>     -               if (real_qp->qp_num == qp_open_attr->qp_num) {
>     -                       qp = __ib_open_qp(real_qp,
>     qp_open_attr->event_handler,
>     -  qp_open_attr->qp_context);
>     -                       break;
>     -               }
>     +       down_read(&xrcd->tgt_qps_rwsem);
>     +       real_qp = xa_load(&xrcd->tgt_qps, qp_open_attr->qp_num);
>     +       if (!real_qp) {
>     +               up_read(&xrcd->tgt_qps_rwsem);
>     +               return ERR_PTR(-EINVAL);
>             }
>     -       mutex_unlock(&xrcd->tgt_qp_mutex);
>     +       qp = __ib_open_qp(real_qp, qp_open_attr->event_handler,
>     +                         qp_open_attr->qp_context);
>     +       up_read(&xrcd->tgt_qps_rwsem);
>             return qp;
>      }
>      EXPORT_SYMBOL(ib_open_qp);
>     @@ -1157,6 +1149,7 @@ static struct ib_qp
>     *create_xrc_qp_user(struct ib_qp *qp,
>                                             struct ib_qp_init_attr
>     *qp_init_attr)
>      {
>             struct ib_qp *real_qp = qp;
>     +       int err;
>
>             qp->event_handler = __ib_shared_qp_event_handler;
>             qp->qp_context = qp;
>     @@ -1172,7 +1165,12 @@ static struct ib_qp
>     *create_xrc_qp_user(struct ib_qp *qp,
>             if (IS_ERR(qp))
>                     return qp;
>
>     -       __ib_insert_xrcd_qp(qp_init_attr->xrcd, real_qp);
>     +       err = xa_err(xa_store(&qp_init_attr->xrcd->tgt_qps,
>     real_qp->qp_num,
>     +                             real_qp, GFP_KERNEL));
>     +       if (err) {
>     +               ib_close_qp(qp);
>     +               return ERR_PTR(err);
>     +       }
>             return qp;
>      }
>
>     @@ -1888,21 +1886,18 @@ static int __ib_destroy_shared_qp(struct
>     ib_qp *qp)
>
>             real_qp = qp->real_qp;
>             xrcd = real_qp->xrcd;
>     -
>     -       mutex_lock(&xrcd->tgt_qp_mutex);
>     +       down_write(&xrcd->tgt_qps_rwsem);
>             ib_close_qp(qp);
>             if (atomic_read(&real_qp->usecnt) == 0)
>     -               list_del(&real_qp->xrcd_list);
>     +               xa_erase(&xrcd->tgt_qps, real_qp->qp_num);
>             else
>                     real_qp = NULL;
>     -       mutex_unlock(&xrcd->tgt_qp_mutex);
>     +       up_write(&xrcd->tgt_qps_rwsem);
>
>             if (real_qp) {
>                     ret = ib_destroy_qp(real_qp);
>                     if (!ret)
>                             atomic_dec(&xrcd->usecnt);
>     -               else
>     -                       __ib_insert_xrcd_qp(xrcd, real_qp);
>             }
>
>             return 0;
>     @@ -2308,8 +2303,8 @@ struct ib_xrcd *ib_alloc_xrcd_user(struct
>     ib_device *device,
>                     xrcd->device = device;
>                     xrcd->inode = inode;
>                     atomic_set(&xrcd->usecnt, 0);
>     -               mutex_init(&xrcd->tgt_qp_mutex);
>     -               INIT_LIST_HEAD(&xrcd->tgt_qp_list);
>     +               init_rwsem(&xrcd->tgt_qps_rwsem);
>     +               xa_init(&xrcd->tgt_qps);
>             }
>
>             return xrcd;
>     @@ -2318,19 +2313,18 @@ EXPORT_SYMBOL(ib_alloc_xrcd_user);
>
>      int ib_dealloc_xrcd_user(struct ib_xrcd *xrcd, struct ib_udata
>     *udata)
>      {
>     +       unsigned long index;
>             struct ib_qp *qp;
>             int ret;
>
>             if (atomic_read(&xrcd->usecnt))
>                     return -EBUSY;
>
>     -       while (!list_empty(&xrcd->tgt_qp_list)) {
>     -               qp = list_entry(xrcd->tgt_qp_list.next, struct
>     ib_qp, xrcd_list);
>     +       xa_for_each(&xrcd->tgt_qps, index, qp) {
>                     ret = ib_destroy_qp(qp);
>                     if (ret)
>                             return ret;
>             }
>     -       mutex_destroy(&xrcd->tgt_qp_mutex);
>
>             return xrcd->device->ops.dealloc_xrcd(xrcd, udata);
>      }
>     diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>     index f785a4f1e58b..9b973b3b6f4c 100644
>     --- a/include/rdma/ib_verbs.h
>     +++ b/include/rdma/ib_verbs.h
>     @@ -1568,9 +1568,8 @@ struct ib_xrcd {
>             struct ib_device       *device;
>             atomic_t                usecnt; /* count all exposed
>     resources */
>             struct inode           *inode;
>     -
>     -       struct mutex            tgt_qp_mutex;
>     -       struct list_head        tgt_qp_list;
>     +       struct rw_semaphore     tgt_qps_rwsem;
>     +       struct xarray           tgt_qps;
>      };
>
>      struct ib_ah {
>     -- 
>     2.26.2
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH rdma-next 2/2] RDMA/core: Optimize XRC target lookup
  2020-06-21 10:41 ` [PATCH rdma-next 2/2] RDMA/core: Optimize XRC target lookup Leon Romanovsky
       [not found]   ` <CAD+HZHUnW53ni=16=XL6hY1AHoNtsa88_V5P+XOHb55Fm83zZQ@mail.gmail.com>
@ 2020-06-22 12:29   ` Jason Gunthorpe
  2020-06-22 12:57     ` Maor Gottlieb
  1 sibling, 1 reply; 8+ messages in thread
From: Jason Gunthorpe @ 2020-06-22 12:29 UTC (permalink / raw)
  To: Leon Romanovsky; +Cc: Doug Ledford, Maor Gottlieb, linux-rdma

On Sun, Jun 21, 2020 at 01:41:10PM +0300, Leon Romanovsky wrote:
> @@ -2318,19 +2313,18 @@ EXPORT_SYMBOL(ib_alloc_xrcd_user);
>  
>  int ib_dealloc_xrcd_user(struct ib_xrcd *xrcd, struct ib_udata *udata)
>  {
> +	unsigned long index;
>  	struct ib_qp *qp;
>  	int ret;
>  
>  	if (atomic_read(&xrcd->usecnt))
>  		return -EBUSY;
>  
> -	while (!list_empty(&xrcd->tgt_qp_list)) {
> -		qp = list_entry(xrcd->tgt_qp_list.next, struct ib_qp, xrcd_list);
> +	xa_for_each(&xrcd->tgt_qps, index, qp) {
>  		ret = ib_destroy_qp(qp);
>  		if (ret)
>  			return ret;
>  	}

Why doesn't this need to hold the tgt_qps_rwsem? 

Jason

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH rdma-next 2/2] RDMA/core: Optimize XRC target lookup
  2020-06-22 12:29   ` Jason Gunthorpe
@ 2020-06-22 12:57     ` Maor Gottlieb
  2020-06-22 13:05       ` Jason Gunthorpe
  0 siblings, 1 reply; 8+ messages in thread
From: Maor Gottlieb @ 2020-06-22 12:57 UTC (permalink / raw)
  To: Jason Gunthorpe, Leon Romanovsky; +Cc: Doug Ledford, linux-rdma


On 6/22/2020 3:29 PM, Jason Gunthorpe wrote:
> On Sun, Jun 21, 2020 at 01:41:10PM +0300, Leon Romanovsky wrote:
>> @@ -2318,19 +2313,18 @@ EXPORT_SYMBOL(ib_alloc_xrcd_user);
>>   
>>   int ib_dealloc_xrcd_user(struct ib_xrcd *xrcd, struct ib_udata *udata)
>>   {
>> +	unsigned long index;
>>   	struct ib_qp *qp;
>>   	int ret;
>>   
>>   	if (atomic_read(&xrcd->usecnt))
>>   		return -EBUSY;
>>   
>> -	while (!list_empty(&xrcd->tgt_qp_list)) {
>> -		qp = list_entry(xrcd->tgt_qp_list.next, struct ib_qp, xrcd_list);
>> +	xa_for_each(&xrcd->tgt_qps, index, qp) {
>>   		ret = ib_destroy_qp(qp);
>>   		if (ret)
>>   			return ret;
>>   	}
> Why doesn't this need to hold the tgt_qps_rwsem?
>
> Jason

Actually, we don't need this part of code. if usecnt is zero so we don't 
have any tgt qp in the list. I guess it is leftovers of ib_release_qp 
which was already deleted.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH rdma-next 2/2] RDMA/core: Optimize XRC target lookup
  2020-06-22 12:57     ` Maor Gottlieb
@ 2020-06-22 13:05       ` Jason Gunthorpe
  2020-06-22 13:39         ` Leon Romanovsky
  0 siblings, 1 reply; 8+ messages in thread
From: Jason Gunthorpe @ 2020-06-22 13:05 UTC (permalink / raw)
  To: Maor Gottlieb; +Cc: Leon Romanovsky, Doug Ledford, linux-rdma

On Mon, Jun 22, 2020 at 03:57:29PM +0300, Maor Gottlieb wrote:
> 
> On 6/22/2020 3:29 PM, Jason Gunthorpe wrote:
> > On Sun, Jun 21, 2020 at 01:41:10PM +0300, Leon Romanovsky wrote:
> > > @@ -2318,19 +2313,18 @@ EXPORT_SYMBOL(ib_alloc_xrcd_user);
> > >   int ib_dealloc_xrcd_user(struct ib_xrcd *xrcd, struct ib_udata *udata)
> > >   {
> > > +	unsigned long index;
> > >   	struct ib_qp *qp;
> > >   	int ret;
> > >   	if (atomic_read(&xrcd->usecnt))
> > >   		return -EBUSY;
> > > -	while (!list_empty(&xrcd->tgt_qp_list)) {
> > > -		qp = list_entry(xrcd->tgt_qp_list.next, struct ib_qp, xrcd_list);
> > > +	xa_for_each(&xrcd->tgt_qps, index, qp) {
> > >   		ret = ib_destroy_qp(qp);
> > >   		if (ret)
> > >   			return ret;
> > >   	}
> > Why doesn't this need to hold the tgt_qps_rwsem?
> > 
> > Jason
> 
> Actually, we don't need this part of code. if usecnt is zero so we don't
> have any tgt qp in the list. I guess it is leftovers of ib_release_qp which
> was already deleted.

Then have a WARN_ON that the xarray is empty

Jason

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH rdma-next 2/2] RDMA/core: Optimize XRC target lookup
  2020-06-22 13:05       ` Jason Gunthorpe
@ 2020-06-22 13:39         ` Leon Romanovsky
  0 siblings, 0 replies; 8+ messages in thread
From: Leon Romanovsky @ 2020-06-22 13:39 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Maor Gottlieb, Doug Ledford, linux-rdma

On Mon, Jun 22, 2020 at 10:05:20AM -0300, Jason Gunthorpe wrote:
> On Mon, Jun 22, 2020 at 03:57:29PM +0300, Maor Gottlieb wrote:
> >
> > On 6/22/2020 3:29 PM, Jason Gunthorpe wrote:
> > > On Sun, Jun 21, 2020 at 01:41:10PM +0300, Leon Romanovsky wrote:
> > > > @@ -2318,19 +2313,18 @@ EXPORT_SYMBOL(ib_alloc_xrcd_user);
> > > >   int ib_dealloc_xrcd_user(struct ib_xrcd *xrcd, struct ib_udata *udata)
> > > >   {
> > > > +	unsigned long index;
> > > >   	struct ib_qp *qp;
> > > >   	int ret;
> > > >   	if (atomic_read(&xrcd->usecnt))
> > > >   		return -EBUSY;
> > > > -	while (!list_empty(&xrcd->tgt_qp_list)) {
> > > > -		qp = list_entry(xrcd->tgt_qp_list.next, struct ib_qp, xrcd_list);
> > > > +	xa_for_each(&xrcd->tgt_qps, index, qp) {
> > > >   		ret = ib_destroy_qp(qp);
> > > >   		if (ret)
> > > >   			return ret;
> > > >   	}
> > > Why doesn't this need to hold the tgt_qps_rwsem?
> > >
> > > Jason
> >
> > Actually, we don't need this part of code. if usecnt is zero so we don't
> > have any tgt qp in the list. I guess it is leftovers of ib_release_qp which
> > was already deleted.
>
> Then have a WARN_ON that the xarray is empty

No problem.

Thanks

>
> Jason

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-06-22 13:40 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-21 10:41 [PATCH rdma-next 0/2] Convert XRC to use xarray Leon Romanovsky
2020-06-21 10:41 ` [PATCH rdma-next 1/2] RDMA: Clean ib_alloc_xrcd() and reuse it to allocate XRC domain Leon Romanovsky
2020-06-21 10:41 ` [PATCH rdma-next 2/2] RDMA/core: Optimize XRC target lookup Leon Romanovsky
     [not found]   ` <CAD+HZHUnW53ni=16=XL6hY1AHoNtsa88_V5P+XOHb55Fm83zZQ@mail.gmail.com>
2020-06-21 14:41     ` Maor Gottlieb
2020-06-22 12:29   ` Jason Gunthorpe
2020-06-22 12:57     ` Maor Gottlieb
2020-06-22 13:05       ` Jason Gunthorpe
2020-06-22 13:39         ` Leon Romanovsky

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).