[PATCH] net: dst_release() cleanup

[PATCH] net: dst_release() cleanup

[Eric Dumazet]

atomic_dec_return() is a full memory barrier, we can omit
the smp_mb__before_atomic_dec() call.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/core/dst.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/net/core/dst.c b/net/core/dst.c
index 57bc4d5..cdec1d1 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -262,13 +262,8 @@ again:
 
 void dst_release(struct dst_entry *dst)
 {
-	if (dst) {
-               int newrefcnt;
-
-		smp_mb__before_atomic_dec();
-               newrefcnt = atomic_dec_return(&dst->__refcnt);
-               WARN_ON(newrefcnt < 0);
-	}
+	if (dst)
+		WARN_ON(atomic_dec_return(&dst->__refcnt) < 0);
 }
 EXPORT_SYMBOL(dst_release);
 

Re: [PATCH] net: dst_release() cleanup

[Stephen Hemminger]

On Sat, 05 Dec 2009 13:02:13 +0100
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> atomic_dec_return() is a full memory barrier, we can omit
> the smp_mb__before_atomic_dec() call.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  net/core/dst.c |    9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/net/core/dst.c b/net/core/dst.c
> index 57bc4d5..cdec1d1 100644
> --- a/net/core/dst.c
> +++ b/net/core/dst.c
> @@ -262,13 +262,8 @@ again:
>  
>  void dst_release(struct dst_entry *dst)
>  {
> -	if (dst) {
> -               int newrefcnt;
> -
> -		smp_mb__before_atomic_dec();
> -               newrefcnt = atomic_dec_return(&dst->__refcnt);
> -               WARN_ON(newrefcnt < 0);
> -	}
> +	if (dst)
> +		WARN_ON(atomic_dec_return(&dst->__refcnt) < 0);
>  }
>  EXPORT_SYMBOL(dst_release);

I don't like to put actual necessary code in WARN or BUG macro
args because some embedded type developer is likely to build
with

#define WARN_ON(x)

to get rid of all warnings.

Re: [PATCH] net: dst_release() cleanup

[Eric Dumazet]

Stephen Hemminger a écrit :
 
> I don't like to put actual necessary code in WARN or BUG macro
> args because some embedded type developer is likely to build
> with
> 
> #define WARN_ON(x)
> 
> to get rid of all warnings.

Oops, I thought WARN_ON(X) must evaluate X once, my bad, since its not documented.

Thanks

[PATCH] net: dst_release() cleanup

atomic_dec_return() is a full memory barrier, we can omit
the smp_mb__before_atomic_dec() call.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/core/dst.c b/net/core/dst.c
index 57bc4d5..c3d0cfa 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -263,11 +263,9 @@ again:
 void dst_release(struct dst_entry *dst)
 {
 	if (dst) {
-               int newrefcnt;
+		int newrefcnt = atomic_dec_return(&dst->__refcnt);
 
-		smp_mb__before_atomic_dec();
-               newrefcnt = atomic_dec_return(&dst->__refcnt);
-               WARN_ON(newrefcnt < 0);
+		WARN_ON(newrefcnt < 0);
 	}
 }
 EXPORT_SYMBOL(dst_release);

Re: [PATCH] net: dst_release() cleanup

[Stephen Hemminger]

On Mon, 07 Dec 2009 19:12:03 +0100
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Stephen Hemminger a écrit :
>  
> > I don't like to put actual necessary code in WARN or BUG macro
> > args because some embedded type developer is likely to build
> > with
> > 
> > #define WARN_ON(x)
> > 
> > to get rid of all warnings.
> 
> Oops, I thought WARN_ON(X) must evaluate X once, my bad, since its not documented.
> 

If done correctly, it would. The correct way to ignore warnings would be to do
something like:

#define WARN_ON(x) (x)

Re: [PATCH] net: dst_release() cleanup

[Ilpo Järvinen]

On Mon, 7 Dec 2009, Stephen Hemminger wrote:

> On Sat, 05 Dec 2009 13:02:13 +0100
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > atomic_dec_return() is a full memory barrier, we can omit
> > the smp_mb__before_atomic_dec() call.
> > 
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > ---
> >  net/core/dst.c |    9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/net/core/dst.c b/net/core/dst.c
> > index 57bc4d5..cdec1d1 100644
> > --- a/net/core/dst.c
> > +++ b/net/core/dst.c
> > @@ -262,13 +262,8 @@ again:
> >  
> >  void dst_release(struct dst_entry *dst)
> >  {
> > -	if (dst) {
> > -               int newrefcnt;
> > -
> > -		smp_mb__before_atomic_dec();
> > -               newrefcnt = atomic_dec_return(&dst->__refcnt);
> > -               WARN_ON(newrefcnt < 0);
> > -	}
> > +	if (dst)
> > +		WARN_ON(atomic_dec_return(&dst->__refcnt) < 0);
> >  }
> >  EXPORT_SYMBOL(dst_release);
> 
> I don't like to put actual necessary code in WARN or BUG macro
> args because some embedded type developer is likely to build
> with
> 
> #define WARN_ON(x)
> 
> to get rid of all warnings.

That's their problem then to fix all relevant places. ...That won't even 
compile because of constructs like this:

	if (WARN_ON(x))

...To my knowing this perfectly legal way of doing this.

-- 
 i.

Re: [PATCH] net: dst_release() cleanup

[Eric Dumazet]

Ilpo Järvinen a écrit :

> That's their problem then to fix all relevant places. ...That won't even 
> compile because of constructs like this:
> 
> 	if (WARN_ON(x))
> 
> ...To my knowing this perfectly legal way of doing this.
> 

Sure, but let be conservative :)

Re: [PATCH] net: dst_release() cleanup

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 07 Dec 2009 20:13:14 +0100

> Ilpo J^[$(D+#^[(Brvinen a ^[$(D+1^[(Bcrit :
> 
>> That's their problem then to fix all relevant places. ...That won't even 
>> compile because of constructs like this:
>> 
>> 	if (WARN_ON(x))
>> 
>> ...To my knowing this perfectly legal way of doing this.
>> 
> 
> Sure, but let be conservative :)

It certainly jumped out at me.

Using side effects in a debugging macro, that's always asking for
trouble.  Every time I see this thing I'm going to do a double
take on it.

We can pull the return value into an 'int' with a descriptive name
such as "orig_dst_refcnt", and also use WARN() to make a descriptive
error message for kerneloops.org to log if it triggers.

Ok Eric?

Re: [PATCH] net: dst_release() cleanup

[Eric Dumazet]

Le 09/12/2009 05:35, David Miller a écrit :
> 
> Using side effects in a debugging macro, that's always asking for
> trouble.  Every time I see this thing I'm going to do a double
> take on it.
> 
> We can pull the return value into an 'int' with a descriptive name
> such as "orig_dst_refcnt", and also use WARN() to make a descriptive
> error message for kerneloops.org to log if it triggers.
> 
> Ok Eric?

Sure, I'll submit this when net-next-2.6 reopens :)