net/mlx5e: Use refcount_t for refcount

refcount_t is better for reference counters since its implementation can prevent overflows.
So convert atomic_t ref counters to refcount_t.

Signed-off-by Chuhong Yuan <[email protected]>

---
Changes in v2:
- Add #include.

 drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c +5 -5

index b9d4f4e19ff9..148b55c3db7a 100644
@@ -32,6 +32,7 @@
 
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/refcount.h>
 #include <linux/mlx5/driver.h>
 #include <net/vxlan.h>
 #include "mlx5_core.h"

@@ -48,7 +49,7 @@ struct mlx5_vxlan {
 
 struct mlx5_vxlan_port {
 	struct hlist_node hlist;
-	atomic_t refcount;
+	refcount_t refcount;
 	u16 udp_port;
 };
 

@@ -113,7 +114,7 @@ int mlx5_vxlan_add_port(struct mlx5_vxlan *vxlan, u16 port)
 
 	vxlanp = mlx5_vxlan_lookup_port(vxlan, port);
 	if (vxlanp) {
-		atomic_inc(&vxlanp->refcount);
+		refcount_inc(&vxlanp->refcount);
 		return 0;
 	}
 

@@ -137,7 +138,7 @@ int mlx5_vxlan_add_port(struct mlx5_vxlan *vxlan, u16 port)
 	}
 
 	vxlanp->udp_port = port;
-	atomic_set(&vxlanp->refcount, 1);
+	refcount_set(&vxlanp->refcount, 1);
 
 	spin_lock_bh(&vxlan->lock);
 	hash_add(vxlan->htable, &vxlanp->hlist, port);

@@ -170,7 +171,7 @@ int mlx5_vxlan_del_port(struct mlx5_vxlan *vxlan, u16 port)
 		goto out_unlock;
 	}
 
-	if (atomic_dec_and_test(&vxlanp->refcount)) {
+	if (refcount_dec_and_test(&vxlanp->refcount)) {
 		hash_del(&vxlanp->hlist);
 		remove = true;
 	}
--

2.20.1

Quoted 3 lines · Chuhong Yuan

refcount_t is better for reference counters since its implementation can prevent overflows.
So convert atomic_t ref counters to refcount_t.

I'm not thrilled to see those automatic conversion patches, especially for flows which can't overflow. There is nothing wrong in using atomic_t type of variable, do you have in mind flow which will cause to overflow?

Thanks

Quoted 60 lines

Signed-off-by Chuhong Yuan <[email protected]>

---
Changes in v2:
- Add #include.

 drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c +5 -5

index b9d4f4e19ff9..148b55c3db7a 100644
@@ -32,6 +32,7 @@
 
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/refcount.h>
 #include <linux/mlx5/driver.h>
 #include <net/vxlan.h>
 #include "mlx5_core.h"

@@ -48,7 +49,7 @@ struct mlx5_vxlan {
 
 struct mlx5_vxlan_port {
      struct hlist_node hlist;
-     atomic_t refcount;
+     refcount_t refcount;
      u16 udp_port;
 };
 

@@ -113,7 +114,7 @@ int mlx5_vxlan_add_port(struct mlx5_vxlan *vxlan, u16 port)
 
      vxlanp = mlx5_vxlan_lookup_port(vxlan, port);
      if (vxlanp) {
-             atomic_inc(&vxlanp->refcount);
+             refcount_inc(&vxlanp->refcount);
              return 0;
      }
 

@@ -137,7 +138,7 @@ int mlx5_vxlan_add_port(struct mlx5_vxlan *vxlan, u16 port)
      }
 
      vxlanp->udp_port = port;
-     atomic_set(&vxlanp->refcount, 1);
+     refcount_set(&vxlanp->refcount, 1);
 
      spin_lock_bh(&vxlan->lock);
      hash_add(vxlan->htable, &vxlanp->hlist, port);

@@ -170,7 +171,7 @@ int mlx5_vxlan_del_port(struct mlx5_vxlan *vxlan, u16 port)
              goto out_unlock;
      }
 
