On Wed, 6 Mar 2024 11:27:04 -0800 Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > On Wed, 6 Mar 2024 at 11:01, Linus Torvalds > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > In some individual tracing C file where it has a comment above it how > > it's braindamaged and unsafe and talking about why it's ok in that > > particular context? Go wild. > > Actually, I take that back. > > Even in a random C file, the naming makes no sense. There's no "once" about it. > > So if you want to do something like > > #define UNSAFE_INCREMENTISH(x) (WRITE_ONCE(a, READ_ONCE(a) + 1)) > > then that's fine, I guess. Because that's what the operation is. > > It's not safe, and it's not an increment, but it _approximates_ an > increment most of the time. So UNSAFE_INCREMENTISH() pretty much > perfectly describes what it is doing. > > Note that you'll also almost certainly end up with worse code > generation, ie don't expect to see a single "inc" instruction (or "add > $1") for the above. > > Because at least for gcc, the volatiles involved with those "ONCE" > operations end up often generating much worse code, so rather than an > "inc" instruction, you'll almost certainly get "load+add+store" and > the inevitable code expansion and extra register use. > > I really don't know what you want to do, but it smells bad. A big > comment about why you'd want that "incrementish" operation will be > needed. > > To me, this smells like "Steven did something fundamentally wrong > again, some tool is now complaining about it, and Steven doesn't want > to fix the problem but instead paper over it again". > > Not a good look. > > But I don't have a link to the original report, and I'm not thrilled > enough about this to go looking for it. Well, it's not the above, and I was afraid you would even think that. Here's the back story. I received the following patch: https://lore.kernel.org/all/tencent_BA1473492BC618B473864561EA3AB1418908@xxxxxx/ I didn't like it. My reply was: > - rbwork->wait_index++; > + WRITE_ONCE(rbwork->wait_index, READ_ONCE(rbwork->wait_index) + 1); I mean the above is really ugly. If this is the new thing to do, we need better macros. If anything, just convert it to an atomic_t. Then because I'm a reviewer of RCU patches, I saw the same fix for RCU (this thread): https://lore.kernel.org/all/tencent_B51A9DA220288A95A435E3435A0443BEB007@xxxxxx/ Where it was recommended to do the same WRITE_ONCE(a, READ_ONCE(a) + 1), and this is when I Cc'd you into the conversation to get your view of the situation. Sounds like my original gut feeling that this is a non-issue is proving to be correct. I even argued against using the _ONCE() macros. If WRITE_ONCE(a, READ_ONCE(a) + 1) is a valid operation, I sure as hell don't want it in my code, but I would be OK if it had a macro wrapper. Which you seem to be against, so I'm against it too. -- Steve
