[PATCH for-next v2 0/6] RDMA/rxe: Misc rxe_pool cleanups

[PATCH for-next v2 0/6] RDMA/rxe: Misc rxe_pool cleanups

[Bob Pearson]

This series of patches corrects a bug introduced in rxe_pool.c
by a recent commit and then addresses several issues raised
during discussion of the bug and the proposed fix.

The first patch fixes a real bug but the other five are
stylistic and cleanup changes.

Signed-off-by: Bob Pearson <rpearson@hpe.com>

Bob Pearson (6):
  RDMA/rxe: Fix bug in rxe_alloc
  RDMA/rxe: Fix misleading comments and names
  RDMA/rxe: Remove RXE_POOL_ATOMIC
  RDMA/rxe: Remove references to ib_device and pool
  RDMA/rxe: Remove unneeded pool->state
  RDMA/rxe: Replace missing rxe_pool_get_index__

 drivers/infiniband/sw/rxe/rxe_mcast.c |   8 +-
 drivers/infiniband/sw/rxe/rxe_pool.c  | 132 +++++++++-----------------
 drivers/infiniband/sw/rxe/rxe_pool.h  |  57 ++++++-----
 3 files changed, 76 insertions(+), 121 deletions(-)

-- 
2.27.0

[PATCH for-next v2 1/6] RDMA/rxe: Fix bug in rxe_alloc

[Bob Pearson]

A recent patch which added an 'unlocked' version of rxe_alloc
introduced a bug causing kzalloc(..., GFP_KERNEL) to be called
while holding a spin lock. This patch corrects that error.

rxe_alloc_nl() should always be called while holding the pool->pool_lock
so the 2nd argument to kzalloc there should be GFP_ATOMIC.

rxe_alloc() prior to the change only locked the code around checking
that pool->state is RXE_POOL_STATE_VALID to avoid races between
working threads and a thread shutting down the rxe driver. This patch
reverts rxe_alloc() to this behavior so the lock is not held when
kzalloc() is called.

Reported-by: syzbot+ec2fd72374785d0e558e@syzkaller.appspotmail.com
Fixes: 3853c35e243d ("RDMA/rxe: Add unlocked versions of pool APIs")
Signed-off-by: Bob Pearson <rpearson@hpe.com>
---
 drivers/infiniband/sw/rxe/rxe_pool.c | 41 ++++++++++++++++++++++++----
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
index d26730eec720..cfcd55175572 100644
--- a/drivers/infiniband/sw/rxe/rxe_pool.c
+++ b/drivers/infiniband/sw/rxe/rxe_pool.c
@@ -343,8 +343,6 @@ void *rxe_alloc_nl(struct rxe_pool *pool)
 	struct rxe_pool_entry *elem;
 	u8 *obj;
 
-	might_sleep_if(!(pool->flags & RXE_POOL_ATOMIC));
-
 	if (pool->state != RXE_POOL_STATE_VALID)
 		return NULL;
 
@@ -356,8 +354,7 @@ void *rxe_alloc_nl(struct rxe_pool *pool)
 	if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
 		goto out_cnt;
 
-	obj = kzalloc(info->size, (pool->flags & RXE_POOL_ATOMIC) ?
-		      GFP_ATOMIC : GFP_KERNEL);
+	obj = kzalloc(info->size, GFP_ATOMIC);
 	if (!obj)
 		goto out_cnt;
 
@@ -378,14 +375,46 @@ void *rxe_alloc_nl(struct rxe_pool *pool)
 
 void *rxe_alloc(struct rxe_pool *pool)
 {
-	u8 *obj;
 	unsigned long flags;
+	struct rxe_type_info *info = &rxe_type_info[pool->type];
+	struct rxe_pool_entry *elem;
+	u8 *obj;
+
+	might_sleep_if(!(pool->flags & RXE_POOL_ATOMIC));
 
 	read_lock_irqsave(&pool->pool_lock, flags);
-	obj = rxe_alloc_nl(pool);
+	if (pool->state != RXE_POOL_STATE_VALID) {
+		read_unlock_irqrestore(&pool->pool_lock, flags);
+		return NULL;
+	}
+
+	kref_get(&pool->ref_cnt);
 	read_unlock_irqrestore(&pool->pool_lock, flags);
 
+	if (!ib_device_try_get(&pool->rxe->ib_dev))
+		goto out_put_pool;
+
+	if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
+		goto out_cnt;
+
+	obj = kzalloc(info->size, (pool->flags & RXE_POOL_ATOMIC) ?
+		      GFP_ATOMIC : GFP_KERNEL);
+	if (!obj)
+		goto out_cnt;
+
+	elem = (struct rxe_pool_entry *)(obj + info->elem_offset);
+
+	elem->pool = pool;
+	kref_init(&elem->ref_cnt);
+
 	return obj;
+
+out_cnt:
+	atomic_dec(&pool->num_elem);
+	ib_device_put(&pool->rxe->ib_dev);
+out_put_pool:
+	rxe_pool_put(pool);
+	return NULL;
 }
 
 int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_entry *elem)
-- 
2.27.0

[PATCH for-next v2 2/6] RDMA/rxe: Fix misleading comments and names

[Bob Pearson]

The names and comments of the 'unlocked' pool APIs are very
misleading and not what was intended. This patch replaces
'rxe_xxx_nl' with 'rxe_xxx__' with comments indicating that the
caller is expected to hold the rxe pool lock.