-     if (atomic_dec_and_test(&vxlanp->refcount)) {
+     if (refcount_dec_and_test(&vxlanp->refcount)) {
              hash_del(&vxlanp->hlist);
              remove = true;
      }
--

2.20.1

Quoted 10 lines · Leon Romanovsky

Quoted 3 lines · Chuhong Yuan

refcount_t is better for reference counters since its implementation can prevent overflows.
So convert atomic_t ref counters to refcount_t.

I'm not thrilled to see those automatic conversion patches, especially for flows which can't overflow. There is nothing wrong in using atomic_t type of variable, do you have in mind flow which will cause to overflow?

Quoted 1 line

Thanks

I have to say that these patches are not done automatically... Only the detection of problems is done by a script. All conversions are done manually.

I am not sure whether the flow can cause an overflow.

But I think it is hard to ensure that a data path is impossible to have problems in any cases including being attacked.

So I think it is better to do this minor revision to prevent potential risk, just like we have done in mlx5/core/cq.c.

Regards,
Chuhong

Quoted 60 lines

Quoted 60 lines

Signed-off-by Chuhong Yuan <[email protected]>

---
Changes in v2:
- Add #include.

 drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c +5 -5

index b9d4f4e19ff9..148b55c3db7a 100644
@@ -32,6 +32,7 @@
 
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/refcount.h>
 #include <linux/mlx5/driver.h>
 #include <net/vxlan.h>
 #include "mlx5_core.h"

@@ -48,7 +49,7 @@ struct mlx5_vxlan {
 
 struct mlx5_vxlan_port {
      struct hlist_node hlist;
-     atomic_t refcount;
+     refcount_t refcount;
      u16 udp_port;
 };
 

@@ -113,7 +114,7 @@ int mlx5_vxlan_add_port(struct mlx5_vxlan *vxlan, u16 port)
 
      vxlanp = mlx5_vxlan_lookup_port(vxlan, port);
      if (vxlanp) {
-             atomic_inc(&vxlanp->refcount);
+             refcount_inc(&vxlanp->refcount);
              return 0;
      }
 

@@ -137,7 +138,7 @@ int mlx5_vxlan_add_port(struct mlx5_vxlan *vxlan, u16 port)
      }
 
      vxlanp->udp_port = port;
-     atomic_set(&vxlanp->refcount, 1);
+     refcount_set(&vxlanp->refcount, 1);
 
      spin_lock_bh(&vxlan->lock);
      hash_add(vxlan->htable, &vxlanp->hlist, port);

@@ -170,7 +171,7 @@ int mlx5_vxlan_del_port(struct mlx5_vxlan *vxlan, u16 port)
              goto out_unlock;
      }
 
-     if (atomic_dec_and_test(&vxlanp->refcount)) {
+     if (refcount_dec_and_test(&vxlanp->refcount)) {
              hash_del(&vxlanp->hlist);
              remove = true;
      }
--

2.20.1

Quoted 11 lines · Chuhong Yuan

Quoted 10 lines · Leon Romanovsky

Quoted 3 lines · Chuhong Yuan

refcount_t is better for reference counters since its implementation can prevent overflows.
So convert atomic_t ref counters to refcount_t.

I'm not thrilled to see those automatic conversion patches, especially for flows which can't overflow. There is nothing wrong in using atomic_t type of variable, do you have in mind flow which will cause to overflow?

Quoted 5 lines

Quoted 1 line

Thanks

I have to say that these patches are not done automatically... Only the detection of problems is done by a script. All conversions are done manually.

Even worse, you need to audit usage of atomic_t and replace there it can overflow.

Quoted 2 lines

I am not sure whether the flow can cause an overflow.

It can't.

Quoted 2 lines

But I think it is hard to ensure that a data path is impossible to have problems in any cases including being attacked.

It is not data path, and I doubt that such conversion will be allowed in data paths without proving that no performance regression is introduced.

Quoted 4 lines

So I think it is better to do this minor revision to prevent potential risk, just like we have done in mlx5/core/cq.c.

mlx5/core/cq.c is a different beast, refcount there means actual users of CQ which are limited in SW, so in theory, they have potential to be overflown.

It is not the case here, there your are adding new port. There is nothing wrong with atomic_t.

Thanks

Quoted 64 lines

Regards,
Chuhong

Quoted 60 lines

Quoted 60 lines

Signed-off-by Chuhong Yuan <[email protected]>

---
Changes in v2:
- Add #include.

 drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c +5 -5

index b9d4f4e19ff9..148b55c3db7a 100644
@@ -32,6 +32,7 @@
 
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/refcount.h>
 #include <linux/mlx5/driver.h>
 #include <net/vxlan.h>
 #include "mlx5_core.h"

@@ -48,7 +49,7 @@ struct mlx5_vxlan {
 
 struct mlx5_vxlan_port {
      struct hlist_node hlist;
-     atomic_t refcount;
+     refcount_t refcount;
      u16 udp_port;
 };
 

@@ -113,7 +114,7 @@ int mlx5_vxlan_add_port(struct mlx5_vxlan *vxlan, u16 port)
 
      vxlanp = mlx5_vxlan_lookup_port(vxlan, port);
      if (vxlanp) {
-             atomic_inc(&vxlanp->refcount);
+             refcount_inc(&vxlanp->refcount);
              return 0;
      }
 

@@ -137,7 +138,7 @@ int mlx5_vxlan_add_port(struct mlx5_vxlan *vxlan, u16 port)
      }
 
      vxlanp->udp_port = port;
-     atomic_set(&vxlanp->refcount, 1);
+     refcount_set(&vxlanp->refcount, 1);
 
      spin_lock_bh(&vxlan->lock);
      hash_add(vxlan->htable, &vxlanp->hlist, port);

@@ -170,7 +171,7 @@ int mlx5_vxlan_del_port(struct mlx5_vxlan *vxlan, u16 port)
              goto out_unlock;
      }
 
