Date:   Tue, 28 Feb 2023 16:41:24 -0500
From:   Steven Rostedt <rostedt@goodmis.org>
To:     "Paul E. McKenney" <paulmck@kernel.org>
Cc:     Joel Fernandes <joel@joelfernandes.org>,
        Uros Bizjak <ubizjak@gmail.com>, rcu@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Frederic Weisbecker <frederic@kernel.org>,
        Neeraj Upadhyay <quic_neeraju@quicinc.com>,
        Josh Triplett <josh@joshtriplett.org>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Lai Jiangshan <jiangshanlai@gmail.com>
Subject: Re: [PATCH] rcu: use try_cmpxchg in check_cpu_stall
Message-ID: <20230228164124.77c126d2@gandalf.local.home>
In-Reply-To: <20230228212911.GX2948950@paulmck-ThinkPad-P17-Gen-1>
References: <20230228155121.3416-1-ubizjak@gmail.com>
        <Y/5mguXPPqdP3MZF@google.com>
        <20230228160324.2a7c1012@gandalf.local.home>
        <20230228212911.GX2948950@paulmck-ThinkPad-P17-Gen-1>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 28 Feb 2023 13:29:11 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> All well and good, but the stall-warning code is nowhere near a fastpath.
> 
> Is try_cmpxchg() considered more readable in this context?


-	    cmpxchg(&rcu_state.jiffies_stall, js, jn) == js) {
+	    try_cmpxchg(&rcu_state.jiffies_stall, &js, jn)) {

It's basically the same :-/

But looking at this use case, I'd actually NAK it, as it is misleading.

As try_cmpxchg() is used to get rid of the updating of the old value. As in
the ring buffer code we had:

void ring_buffer_record_off(struct trace_buffer *buffer)
{
	unsigned int rd;
	unsigned int new_rd;

	do {
		rd = atomic_read(&buffer->record_disabled);
		new_rd = rd | RB_BUFFER_OFF;
	} while (!atomic_cmpxchg(&buffer->record_disabled, &rd, new_rd) != rd);
}

and the try_cmpxchg() converted it to:

void ring_buffer_record_off(struct trace_buffer *buffer)
{
	unsigned int rd;
	unsigned int new_rd;

	rd = atomic_read(&buffer->record_disabled);
	do {
		new_rd = rd | RB_BUFFER_OFF;
	} while (!atomic_try_cmpxchg(&buffer->record_disabled, &rd, new_rd));
}

Which got rid of the need to constantly update the rd variable (cmpxchg
will load rax with the value read, so it removes the need for an extra
move).

But in your case, we don't need to update js, in which case the
try_cmpxchg() does.

The patch that Uros sent me for the ring buffer code also does some of
that, which I feel is wrong.

So with that, I would nack the patch.

-- Steve