Reported-by: Hillf Danton <hdanton@sina.com>
Signed-off-by: Bob Pearson <rpearson@hpe.com>
---
 drivers/infiniband/sw/rxe/rxe_mcast.c |  8 ++--
 drivers/infiniband/sw/rxe/rxe_pool.c  | 22 +++++------
 drivers/infiniband/sw/rxe/rxe_pool.h  | 55 +++++++++++++--------------
 3 files changed, 42 insertions(+), 43 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_mcast.c b/drivers/infiniband/sw/rxe/rxe_mcast.c
index 5be47ce7d319..b9f06f87866e 100644
--- a/drivers/infiniband/sw/rxe/rxe_mcast.c
+++ b/drivers/infiniband/sw/rxe/rxe_mcast.c
@@ -15,18 +15,18 @@ static struct rxe_mc_grp *create_grp(struct rxe_dev *rxe,
 	int err;
 	struct rxe_mc_grp *grp;
 
-	grp = rxe_alloc_nl(&rxe->mc_grp_pool);
+	grp = rxe_alloc__(&rxe->mc_grp_pool);
 	if (!grp)
 		return ERR_PTR(-ENOMEM);
 
 	INIT_LIST_HEAD(&grp->qp_list);
 	spin_lock_init(&grp->mcg_lock);
 	grp->rxe = rxe;
-	rxe_add_key_nl(grp, mgid);
+	rxe_add_key__(grp, mgid);
 
 	err = rxe_mcast_add(rxe, mgid);
 	if (unlikely(err)) {
-		rxe_drop_key_nl(grp);
+		rxe_drop_key__(grp);
 		rxe_drop_ref(grp);
 		return ERR_PTR(err);
 	}
@@ -47,7 +47,7 @@ int rxe_mcast_get_grp(struct rxe_dev *rxe, union ib_gid *mgid,
 
 	write_lock_irqsave(&pool->pool_lock, flags);
 
-	grp = rxe_pool_get_key_nl(pool, mgid);
+	grp = rxe_pool_get_key__(pool, mgid);
 	if (grp)
 		goto done;
 
diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
index cfcd55175572..80deec1e2924 100644
--- a/drivers/infiniband/sw/rxe/rxe_pool.c
+++ b/drivers/infiniband/sw/rxe/rxe_pool.c
@@ -266,7 +266,7 @@ static void insert_key(struct rxe_pool *pool, struct rxe_pool_entry *new)
 	return;
 }
 
-void __rxe_add_key_nl(struct rxe_pool_entry *elem, void *key)
+void __rxe_add_key__(struct rxe_pool_entry *elem, void *key)
 {
 	struct rxe_pool *pool = elem->pool;
 
@@ -280,11 +280,11 @@ void __rxe_add_key(struct rxe_pool_entry *elem, void *key)
 	unsigned long flags;
 
 	write_lock_irqsave(&pool->pool_lock, flags);
-	__rxe_add_key_nl(elem, key);
+	__rxe_add_key__(elem, key);
 	write_unlock_irqrestore(&pool->pool_lock, flags);
 }
 
-void __rxe_drop_key_nl(struct rxe_pool_entry *elem)
+void __rxe_drop_key__(struct rxe_pool_entry *elem)
 {
 	struct rxe_pool *pool = elem->pool;
 
@@ -297,11 +297,11 @@ void __rxe_drop_key(struct rxe_pool_entry *elem)
 	unsigned long flags;
 
 	write_lock_irqsave(&pool->pool_lock, flags);
-	__rxe_drop_key_nl(elem);
+	__rxe_drop_key__(elem);
 	write_unlock_irqrestore(&pool->pool_lock, flags);
 }
 
-void __rxe_add_index_nl(struct rxe_pool_entry *elem)
+void __rxe_add_index__(struct rxe_pool_entry *elem)
 {
 	struct rxe_pool *pool = elem->pool;
 
@@ -315,11 +315,11 @@ void __rxe_add_index(struct rxe_pool_entry *elem)
 	unsigned long flags;
 
 	write_lock_irqsave(&pool->pool_lock, flags);
-	__rxe_add_index_nl(elem);
+	__rxe_add_index__(elem);
 	write_unlock_irqrestore(&pool->pool_lock, flags);
 }
 
-void __rxe_drop_index_nl(struct rxe_pool_entry *elem)
+void __rxe_drop_index__(struct rxe_pool_entry *elem)
 {
 	struct rxe_pool *pool = elem->pool;
 
@@ -333,11 +333,11 @@ void __rxe_drop_index(struct rxe_pool_entry *elem)
 	unsigned long flags;
 
 	write_lock_irqsave(&pool->pool_lock, flags);
-	__rxe_drop_index_nl(elem);
+	__rxe_drop_index__(elem);
 	write_unlock_irqrestore(&pool->pool_lock, flags);
 }
 
-void *rxe_alloc_nl(struct rxe_pool *pool)
+void *rxe_alloc__(struct rxe_pool *pool)
 {
 	struct rxe_type_info *info = &rxe_type_info[pool->type];
 	struct rxe_pool_entry *elem;
@@ -507,7 +507,7 @@ void *rxe_pool_get_index(struct rxe_pool *pool, u32 index)
 	return obj;
 }
 
-void *rxe_pool_get_key_nl(struct rxe_pool *pool, void *key)
+void *rxe_pool_get_key__(struct rxe_pool *pool, void *key)
 {
 	struct rxe_type_info *info = &rxe_type_info[pool->type];
 	struct rb_node *node;
@@ -551,7 +551,7 @@ void *rxe_pool_get_key(struct rxe_pool *pool, void *key)
 	unsigned long flags;
 
 	read_lock_irqsave(&pool->pool_lock, flags);
-	obj = rxe_pool_get_key_nl(pool, key);
+	obj = rxe_pool_get_key__(pool, key);
 	read_unlock_irqrestore(&pool->pool_lock, flags);
 
 	return obj;
diff --git a/drivers/infiniband/sw/rxe/rxe_pool.h b/drivers/infiniband/sw/rxe/rxe_pool.h
index 373e08554c1c..b9680f9e907c 100644
--- a/drivers/infiniband/sw/rxe/rxe_pool.h
+++ b/drivers/infiniband/sw/rxe/rxe_pool.h
@@ -106,11 +106,10 @@ int rxe_pool_init(struct rxe_dev *rxe, struct rxe_pool *pool,
 /* free resources from object pool */
 void rxe_pool_cleanup(struct rxe_pool *pool);
 
-/* allocate an object from pool */
-void *rxe_alloc(struct rxe_pool *pool);
+/* allocate an object from pool holding and not holding the pool lock */
+void *rxe_alloc__(struct rxe_pool *pool);
 
-/* allocate an object from pool - no lock */
-void *rxe_alloc_nl(struct rxe_pool *pool);
+void *rxe_alloc(struct rxe_pool *pool);
 
 /* connect already allocated object to pool */
 int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_entry *elem);
@@ -118,60 +117,60 @@ int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_entry *elem);
 #define rxe_add_to_pool(pool, obj) __rxe_add_to_pool(pool, &(obj)->pelem)
 
 /* assign an index to an indexed object and insert object into
- *  pool's rb tree with and without holding the pool_lock
+ *  pool's rb tree holding and not holding the pool_lock
  */
-void __rxe_add_index(struct rxe_pool_entry *elem);
+void __rxe_add_index__(struct rxe_pool_entry *elem);
 
-#define rxe_add_index(obj) __rxe_add_index(&(obj)->pelem)
+#define rxe_add_index__(obj) __rxe_add_index__(&(obj)->pelem)
 
-void __rxe_add_index_nl(struct rxe_pool_entry *elem);
+void __rxe_add_index(struct rxe_pool_entry *elem);
 
-#define rxe_add_index_nl(obj) __rxe_add_index_nl(&(obj)->pelem)
+#define rxe_add_index(obj) __rxe_add_index(&(obj)->pelem)
 
 /* drop an index and remove object from rb tree
- * with and without holding the pool_lock
+ * holding and not holding the pool_lock
  */
-void __rxe_drop_index(struct rxe_pool_entry *elem);
+void __rxe_drop_index__(struct rxe_pool_entry *elem);
 
-#define rxe_drop_index(obj) __rxe_drop_index(&(obj)->pelem)
+#define rxe_drop_index__(obj) __rxe_drop_index__(&(obj)->pelem)
 
-void __rxe_drop_index_nl(struct rxe_pool_entry *elem);
+void __rxe_drop_index(struct rxe_pool_entry *elem);
 
-#define rxe_drop_index_nl(obj) __rxe_drop_index_nl(&(obj)->pelem)
+#define rxe_drop_index(obj) __rxe_drop_index(&(obj)->pelem)
 
 /* assign a key to a keyed object and insert object into
- * pool's rb tree with and without holding pool_lock
+ * pool's rb tree holding and not holding pool_lock
  */
+void __rxe_add_key__(struct rxe_pool_entry *elem, void *key);
+
+#define rxe_add_key__(obj, key) __rxe_add_key__(&(obj)->pelem, key)
+
 void __rxe_add_key(struct rxe_pool_entry *elem, void *key);
 
 #define rxe_add_key(obj, key) __rxe_add_key(&(obj)->pelem, key)
 
-void __rxe_add_key_nl(struct rxe_pool_entry *elem, void *key);
+/* remove elem from rb tree holding and not holding the pool_lock */
+void __rxe_drop_key__(struct rxe_pool_entry *elem);
 
-#define rxe_add_key_nl(obj, key) __rxe_add_key_nl(&(obj)->pelem, key)
+#define rxe_drop_key__(obj) __rxe_drop_key__(&(obj)->pelem)
 
-/* remove elem from rb tree with and without holding pool_lock */
 void __rxe_drop_key(struct rxe_pool_entry *elem);
 
 #define rxe_drop_key(obj) __rxe_drop_key(&(obj)->pelem)
 
-void __rxe_drop_key_nl(struct rxe_pool_entry *elem);
-
-#define rxe_drop_key_nl(obj) __rxe_drop_key_nl(&(obj)->pelem)
-
-/* lookup an indexed object from index with and without holding pool_lock.
+/* lookup an indexed object from index holding and not holding the pool_lock.
  * takes a reference on object
  */
-void *rxe_pool_get_index(struct rxe_pool *pool, u32 index);
+void *rxe_pool_get_index__(struct rxe_pool *pool, u32 index);
 
-void *rxe_pool_get_index_nl(struct rxe_pool *pool, u32 index);
+void *rxe_pool_get_index(struct rxe_pool *pool, u32 index);
 
-/* lookup keyed object from key with and without holding pool_lock.
+/* lookup keyed object from key holding and not holding the pool_lock.
  * takes a reference on the objecti
  */
-void *rxe_pool_get_key(struct rxe_pool *pool, void *key);
+void *rxe_pool_get_key__(struct rxe_pool *pool, void *key);
 
-void *rxe_pool_get_key_nl(struct rxe_pool *pool, void *key);
+void *rxe_pool_get_key(struct rxe_pool *pool, void *key);
 
 /* cleanup an object when all references are dropped */
 void rxe_elem_release(struct kref *kref);
-- 
2.27.0

[PATCH for-next v2 3/6] RDMA/rxe: Remove RXE_POOL_ATOMIC

[Bob Pearson]

rxe_alloc() used the RXE_POOL_ATOMIC flag in rxe_type_info
to select GFP_ATOMIC in calls to kzalloc(). This was intended
to handle cases where an object could be created in interrupt
context. This no longer occurs since allocating those objects
has moved into the core so this flag is not necessary. An
incorrect use of this flag was still present for rxe_mc_elem
objects and is removed.

Signed-off-by: Bob Pearson <rpearson@hpe.com>
---
 drivers/infiniband/sw/rxe/rxe_pool.c | 6 +-----
 drivers/infiniband/sw/rxe/rxe_pool.h | 1 -
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
index 80deec1e2924..7e68f99558a7 100644
--- a/drivers/infiniband/sw/rxe/rxe_pool.c
+++ b/drivers/infiniband/sw/rxe/rxe_pool.c
@@ -84,7 +84,6 @@ struct rxe_type_info rxe_type_info[RXE_NUM_TYPES] = {
 		.name		= "rxe-mc_elem",
 		.size		= sizeof(struct rxe_mc_elem),
 		.elem_offset	= offsetof(struct rxe_mc_elem, pelem),
-		.flags		= RXE_POOL_ATOMIC,
 	},
 };
 
