CMA fix and small improvements

PATCH Applied Leon Romanovsky 2 participants 10 messages Jan 26, 2020 – Feb 11, 2020 InfiniBand

lore

Share & tools

lore

Leon Romanovsky

author

2020-01-26 14:26 UTC #0 lore raw

cover CMA fix and small improvements

From: Leon Romanovsky <[email protected]>

From Parav,

This series covers a fix for a reference count leak and few small code improvements to the RDMA CM code as below.

Patch-1: Fixes a reference count leak where reference count increment was missing.
Patch-2: Uses helper function to hold refcount and to enqueue work to avoid errors.
Patch-3: Uses RDMA port iterator API and avoids open coding.
Patch-4: Renames cma device's cma_ref/deref_dev() to cma_dev_get/put() to align it to rest of kernel for similar use.
Patch-5: Uses refcount APIs to get/put reference to CMA device.
Patch-6: Renames cma cm_id's ref helpers to cma_id_get/put() to align to rest of the kernel for similar use.
Patch-7: Uses refcount APIs to get/put reference to CM id.

Thanks

Parav Pandit (7):
RDMA/cma: Fix unbalanced cm_id reference count during address resolve RDMA/cma: Use helper function to enqueue resolve work item RDMA/cma: Use RDMA device port iterator
RDMA/cma: Rename cma_device ref/deref helpers to to get/put RDMA/cma: Use refcount API to reflect refcount
RDMA/cma: Rename cma_device ref/deref helpers to to get/put RDMA/cma: Use refcount API to reflect refcount

 drivers/infiniband/core/cma.c          | 99 ++++++++++++++------------
 drivers/infiniband/core/cma_configfs.c |  6 +-
 drivers/infiniband/core/cma_priv.h     |  6 +-
 3 files changed, 60 insertions(+), 51 deletions(-)

--
2.24.1

Leon Romanovsky

author

2020-01-26 14:27 UTC #1 lore raw

PATCH 1/7 RDMA/cma: Fix unbalanced cm_id reference count during address resolve

From: Parav Pandit <[email protected]>

Below cited commit missed to consider AF_IB and loopback code flow in rdma_resolve_addr().
This leads to unbalanced cm_id refcount in cma_work_handler() which puts the refcount which was not incremented in rdma_resolve_addr().

A call trace is observed with such code flow.

BUG: unable to handle kernel NULL pointer dereference at (null)
[<ffffffff96b67e16>] __mutex_lock_slowpath+0x166/0x1d0
[<ffffffff96b6715f>] mutex_lock+0x1f/0x2f
[<ffffffffc0beabb5>] cma_work_handler+0x25/0xa0
[<ffffffff964b9ebf>] process_one_work+0x17f/0x440
[<ffffffff964baf56>] worker_thread+0x126/0x3c0

Hence, hold the cm_id reference when scheduling resolve work item.

Fixes 722c7b2bfead ("RDMA/{cma, core}: Avoid callback on rdma_addr_cancel()")

Signed-off-by Parav Pandit <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/cma.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/infiniband/core/cma.c +2 -0

index 55a9afacfedd..72f032160c4b 100644
@@ -3148,6 +3148,7 @@ static int cma_resolve_loopback(struct rdma_id_private *id_priv)
 	rdma_addr_get_sgid(&id_priv->id.route.addr.dev_addr, &gid);
 	rdma_addr_set_dgid(&id_priv->id.route.addr.dev_addr, &gid);
 
+	atomic_inc(&id_priv->refcount);
 	cma_init_resolve_addr_work(work, id_priv);
 	queue_work(cma_wq, &work->work);
 	return 0;

@@ -3174,6 +3175,7 @@ static int cma_resolve_ib_addr(struct rdma_id_private *id_priv)
 	rdma_addr_set_dgid(&id_priv->id.route.addr.dev_addr, (union ib_gid *)
 		&(((struct sockaddr_ib *) &id_priv->id.route.addr.dst_addr)->sib_addr));
 
+	atomic_inc(&id_priv->refcount);
 	cma_init_resolve_addr_work(work, id_priv);
 	queue_work(cma_wq, &work->work);
 	return 0;

Signature · 1 line

2.24.1

Jason Gunthorpe

2020-01-28 18:25 UTC #2 lore raw

