diff mbox

RDS: use rb_entry()

Message ID 2cd84448fe04ffb7023be892c5ed04bbfc759c87.1482204342.git.geliangtang@gmail.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Geliang Tang Dec. 20, 2016, 2:02 p.m. UTC
To make the code clearer, use rb_entry() instead of container_of() to
deal with rbtree.

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 net/rds/rdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Leon Romanovsky Dec. 20, 2016, 2:20 p.m. UTC | #1
On Tue, Dec 20, 2016 at 10:02:18PM +0800, Geliang Tang wrote:
> To make the code clearer, use rb_entry() instead of container_of() to
> deal with rbtree.
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
>  net/rds/rdma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks,
Reviewed-by: Leon Romanovsky <leonro@mellanox.com>

>
> diff --git a/net/rds/rdma.c b/net/rds/rdma.c
> index 4c93bad..ea96114 100644
> --- a/net/rds/rdma.c
> +++ b/net/rds/rdma.c
> @@ -135,7 +135,7 @@ void rds_rdma_drop_keys(struct rds_sock *rs)
>  	/* Release any MRs associated with this socket */
>  	spin_lock_irqsave(&rs->rs_rdma_lock, flags);
>  	while ((node = rb_first(&rs->rs_rdma_keys))) {
> -		mr = container_of(node, struct rds_mr, r_rb_node);
> +		mr = rb_entry(node, struct rds_mr, r_rb_node);
>  		if (mr->r_trans == rs->rs_transport)
>  			mr->r_invalidate = 0;
>  		rb_erase(&mr->r_rb_node, &rs->rs_rdma_keys);
> --
> 2.9.3
>
Santosh Shilimkar Dec. 20, 2016, 4:34 p.m. UTC | #2
On 12/20/2016 6:02 AM, Geliang Tang wrote:
> To make the code clearer, use rb_entry() instead of container_of() to
> deal with rbtree.
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
Looks fine.
Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>
David Miller Dec. 20, 2016, 7:23 p.m. UTC | #3
From: Geliang Tang <geliangtang@gmail.com>
Date: Tue, 20 Dec 2016 22:02:18 +0800

> To make the code clearer, use rb_entry() instead of container_of() to
> deal with rbtree.
> 
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>

Applied.
Doug Ledford Dec. 22, 2016, 3:33 a.m. UTC | #4
On 12/20/2016 9:02 AM, Geliang Tang wrote:
> To make the code clearer, use rb_entry() instead of container_of() to
> deal with rbtree.
> 
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
>  net/rds/rdma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/rds/rdma.c b/net/rds/rdma.c
> index 4c93bad..ea96114 100644
> --- a/net/rds/rdma.c
> +++ b/net/rds/rdma.c
> @@ -135,7 +135,7 @@ void rds_rdma_drop_keys(struct rds_sock *rs)
>  	/* Release any MRs associated with this socket */
>  	spin_lock_irqsave(&rs->rs_rdma_lock, flags);
>  	while ((node = rb_first(&rs->rs_rdma_keys))) {
> -		mr = container_of(node, struct rds_mr, r_rb_node);
> +		mr = rb_entry(node, struct rds_mr, r_rb_node);
>  		if (mr->r_trans == rs->rs_transport)
>  			mr->r_invalidate = 0;
>  		rb_erase(&mr->r_rb_node, &rs->rs_rdma_keys);
> 

Dave, I know you already took this, but am I the only one that thinks
these patches are a step backwards?  They claim to promote readability,
but I disagree that they actually do so.  The original code used the
container_of() API with three specific arguments that made sense in the
context of a function named container_of().  The new API uses the exact
same three arguments, but they no longer make the same sense just
comparing the arguments to the function name.  The relationship has been
lost.  And on top of that, if you do this for all of the standard things
in the kernel (rb_entry, list_item, etc.), then you've created a myriad
of APIs that all duplicate one functional API that made sense.  Is it
really an improvement to go from one generic function that makes sense
and works everywhere to multiple implementations of basically just name
wrappers that mean you now need to know many aliases for the same
function?  How do we justify API bloat like this as better or easier to
read when it requires useless API memorization?
diff mbox

Patch

diff --git a/net/rds/rdma.c b/net/rds/rdma.c
index 4c93bad..ea96114 100644
--- a/net/rds/rdma.c
+++ b/net/rds/rdma.c
@@ -135,7 +135,7 @@  void rds_rdma_drop_keys(struct rds_sock *rs)
 	/* Release any MRs associated with this socket */
 	spin_lock_irqsave(&rs->rs_rdma_lock, flags);
 	while ((node = rb_first(&rs->rs_rdma_keys))) {
-		mr = container_of(node, struct rds_mr, r_rb_node);
+		mr = rb_entry(node, struct rds_mr, r_rb_node);
 		if (mr->r_trans == rs->rs_transport)
 			mr->r_invalidate = 0;
 		rb_erase(&mr->r_rb_node, &rs->rs_rdma_keys);