@@ -380,8 +379,6 @@ void *rxe_alloc(struct rxe_pool *pool)
 	struct rxe_pool_entry *elem;
 	u8 *obj;
 
-	might_sleep_if(!(pool->flags & RXE_POOL_ATOMIC));
-
 	read_lock_irqsave(&pool->pool_lock, flags);
 	if (pool->state != RXE_POOL_STATE_VALID) {
 		read_unlock_irqrestore(&pool->pool_lock, flags);
@@ -397,8 +394,7 @@ void *rxe_alloc(struct rxe_pool *pool)
 	if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
 		goto out_cnt;
 
-	obj = kzalloc(info->size, (pool->flags & RXE_POOL_ATOMIC) ?
-		      GFP_ATOMIC : GFP_KERNEL);
+	obj = kzalloc(info->size, GFP_KERNEL);
 	if (!obj)
 		goto out_cnt;
 
diff --git a/drivers/infiniband/sw/rxe/rxe_pool.h b/drivers/infiniband/sw/rxe/rxe_pool.h
index b9680f9e907c..f916ad4d0406 100644
--- a/drivers/infiniband/sw/rxe/rxe_pool.h
+++ b/drivers/infiniband/sw/rxe/rxe_pool.h
@@ -11,7 +11,6 @@
 #define RXE_POOL_CACHE_FLAGS	(0)
 
 enum rxe_pool_flags {
-	RXE_POOL_ATOMIC		= BIT(0),
 	RXE_POOL_INDEX		= BIT(1),
 	RXE_POOL_KEY		= BIT(2),
 	RXE_POOL_NO_ALLOC	= BIT(4),
-- 
2.27.0

[PATCH for-next v2 4/6] RDMA/rxe: Remove references to ib_device and pool

[Bob Pearson]

rxe_pool.c takes references to the pool and ib_device structs
for each object allocated and also keeps an atomic num_elem
count in each pool. This is more work than is needed. Pool
allocation is only called from verbs APIs which already have
references to ib_device and pools are only diasbled when the
driver is removed so no protection of the pool addresses
are needed. The elem count is used to warn if elements are
still present in a pool when it is cleaned up which is useful.

This patch eliminates the references to the ib_device and pool
structs.

The previous version only removed the ib_device reference.

Signed-off-by: Bob Pearson <rpearson@hpe.com>
---
 drivers/infiniband/sw/rxe/rxe_pool.c | 42 ++--------------------------
 drivers/infiniband/sw/rxe/rxe_pool.h |  1 -
 2 files changed, 2 insertions(+), 41 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
index 7e68f99558a7..09d8665c5343 100644
--- a/drivers/infiniband/sw/rxe/rxe_pool.c
+++ b/drivers/infiniband/sw/rxe/rxe_pool.c
@@ -142,8 +142,6 @@ int rxe_pool_init(
 
 	atomic_set(&pool->num_elem, 0);
 
-	kref_init(&pool->ref_cnt);
-
 	rwlock_init(&pool->pool_lock);
 
 	if (rxe_type_info[type].flags & RXE_POOL_INDEX) {
@@ -165,19 +163,6 @@ int rxe_pool_init(
 	return err;
 }
 
-static void rxe_pool_release(struct kref *kref)
-{
-	struct rxe_pool *pool = container_of(kref, struct rxe_pool, ref_cnt);
-
-	pool->state = RXE_POOL_STATE_INVALID;
-	kfree(pool->index.table);
-}
-
-static void rxe_pool_put(struct rxe_pool *pool)
-{
-	kref_put(&pool->ref_cnt, rxe_pool_release);
-}
-
 void rxe_pool_cleanup(struct rxe_pool *pool)
 {
 	unsigned long flags;
@@ -189,7 +174,8 @@ void rxe_pool_cleanup(struct rxe_pool *pool)
 			pool_name(pool));
 	write_unlock_irqrestore(&pool->pool_lock, flags);
 
-	rxe_pool_put(pool);
+	pool->state = RXE_POOL_STATE_INVALID;
+	kfree(pool->index.table);
 }
 
 static u32 alloc_index(struct rxe_pool *pool)
@@ -345,11 +331,6 @@ void *rxe_alloc__(struct rxe_pool *pool)
 	if (pool->state != RXE_POOL_STATE_VALID)
 		return NULL;
 
-	kref_get(&pool->ref_cnt);
-
-	if (!ib_device_try_get(&pool->rxe->ib_dev))
-		goto out_put_pool;
-
 	if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
 		goto out_cnt;
 
@@ -366,9 +347,6 @@ void *rxe_alloc__(struct rxe_pool *pool)
 
 out_cnt:
 	atomic_dec(&pool->num_elem);
-	ib_device_put(&pool->rxe->ib_dev);
-out_put_pool:
-	rxe_pool_put(pool);
 	return NULL;
 }
 
@@ -385,12 +363,8 @@ void *rxe_alloc(struct rxe_pool *pool)
 		return NULL;
 	}
 
-	kref_get(&pool->ref_cnt);
 	read_unlock_irqrestore(&pool->pool_lock, flags);
 
-	if (!ib_device_try_get(&pool->rxe->ib_dev))
-		goto out_put_pool;
-
 	if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
 		goto out_cnt;
 
@@ -407,9 +381,6 @@ void *rxe_alloc(struct rxe_pool *pool)
 
 out_cnt:
 	atomic_dec(&pool->num_elem);
-	ib_device_put(&pool->rxe->ib_dev);
-out_put_pool:
-	rxe_pool_put(pool);
 	return NULL;
 }
 
