Re: [PATCH] rcutorture: Fix rcu_torture_pipe_update_one()/rcu_torture_writer() data race and concurrency bug

[Steven Rostedt]

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