net: dst_release() cleanup
Subsystems (2)
PATCH 1/1 net: dst_release() cleanup
atomic_dec_return() is a full memory barrier, we can omit the smp_mb__before_atomic_dec() call.
--- net/core/dst.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
net/core/dst.c +2 -7
@@ -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);
Quoted 17 lines · On Sat, 05 Dec 2009 13:02:13 +0100 Eric Dumazet <[email protected]> wrote:
atomic_dec_return()is a full memory barrier, we can omit thesmp_mb__before_atomic_dec()call.Signed-off-by Eric Dumazet <[email protected]>--- net/core/dst.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)net/core/dst.c +0
-1@@ -262,13 +262,8 @@ again:void dst_release (struct dst_entry *dst) { -if (dst) {
net/core/dst.c again: · line 268
Quoted 10 lines
net/core/dst.c +2
-6-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.
Quoted 7 lines · Stephen Hemminger
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.
---
net/core/dst.c +2 -4
@@ -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);
Quoted 6 lines · On Mon, 07 Dec 2009 19:12:03 +0100 Eric Dumazet <[email protected]> wrote:
Quoted 4 lines · Stephen Hemminger
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
Quoted 6 lines
Quoted 3 lines
#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)
Quoted 35 lines · Stephen Hemminger
Quoted 17 lines · On Sat, 05 Dec 2009 13:02:13 +0100 Eric Dumazet <[email protected]> wrote:
atomic_dec_return()is a full memory barrier, we can omit thesmp_mb__before_atomic_dec()call.Signed-off-by Eric Dumazet <[email protected]>--- net/core/dst.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)net/core/dst.c +0
-1@@ -262,13 +262,8 @@ again:void dst_release (struct dst_entry *dst) { -if (dst) {net/core/dst.c again: · line 268
Quoted 10 lines
net/core/dst.c +2
-6-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
Quoted 3 lines
#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.
Signature · 1 line
i.
Quoted 7 lines · Ilpo Järvinen
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 :)
David Miller
maintainerFrom: Eric Dumazet <[email protected]>
Date: Mon, 07 Dec 2009 20:13:14 +0100
Quoted 11 lines
Ilpo J$(D+#(Brvinen a $(D+1(Bcrit :
Quoted 7 lines
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?
Quoted 10 lines · David Miller
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 useWARN()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 :)