Re: [PATCH rdma-next 1/7] RDMA/cma: Fix unbalanced cm_id reference count during address resolve

Quoted 16 lines · Leon Romanovsky

From: Parav Pandit <[email protected]>
Below cited commit missed to consider AF_IB and loopback code flow in rdma_resolve_addr().
This leads to unbalanced cm_id refcount in cma_work_handler() which puts the refcount which was not incremented in rdma_resolve_addr().
A call trace is observed with such code flow.
BUG: unable to handle kernel NULL pointer dereference at (null)
[<ffffffff96b67e16>] __mutex_lock_slowpath+0x166/0x1d0
[<ffffffff96b6715f>] mutex_lock+0x1f/0x2f
[<ffffffffc0beabb5>] cma_work_handler+0x25/0xa0
[<ffffffff964b9ebf>] process_one_work+0x17f/0x440
[<ffffffff964baf56>] worker_thread+0x126/0x3c0

Quoted 8 lines

Hence, hold the cm_id reference when scheduling resolve work item.
Fixes 722c7b2bfead ("RDMA/{cma, core}: Avoid callback on rdma_addr_cancel()")
Signed-off-by Parav Pandit <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/core/cma.c | 2 ++
 1 file changed, 2 insertions(+)

It is very hard to read this flow around the 'work', the incr of the refcount in rdma_resolve_route() seems very poorly placed.

But this looks correct, so applied to for-next

Thanks,
Jason

Leon Romanovsky

author

2020-01-26 14:27 UTC #3 lore raw

PATCH 2/7 RDMA/cma: Use helper function to enqueue resolve work item

In reply to Leon Romanovsky

From: Parav Pandit <[email protected]>

To avoid errors, to attach ownership of work item and its cm_id refcount which is decremented in work handler, tie them up in single helper function. Also avoid code duplication.

Signed-off-by Parav Pandit <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/cma.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

drivers/infiniband/core/cma.c +8 -8

index 72f032160c4b..8f16ebb413c2 100644
@@ -2687,14 +2687,18 @@ static void cma_init_resolve_route_work(struct cma_work *work,
 	work->event.event = RDMA_CM_EVENT_ROUTE_RESOLVED;
 }
 
