Message ID | 1319987765.13597.60.camel@edumazet-laptop |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
On Sun, Oct 30, 2011 at 8:16 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > Because it doesnt work if x is const. Just remove the const. Problem solved. Both cases of 'const' are totally arbitrary and useless. The test_bit() one is literally a cast to const (admittedly also *from* const, but nobody cares), and the atomic_read() one is just because it uses a silly inline function where a macro would be simpler. Linus -- 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 dimanche 30 octobre 2011 à 10:07 -0700, Linus Torvalds a écrit : > On Sun, Oct 30, 2011 at 8:16 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > Because it doesnt work if x is const. > > Just remove the const. Problem solved. > > Both cases of 'const' are totally arbitrary and useless. The > test_bit() one is literally a cast to const (admittedly also *from* > const, but nobody cares), and the atomic_read() one is just because it > uses a silly inline function where a macro would be simpler. > Oh well, I am lost. I always considered inline functione better because of prototype checks. Changing atomic_read(const atomic_t *v) prototype to atomic_read(atomic_t *v) is not an option. To save your time and my time, please select your favorite between : 1) The patch I did 2) static inline int atomic_read(const atomic_t *v) { return ACCESS_AT_MOST_ONCE(((atomic_t *)v)->counter); } 3) static inline int atomic_read(const atomic_t *v) { return ACCESS_AT_MOST_ONCE(*(int *)&(v)->counter); } 4) macro (I personnaly dont like it) #define atomic_read(v) ACCESS_AT_MOST_ONCE(*(int *)&(v)->counter) 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
On Sun, Oct 30, 2011 at 10:41 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > Changing atomic_read(const atomic_t *v) prototype to > atomic_read(atomic_t *v) is not an option. Why not? #define atomic_read(v) ACCESS_AT_MOST_ONCE((v)->counter) seems to be the cleanest thing. And if you don't think this is "an option", I really can't see why you care about the extra instructions in the code stream either. > 4) macro (I personnaly dont like it) > #define atomic_read(v) ACCESS_AT_MOST_ONCE(*(int *)&(v)->counter) Why the *hell* would you have that cast there? If somebody passes "const atomic_t"'s around, then just shoot the bastard. The concept makes no sense. Grepping for "const atomic_t" shows absolutely *zero* users, except for the crazy inline function declaration itself. Stop the insanity already. Get rid of the f*cking "const". Linus -- 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 dimanche 30 octobre 2011 à 10:48 -0700, Linus Torvalds a écrit : > On Sun, Oct 30, 2011 at 10:41 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > Changing atomic_read(const atomic_t *v) prototype to > > atomic_read(atomic_t *v) is not an option. > > Why not? > > #define atomic_read(v) ACCESS_AT_MOST_ONCE((v)->counter) > > seems to be the cleanest thing. > As I said, because v can be a const pointer provided by the caller. Try it yourself and you'll discover hundred of call sites doing .... some_function(const struct *xxx, ...) { if (atomic_read(&xxx->refcnt) <= 0) do_something(); else do_otherthing(); } > And if you don't think this is "an option", I really can't see why you > care about the extra instructions in the code stream either. > Not an option if we have to change all callers that expected to be able to use a const atomic_t pointer. OK, I now have to leave the net. -- 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 Sun, Oct 30, 2011 at 10:59 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > As I said, because v can be a const pointer provided by the caller. > > Try it yourself and you'll discover hundred of call sites doing > > .... some_function(const struct *xxx, ...) > { > if (atomic_read(&xxx->refcnt) <= 0) > do_something(); Argh. Ok. Testing a refcount in a const struct doesn't make much sense, but there does seem to be perfectly valid uses of it (sk_wmem_alloc etc). Annoying. I guess we have to have those casts. Grr. Linus -- 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/arch/x86/include/asm/atomic.h b/arch/x86/include/asm/atomic.h index 58cb6d4..b1f0c6b 100644 --- a/arch/x86/include/asm/atomic.h +++ b/arch/x86/include/asm/atomic.h @@ -22,7 +22,7 @@ */ static inline int atomic_read(const atomic_t *v) { - return (*(volatile int *)&(v)->counter); + return ACCESS_AT_MOST_ONCE(v->counter); } /** diff --git a/arch/x86/include/asm/atomic64_64.h b/arch/x86/include/asm/atomic64_64.h index 0e1cbfc..bdca6fa 100644 --- a/arch/x86/include/asm/atomic64_64.h +++ b/arch/x86/include/asm/atomic64_64.h @@ -18,7 +18,7 @@ */ static inline long atomic64_read(const atomic64_t *v) { - return (*(volatile long *)&(v)->counter); + return ACCESS_AT_MOST_ONCE(v->counter); } /** diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h index 1775d6e..e30a190 100644 --- a/arch/x86/include/asm/bitops.h +++ b/arch/x86/include/asm/bitops.h @@ -308,8 +308,11 @@ static inline int test_and_change_bit(int nr, volatile unsigned long *addr) static __always_inline int constant_test_bit(unsigned int nr, const volatile unsigned long *addr) { - return ((1UL << (nr % BITS_PER_LONG)) & - (addr[nr / BITS_PER_LONG])) != 0; + const unsigned long *word = (const unsigned long *)addr + + (nr / BITS_PER_LONG); + unsigned long bit = 1UL << (nr % BITS_PER_LONG); + + return (bit & ACCESS_AT_MOST_ONCE(*word)) != 0; } static inline int variable_test_bit(int nr, volatile const unsigned long *addr) diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h index e37963c..c05e21f 100644 --- a/include/asm-generic/atomic.h +++ b/include/asm-generic/atomic.h @@ -39,7 +39,7 @@ * Atomically reads the value of @v. */ #ifndef atomic_read -#define atomic_read(v) (*(volatile int *)&(v)->counter) +#define atomic_read(v) ACCESS_AT_MOST_ONCE((v)->counter) #endif /** diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 320d6c9..bd18562 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -308,4 +308,19 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect); */ #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x)) +#ifndef __ASSEMBLY__ +/* + * Like ACCESS_ONCE, but can be optimized away if nothing uses the value, + * and/or merged with previous non-ONCE accesses. + */ +extern void ACCESS_AT_MOST_ONCE_bad(void); +#define ACCESS_AT_MOST_ONCE(x) \ + ({ unsigned long __y; \ + if (sizeof(x) > sizeof(__y)) \ + ACCESS_AT_MOST_ONCE_bad(); \ + asm("":"=r" (__y):"0" (x)); \ + (__force __typeof__(x)) __y; \ + }) +#endif /* __ASSEMBLY__ */ + #endif /* __LINUX_COMPILER_H */