Re: [GIT PULL] NOHZ updates for v4.6

[Linus Torvalds]

On Mon, Mar 14, 2016 at 5:32 AM, Ingo Molnar <mingo@xxxxxxxxxx> 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