linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] net/mlx5e: Use refcount_t for refcount
@ 2019-08-02 16:48 Chuhong Yuan
  2019-08-04 12:58 ` Leon Romanovsky
  2019-08-06 20:42 ` Saeed Mahameed
  0 siblings, 2 replies; 12+ messages in thread
From: Chuhong Yuan @ 2019-08-02 16:48 UTC (permalink / raw)
  Cc: Saeed Mahameed, Leon Romanovsky, David S . Miller, netdev,
	linux-rdma, linux-kernel, Chuhong Yuan

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 <hslester96@gmail.com>
---
Changes in v2:
  - Add #include.

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

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
index b9d4f4e19ff9..148b55c3db7a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
@@ -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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net/mlx5e: Use refcount_t for refcount
  2019-08-02 16:48 [PATCH v2] net/mlx5e: Use refcount_t for refcount Chuhong Yuan
@ 2019-08-04 12:58 ` Leon Romanovsky
  2019-08-04 14:44   ` Chuhong Yuan
  2019-08-06 20:42 ` Saeed Mahameed
  1 sibling, 1 reply; 12+ messages in thread
From: Leon Romanovsky @ 2019-08-04 12:58 UTC (permalink / raw)
  To: Chuhong Yuan
  Cc: Saeed Mahameed, David S . Miller, netdev, linux-rdma, linux-kernel

On Sat, Aug 03, 2019 at 12:48:28AM +0800, Chuhong Yuan wrote:
> 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
>
> Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> ---
> Changes in v2:
>   - Add #include.
>
>  drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
> index b9d4f4e19ff9..148b55c3db7a 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
> @@ -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
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net/mlx5e: Use refcount_t for refcount
  2019-08-04 12:58 ` Leon Romanovsky
@ 2019-08-04 14:44   ` Chuhong Yuan
  2019-08-05  6:13     ` Leon Romanovsky
  0 siblings, 1 reply; 12+ messages in thread
From: Chuhong Yuan @ 2019-08-04 14:44 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Saeed Mahameed, David S . Miller, Netdev, linux-rdma, LKML

On Sun, Aug 4, 2019 at 8:59 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Sat, Aug 03, 2019 at 12:48:28AM +0800, Chuhong Yuan wrote:
> > 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

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

> >
> > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> > ---
> > Changes in v2:
> >   - Add #include.
> >
> >  drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
> > index b9d4f4e19ff9..148b55c3db7a 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
> > @@ -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
> >

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net/mlx5e: Use refcount_t for refcount
  2019-08-04 14:44   ` Chuhong Yuan
@ 2019-08-05  6:13     ` Leon Romanovsky
  2019-08-05  6:55       ` Chuhong Yuan
  0 siblings, 1 reply; 12+ messages in thread
From: Leon Romanovsky @ 2019-08-05  6:13 UTC (permalink / raw)
  To: Chuhong Yuan; +Cc: Saeed Mahameed, David S . Miller, Netdev, linux-rdma, LKML

On Sun, Aug 04, 2019 at 10:44:47PM +0800, Chuhong Yuan wrote:
> On Sun, Aug 4, 2019 at 8:59 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Sat, Aug 03, 2019 at 12:48:28AM +0800, Chuhong Yuan wrote:
> > > 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
>
> 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.

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

It can't.

> 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.

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

>
> Regards,
> Chuhong
>
> > >
> > > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> > > ---
> > > Changes in v2:
> > >   - Add #include.
> > >
> > >  drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
> > > index b9d4f4e19ff9..148b55c3db7a 100644
> > > --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
> > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
> > > @@ -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
> > >

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net/mlx5e: Use refcount_t for refcount
  2019-08-05  6:13     ` Leon Romanovsky
@ 2019-08-05  6:55       ` Chuhong Yuan
  2019-08-05 20:06         ` Saeed Mahameed
  0 siblings, 1 reply; 12+ messages in thread
From: Chuhong Yuan @ 2019-08-05  6:55 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Saeed Mahameed, David S . Miller, Netdev, linux-rdma, LKML

On Mon, Aug 5, 2019 at 2:13 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Sun, Aug 04, 2019 at 10:44:47PM +0800, Chuhong Yuan wrote:
> > On Sun, Aug 4, 2019 at 8:59 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Sat, Aug 03, 2019 at 12:48:28AM +0800, Chuhong Yuan wrote:
> > > > 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
> >
> > 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.
>
> >
> > I am not sure whether the flow can cause an overflow.
>
> It can't.
>
> > 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.
>>
>
> > 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 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...

Regards,
Chuhong