@@ -422,12 +393,8 @@ int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_entry *elem)
 		read_unlock_irqrestore(&pool->pool_lock, flags);
 		return -EINVAL;
 	}
-	kref_get(&pool->ref_cnt);
 	read_unlock_irqrestore(&pool->pool_lock, flags);
 
-	if (!ib_device_try_get(&pool->rxe->ib_dev))
-		goto out_put_pool;
-
 	if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
 		goto out_cnt;
 
@@ -438,9 +405,6 @@ int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_entry *elem)
 
 out_cnt:
 	atomic_dec(&pool->num_elem);
-	ib_device_put(&pool->rxe->ib_dev);
-out_put_pool:
-	rxe_pool_put(pool);
 	return -EINVAL;
 }
 
@@ -461,8 +425,6 @@ void rxe_elem_release(struct kref *kref)
 	}
 
 	atomic_dec(&pool->num_elem);
-	ib_device_put(&pool->rxe->ib_dev);
-	rxe_pool_put(pool);
 }
 
 void *rxe_pool_get_index(struct rxe_pool *pool, u32 index)
diff --git a/drivers/infiniband/sw/rxe/rxe_pool.h b/drivers/infiniband/sw/rxe/rxe_pool.h
index f916ad4d0406..294839d1eed8 100644
--- a/drivers/infiniband/sw/rxe/rxe_pool.h
+++ b/drivers/infiniband/sw/rxe/rxe_pool.h
@@ -68,7 +68,6 @@ struct rxe_pool {
 	struct rxe_dev		*rxe;
 	rwlock_t		pool_lock; /* protects pool add/del/search */
 	size_t			elem_size;
-	struct kref		ref_cnt;
 	void			(*cleanup)(struct rxe_pool_entry *obj);
 	enum rxe_pool_state	state;
 	enum rxe_pool_flags	flags;
-- 
2.27.0

[PATCH for-next v2 5/6] RDMA/rxe: Remove unneeded pool->state

[Bob Pearson]

rxe_pool.c uses the field pool->state to mark a pool as invalid
when it is shut down and checks it in several pool APIs to verify
that the pool has not been shut dowm. This is unneeded because the
pools are not marked invalid unless the entire driver is being
removed at which point no functional APIs should or could be
executing. This patch removes this field and associated code.

Signed-off-by: Bob Pearson <rpearson@hpe.com>
---
 drivers/infiniband/sw/rxe/rxe_pool.c | 38 +---------------------------
 1 file changed, 1 insertion(+), 37 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
index 09d8665c5343..7a03d49b263d 100644
--- a/drivers/infiniband/sw/rxe/rxe_pool.c
+++ b/drivers/infiniband/sw/rxe/rxe_pool.c
@@ -157,24 +157,16 @@ int rxe_pool_init(
 		pool->key.key_size = rxe_type_info[type].key_size;
 	}
 
-	pool->state = RXE_POOL_STATE_VALID;
-
 out:
 	return err;
 }
 
 void rxe_pool_cleanup(struct rxe_pool *pool)
 {
-	unsigned long flags;
-
-	write_lock_irqsave(&pool->pool_lock, flags);
-	pool->state = RXE_POOL_STATE_INVALID;
 	if (atomic_read(&pool->num_elem) > 0)
 		pr_warn("%s pool destroyed with unfree'd elem\n",
 			pool_name(pool));
-	write_unlock_irqrestore(&pool->pool_lock, flags);
 
-	pool->state = RXE_POOL_STATE_INVALID;
 	kfree(pool->index.table);
 }
 
