Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1358153
| From | Frederic Weisbecker <fweisbec@gmail.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' |
| Date | 2016-03-15 18:10 +0100 |
| Message-ID | <rd0DN-4Yj-13@gated-at.bofh.it> (permalink) |
| References | <rczWV-3JY-15@gated-at.bofh.it> <rcNdw-49L-5@gated-at.bofh.it> <rcTCk-bX-35@gated-at.bofh.it> <rcWgP-21h-27@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
Thanks a lot for fixing this!
Some comments below:
On Tue, Mar 15, 2016 at 01:21:45PM +0100, Ingo Molnar wrote:
> Linus noticed a couple of problems with the fetch_or() implementation introduced
> by 5fd7a09cfb8c ("atomic: Export fetch_or()"):
>
> - Sloppy macro implementation: 'mask' and 'ptr' is evaluated multiple times,
> which will break if arguments have side effects. Also, it uses
> double-underscore temporary variables - which is dangerous as low level asm
> ones are using those too and they may alias in unexpected ways.
>
> - Sloppy semantics: the naming and structure of the macro is ambigious, with
> no well-defined semantics. It's neither an atomic nor a cmpxchg() interface,
> but still it lives in include/linux/atomic.h.
>
> Solve this by:
>
> - Extracting the arguments into helper variables once, and never
> using the original arguments from that point on. This makes it
> pretty clear that there can be no unwanted macro evaluation
> side effects.
>
> - Using single-underscore temporary variables.
>
> - Renaming fetch_or() to xchg_or(), recognizing that the semantics
> are xchg()-alike.
I wouldn't mind that much having xchg_or() but I think Peterz has good arguments in favour
of keeping the original name.
>
> Also propagate the rename to the call sites.
>
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Chris Metcalf <cmetcalf@ezchip.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Luiz Capitulino <lcapitulino@redhat.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Link: http://lkml.kernel.org/r/20160315093245.GA7943@gmail.com
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
> include/linux/atomic.h | 26 ++++++++++++++++----------
> kernel/sched/core.c | 2 +-
> kernel/time/tick-sched.c | 4 ++--
> 3 files changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/atomic.h b/include/linux/atomic.h
> index 6c502cb13c95..7bc5297bcca8 100644
> --- a/include/linux/atomic.h
> +++ b/include/linux/atomic.h
> @@ -549,22 +549,28 @@ static inline int atomic_dec_if_positive(atomic_t *v)
> #endif
>
> /**
> - * fetch_or - perform *ptr |= mask and return old value of *ptr
> - * @ptr: pointer to value
> + * xchg_or - perform *ptr |= mask atomically and return old value of *ptr
> + * @ptr: pointer to value (cmpxchg() compatible integer pointer type)
> * @mask: mask to OR on the value
> *
> - * cmpxchg based fetch_or, macro so it works for different integer types
> + * cmpxchg() based, it's a macro so it works for different integer types.
> */
> -#ifndef fetch_or
> -#define fetch_or(ptr, mask) \
> -({ typeof(*(ptr)) __old, __val = *(ptr); \
> +#ifndef xchg_or
> +# define xchg_or(ptr, mask) \
> +({ \
> + typeof(ptr) _ptr = (ptr); \
Can we add a comment above to prevent from future mistakes with cmpxchg
variables aliasing?
I'm suprised that GCC doesn't warn about that actually.
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[GIT PULL] NOHZ updates for v4.6 Ingo Molnar <mingo@kernel.org> - 2016-03-14 13:40 +0100
Re: [GIT PULL] NOHZ updates for v4.6 Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-15 03:50 +0100
Re: [GIT PULL] NOHZ updates for v4.6 Peter Zijlstra <peterz@infradead.org> - 2016-03-15 09:50 +0100
Re: [GIT PULL] NOHZ updates for v4.6 Ingo Molnar <mingo@kernel.org> - 2016-03-15 10:50 +0100
[PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Ingo Molnar <mingo@kernel.org> - 2016-03-15 10:40 +0100
Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Peter Zijlstra <peterz@infradead.org> - 2016-03-15 12:00 +0100
Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Ingo Molnar <mingo@kernel.org> - 2016-03-15 13:10 +0100
Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Peter Zijlstra <peterz@infradead.org> - 2016-03-15 13:50 +0100
Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Peter Zijlstra <peterz@infradead.org> - 2016-03-15 12:10 +0100
Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Ingo Molnar <mingo@kernel.org> - 2016-03-15 13:10 +0100
Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Ingo Molnar <mingo@kernel.org> - 2016-03-15 13:40 +0100
Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Ingo Molnar <mingo@kernel.org> - 2016-03-15 13:40 +0100
Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Peter Zijlstra <peterz@infradead.org> - 2016-03-15 14:20 +0100
Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Peter Zijlstra <peterz@infradead.org> - 2016-03-15 13:30 +0100
[PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Ingo Molnar <mingo@kernel.org> - 2016-03-15 13:30 +0100
Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Peter Zijlstra <peterz@infradead.org> - 2016-03-15 14:30 +0100
Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Ingo Molnar <mingo@kernel.org> - 2016-03-16 09:10 +0100
Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Peter Zijlstra <peterz@infradead.org> - 2016-03-16 09:40 +0100
Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Frederic Weisbecker <fweisbec@gmail.com> - 2016-03-15 18:10 +0100
Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Ingo Molnar <mingo@kernel.org> - 2016-03-16 09:20 +0100
Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Frederic Weisbecker <fweisbec@gmail.com> - 2016-03-17 02:00 +0100
Re: [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-15 17:20 +0100
[PATCH] nohz: Change tick_dep_mask from 'unsigned long' to 'unsigned int' Ingo Molnar <mingo@kernel.org> - 2016-03-15 11:00 +0100
Re: [PATCH] nohz: Change tick_dep_mask from 'unsigned long' to 'unsigned int' Ingo Molnar <mingo@kernel.org> - 2016-03-15 13:20 +0100
Re: [PATCH] nohz: Change tick_dep_mask from 'unsigned long' to 'unsigned int' Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-15 17:40 +0100
Re: [PATCH] nohz: Change tick_dep_mask from 'unsigned long' to 'unsigned int' Frederic Weisbecker <fweisbec@gmail.com> - 2016-03-15 18:30 +0100
Re: [PATCH] nohz: Change tick_dep_mask from 'unsigned long' to 'unsigned int' Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-15 18:40 +0100
csiph-web