> Thanks
>
> >
> > Regards,
> > Chuhong
> >
> > > >
> > > > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> > > > ---
> > > > Changes in v2:
> > > >   - Add #include.
> > > >
> > > >  drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
> > > > index b9d4f4e19ff9..148b55c3db7a 100644
> > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
> > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
> > > > @@ -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
> > > >

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net/mlx5e: Use refcount_t for refcount
  2019-08-05  6:55       ` Chuhong Yuan
@ 2019-08-05 20:06         ` Saeed Mahameed
  2019-08-06  6:59           ` Leon Romanovsky
  0 siblings, 1 reply; 12+ messages in thread
From: Saeed Mahameed @ 2019-08-05 20:06 UTC (permalink / raw)
  To: hslester96, leon; +Cc: davem, netdev, linux-rdma, linux-kernel

On Mon, 2019-08-05 at 14:55 +0800, Chuhong Yuan wrote:
> On Mon, Aug 5, 2019 at 2:13 PM Leon Romanovsky <leon@kernel.org>
> wrote:
> > On Sun, Aug 04, 2019 at 10:44:47PM +0800, Chuhong Yuan wrote:
> > > On Sun, Aug 4, 2019 at 8:59 PM Leon Romanovsky <leon@kernel.org>
> > > wrote:
> > > > On Sat, Aug 03, 2019 at 12:48:28AM +0800, Chuhong Yuan wrote:
> > > > > 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
> > > 
> > > 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.
> > 
> > > I am not sure whether the flow can cause an overflow.
> > 
> > It can't.
> > 
> > > 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.
> > > 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 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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net/mlx5e: Use refcount_t for refcount
  2019-08-05 20:06         ` Saeed Mahameed
@ 2019-08-06  6:59           ` Leon Romanovsky
  2019-08-06 18:38             ` Jason Gunthorpe
  0 siblings, 1 reply; 12+ messages in thread
From: Leon Romanovsky @ 2019-08-06  6:59 UTC (permalink / raw)
  To: Saeed Mahameed; +Cc: hslester96, davem, netdev, linux-rdma, linux-kernel

On Mon, Aug 05, 2019 at 08:06:36PM +0000, Saeed Mahameed wrote:
> On Mon, 2019-08-05 at 14:55 +0800, Chuhong Yuan wrote:
> > On Mon, Aug 5, 2019 at 2:13 PM Leon Romanovsky <leon@kernel.org>
> > wrote:
> > > On Sun, Aug 04, 2019 at 10:44:47PM +0800, Chuhong Yuan wrote:
> > > > On Sun, Aug 4, 2019 at 8:59 PM Leon Romanovsky <leon@kernel.org>
> > > > wrote:
> > > > > On Sat, Aug 03, 2019 at 12:48:28AM +0800, Chuhong Yuan wrote:
> > > > > > 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
> > > >
> > > > 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.
> > >
> > > > I am not sure whether the flow can cause an overflow.
> > >
> > > It can't.
> > >
> > > > 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.
> > > > 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 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.

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

Thanks

>
> Thanks,
> Saeed.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net/mlx5e: Use refcount_t for refcount
  2019-08-06  6:59           ` Leon Romanovsky
@ 2019-08-06 18:38             ` Jason Gunthorpe
  2019-08-06 18:54               ` Saeed Mahameed
  0 siblings, 1 reply; 12+ messages in thread
From: Jason Gunthorpe @ 2019-08-06 18:38 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Saeed Mahameed, hslester96, davem, netdev, linux-rdma, linux-kernel

On Tue, Aug 06, 2019 at 09:59:58AM +0300, Leon Romanovsky wrote:
> On Mon, Aug 05, 2019 at 08:06:36PM +0000, Saeed Mahameed wrote:
> > On Mon, 2019-08-05 at 14:55 +0800, Chuhong Yuan wrote:
> > > On Mon, Aug 5, 2019 at 2:13 PM Leon Romanovsky <leon@kernel.org>
> > > wrote:
> > > > On Sun, Aug 04, 2019 at 10:44:47PM +0800, Chuhong Yuan wrote:
> > > > > On Sun, Aug 4, 2019 at 8:59 PM Leon Romanovsky <leon@kernel.org>
> > > > > wrote:
> > > > > > On Sat, Aug 03, 2019 at 12:48:28AM +0800, Chuhong Yuan wrote:
> > > > > > > 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
> > > > >
> > > > > 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.
> > > >
> > > > > I am not sure whether the flow can cause an overflow.
> > > >
> > > > It can't.
> > > >
> > > > > 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.
> > > > > 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 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.
> 
> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net/mlx5e: Use refcount_t for refcount
  2019-08-06 18:38             ` Jason Gunthorpe