@@ -328,9 +320,6 @@ void *rxe_alloc__(struct rxe_pool *pool)
 	struct rxe_pool_entry *elem;
 	u8 *obj;
 
-	if (pool->state != RXE_POOL_STATE_VALID)
-		return NULL;
-
 	if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
 		goto out_cnt;
 
@@ -352,19 +341,10 @@ void *rxe_alloc__(struct rxe_pool *pool)
 
 void *rxe_alloc(struct rxe_pool *pool)
 {
-	unsigned long flags;
 	struct rxe_type_info *info = &rxe_type_info[pool->type];
 	struct rxe_pool_entry *elem;
 	u8 *obj;
 
-	read_lock_irqsave(&pool->pool_lock, flags);
-	if (pool->state != RXE_POOL_STATE_VALID) {
-		read_unlock_irqrestore(&pool->pool_lock, flags);
-		return NULL;
-	}
-
-	read_unlock_irqrestore(&pool->pool_lock, flags);
-
 	if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
 		goto out_cnt;
 
@@ -386,15 +366,6 @@ void *rxe_alloc(struct rxe_pool *pool)
 
 int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_entry *elem)
 {
-	unsigned long flags;
-
-	read_lock_irqsave(&pool->pool_lock, flags);
-	if (pool->state != RXE_POOL_STATE_VALID) {
-		read_unlock_irqrestore(&pool->pool_lock, flags);
-		return -EINVAL;
-	}
-	read_unlock_irqrestore(&pool->pool_lock, flags);
-
 	if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
 		goto out_cnt;
 
@@ -437,9 +408,6 @@ void *rxe_pool_get_index(struct rxe_pool *pool, u32 index)
 
 	read_lock_irqsave(&pool->pool_lock, flags);
 
-	if (pool->state != RXE_POOL_STATE_VALID)
-		goto out;
-
 	node = pool->index.tree.rb_node;
 
 	while (node) {
@@ -460,8 +428,8 @@ void *rxe_pool_get_index(struct rxe_pool *pool, u32 index)
 		obj = NULL;
 	}
 
-out:
 	read_unlock_irqrestore(&pool->pool_lock, flags);
+
 	return obj;
 }
 
