Re: [PATCH] rcu: use try_cmpxchg in check_cpu_stall

[Joel Fernandes]

> On Mar 1, 2023, at 3:08 PM, Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
> 
> On Wed, Mar 01, 2023 at 07:43:34PM +0100, Uros Bizjak wrote:
>>> On Wed, Mar 1, 2023 at 5:38 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>>> 
>>> On Wed, 1 Mar 2023 11:28:47 +0100
>>> Uros Bizjak <ubizjak@xxxxxxxxx> wrote:
>>> 
>>>> 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].
>>> 
>>> 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 the
>>> old variable.
>>> 
>>> I think that would allow for the arch optimizations that you are trying to
>>> achieve, as well as remove the side effect that might cause issues down the
>>> road.
>> 
>> Attached patch implements this suggestion.
> 
> Please help me out here.
> 
> Why on earth are we even discussing making this change to code that
> normally never executes?  Performance is not a consideration here.
> 
> 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 further
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 


> 
>                            Thanx, Paul
> 
>> Uros.
> 
>> 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();
>> }
>> 
>> +#define cmpxchg_success(ptr, old, new)                \
>> +({                                \
>> +    __typeof__(*(ptr)) __tmp = (old);            \
>> +    try_cmpxchg((ptr), &__tmp, (new));            \
>> +})
>> +
>> static void check_cpu_stall(struct rcu_data *rdp)
>> {
>>    bool didstall = false;
>> @@ -760,7 +766,7 @@ static void check_cpu_stall(struct rcu_data *rdp)
>>    jn = jiffies + ULONG_MAX / 2;
>>    if (rcu_gp_in_progress() &&
>>        (READ_ONCE(rnp->qsmask) & rdp->grpmask) &&
>> -        cmpxchg(&rcu_state.jiffies_stall, js, jn) == js) {
>> +        cmpxchg_success(&rcu_state.jiffies_stall, js, jn)) {
>> 
>>        /*
>>         * 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)
>> 
>>    } else if (rcu_gp_in_progress() &&
>>           ULONG_CMP_GE(j, js + RCU_STALL_RAT_DELAY) &&
>> -           cmpxchg(&rcu_state.jiffies_stall, js, jn) == js) {
>> +           cmpxchg_success(&rcu_state.jiffies_stall, js, jn)) {
>> 
>>        /*
>>         * If a virtual machine is stopped by the host it can look to
>