diff mbox

net/mlx4: use rb_entry()

Message ID 6c1c772de8f70113580dade04f89a377174d8c88.1484817025.git.geliangtang@gmail.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Geliang Tang Jan. 20, 2017, 2:36 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>
---
 drivers/net/ethernet/mellanox/mlx4/resource_tracker.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Leon Romanovsky Jan. 22, 2017, 7:48 a.m. UTC | #1
On Fri, Jan 20, 2017 at 10:36:57PM +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>
> ---
>  drivers/net/ethernet/mellanox/mlx4/resource_tracker.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

I don't understand completely the rationale behind this conversion.
rb_entry == container_of, why do we need another name for it?

>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
> index 1822382..6da6e01 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
> @@ -236,8 +236,8 @@ static void *res_tracker_lookup(struct rb_root *root, u64 res_id)
>  	struct rb_node *node = root->rb_node;
>
>  	while (node) {
> -		struct res_common *res = container_of(node, struct res_common,
> -						      node);
> +		struct res_common *res = rb_entry(node, struct res_common,
> +						  node);
>
>  		if (res_id < res->res_id)
>  			node = node->rb_left;
> @@ -255,8 +255,8 @@ static int res_tracker_insert(struct rb_root *root, struct res_common *res)
>
>  	/* Figure out where to put new node */
>  	while (*new) {
> -		struct res_common *this = container_of(*new, struct res_common,
> -						       node);
> +		struct res_common *this = rb_entry(*new, struct res_common,
> +						   node);
>
>  		parent = *new;
>  		if (res->res_id < this->res_id)
> --
> 2.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geliang Tang Jan. 22, 2017, 2:42 p.m. UTC | #2
On Sun, Jan 22, 2017 at 09:48:39AM +0200, Leon Romanovsky wrote:
> On Fri, Jan 20, 2017 at 10:36:57PM +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>
> > ---
> >  drivers/net/ethernet/mellanox/mlx4/resource_tracker.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> I don't understand completely the rationale behind this conversion.
> rb_entry == container_of, why do we need another name for it?
> 

There are several *_entry macros which are defined in kernel data
structures, like list_entry, hlist_entry, rb_entry, etc. Each of them is
just another name for container_of. We use different *_entry so that we
could identify the specific type of data structure that we are dealing
with.

-Geliang
Leon Romanovsky Jan. 22, 2017, 7:05 p.m. UTC | #3
On Sun, Jan 22, 2017 at 10:42:25PM +0800, Geliang Tang wrote:
> On Sun, Jan 22, 2017 at 09:48:39AM +0200, Leon Romanovsky wrote:
> > On Fri, Jan 20, 2017 at 10:36:57PM +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>
> > > ---
> > >  drivers/net/ethernet/mellanox/mlx4/resource_tracker.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > I don't understand completely the rationale behind this conversion.
> > rb_entry == container_of, why do we need another name for it?
> >
>
> There are several *_entry macros which are defined in kernel data
> structures, like list_entry, hlist_entry, rb_entry, etc. Each of them is
> just another name for container_of. We use different *_entry so that we
> could identify the specific type of data structure that we are dealing
> with.

Your proposed patch doesn't support the importance of such knowledge for
rb_entry. The list_entry case is totally different, because you perform
operation on it.

Anyway, It doesn't matter.
Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
David Miller Jan. 22, 2017, 9:48 p.m. UTC | #4
From: Geliang Tang <geliangtang@gmail.com>
Date: Fri, 20 Jan 2017 22:36:57 +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.
David Miller Jan. 22, 2017, 9:48 p.m. UTC | #5
From: Leon Romanovsky <leon@kernel.org>
Date: Sun, 22 Jan 2017 09:48:39 +0200

> I don't understand completely the rationale behind this conversion.
> rb_entry == container_of, why do we need another name for it?

Because it's an annotation.

Either you agree that the macro exists and it should be used in
every spot where those types are being used, or you don't and
therefore argue for the macro and it's usage completely.
diff mbox

Patch

diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
index 1822382..6da6e01 100644
--- a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
+++ b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
@@ -236,8 +236,8 @@  static void *res_tracker_lookup(struct rb_root *root, u64 res_id)
 	struct rb_node *node = root->rb_node;
 
 	while (node) {
-		struct res_common *res = container_of(node, struct res_common,
-						      node);
+		struct res_common *res = rb_entry(node, struct res_common,
+						  node);
 
 		if (res_id < res->res_id)
 			node = node->rb_left;
@@ -255,8 +255,8 @@  static int res_tracker_insert(struct rb_root *root, struct res_common *res)
 
 	/* Figure out where to put new node */
 	while (*new) {
-		struct res_common *this = container_of(*new, struct res_common,
-						       node);
+		struct res_common *this = rb_entry(*new, struct res_common,
+						   node);
 
 		parent = *new;
 		if (res->res_id < this->res_id)