@@ -473,9 +441,6 @@ void *rxe_pool_get_key__(struct rxe_pool *pool, void *key)
 	u8 *obj = NULL;
 	int cmp;
 
-	if (pool->state != RXE_POOL_STATE_VALID)
-		goto out;
-
 	node = pool->key.tree.rb_node;
 
 	while (node) {
@@ -499,7 +464,6 @@ void *rxe_pool_get_key__(struct rxe_pool *pool, void *key)
 		obj = NULL;
 	}
 
-out:
 	return obj;
 }
 
-- 
2.27.0

[PATCH for-next v2 6/6] RDMA/rxe: Replace missing rxe_pool_get_index__

[Bob Pearson]

One of the pool APIs for when caller is holding lock was not defined
but is declared in rxe_pool.h. This patch adds the definition.

Signed-off-by: Bob Pearson <rpearson@hpe.com>
---
 drivers/infiniband/sw/rxe/rxe_pool.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
index 7a03d49b263d..3755e163f257 100644
--- a/drivers/infiniband/sw/rxe/rxe_pool.c
+++ b/drivers/infiniband/sw/rxe/rxe_pool.c
@@ -398,15 +398,12 @@ void rxe_elem_release(struct kref *kref)
 	atomic_dec(&pool->num_elem);
 }
 
-void *rxe_pool_get_index(struct rxe_pool *pool, u32 index)
+void *rxe_pool_get_index__(struct rxe_pool *pool, u32 index)
 {
 	struct rxe_type_info *info = &rxe_type_info[pool->type];
 	struct rb_node *node;
 	struct rxe_pool_entry *elem;
-	u8 *obj = NULL;
-	unsigned long flags;
-
-	read_lock_irqsave(&pool->pool_lock, flags);
+	u8 *obj;
 
 	node = pool->index.tree.rb_node;
 
@@ -428,6 +425,16 @@ void *rxe_pool_get_index(struct rxe_pool *pool, u32 index)
 		obj = NULL;
 	}
 
