| Message ID | 1288980046.2882.1054.camel@edumazet-laptop |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 05 Nov 2010 19:00:46 +0100 Eric Dumazet <eric.dumazet@gmail.com> wrote: > Le vendredi 05 novembre 2010 __ 10:20 -0700, Andrew Morton a __crit : > > > It totally makes sense to add include/linu/atomic.h for common things. > > Perhaps there's already code in arch/*/include/asm/atomic.h which > > should be hoisted up there. But that can't reliably be done until a > > million files have had their #includes switched :( > > > > Maybe including <linux/atomic.h> only from the end of various > > arch/*/include/asm/atomic.h ? heh, I guess that would work. It breaks the standard way of doing these things (I think?) so let's not go there unless we have a need? > In this case, I remove the include <asm/atomic.h> from linux/atomic.h Oh. Why? I thought it was better the previous, standard way: thou shalt henceforth include liunx/atomic.h, not asm/atomic.h. And the presence of linux/atomic.h will in fact trigger the checkpatch warning telling people to use that when they try to use asm/atomic.h. > > > +static inline int atomic_inc_not_zero_hint(atomic_t *v, int hint) > > > +{ > > > + int val, c = hint; > > > + > > > + do { > > > + val = atomic_cmpxchg(v, c, c + 1); > > > + if (val == c) > > > + return 1; > > > + c = val; > > > + } while (c); > > > + > > > + return 0; > > > +} > > > > Should/could this have implemented a more general > > atomic_add_not_zero_hint() and made atomic_inc_not_zero_hint() a > > wrapper around that? > > Well, I see no practical use for this, but yes, this could be done. > > As atomic_add_not_zero() doesnt exist, I am not sure we need an > atomic_add_not_zero_hint() yet ? hm, OK. I was just checking ;) -- 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 vendredi 05 novembre 2010 à 11:08 -0700, Andrew Morton a écrit : > On Fri, 05 Nov 2010 19:00:46 +0100 > Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > Le vendredi 05 novembre 2010 __ 10:20 -0700, Andrew Morton a __crit : > > > > > It totally makes sense to add include/linu/atomic.h for common things. > > > Perhaps there's already code in arch/*/include/asm/atomic.h which > > > should be hoisted up there. But that can't reliably be done until a > > > million files have had their #includes switched :( > > > > > > > Maybe including <linux/atomic.h> only from the end of various > > > > arch/*/include/asm/atomic.h ? > > heh, I guess that would work. It breaks the standard way of doing > these things (I think?) so let's not go there unless we have a need? > > > In this case, I remove the include <asm/atomic.h> from linux/atomic.h > > Oh. Why? I thought it was better the previous, standard way: thou > shalt henceforth include liunx/atomic.h, not asm/atomic.h. And the > presence of linux/atomic.h will in fact trigger the checkpatch warning > telling people to use that when they try to use asm/atomic.h. Hmm, if we want to move the common stuff from arch/*/include/asm/atomic.h to this new file (include/linux/atomic.h), then we would have to change hundred of #include <asm/atomic.h> to #include <linux/atomic.h> This seems a big task to me ? Or just make a whole tree replace ? -- 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 Fri, 05 Nov 2010 19:20:24 +0100 Eric Dumazet <eric.dumazet@gmail.com> wrote: > Le vendredi 05 novembre 2010 __ 11:08 -0700, Andrew Morton a __crit : > > On Fri, 05 Nov 2010 19:00:46 +0100 > > Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > > Le vendredi 05 novembre 2010 __ 10:20 -0700, Andrew Morton a __crit : > > > > > > > It totally makes sense to add include/linu/atomic.h for common things. > > > > Perhaps there's already code in arch/*/include/asm/atomic.h which > > > > should be hoisted up there. But that can't reliably be done until a > > > > million files have had their #includes switched :( > > > > > > > > > > Maybe including <linux/atomic.h> only from the end of various > > > > > > arch/*/include/asm/atomic.h ? > > > > heh, I guess that would work. It breaks the standard way of doing > > these things (I think?) so let's not go there unless we have a need? > > > > > In this case, I remove the include <asm/atomic.h> from linux/atomic.h > > > > Oh. Why? I thought it was better the previous, standard way: thou > > shalt henceforth include liunx/atomic.h, not asm/atomic.h. And the > > presence of linux/atomic.h will in fact trigger the checkpatch warning > > telling people to use that when they try to use asm/atomic.h. > > Hmm, if we want to move the common stuff from > arch/*/include/asm/atomic.h to this new file (include/linux/atomic.h), > then we would have to change hundred of > > #include <asm/atomic.h> > > to > > #include <linux/atomic.h> > > This seems a big task to me ? > > Or just make a whole tree replace ? > But we haven't established that there _is_ duplicated code which needs that treatment. Scanning arch/x86/include/asm/atomic.h, perhaps ATOMIC_INIT() is a candidate. But I'm not sure that it _should_ be hoisted up - if every architecture happens to do it the same way then that's just a fluke. -- 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 Fri, Nov 05, 2010 at 07:00:46PM +0100, Eric Dumazet wrote: > Le vendredi 05 novembre 2010 à 10:20 -0700, Andrew Morton a écrit : > > > It totally makes sense to add include/linu/atomic.h for common things. > > Perhaps there's already code in arch/*/include/asm/atomic.h which > > should be hoisted up there. But that can't reliably be done until a > > million files have had their #includes switched :( > > > > Maybe including <linux/atomic.h> only from the end of various > > arch/*/include/asm/atomic.h ? > > In this case, I remove the include <asm/atomic.h> from linux/atomic.h > > > It's quite unobvious *why* `hint' cannot be zero. Can you please add > > the reasoning to the comment? Also, the local `hint' should be > > referred to as "@hint" in a kerneldoc comment. > > Well ;) > > We want to increment the counter if its not zero. > Typically used for refcounts, of RCU protected structures. > > Giving a zero hint would be curious dont you think ? > > I see no usage for this, and using atomic_inc_not_zero() in this case is > the only choice, since we dont ever want to cmpxchg(v, 0, 1) (or risk > double free and crazy things) > > Hmm... maybe I can add a test (compiler should optimize it anyway, since > all usages I can foresee will use a constant hint) > > > > > > +static inline int atomic_inc_not_zero_hint(atomic_t *v, int hint) > > > +{ > > > + int val, c = hint; > > > + > > > + do { > > > + val = atomic_cmpxchg(v, c, c + 1); > > > + if (val == c) > > > + return 1; > > > + c = val; > > > + } while (c); > > > + > > > + return 0; > > > +} > > > > Should/could this have implemented a more general > > atomic_add_not_zero_hint() and made atomic_inc_not_zero_hint() a > > wrapper around that? > > Well, I see no practical use for this, but yes, this could be done. > > As atomic_add_not_zero() doesnt exist, I am not sure we need an > atomic_add_not_zero_hint() yet ? > > > > > Also, it might make sense to add "#ifndef atomic_inc_not_zero_hint" > > wrappers around this function so that the arch can implement an > > overriding custom version. And that becomes a general rule as more > > functions are added to include/linux/atomic.h. > > Ah yes, thats right ! > > Thanks ! > > [PATCH v2] atomic: add atomic_inc_not_zero_hint() > > Followup of perf tools session in Netfilter WorkShop 2010 > > In network stack we make high usage of atomic_inc_not_zero() in contexts > we know the probable value of atomic before increment (2 for udp sockets > for example) > > Using a special version of atomic_inc_not_zero() giving this hint can > help processor to use less bus transactions. > > On x86 (MESI protocol) for example, this avoids entering Shared state, > because "lock cmpxchg" issues an RFO (Read For Ownership) > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > Cc: Christoph Lameter <cl@linux-foundation.org> > Cc: Ingo Molnar <mingo@elte.hu> > Cc: Andi Kleen <andi@firstfloor.org> > Cc: Arnaldo Carvalho de Melo <acme@infradead.org> > Cc: David Miller <davem@davemloft.net> > Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > Cc: Nick Piggin <npiggin@kernel.dk> > --- > V2: add #ifndef atomic_inc_not_zero_hint > kerneldoc changes > test that hint is not null > Meant to be included at end of arch/*/asm/atomic.h files > > include/linux/atomic.h | 36 ++++++++++++++++++++++++++++++++++++ > 1 files changed, 36 insertions(+) > > diff --git a/include/linux/atomic.h b/include/linux/atomic.h > new file mode 100644 > index 0000000..0897bdd > --- /dev/null > +++ b/include/linux/atomic.h > @@ -0,0 +1,36 @@ > +#ifndef _LINUX_ATOMIC_H > +#define _LINUX_ATOMIC_H > + > +/** > + * atomic_inc_not_zero_hint - increment if not null > + * @v: pointer of type atomic_t > + * @hint: probable value of the atomic before the increment > + * > + * This version of atomic_inc_not_zero() gives a hint of probable > + * value of the atomic. This helps processor to not read memory > + * before doing the atomic read/modify/write cycle, lowering > + * number of bus transactions on some arches. > + * Note: @hint MUST not be 0, or increment is not done. > + * Returns: 0 if increment was not done, 1 otherwise. > + */ > +#ifndef atomic_inc_not_zero_hint > +static inline int atomic_inc_not_zero_hint(atomic_t *v, int hint) > +{ > + int val, c = hint; > + > + /* sanity test, should be removed by compiler if hint is a constant */ > + if (!hint) > + return 0; OK, so I cannot resist the challenge... ;-) Suppose that the atomic_inc_not_zero_hint() is in common code that might be invoked from a cleanup path. On the cleanup path, perhaps within an RCU callback, if the reference is zero, we have the only reference and thus don't need to increment the reference count. On the other hand, if the reference is non-zero, we want to obtain a reference in order to safely attempt to encourage the other reference holder to let go more quickly. Perhaps a bit of a stretch, but why not just replace the above "return 0" with "atomic_inc_not_zero(v)"? It will usually be compiled out, right? Thanx, Paul > + do { > + val = atomic_cmpxchg(v, c, c + 1); > + if (val == c) > + return 1; > + c = val; > + } while (c); > + > + return 0; > +} > +#endif > + > +#endif /* _LINUX_ATOMIC_H */ > > -- 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 vendredi 05 novembre 2010 à 11:40 -0700, Paul E. McKenney a écrit : > OK, so I cannot resist the challenge... ;-) > I knew that ;) > Suppose that the atomic_inc_not_zero_hint() is in common code that might > be invoked from a cleanup path. On the cleanup path, perhaps within an > RCU callback, if the reference is zero, we have the only reference and > thus don't need to increment the reference count. On the other hand, > if the reference is non-zero, we want to obtain a reference in order > to safely attempt to encourage the other reference holder to let go > more quickly. > > Perhaps a bit of a stretch, but why not just replace the above > "return 0" with "atomic_inc_not_zero(v)"? It will usually be > compiled out, right? Yes indeed, thanks ! -- 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/include/linux/atomic.h b/include/linux/atomic.h new file mode 100644 index 0000000..0897bdd --- /dev/null +++ b/include/linux/atomic.h @@ -0,0 +1,36 @@ +#ifndef _LINUX_ATOMIC_H +#define _LINUX_ATOMIC_H + +/** + * atomic_inc_not_zero_hint - increment if not null + * @v: pointer of type atomic_t + * @hint: probable value of the atomic before the increment + * + * This version of atomic_inc_not_zero() gives a hint of probable + * value of the atomic. This helps processor to not read memory + * before doing the atomic read/modify/write cycle, lowering + * number of bus transactions on some arches. + * Note: @hint MUST not be 0, or increment is not done. + * Returns: 0 if increment was not done, 1 otherwise. + */ +#ifndef atomic_inc_not_zero_hint +static inline int atomic_inc_not_zero_hint(atomic_t *v, int hint) +{ + int val, c = hint; + + /* sanity test, should be removed by compiler if hint is a constant */ + if (!hint) + return 0; + + do { + val = atomic_cmpxchg(v, c, c + 1); + if (val == c) + return 1; + c = val; + } while (c); + + return 0; +} +#endif + +#endif /* _LINUX_ATOMIC_H */