On Mon, Oct 29, 2018 at 11:45:15PM +0900, Akira Yokosawa wrote: > On 2018/10/28 09:43:55 -0700, Paul E. McKenney wrote: > > On Sun, Oct 28, 2018 at 11:24:41PM +0900, Akira Yokosawa wrote: > >> On 2018/10/27 17:17:23 -0700, Paul E. McKenney 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; \ > >>> }) > >> > >> Oh, my concern was macros I added in counttorture.h to support > >> #ifndef KEEP_GCC_THREAD_LOCAL. > >> > >> But those macros are used solely in the header file, so the current > >> definition might be good enough. > > > > These ones? > > > > #define _wait_all_threads() { \ > > while (READ_ONCE(finalthreadcount) < nthreadsexpected) \ > > poll(NULL, 0, 1);} > > #define _count_unregister_thread(n) count_unregister_thread(n + 1) > > #define final_wait_all_threads() { \ > > WRITE_ONCE(finalthreadcount, nthreadsexpected + 1); \ > > wait_all_threads();} > > > > The _count_unregister_thread() is fine. > > Well, don't we need to protect "n"? That is, > > #define _count_unregister_thread(n) count_unregister_thread((n) + 1) Indeed we do! Color me blind, and good catch!!! > > The _wait_all_threads() and final_wait_all_threads() are fine given their > > current usage. One not-yet-needed way to future-proof them would be as > > follows: > > > > #define _wait_all_threads() \ > > do { \ > > while (READ_ONCE(finalthreadcount) < nthreadsexpected) \ > > poll(NULL, 0, 1); \ > > } while (0) > > #define final_wait_all_threads() \ > > do { \ > > WRITE_ONCE(finalthreadcount, nthreadsexpected + 1); \ > > wait_all_threads(); \ > > } while (0) > > > > Your next question is "why does this matter", to which I would point you > > here: https://kernelnewbies.org/FAQ/DoWhile0 > > This is exactly what I've wanted to know! > > > Mostly because I never > > can remember all of the failure cases that led to the Linux-kernel > > coding-style rules. ;-) > > > >> OTH, macros defined in api-gcc.h should be made as robust as possible. > >> Hence your review of cmpxchg() is quite instructive to me. > > > > Glad it helped! > > > >>> 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. > >> > >> These reasoning might not be obvious to those who are new to > >> C preprocessor programming. Current style guide of kernel doesn't > >> look good enough, partly because of its intended audiences. > > > > To be fair, it doesn't get updated as often as it should. > > > >>> 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. > >> > >> I didn't notice this point. > > > > Neither did I earlier in this thread. ;-) > > > >>> 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. > >> > >> Behavior of overflow of signed integer is undefined in C standard, right? > > > > Exactly. An alternative approach is to do as the Linux kernel does and > > tell gcc to wrap signed integers on overflow, as the standard mandates > > for unsigned integers. > > So, GCC provides such an option. Let's see... > > Ah, include/linux/overflow.h has a variety of helper macros. > They are hard to grasp in a short while, though! Maybe check_add_overflow()? > > My preference would be to avoid signed overflow. > > > >>> 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? > >> > >> Let me think this over. > >> > >> BTW, the purpose of using the name "_____old" and the like may not > >> be obvious either. > >> If we use "old" instead, naming collision can happen if "old" is given > >> as an argument to this macro in its call sites. Am I guessing right? > > > > You got it exactly right! The "old" in the argument is intended to > > refer to something in an outer scope, but it would instead refer to > > the macro's local variable. > > Which might not end up in compile error, and could be hard to track down. > Whew! It can be quite nasty! ;-) Thanx, Paul
