Re: [Possible BUG] count_lim_atomic.c fails on POWER8

["Paul E. McKenney" <paulmck@linux.ibm.com>]

On Sun, Oct 28, 2018 at 08:08:21PM +0800, Junchang Wang wrote:
> On Sun, Oct 28, 2018 at 8:17 AM Paul E. McKenney <paulmck@linux.ibm.com> wrote:
> >
> > On Sat, Oct 27, 2018 at 11:56:54PM +0900, Akira Yokosawa wrote:
> > > On 2018/10/26 08:58:30 +0800, Junchang Wang wrote:
> > > [...]
> > > >
> > > > BTW, I found I'm not good in writing C macro (e.g., cmpxchg). Do you
> > > > know some specification/document on writing C macro functions in
> > > > Linux?
> > >
> > > Although I'm not qualified as a kernel developer,
> > > Linux kernel's "coding style" has a section on this. See:
> > >
> > > https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl
> > >
> > > In that regard, macros I added in commit b2acf6239a95
> > > ("count: Tweak counttorture.h to avoid segfault") do not meet
> > > the style guide in a couple of ways:
> > >
> > >     1) Inline functions are preferable to macros resembling functions
> > >     2) Macros with multiple statements should be enclosed in a do - while block
> > >     3) ...
> > >
> > > Any idea for improving them is more than welcome!
> >
> > Let's see...
> >
> > #define cmpxchg(ptr, o, n) \
> > ({ \
> >         typeof(*ptr) _____actual = (o); \
> >         \
> >         __atomic_compare_exchange_n(ptr, (void *)&_____actual, (n), 1, \
> >                         __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST) ? (o) : (o)+1; \
> > })
> >
> > We cannot do #1 because cmpxchg() is type-generic, and thus cannot be
> > implemented as a C function.  (C++ could use templates, but we are not
> > writing C++ here.)
> >
> > We cannot do #2 because cmpxchg() must return a value.
> >
> > Indentation is not perfect, but given the long names really cannot be
> > improved all that much, if at all.
> >
> > However, we do have a problem, namely the multiple uses of "o", which
> > would be very bad if "o" was an expression with side-effects.
> >
> > How about the following?
> >
> > #define cmpxchg(ptr, o, n) \
> > ({ \
> >         typeof(*ptr) _____old = (o); \
> >         typeof(*ptr) _____actual = _____old; \
> >         \
> >         __atomic_compare_exchange_n(ptr, (void *)&_____actual, (n), 1, \
> >                         __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST)
> >                         ? _____old : _____old + 1; \
> > })
> >
> > This still might have problems with signed integer overflow, but I am
> > inclined to ignore that for the moment.  Because paying attention to it
> > results in something like this:
> >
> > #define cmpxchg(ptr, o, n) \
> > ({ \
> >         typeof(*ptr) _____old = (o); \
> >         typeof(*ptr) _____actual = _____old; \
> >         \
> >         __atomic_compare_exchange_n(ptr, (void *)&_____actual, (n), 1, \
> >                         __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST) \
> >                         ? _____old \
> >                         : _____old > 0 ? _____old - 1; : _____old + 1; \
> > })
> >
> > Thoughts?  Most especially, any better ideas?
> >
> 
> Hi Paul and Akira,
> 
> Thanks a lot for the mail. I have been curious about why cmpxchg()
> sticks to returning the old value to notify the caller if the CAS
> operation succeeds. Besides the overflow issue mentioned in Paul's
> last mail, current cmpxchg() can only be used in the control flow of
> "if CAS fails, do something" (Control Flow 1). However, it cannot be
> used in the control flow of "if CAS succeeds, do something" (Control
> Flow 2).
> 
> So another option is that cmpxchg() could just return true or false,
> and if the caller needs the current value of the content of the
> specified memory address, it could read the value out of field *old*.
> Of course, we must adjust the parameters of cmpxchg() slightly by
> passing the address of *old* to the function. Here is how cmpxchg()
> looks like in my mind:
> 
> #define cmpxchg(ptr, o, n) \
> ({ \
>         __atomic_compare_exchange_n(ptr, (void *)(o), (n), 1, \
>                         __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); \
> })
> 
> static __inline__ int atomic_cmpxchg(atomic_t *v, int *old, int new)
> {
>         return cmpxchg(&v->counter, old, new);
> }
> 
> Any thoughts? Or did I miss something here? I will send the full patch
> in another thread in case you want to review the code.

The reason perfbook's cmpxchg() returns the old value is that the Linux
kernel's cmpxchg() returns the old value.  The reason that the Linux
kernel's cmpxchg() returns the old value is that doing so allows a tighter
retry loop -- it is not necessary to separately reload the current value.
This does mean that one disadvantage of returning a made-up value is
that the next iteration would likely be starting from the wrong value,
but this should not be a real problem given that spurious failure should
be rare.  After all, if spurious failure is not rare, we have way more
serious performance issues.  ;-)

							Thanx, Paul