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) > > 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! > 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! Thanks, Akira > > Thanx, Paul >