@ 2019-08-06 18:54               ` Saeed Mahameed
  0 siblings, 0 replies; 12+ messages in thread
From: Saeed Mahameed @ 2019-08-06 18:54 UTC (permalink / raw)
  To: jgg, leon; +Cc: linux-kernel, davem, hslester96, linux-rdma, netdev

On Tue, 2019-08-06 at 15:38 -0300, Jason Gunthorpe wrote:
> On Tue, Aug 06, 2019 at 09:59:58AM +0300, Leon Romanovsky wrote:
> > On Mon, Aug 05, 2019 at 08:06:36PM +0000, Saeed Mahameed wrote:
> > > On Mon, 2019-08-05 at 14:55 +0800, Chuhong Yuan wrote:
> > > > On Mon, Aug 5, 2019 at 2:13 PM Leon Romanovsky <leon@kernel.org
> > > > >
> > > > wrote:
> > > > > On Sun, Aug 04, 2019 at 10:44:47PM +0800, Chuhong Yuan wrote:
> > > > > > On Sun, Aug 4, 2019 at 8:59 PM Leon Romanovsky <
> > > > > > leon@kernel.org>
> > > > > > wrote:
> > > > > > > On Sat, Aug 03, 2019 at 12:48:28AM +0800, Chuhong Yuan
> > > > > > > wrote:
> > > > > > > > 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
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > > I am not sure whether the flow can cause an overflow.
> > > > > 
> > > > > It can't.
> > > > > 
> > > > > > 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.
> > > > > > 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 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.
> > 
> > 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

Indeed, thanks Jason, I will take this patch to net-next-mlx5.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net/mlx5e: Use refcount_t for refcount
  2019-08-02 16:48 [PATCH v2] net/mlx5e: Use refcount_t for refcount Chuhong Yuan
  2019-08-04 12:58 ` Leon Romanovsky
@ 2019-08-06 20:42 ` Saeed Mahameed
  2019-08-06 20:48   ` David Miller
  1 sibling, 1 reply; 12+ messages in thread
From: Saeed Mahameed @ 2019-08-06 20:42 UTC (permalink / raw)
  To: hslester96; +Cc: linux-rdma, davem, netdev, leon, linux-kernel

On Sat, 2019-08-03 at 00:48 +0800, Chuhong Yuan wrote:
> 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 <hslester96@gmail.com>
> ---
> Changes in v2:
>   - Add #include.
> 

Acked-by: Saeed Mahameed <saeedm@mellanox.com>

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net/mlx5e: Use refcount_t for refcount
  2019-08-06 20:42 ` Saeed Mahameed
@ 2019-08-06 20:48   ` David Miller
  2019-08-06 21:25     ` Saeed Mahameed
  0 siblings, 1 reply; 12+ messages in thread
From: David Miller @ 2019-08-06 20:48 UTC (permalink / raw)
  To: saeedm; +Cc: hslester96, linux-rdma, netdev, leon, linux-kernel

From: Saeed Mahameed <saeedm@mellanox.com>
Date: Tue, 6 Aug 2019 20:42:56 +0000

> On Sat, 2019-08-03 at 00:48 +0800, Chuhong Yuan wrote:
>> 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 <hslester96@gmail.com>
>> ---
>> Changes in v2:
>>   - Add #include.
>> 
> 
> Acked-by: Saeed Mahameed <saeedm@mellanox.com>
> 
> Dave, up to you take it, or leave it to me :).

Please take it, thank you.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net/mlx5e: Use refcount_t for refcount
  2019-08-06 20:48   ` David Miller
@ 2019-08-06 21:25     ` Saeed Mahameed
  0 siblings, 0 replies; 12+ messages in thread
From: Saeed Mahameed @ 2019-08-06 21:25 UTC (permalink / raw)
  To: davem; +Cc: linux-kernel, netdev, hslester96, linux-rdma, leon

On Tue, 2019-08-06 at 13:48 -0700, David Miller wrote:
> From: Saeed Mahameed <saeedm@mellanox.com>
> Date: Tue, 6 Aug 2019 20:42:56 +0000
> 
> > On Sat, 2019-08-03 at 00:48 +0800, Chuhong Yuan wrote:
> > > 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 <hslester96@gmail.com>
> > > ---
> > > Changes in v2:
> > >   - Add #include.
> > > 
> > 
> > Acked-by: Saeed Mahameed <saeedm@mellanox.com>
> > 
> > Dave, up to you take it, or leave it to me :).
> 
> Please take it, thank you.

Ok, running some build tests will apply shortly to net-next-mlx5.
Thanks everyone!

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-08-06 21:26 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-02 16:48 [PATCH v2] net/mlx5e: Use refcount_t for refcount Chuhong Yuan
2019-08-04 12:58 ` Leon Romanovsky
2019-08-04 14:44   ` Chuhong Yuan
2019-08-05  6:13     ` Leon Romanovsky
2019-08-05  6:55       ` Chuhong Yuan
2019-08-05 20:06         ` Saeed Mahameed
2019-08-06  6:59           ` Leon Romanovsky
2019-08-06 18:38             ` Jason Gunthorpe
2019-08-06 18:54               ` Saeed Mahameed
2019-08-06 20:42 ` Saeed Mahameed
2019-08-06 20:48   ` David Miller
2019-08-06 21:25     ` Saeed Mahameed

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).