Date:   Tue, 28 Feb 2023 19:08:46 -0500
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Joel Fernandes <joel@joelfernandes.org>
Cc:     "Paul E. McKenney" <paulmck@kernel.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: <20230228190846.79b06089@gandalf.local.home>
In-Reply-To: <CAEXW_YQ515_DOLVUm48GvDADuaY2mSrYTaKa7u6jYDNqBncJww@mail.gmail.com>
References: <20230228155121.3416-1-ubizjak@gmail.com>
        <Y/5mguXPPqdP3MZF@google.com>
        <20230228160324.2a7c1012@gandalf.local.home>
        <20230228212911.GX2948950@paulmck-ThinkPad-P17-Gen-1>
        <20230228164124.77c126d2@gandalf.local.home>
        <CAEXW_YQ515_DOLVUm48GvDADuaY2mSrYTaKa7u6jYDNqBncJww@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 28 Feb 2023 18:30:14 -0500
Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> > But looking at this use case, I'd actually NAK it, as it is misleading.  
> 
> I'm trying to parse this. You are saying it is misleading, because it
> updates js when it doesn't need to?

Correct.

> 
> > 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);  
> 
> Hear you actually meant "rd" as the second parameter without the & ?

Yes, I cut and pasted the updated code and incorrectly try to revert it in
this example :-p

> 
> > }
> >
> > 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).  
> 
> So that's a good thing?

Yes. For looping, try_cmpxchg() is the proper function to use. But in the
RCU case (and other cases in the ring-buffer patch) there is no loop, and
no need to modify the "old" variable.

> 
> >
> > But in your case, we don't need to update js, in which case the
> > try_cmpxchg() does.  
> 
> Right, it has lesser value here but I'm curious why you feel it also
> doesn't belong in that ring buffer loop you shared (or did you mean,
> it does belong there but not in other ftrace code modified by Uros?).

The ring buffer patch had more than one change, where half the updates were
fine, and half were not.

-- Steve