diff mbox series

[for-next,2/5] RDMA/rxe: Fix misleading comments and names

Message ID 20210122042313.3336-3-rpearson@hpe.com (mailing list archive)
State Superseded
Headers show
Series RDMA/rxe: Fix rxe_alloc() bug | expand

Commit Message

Bob Pearson Jan. 22, 2021, 4:23 a.m. UTC 
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(-)

Comments

Jason Gunthorpe Jan. 22, 2021, 7:27 p.m. UTC | #1 
On Thu, Jan 21, 2021 at 10:23:10PM -0600, Bob Pearson wrote:
> 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);

Everything else seems fine, but this trailing __ is too weird

If a lock is supposed to be held then name it foo_locked() or locked_()

If it auto-locks then name it foo()

If there is some #define wrapper then it is 

#define foo() __foo()

Jason
Pearson, Robert B Jan. 22, 2021, 9:48 p.m. UTC | #2 
-----Original Message-----
From: Jason Gunthorpe <jgg@nvidia.com> 
Sent: Friday, January 22, 2021 1:27 PM
To: Bob Pearson <rpearsonhpe@gmail.com>
Cc: zyjzyj2000@gmail.com; linux-rdma@vger.kernel.org; Pearson, Robert B <robert.pearson2@hpe.com>; Hillf Danton <hdanton@sina.com>
Subject: Re: [PATCH for-next 2/5] RDMA/rxe: Fix misleading comments and names

On Thu, Jan 21, 2021 at 10:23:10PM -0600, Bob Pearson wrote:
> 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);

Everything else seems fine, but this trailing __ is too weird

If a lock is supposed to be held then name it foo_locked() or locked_()

If it auto-locks then name it foo()

If there is some #define wrapper then it is 

#define foo() __foo()

Jason


I have both in some cases. A #define and a locked version in some cases. I'll try xxx_locked().
To be clear that means caller should hold some lock, yes?

bob
Jason Gunthorpe Jan. 25, 2021, 3:15 p.m. UTC | #3 
On Fri, Jan 22, 2021 at 09:48:21PM +0000, Pearson, Robert B wrote:

> I have both in some cases. A #define and a locked version in some
> cases. I'll try xxx_locked().  To be clear that means caller should
> hold some lock, yes?

Yes

Jason
diff mbox series

Patch

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);