Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1357774
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [GIT PULL] NOHZ updates for v4.6 |
| Date | 2016-03-15 03:50 +0100 |
| Message-ID | <rcNdw-49L-5@gated-at.bofh.it> (permalink) |
| References | <rczWV-3JY-15@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
On Mon, Mar 14, 2016 at 5:32 AM, Ingo Molnar <mingo@kernel.org> wrote:
> +/**
> + * fetch_or - perform *ptr |= mask and return old value of *ptr
> + * @ptr: pointer to value
> + * @mask: mask to OR on the value
> + *
> + * cmpxchg based fetch_or, macro so it works for different integer types
> + */
> +#ifndef fetch_or
> +#define fetch_or(ptr, mask) \
> +({ typeof(*(ptr)) __old, __val = *(ptr); \
> + for (;;) { \
> + __old = cmpxchg((ptr), __val, __val | (mask)); \
> + if (__old == __val) \
> + break; \
> + __val = __old; \
> + } \
> + __old; \
> +})
> +#endif
This is garbage.
This macro re-uses the "mask" argument potentially many many times, so
semantically it's very dubious.
That may have been acceptable in the old situation when this was
internal to sched.c, but now the code was moved to a generic header
file. And this kind of broken macro is *not* acceptable in that
context any more.
It is now in asm-generic/atomic.h, so it should now conform to the
rest of the code there. Try to model it around ATOMIC_OP_RETURN(),
although obviously this fetch_or() function returns the value *before*
the logical 'or' rather than the end result.
It would be lovely if it were piossible to just use an "atomic_t"
type, but it looks like it is used on thread_info->flags. Which
doesn't have a good type, sadly.
As a result, the code then makes a big deal about how this works with
any integer type, but then the new code that uses it uses a stupid
type that isn't appropriate. Why would it be using an "unsigned long",
when
- it holds a fixed number of bits that don't depend on architecture
and certainly is not 64 (or even close to 32).
- the structure it is in was just preceded by an "int", so on 64-bit
it generates pointless padding in addition to the pointless 64-bit
field.
The only reason to use a "unsigned long" is because "fetch_or()" would
be hardcoded to that type, which doesn't seem to be true.
Now, in practice, the code looks like it should *work* fine, so I'm
going to pull this, but I do want to lodge a protest on sloppiness and
general and unnecessary uglicity of this code.
So please get this fixed.
Linus
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