+	return obj;
+}
+
+void *rxe_pool_get_index(struct rxe_pool *pool, u32 index)
+{
+	u8 *obj;
+	unsigned long flags;
+
+	read_lock_irqsave(&pool->pool_lock, flags);
+	obj = rxe_pool_get_index__(pool, index);
 	read_unlock_irqrestore(&pool->pool_lock, flags);
 
 	return obj;
@@ -438,7 +445,7 @@ void *rxe_pool_get_key__(struct rxe_pool *pool, void *key)
 	struct rxe_type_info *info = &rxe_type_info[pool->type];
 	struct rb_node *node;
 	struct rxe_pool_entry *elem;
-	u8 *obj = NULL;
+	u8 *obj;
 	int cmp;
 
 	node = pool->key.tree.rb_node;
@@ -469,7 +476,7 @@ void *rxe_pool_get_key__(struct rxe_pool *pool, void *key)
 
 void *rxe_pool_get_key(struct rxe_pool *pool, void *key)
 {
-	u8 *obj = NULL;
+	u8 *obj;
 	unsigned long flags;
 
 	read_lock_irqsave(&pool->pool_lock, flags);
-- 
2.27.0

Re: [PATCH for-next v2 5/6] RDMA/rxe: Remove unneeded pool->state

[Zhu Yanjun]

On Sat, Jan 23, 2021 at 3:30 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>
> rxe_pool.c uses the field pool->state to mark a pool as invalid
> when it is shut down and checks it in several pool APIs to verify
> that the pool has not been shut dowm.

s/dowm/down

 This is unneeded because the
> pools are not marked invalid unless the entire driver is being
> removed at which point no functional APIs should or could be
> executing. This patch removes this field and associated code.
>
> Signed-off-by: Bob Pearson <rpearson@hpe.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_pool.c | 38 +---------------------------
>  1 file changed, 1 insertion(+), 37 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
> index 09d8665c5343..7a03d49b263d 100644
> --- a/drivers/infiniband/sw/rxe/rxe_pool.c
> +++ b/drivers/infiniband/sw/rxe/rxe_pool.c
> @@ -157,24 +157,16 @@ int rxe_pool_init(
>                 pool->key.key_size = rxe_type_info[type].key_size;
>         }
>
> -       pool->state = RXE_POOL_STATE_VALID;
> -
>  out:
>         return err;
>  }
>
>  void rxe_pool_cleanup(struct rxe_pool *pool)
>  {
> -       unsigned long flags;
> -
> -       write_lock_irqsave(&pool->pool_lock, flags);
> -       pool->state = RXE_POOL_STATE_INVALID;
>         if (atomic_read(&pool->num_elem) > 0)
>                 pr_warn("%s pool destroyed with unfree'd elem\n",
>                         pool_name(pool));
> -       write_unlock_irqrestore(&pool->pool_lock, flags);
>
> -       pool->state = RXE_POOL_STATE_INVALID;
>         kfree(pool->index.table);
>  }
>
> @@ -328,9 +320,6 @@ void *rxe_alloc__(struct rxe_pool *pool)
>         struct rxe_pool_entry *elem;
>         u8 *obj;
>
> -       if (pool->state != RXE_POOL_STATE_VALID)
> -               return NULL;
> -
>         if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
>                 goto out_cnt;
>
> @@ -352,19 +341,10 @@ void *rxe_alloc__(struct rxe_pool *pool)
>
>  void *rxe_alloc(struct rxe_pool *pool)
>  {
> -       unsigned long flags;
>         struct rxe_type_info *info = &rxe_type_info[pool->type];
>         struct rxe_pool_entry *elem;
>         u8 *obj;
>
> -       read_lock_irqsave(&pool->pool_lock, flags);
> -       if (pool->state != RXE_POOL_STATE_VALID) {
> -               read_unlock_irqrestore(&pool->pool_lock, flags);
> -               return NULL;
> -       }
> -
> -       read_unlock_irqrestore(&pool->pool_lock, flags);
> -
>         if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
>                 goto out_cnt;
>
> @@ -386,15 +366,6 @@ void *rxe_alloc(struct rxe_pool *pool)
>
>  int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_entry *elem)
>  {
> -       unsigned long flags;
> -
> -       read_lock_irqsave(&pool->pool_lock, flags);
> -       if (pool->state != RXE_POOL_STATE_VALID) {
> -               read_unlock_irqrestore(&pool->pool_lock, flags);
> -               return -EINVAL;
> -       }
> -       read_unlock_irqrestore(&pool->pool_lock, flags);
> -
>         if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
>                 goto out_cnt;
>
> @@ -437,9 +408,6 @@ void *rxe_pool_get_index(struct rxe_pool *pool, u32 index)
>
>         read_lock_irqsave(&pool->pool_lock, flags);
>
> -       if (pool->state != RXE_POOL_STATE_VALID)
> -               goto out;
> -
>         node = pool->index.tree.rb_node;
>
>         while (node) {
> @@ -460,8 +428,8 @@ void *rxe_pool_get_index(struct rxe_pool *pool, u32 index)
>                 obj = NULL;
>         }
>
> -out:
>         read_unlock_irqrestore(&pool->pool_lock, flags);
> +
>         return obj;
>  }
>
> @@ -473,9 +441,6 @@ void *rxe_pool_get_key__(struct rxe_pool *pool, void *key)
>         u8 *obj = NULL;
>         int cmp;
>
> -       if (pool->state != RXE_POOL_STATE_VALID)
> -               goto out;
> -
>         node = pool->key.tree.rb_node;
>
>         while (node) {
> @@ -499,7 +464,6 @@ void *rxe_pool_get_key__(struct rxe_pool *pool, void *key)
>                 obj = NULL;
>         }
>
> -out:
>         return obj;
>  }
>
> --
> 2.27.0
>

Re: [PATCH for-next v2 4/6] RDMA/rxe: Remove references to ib_device and pool

[Zhu Yanjun]

