| Message ID | 4B1A4BC5.3050300@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 07 Dec 2009 20:13:14 +0100 > 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 :) 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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html