Received-SPF: pass (google.com: domain of linux-kernel+bounces-94500-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 6 Mar 2024 14:47:13 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>, linke li <lilinke99@qq.com>,
 joel@joelfernandes.org, boqun.feng@gmail.com, dave@stgolabs.net,
 frederic@kernel.org, jiangshanlai@gmail.com, josh@joshtriplett.org,
 linux-kernel@vger.kernel.org, mathieu.desnoyers@efficios.com,
 qiang.zhang1211@gmail.com, quic_neeraju@quicinc.com, rcu@vger.kernel.org
Subject: Re: [PATCH] rcutorture: Fix
 rcu_torture_pipe_update_one()/rcu_torture_writer() data race and
 concurrency bug
Message-ID: <20240306144713.2e1709ad@gandalf.local.home>
In-Reply-To: <CAHk-=whvyzsbqLkw4kwEC-KzAMKFuy5VuMtJFn-mCerSYoRnqw@mail.gmail.com>
References: <f3624f39-bbb1-451d-8161-8518e4108d8e@joelfernandes.org>
	<tencent_9882B228F292088CDD68F10CF1C228742009@qq.com>
	<20240306103719.1d241b93@gandalf.local.home>
	<27665890-8314-4252-8622-1e019fee27e4@paulmck-laptop>
	<20240306130103.6da71ddf@gandalf.local.home>
	<CAHk-=wgG6Dmt1JTXDbrbXh_6s2yLjL=9pHo7uv0==LHFD+aBtg@mail.gmail.com>
	<20240306135504.2b3872ef@gandalf.local.home>
	<CAHk-=wjbDgMKLgxbV+yK4LKZ+2Qj6zVL_sHeb+L9KDia980Q8Q@mail.gmail.com>
	<CAHk-=whvyzsbqLkw4kwEC-KzAMKFuy5VuMtJFn-mCerSYoRnqw@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Wed, 6 Mar 2024 11:27:04 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, 6 Mar 2024 at 11:01, Linus Torvalds
> <torvalds@linux-foundation.org> 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@qq.com/

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@qq.com/

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