On Sat, Jan 23, 2021 at 3:30 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>
> rxe_pool.c takes references to the pool and ib_device structs
> for each object allocated and also keeps an atomic num_elem
> count in each pool. This is more work than is needed. Pool
> allocation is only called from verbs APIs which already have
> references to ib_device and pools are only diasbled when the
> driver is removed so no protection of the pool addresses
> are needed. The elem count is used to warn if elements are
> still present in a pool when it is cleaned up which is useful.
>
> This patch eliminates the references to the ib_device and pool
> structs.
>
> The previous version only removed the ib_device reference.
>
> Signed-off-by: Bob Pearson <rpearson@hpe.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_pool.c | 42 ++--------------------------
>  drivers/infiniband/sw/rxe/rxe_pool.h |  1 -
>  2 files changed, 2 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
> index 7e68f99558a7..09d8665c5343 100644
> --- a/drivers/infiniband/sw/rxe/rxe_pool.c
> +++ b/drivers/infiniband/sw/rxe/rxe_pool.c
> @@ -142,8 +142,6 @@ int rxe_pool_init(
>
>         atomic_set(&pool->num_elem, 0);
>
> -       kref_init(&pool->ref_cnt);

The pool->ref_cnt is useful to the statistics of usage of pool.
Not sure if it is good to remove it.

> -
>         rwlock_init(&pool->pool_lock);
>
>         if (rxe_type_info[type].flags & RXE_POOL_INDEX) {
> @@ -165,19 +163,6 @@ int rxe_pool_init(
>         return err;
>  }
>
> -static void rxe_pool_release(struct kref *kref)
> -{
> -       struct rxe_pool *pool = container_of(kref, struct rxe_pool, ref_cnt);
> -
> -       pool->state = RXE_POOL_STATE_INVALID;
> -       kfree(pool->index.table);
> -}
> -
> -static void rxe_pool_put(struct rxe_pool *pool)
> -{
> -       kref_put(&pool->ref_cnt, rxe_pool_release);
> -}
> -
>  void rxe_pool_cleanup(struct rxe_pool *pool)
>  {
>         unsigned long flags;
> @@ -189,7 +174,8 @@ void rxe_pool_cleanup(struct rxe_pool *pool)
>                         pool_name(pool));
>         write_unlock_irqrestore(&pool->pool_lock, flags);
>
> -       rxe_pool_put(pool);
> +       pool->state = RXE_POOL_STATE_INVALID;
> +       kfree(pool->index.table);
>  }
>
>  static u32 alloc_index(struct rxe_pool *pool)
> @@ -345,11 +331,6 @@ void *rxe_alloc__(struct rxe_pool *pool)
>         if (pool->state != RXE_POOL_STATE_VALID)
>                 return NULL;
>
> -       kref_get(&pool->ref_cnt);
> -
> -       if (!ib_device_try_get(&pool->rxe->ib_dev))
> -               goto out_put_pool;
> -
>         if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
>                 goto out_cnt;
>
> @@ -366,9 +347,6 @@ void *rxe_alloc__(struct rxe_pool *pool)
>
>  out_cnt:
>         atomic_dec(&pool->num_elem);
> -       ib_device_put(&pool->rxe->ib_dev);
> -out_put_pool:
> -       rxe_pool_put(pool);
>         return NULL;
>  }
>
> @@ -385,12 +363,8 @@ void *rxe_alloc(struct rxe_pool *pool)
>                 return NULL;
>         }
>
> -       kref_get(&pool->ref_cnt);
>         read_unlock_irqrestore(&pool->pool_lock, flags);
>
> -       if (!ib_device_try_get(&pool->rxe->ib_dev))
> -               goto out_put_pool;
> -
>         if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
>                 goto out_cnt;
>
> @@ -407,9 +381,6 @@ void *rxe_alloc(struct rxe_pool *pool)
>
>  out_cnt:
>         atomic_dec(&pool->num_elem);
> -       ib_device_put(&pool->rxe->ib_dev);
> -out_put_pool:
> -       rxe_pool_put(pool);
>         return NULL;
>  }
>
> @@ -422,12 +393,8 @@ int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_entry *elem)
>                 read_unlock_irqrestore(&pool->pool_lock, flags);
>                 return -EINVAL;
>         }
> -       kref_get(&pool->ref_cnt);
>         read_unlock_irqrestore(&pool->pool_lock, flags);
>
> -       if (!ib_device_try_get(&pool->rxe->ib_dev))
> -               goto out_put_pool;
> -
>         if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
>                 goto out_cnt;
>
> @@ -438,9 +405,6 @@ int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_entry *elem)
>
>  out_cnt:
>         atomic_dec(&pool->num_elem);
> -       ib_device_put(&pool->rxe->ib_dev);
> -out_put_pool:
> -       rxe_pool_put(pool);
>         return -EINVAL;
>  }
>
> @@ -461,8 +425,6 @@ void rxe_elem_release(struct kref *kref)
>         }
>
>         atomic_dec(&pool->num_elem);
> -       ib_device_put(&pool->rxe->ib_dev);
> -       rxe_pool_put(pool);
>  }
>
>  void *rxe_pool_get_index(struct rxe_pool *pool, u32 index)
> diff --git a/drivers/infiniband/sw/rxe/rxe_pool.h b/drivers/infiniband/sw/rxe/rxe_pool.h
> index f916ad4d0406..294839d1eed8 100644
> --- a/drivers/infiniband/sw/rxe/rxe_pool.h
> +++ b/drivers/infiniband/sw/rxe/rxe_pool.h
> @@ -68,7 +68,6 @@ struct rxe_pool {
>         struct rxe_dev          *rxe;
>         rwlock_t                pool_lock; /* protects pool add/del/search */
>         size_t                  elem_size;
> -       struct kref             ref_cnt;
>         void                    (*cleanup)(struct rxe_pool_entry *obj);
>         enum rxe_pool_state     state;
>         enum rxe_pool_flags     flags;
> --
> 2.27.0
>