-static void cma_init_resolve_addr_work(struct cma_work *work,
-				       struct rdma_id_private *id_priv)
+static void enqueue_resolve_addr_work(struct cma_work *work,
+				      struct rdma_id_private *id_priv)
 {
+	atomic_inc(&id_priv->refcount);
+
 	work->id = id_priv;
 	INIT_WORK(&work->work, cma_work_handler);
 	work->old_state = RDMA_CM_ADDR_QUERY;
 	work->new_state = RDMA_CM_ADDR_RESOLVED;
 	work->event.event = RDMA_CM_EVENT_ADDR_RESOLVED;
+
+	queue_work(cma_wq, &work->work);
 }
 
 static int cma_resolve_ib_route(struct rdma_id_private *id_priv,

@@ -3148,9 +3152,7 @@ static int cma_resolve_loopback(struct rdma_id_private *id_priv)
 	rdma_addr_get_sgid(&id_priv->id.route.addr.dev_addr, &gid);
 	rdma_addr_set_dgid(&id_priv->id.route.addr.dev_addr, &gid);
 
-	atomic_inc(&id_priv->refcount);
-	cma_init_resolve_addr_work(work, id_priv);
-	queue_work(cma_wq, &work->work);
+	enqueue_resolve_addr_work(work, id_priv);
 	return 0;
 err:
 	kfree(work);

@@ -3175,9 +3177,7 @@ static int cma_resolve_ib_addr(struct rdma_id_private *id_priv)
 	rdma_addr_set_dgid(&id_priv->id.route.addr.dev_addr, (union ib_gid *)
 		&(((struct sockaddr_ib *) &id_priv->id.route.addr.dst_addr)->sib_addr));
 
-	atomic_inc(&id_priv->refcount);
-	cma_init_resolve_addr_work(work, id_priv);
-	queue_work(cma_wq, &work->work);
+	enqueue_resolve_addr_work(work, id_priv);
 	return 0;
 err:
 	kfree(work);

Signature · 1 line

2.24.1

Leon Romanovsky

author

2020-01-26 14:27 UTC #4 lore raw

PATCH 3/7 RDMA/cma: Use RDMA device port iterator

In reply to Leon Romanovsky

From: Parav Pandit <[email protected]>

Use RDMA device port iterator and avoid open coding.

Signed-off-by Parav Pandit <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/cma.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

drivers/infiniband/core/cma.c +7 -6

index 8f16ebb413c2..34c62eae08d8 100644
@@ -728,8 +728,8 @@ static int cma_iw_acquire_dev(struct rdma_id_private *id_priv,
 	struct cma_device *cma_dev;
 	enum ib_gid_type gid_type;
 	int ret = -ENODEV;
+	unsigned int port;
 	union ib_gid gid;
-	u8 port;
 
 	if (dev_addr->dev_type != ARPHRD_INFINIBAND &&
 	    id_priv->id.ps == RDMA_PS_IPOIB)

@@ -753,7 +753,8 @@ static int cma_iw_acquire_dev(struct rdma_id_private *id_priv,
 	}
 
 	list_for_each_entry(cma_dev, &dev_list, list) {
-		for (port = 1; port <= cma_dev->device->phys_port_cnt; ++port) {
+		rdma_for_each_port (cma_dev->device, port) {
+
 			if (listen_id_priv->cma_dev == cma_dev &&
 			    listen_id_priv->id.port_num == port)
 				continue;

@@ -786,8 +787,8 @@ static int cma_resolve_ib_dev(struct rdma_id_private *id_priv)
 	struct cma_device *cma_dev, *cur_dev;
 	struct sockaddr_ib *addr;
 	union ib_gid gid, sgid, *dgid;
+	unsigned int p;
 	u16 pkey, index;
-	u8 p;
 	enum ib_port_state port_state;
 	int i;

@@ -798,7 +799,7 @@ static int cma_resolve_ib_dev(struct rdma_id_private *id_priv)
 
 	mutex_lock(&lock);
 	list_for_each_entry(cur_dev, &dev_list, list) {
-		for (p = 1; p <= cur_dev->device->phys_port_cnt; ++p) {
+		rdma_for_each_port (cur_dev->device, p) {
 			if (!rdma_cap_af_ib(cur_dev->device, p))
 				continue;

@@ -3029,9 +3030,9 @@ static int cma_bind_loopback(struct rdma_id_private *id_priv)
 	struct cma_device *cma_dev, *cur_dev;
 	union ib_gid gid;
 	enum ib_port_state port_state;
+	unsigned int p;
 	u16 pkey;
 	int ret;
-	u8 p;
 
 	cma_dev = NULL;
 	mutex_lock(&lock);

@@ -3043,7 +3044,7 @@ static int cma_bind_loopback(struct rdma_id_private *id_priv)
 		if (!cma_dev)
 			cma_dev = cur_dev;
 
-		for (p = 1; p <= cur_dev->device->phys_port_cnt; ++p) {
+		rdma_for_each_port (cur_dev->device, p) {
 			if (!ib_get_cached_port_state(cur_dev->device, p, &port_state) &&
 			    port_state == IB_PORT_ACTIVE) {
 				cma_dev = cur_dev;

Signature · 1 line

2.24.1

Leon Romanovsky

author

2020-01-26 14:27 UTC #5 lore raw

PATCH 4/7 RDMA/cma: Rename cma_device ref/deref helpers to to get/put

In reply to Leon Romanovsky

From: Parav Pandit <[email protected]>

Helper functions which increment/decrement reference count of the structure are read better when they are named with get/put suffix.

Hence, rename cma_ref/deref_dev() to cma_dev_get/put().

Signed-off-by Parav Pandit <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/cma.c          | 22 +++++++++++-----------
 drivers/infiniband/core/cma_configfs.c |  6 +++---
 drivers/infiniband/core/cma_priv.h     |  4 ++--
 3 files changed, 16 insertions(+), 16 deletions(-)

drivers/infiniband/core/cma.c +11 ~~-11~~

index 34c62eae08d8..7e16d1b001ff 100644
@@ -247,11 +247,17 @@ enum {
 	CMA_OPTION_AFONLY,
 };
 
-void cma_ref_dev(struct cma_device *cma_dev)
+void cma_dev_get(struct cma_device *cma_dev)
 {
 	atomic_inc(&cma_dev->refcount);
 }
 
+void cma_dev_put(struct cma_device *cma_dev)
+{
+	if (atomic_dec_and_test(&cma_dev->refcount))
+		complete(&cma_dev->comp);
+}
+
 struct cma_device *cma_enum_devices_by_ibdev(cma_device_filter	filter,
 					     void		*cookie)
 {

@@ -267,7 +273,7 @@ struct cma_device *cma_enum_devices_by_ibdev(cma_device_filter	filter,
 		}
 
 	if (found_cma_dev)
-		cma_ref_dev(found_cma_dev);
+		cma_dev_get(found_cma_dev);
 	mutex_unlock(&lock);
 	return found_cma_dev;
 }

@@ -463,7 +469,7 @@ static int cma_igmp_send(struct net_device *ndev, union ib_gid *mgid, bool join)
 static void _cma_attach_to_dev(struct rdma_id_private *id_priv,
 			       struct cma_device *cma_dev)
 {
-	cma_ref_dev(cma_dev);
+	cma_dev_get(cma_dev);
 	id_priv->cma_dev = cma_dev;
 	id_priv->id.device = cma_dev->device;
 	id_priv->id.route.addr.dev_addr.transport =

@@ -484,12 +490,6 @@ static void cma_attach_to_dev(struct rdma_id_private *id_priv,
 					  rdma_start_port(cma_dev->device)];
 }
 
-void cma_deref_dev(struct cma_device *cma_dev)
-{
-	if (atomic_dec_and_test(&cma_dev->refcount))
-		complete(&cma_dev->comp);
-}
-
 static inline void release_mc(struct kref *kref)
 {
 	struct cma_multicast *mc = container_of(kref, struct cma_multicast, mcref);

@@ -502,7 +502,7 @@ static void cma_release_dev(struct rdma_id_private *id_priv)
 {
 	mutex_lock(&lock);
 	list_del(&id_priv->list);
-	cma_deref_dev(id_priv->cma_dev);
+	cma_dev_put(id_priv->cma_dev);
 	id_priv->cma_dev = NULL;
 	mutex_unlock(&lock);
 }

@@ -4728,7 +4728,7 @@ static void cma_process_remove(struct cma_device *cma_dev)
 	}
 	mutex_unlock(&lock);
 
-	cma_deref_dev(cma_dev);
+	cma_dev_put(cma_dev);
 	wait_for_completion(&cma_dev->comp);
 }

drivers/infiniband/core/cma_configfs.c +3 -3

index 8b0b5ae22e4c..c672a4978bfd 100644
@@ -94,7 +94,7 @@ static int cma_configfs_params_get(struct config_item *item,
 
 static void cma_configfs_params_put(struct cma_device *cma_dev)
 {
-	cma_deref_dev(cma_dev);
+	cma_dev_put(cma_dev);
 }
 
 static ssize_t default_roce_mode_show(struct config_item *item,

@@ -312,12 +312,12 @@ static struct config_group *make_cma_dev(struct config_group *group,
 	configfs_add_default_group(&cma_dev_group->ports_group,
 			&cma_dev_group->device_group);
 
-	cma_deref_dev(cma_dev);
+	cma_dev_put(cma_dev);
 	return &cma_dev_group->device_group;
 
 fail:
 	if (cma_dev)
-		cma_deref_dev(cma_dev);
+		cma_dev_put(cma_dev);
 	kfree(cma_dev_group);
 	return ERR_PTR(err);
 }

drivers/infiniband/core/cma_priv.h +2 -2

index ca7307277518..4e04c442ff86 100644
@@ -111,8 +111,8 @@ static inline void cma_configfs_exit(void)
 }
 #endif
 
-void cma_ref_dev(struct cma_device *dev);
-void cma_deref_dev(struct cma_device *dev);
+void cma_dev_get(struct cma_device *dev);
+void cma_dev_put(struct cma_device *dev);
 typedef bool (*cma_device_filter)(struct ib_device *, void *);
 struct cma_device *cma_enum_devices_by_ibdev(cma_device_filter filter,
 					     void *cookie);

Signature · 1 line

2.24.1

Leon Romanovsky

author

2020-01-26 14:27 UTC #6 lore raw

PATCH 5/7 RDMA/cma: Use refcount API to reflect refcount

In reply to Leon Romanovsky

From: Parav Pandit <[email protected]>

Use the refcount variant to capture the reference counting of the cma device structure.

Signed-off-by Parav Pandit <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/cma.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/infiniband/core/cma.c +4 -4

index 7e16d1b001ff..a5fd44194d77 100644
@@ -199,7 +199,7 @@ struct cma_device {
 	struct list_head	list;
 	struct ib_device	*device;
 	struct completion	comp;
-	atomic_t		refcount;
+	refcount_t refcount;
 	struct list_head	id_list;
 	enum ib_gid_type	*default_gid_type;
 	u8			*default_roce_tos;

@@ -249,12 +249,12 @@ enum {
 
 void cma_dev_get(struct cma_device *cma_dev)
 {
-	atomic_inc(&cma_dev->refcount);
+	refcount_inc(&cma_dev->refcount);
 }
 
 void cma_dev_put(struct cma_device *cma_dev)
 {
-	if (atomic_dec_and_test(&cma_dev->refcount))
+	if (refcount_dec_and_test(&cma_dev->refcount))
 		complete(&cma_dev->comp);
 }

@@ -4657,7 +4657,7 @@ static void cma_add_one(struct ib_device *device)
 	}
 
 	init_completion(&cma_dev->comp);
-	atomic_set(&cma_dev->refcount, 1);
+	refcount_set(&cma_dev->refcount, 1);
 	INIT_LIST_HEAD(&cma_dev->id_list);
 	ib_set_client_data(device, &cma_client, cma_dev);

Signature · 1 line

2.24.1

Leon Romanovsky

author

2020-01-26 14:27 UTC #7 lore raw

PATCH 6/7 RDMA/cma: Rename cma_device ref/deref helpers to to get/put

In reply to Leon Romanovsky

From: Parav Pandit <[email protected]>

Helper functions which increment/decrement reference count of the structure are read better when they are named with get/put suffix.

Hence, rename cma_ref/deref_id() to cma_id_get/put(). Also use cma_get_id() wrapper to find the balancing put() calls.

Signed-off-by Parav Pandit <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/cma.c | 42 ++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 18 deletions(-)

drivers/infiniband/core/cma.c +24 ~~-18~~

index a5fd44194d77..d6355e21cc87 100644
@@ -841,7 +841,12 @@ static int cma_resolve_ib_dev(struct rdma_id_private *id_priv)
 	return 0;
 }
 
-static void cma_deref_id(struct rdma_id_private *id_priv)
+static void cma_id_get(struct rdma_id_private *id_priv)
+{
+	atomic_inc(&id_priv->refcount);
+}
+
+static void cma_id_put(struct rdma_id_private *id_priv)
 {
 	if (atomic_dec_and_test(&id_priv->refcount))
 		complete(&id_priv->comp);

@@ -1847,11 +1852,11 @@ void rdma_destroy_id(struct rdma_cm_id *id)
 	}
 
 	cma_release_port(id_priv);
-	cma_deref_id(id_priv);
+	cma_id_put(id_priv);
 	wait_for_completion(&id_priv->comp);
 
 	if (id_priv->internal_id)
-		cma_deref_id(id_priv->id.context);
+		cma_id_put(id_priv->id.context);
 
 	kfree(id_priv->id.route.path_rec);

@@ -2188,7 +2193,7 @@ static int cma_ib_req_handler(struct ib_cm_id *cm_id,
 	 * Protect against the user destroying conn_id from another thread
 	 * until we're done accessing it.
 	 */
-	atomic_inc(&conn_id->refcount);
+	cma_id_get(conn_id);
 	ret = cma_cm_event_handler(conn_id, &event);
 	if (ret)
 		goto err3;

@@ -2205,13 +2210,13 @@ static int cma_ib_req_handler(struct ib_cm_id *cm_id,
 	mutex_unlock(&lock);
 	mutex_unlock(&conn_id->handler_mutex);
 	mutex_unlock(&listen_id->handler_mutex);
-	cma_deref_id(conn_id);
+	cma_id_put(conn_id);
 	if (net_dev)
 		dev_put(net_dev);
 	return 0;
 
 err3:
-	cma_deref_id(conn_id);
+	cma_id_put(conn_id);
 	/* Destroy the CM ID by returning a non-zero value. */
 	conn_id->cm_id.ib = NULL;
 err2:

@@ -2392,7 +2397,7 @@ static int iw_conn_req_handler(struct iw_cm_id *cm_id,
 	 * Protect against the user destroying conn_id from another thread
 	 * until we're done accessing it.
 	 */
-	atomic_inc(&conn_id->refcount);
+	cma_id_get(conn_id);
 	ret = cma_cm_event_handler(conn_id, &event);
 	if (ret) {
 		/* User wants to destroy the CM ID */

@@ -2400,13 +2405,13 @@ static int iw_conn_req_handler(struct iw_cm_id *cm_id,
 		cma_exch(conn_id, RDMA_CM_DESTROYING);
 		mutex_unlock(&conn_id->handler_mutex);
 		mutex_unlock(&listen_id->handler_mutex);
-		cma_deref_id(conn_id);
+		cma_id_put(conn_id);
 		rdma_destroy_id(&conn_id->id);
 		return ret;
 	}
 
 	mutex_unlock(&conn_id->handler_mutex);
-	cma_deref_id(conn_id);
+	cma_id_put(conn_id);
 
 out:
 	mutex_unlock(&listen_id->handler_mutex);

@@ -2493,7 +2498,7 @@ static void cma_listen_on_dev(struct rdma_id_private *id_priv,
 
 	_cma_attach_to_dev(dev_id_priv, cma_dev);
 	list_add_tail(&dev_id_priv->listen_list, &id_priv->listen_list);
-	atomic_inc(&id_priv->refcount);
+	cma_id_get(id_priv);
 	dev_id_priv->internal_id = 1;
 	dev_id_priv->afonly = id_priv->afonly;
 	dev_id_priv->tos_set = id_priv->tos_set;

@@ -2648,7 +2653,7 @@ static void cma_work_handler(struct work_struct *_work)
 	}
 out:
 	mutex_unlock(&id_priv->handler_mutex);
-	cma_deref_id(id_priv);
+	cma_id_put(id_priv);
 	if (destroy)
 		rdma_destroy_id(&id_priv->id);
 	kfree(work);

@@ -2672,7 +2677,7 @@ static void cma_ndev_work_handler(struct work_struct *_work)
 
 out:
 	mutex_unlock(&id_priv->handler_mutex);
-	cma_deref_id(id_priv);
+	cma_id_put(id_priv);
 	if (destroy)
 		rdma_destroy_id(&id_priv->id);
 	kfree(work);

@@ -2691,7 +2696,8 @@ static void cma_init_resolve_route_work(struct cma_work *work,
 static void enqueue_resolve_addr_work(struct cma_work *work,
 				      struct rdma_id_private *id_priv)
 {
-	atomic_inc(&id_priv->refcount);
+	/* Balances with cma_id_put() in cma_work_handler */
+	cma_id_get(id_priv);
 
 	work->id = id_priv;
 	INIT_WORK(&work->work, cma_work_handler);

@@ -2987,7 +2993,7 @@ int rdma_resolve_route(struct rdma_cm_id *id, unsigned long timeout_ms)
 	if (!cma_comp_exch(id_priv, RDMA_CM_ADDR_RESOLVED, RDMA_CM_ROUTE_QUERY))
 		return -EINVAL;
 
-	atomic_inc(&id_priv->refcount);
+	cma_id_get(id_priv);
 	if (rdma_cap_ib_sa(id->device, id->port_num))
 		ret = cma_resolve_ib_route(id_priv, timeout_ms);
 	else if (rdma_protocol_roce(id->device, id->port_num))

@@ -3003,7 +3009,7 @@ int rdma_resolve_route(struct rdma_cm_id *id, unsigned long timeout_ms)
 	return 0;
 err:
 	cma_comp_exch(id_priv, RDMA_CM_ROUTE_QUERY, RDMA_CM_ADDR_RESOLVED);
-	cma_deref_id(id_priv);
+	cma_id_put(id_priv);
 	return ret;
 }
 EXPORT_SYMBOL(rdma_resolve_route);

@@ -4582,7 +4588,7 @@ static int cma_netdev_change(struct net_device *ndev, struct rdma_id_private *id
 		INIT_WORK(&work->work, cma_ndev_work_handler);
 		work->id = id_priv;
 		work->event.event = RDMA_CM_EVENT_ADDR_CHANGE;
-		atomic_inc(&id_priv->refcount);
+		cma_id_get(id_priv);
 		queue_work(cma_wq, &work->work);
 	}

@@ -4716,11 +4722,11 @@ static void cma_process_remove(struct cma_device *cma_dev)
 
 		list_del(&id_priv->listen_list);
 		list_del_init(&id_priv->list);
-		atomic_inc(&id_priv->refcount);
+		cma_id_get(id_priv);
 		mutex_unlock(&lock);
 
 		ret = id_priv->internal_id ? 1 : cma_remove_id_dev(id_priv);
-		cma_deref_id(id_priv);
+		cma_id_put(id_priv);
 		if (ret)
 			rdma_destroy_id(&id_priv->id);

Signature · 1 line

2.24.1

Leon Romanovsky

author

2020-01-26 14:27 UTC #8 lore raw

PATCH 7/7 RDMA/cma: Use refcount API to reflect refcount

In reply to Leon Romanovsky

From: Parav Pandit <[email protected]>

Use the refcount variant to capture the reference counting of the cm_id structure.

Signed-off-by Parav Pandit <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/cma.c      | 6 +++---
 drivers/infiniband/core/cma_priv.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

drivers/infiniband/core/cma.c +3 -3

index d6355e21cc87..af96675e31c4 100644
@@ -843,12 +843,12 @@ static int cma_resolve_ib_dev(struct rdma_id_private *id_priv)
 
 static void cma_id_get(struct rdma_id_private *id_priv)
 {
-	atomic_inc(&id_priv->refcount);
+	refcount_inc(&id_priv->refcount);
 }
 
 static void cma_id_put(struct rdma_id_private *id_priv)
 {
-	if (atomic_dec_and_test(&id_priv->refcount))
+	if (refcount_dec_and_test(&id_priv->refcount))
 		complete(&id_priv->comp);
 }

@@ -876,7 +876,7 @@ struct rdma_cm_id *__rdma_create_id(struct net *net,
 	spin_lock_init(&id_priv->lock);
 	mutex_init(&id_priv->qp_mutex);
 	init_completion(&id_priv->comp);
-	atomic_set(&id_priv->refcount, 1);
+	refcount_set(&id_priv->refcount, 1);
 	mutex_init(&id_priv->handler_mutex);
 	INIT_LIST_HEAD(&id_priv->listen_list);
 	INIT_LIST_HEAD(&id_priv->mc_list);

drivers/infiniband/core/cma_priv.h +1 -1

index 4e04c442ff86..5edcf44a9307 100644
@@ -66,7 +66,7 @@ struct rdma_id_private {
 	struct mutex		qp_mutex;
 
 	struct completion	comp;
-	atomic_t		refcount;
+	refcount_t refcount;
 	struct mutex		handler_mutex;
 
 	int			backlog;

Signature · 1 line

2.24.1

Jason Gunthorpe

Applied

2020-02-11 18:01 UTC #9 lore raw

In reply to Leon Romanovsky

Quoted 21 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>
Quoted 1 line
From Parav,
This series covers a fix for a reference count leak and few small code improvements to the RDMA CM code as below.
Patch-1: Fixes a reference count leak where reference count increment was missing.
Patch-2: Uses helper function to hold refcount and to enqueue work to avoid errors.
Patch-3: Uses RDMA port iterator API and avoids open coding.
Patch-4: Renames cma device's cma_ref/deref_dev() to cma_dev_get/put() to align it to rest of kernel for similar use.
Patch-5: Uses refcount APIs to get/put reference to CMA device.
Patch-6: Renames cma cm_id's ref helpers to cma_id_get/put() to align to rest of the kernel for similar use.
Patch-7: Uses refcount APIs to get/put reference to CM id.
Thanks

Quoted 7 lines

Parav Pandit (7):
RDMA/cma: Use helper function to enqueue resolve work item RDMA/cma: Use RDMA device port iterator
RDMA/cma: Rename cma_device ref/deref helpers to to get/put RDMA/cma: Use refcount API to reflect refcount RDMA/cma: Rename cma_device ref/deref helpers to to get/put RDMA/cma: Use refcount API to reflect refcount

Applied to for-next

Thanks,
Jason