Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   Joel Fernandes <joel@joelfernandes.org>
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH] rcu: use try_cmpxchg in check_cpu_stall
Date:   Wed, 1 Mar 2023 15:18:18 -0500
Message-Id: <8075146A-38A7-4CF0-9809-4E40BD120BF1@joelfernandes.org>
References: <20230301200820.GF2948950@paulmck-ThinkPad-P17-Gen-1>
Cc:     Uros Bizjak <ubizjak@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>, 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>
In-Reply-To: <20230301200820.GF2948950@paulmck-ThinkPad-P17-Gen-1>
To:     paulmck@kernel.org
Precedence: bulk


> On Mar 1, 2023, at 3:08 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
>=20
> =EF=BB=BFOn Wed, Mar 01, 2023 at 07:43:34PM +0100, Uros Bizjak wrote:
>>> On Wed, Mar 1, 2023 at 5:38=E2=80=AFPM Steven Rostedt <rostedt@goodmis.o=
rg> wrote:
>>>=20
>>> On Wed, 1 Mar 2023 11:28:47 +0100
>>> Uros Bizjak <ubizjak@gmail.com> wrote:
>>>=20
>>>> These benefits are the reason the change to try_cmpxchg was accepted
>>>> also in the linear code elsewhere in the linux kernel, e.g. [2,3] to
>>>> name a few commits, with a thumbs-up and a claim that the new code is
>>>> actually *clearer* at the merge commit [4].
>>>=20
>>> I'll say it here too. I really like Joel's suggestion of having a
>>> cmpxchg_success() that does not have the added side effect of modifying t=
he
>>> old variable.
>>>=20
>>> I think that would allow for the arch optimizations that you are trying t=
o
>>> achieve, as well as remove the side effect that might cause issues down t=
he
>>> road.
>>=20
>> Attached patch implements this suggestion.
>=20
> Please help me out here.
>=20
> Why on earth are we even discussing making this change to code that
> normally never executes?  Performance is not a consideration here.
>=20
> What am I missing here?  Is there some sort of forward-progress
> issue that this change addresses?

I do not think it is anything with performance. The suggestion just makes
the code easier to read. In the case of ftrace (not RCU), it results in furt=
her
deleted lines of code.

Maybe it got confusing because we are discussing the change as it
applies to both ftrace and RCU.

You could argue that it has to do with performance in the fast path, but it
is probably down in the noise.

 - Joel=20


>=20
>                            Thanx, Paul
>=20
>> Uros.
>=20
>> diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h
>> index b10b8349bb2a..229263ebba3b 100644
>> --- a/kernel/rcu/tree_stall.h
>> +++ b/kernel/rcu/tree_stall.h
>> @@ -709,6 +709,12 @@ static void print_cpu_stall(unsigned long gps)
>>    set_preempt_need_resched();
>> }
>>=20
>> +#define cmpxchg_success(ptr, old, new)                \
>> +({                                \
>> +    __typeof__(*(ptr)) __tmp =3D (old);            \
>> +    try_cmpxchg((ptr), &__tmp, (new));            \
>> +})
>> +
>> static void check_cpu_stall(struct rcu_data *rdp)
>> {
>>    bool didstall =3D false;
>> @@ -760,7 +766,7 @@ static void check_cpu_stall(struct rcu_data *rdp)
>>    jn =3D jiffies + ULONG_MAX / 2;
>>    if (rcu_gp_in_progress() &&
>>        (READ_ONCE(rnp->qsmask) & rdp->grpmask) &&
>> -        cmpxchg(&rcu_state.jiffies_stall, js, jn) =3D=3D js) {
>> +        cmpxchg_success(&rcu_state.jiffies_stall, js, jn)) {
>>=20
>>        /*
>>         * If a virtual machine is stopped by the host it can look to
>> @@ -778,7 +784,7 @@ static void check_cpu_stall(struct rcu_data *rdp)
>>=20
>>    } else if (rcu_gp_in_progress() &&
>>           ULONG_CMP_GE(j, js + RCU_STALL_RAT_DELAY) &&
>> -           cmpxchg(&rcu_state.jiffies_stall, js, jn) =3D=3D js) {
>> +           cmpxchg_success(&rcu_state.jiffies_stall, js, jn)) {
>>=20
>>        /*
>>         * If a virtual machine is stopped by the host it can look to
>=20