-     if (atomic_dec_and_test(&vxlanp->refcount)) {
+     if (refcount_dec_and_test(&vxlanp->refcount)) {
              hash_del(&vxlanp->hlist);
              remove = true;
      }
--

2.20.1

Quoted 43 lines · Leon Romanovsky

Quoted 14 lines · Chuhong Yuan

Quoted 12 lines · Leon Romanovsky

Quoted 3 lines · Chuhong Yuan

refcount_t is better for reference counters since its implementation can prevent overflows.
So convert atomic_t ref counters to refcount_t.

I'm not thrilled to see those automatic conversion patches, especially
for flows which can't overflow. There is nothing wrong in using atomic_t
type of variable, do you have in mind flow which will cause to overflow?

Quoted 5 lines

Quoted 1 line

Thanks

I have to say that these patches are not done automatically... Only the detection of problems is done by a script. All conversions are done manually.

Even worse, you need to audit usage of atomic_t and replace there it can overflow.

Quoted 1 line

I am not sure whether the flow can cause an overflow.

It can't.

Quoted 2 lines

But I think it is hard to ensure that a data path is impossible to have problems in any cases including being attacked.

It is not data path, and I doubt that such conversion will be allowed
in data paths without proving that no performance regression is introduced.

Quoted 2 lines

So I think it is better to do this minor revision to prevent potential risk, just like we have done in mlx5/core/cq.c.

mlx5/core/cq.c is a different beast, refcount there means actual users
of CQ which are limited in SW, so in theory, they have potential to be overflown.

Quoted 3 lines

It is not the case here, there your are adding new port. There is nothing wrong with atomic_t.

Thanks for your explanation!
I will pay attention to this point in similar cases.
But it seems that the semantic of refcount is not always as clear as here...

Quoted 45 lines · Chuhong Yuan

Quoted 43 lines · Leon Romanovsky

Quoted 14 lines · Chuhong Yuan

Quoted 12 lines · Leon Romanovsky

Quoted 3 lines · Chuhong Yuan

refcount_t is better for reference counters since its implementation can prevent overflows.
So convert atomic_t ref counters to refcount_t.

I'm not thrilled to see those automatic conversion patches, especially
for flows which can't overflow. There is nothing wrong in using atomic_t
type of variable, do you have in mind flow which will cause to overflow?

Quoted 5 lines

Quoted 1 line

Thanks

I have to say that these patches are not done automatically... Only the detection of problems is done by a script. All conversions are done manually.

Even worse, you need to audit usage of atomic_t and replace there it can overflow.

Quoted 1 line

I am not sure whether the flow can cause an overflow.

It can't.

Quoted 2 lines

But I think it is hard to ensure that a data path is impossible to have problems in any cases including being attacked.

It is not data path, and I doubt that such conversion will be allowed
in data paths without proving that no performance regression is introduced.

Quoted 2 lines

So I think it is better to do this minor revision to prevent potential risk, just like we have done in mlx5/core/cq.c.

mlx5/core/cq.c is a different beast, refcount there means actual users
of CQ which are limited in SW, so in theory, they have potential to be overflown.

Quoted 9 lines

Quoted 3 lines

It is not the case here, there your are adding new port. There is nothing wrong with atomic_t.

Thanks for your explanation!
I will pay attention to this point in similar cases.
But it seems that the semantic of refcount is not always as clear as here...

Semantically speaking, there is nothing wrong with moving to refcount_t in the case of vxlan ports.. it also seems more accurate and will provide the type protection, even if it is not necessary. Please let me know what is the verdict here, i can apply this patch to net-next-mlx5.

Thanks,
Saeed.

Quoted 56 lines · Saeed Mahameed

Quoted 54 lines · Chuhong Yuan

Quoted 38 lines · Leon Romanovsky

Quoted 14 lines · Chuhong Yuan

Quoted 12 lines · Leon Romanovsky

Quoted 3 lines · Chuhong Yuan

refcount_t is better for reference counters since its implementation can prevent overflows.
So convert atomic_t ref counters to refcount_t.

I'm not thrilled to see those automatic conversion patches, especially
for flows which can't overflow. There is nothing wrong in using atomic_t
type of variable, do you have in mind flow which will cause to overflow?

Quoted 5 lines

Quoted 1 line

Thanks

I have to say that these patches are not done automatically... Only the detection of problems is done by a script. All conversions are done manually.

Even worse, you need to audit usage of atomic_t and replace there it can overflow.

Quoted 1 line

I am not sure whether the flow can cause an overflow.

It can't.

Quoted 2 lines

But I think it is hard to ensure that a data path is impossible to have problems in any cases including being attacked.

It is not data path, and I doubt that such conversion will be allowed
in data paths without proving that no performance regression is introduced.

Quoted 2 lines

So I think it is better to do this minor revision to prevent potential risk, just like we have done in mlx5/core/cq.c.

Quoted 8 lines

mlx5/core/cq.c is a different beast, refcount there means actual users
of CQ which are limited in SW, so in theory, they have potential to be overflown.

It is not the case here, there your are adding new port. There is nothing wrong with atomic_t.

Thanks for your explanation!
I will pay attention to this point in similar cases.
But it seems that the semantic of refcount is not always as clear as here...

Quoted 4 lines

Semantically speaking, there is nothing wrong with moving to refcount_t in the case of vxlan ports.. it also seems more accurate and will provide the type protection, even if it is not necessary. Please let me know what is the verdict here, i can apply this patch to net-next-mlx5.

There is no verdict here, it is up to you., if you like code churn, go for it.

Quoted 79 lines · Leon Romanovsky

Quoted 70 lines · Saeed Mahameed

Quoted 62 lines · Chuhong Yuan

On Mon, Aug 5, 2019 at 2:13 PM Leon Romanovsky <[email protected]

Quoted 1 line

Quoted 55 lines · wrote:

Quoted 19 lines · Chuhong Yuan

Quoted 16 lines · Leon Romanovsky

Quoted 3 lines · Chuhong Yuan

refcount_t is better for reference counters since its implementation can prevent overflows.
So convert atomic_t ref counters to refcount_t.

I'm not thrilled to see those automatic conversion patches,
especially
for flows which can't overflow. There is nothing wrong in using
atomic_t
type of variable, do you have in mind flow which will cause to
overflow?

Quoted 6 lines

Quoted 1 line

Thanks

I have to say that these patches are not done automatically...
Only the detection of problems is done by a script. All conversions are done manually.

Even worse, you need to audit usage of atomic_t and replace there
it can overflow.

Quoted 1 line

I am not sure whether the flow can cause an overflow.

It can't.

Quoted 3 lines

But I think it is hard to ensure that a data path is impossible
to have problems in any cases including being attacked.

It is not data path, and I doubt that such conversion will be allowed
in data paths without proving that no performance regression is
introduced.

Quoted 3 lines

So I think it is better to do this minor revision to prevent
potential risk, just like we have done in mlx5/core/cq.c.

mlx5/core/cq.c is a different beast, refcount there means actual
users
of CQ which are limited in SW, so in theory, they have potential
to be overflown.

Quoted 3 lines

It is not the case here, there your are adding new port. There is nothing wrong with atomic_t.

Quoted 6 lines

Thanks for your explanation!
I will pay attention to this point in similar cases. But it seems that the semantic of refcount is not always as clear as
here...

Quoted 7 lines

Semantically speaking, there is nothing wrong with moving to refcount_t
in the case of vxlan ports.. it also seems more accurate and will provide the type protection, even if it is not necessary. Please let me
know what is the verdict here, i can apply this patch to net- next-mlx5.

Quoted 3 lines

There is no verdict here, it is up to you., if you like code churn, go
for it.

IMHO CONFIG_REFCOUNT_FULL is a valuable enough reason to not open code
with an atomic even if overflow is not possible.

Races resulting in incrs on 0 refcounts is a common enough mistake.

Jason

refcount_t is better for reference counters since its implementation can prevent overflows.
So convert atomic_t ref counters to refcount_t.

Signed-off-by Chuhong Yuan <[email protected]>

---
Changes in v2:
- Add #include.

Quoted 9 lines

refcount_t is better for reference counters since its implementation can prevent overflows.
So convert atomic_t ref counters to refcount_t.

Signed-off-by Chuhong Yuan <[email protected]>

---
Changes in v2:
- Add #include.

Acked-by Saeed Mahameed <[email protected]>

Dave, up to you take it, or leave it to me :).

From: Saeed Mahameed <[email protected]>
Date: Tue, 6 Aug 2019 20:42:56 +0000

Quoted 11 lines · Chuhong Yuan

Quoted 9 lines

refcount_t is better for reference counters since its implementation can prevent overflows.
So convert atomic_t ref counters to refcount_t.

Signed-off-by Chuhong Yuan <[email protected]>

---
Changes in v2:
- Add #include.

Quoted 3 lines

Acked-by Saeed Mahameed <[email protected]>

Dave, up to you take it, or leave it to me :).

Please take it, thank you.