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(-)
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
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 <[email protected]>
> ---
> 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
>
On Sun, Aug 4, 2019 at 8:59 PM Leon Romanovsky <[email protected]> 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 <[email protected]>
> > ---
> > 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
> >
On Sun, Aug 04, 2019 at 10:44:47PM +0800, Chuhong Yuan wrote:
> On Sun, Aug 4, 2019 at 8:59 PM Leon Romanovsky <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
> > >
On Mon, Aug 5, 2019 at 2:13 PM Leon Romanovsky <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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
> > > >
On Mon, 2019-08-05 at 14:55 +0800, Chuhong Yuan wrote:
> On Mon, Aug 5, 2019 at 2:13 PM Leon Romanovsky <[email protected]>
> 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 <[email protected]>
> > > 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.
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 <[email protected]>
> > 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 <[email protected]>
> > > > 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.
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 <[email protected]>
> > > 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 <[email protected]>
> > > > > 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
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 <[email protected]
> > > > >
> > > > 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 <
> > > > > > [email protected]>
> > > > > > 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.
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 <[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
> 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 <[email protected]>
>> ---
>> Changes in v2:
>> - Add #include.
>>
>
> Acked-by: Saeed Mahameed <[email protected]>
>
> Dave, up to you take it, or leave it to me :).
Please take it, thank you.
On Tue, 2019-08-06 at 13:48 -0700, David Miller wrote:
> From: Saeed Mahameed <[email protected]>
> 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 <[email protected]>
> > > ---
> > > Changes in v2:
> > > - Add #include.
> > >
> >
> > Acked-by: Saeed Mahameed <[email protected]>
> >
> > 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!