Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b8ec5185-f167-0b11-b823-51ba4fc3ca4f@redhat.com>
Date:   Wed, 14 Jun 2023 22:57:22 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH] locking/osq_lock: Fix false sharing of
 optimistic_spin_node in osq_lock
Content-Language: en-US
From:   Waiman Long <longman@redhat.com>
To:     Guo Hui <guohui@uniontech.com>, peterz@infradead.org,
        mingo@redhat.com, will@kernel.org, boqun.feng@gmail.com
Cc:     linux-kernel@vger.kernel.org, wangxiaohua@uniontech.com
References: <20230615012437.21087-1-guohui@uniontech.com>
 <9f5321f9-0b35-7f85-ee09-247d961e5a79@redhat.com>
In-Reply-To: <9f5321f9-0b35-7f85-ee09-247d961e5a79@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 6/14/23 21:58, Waiman Long wrote:
>> diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
>> index d5610ad52b92..bdcd216b73c4 100644
>> --- a/kernel/locking/osq_lock.c
>> +++ b/kernel/locking/osq_lock.c
>> @@ -24,7 +24,7 @@ static inline int encode_cpu(int cpu_nr)
>>     static inline int node_cpu(struct optimistic_spin_node *node)
>>   {
>> -    return node->cpu - 1;
>> +    return node->prev_cpu - 1;
>>   }
>
> From the first glance, the patch looks OK. The only issue that I have 
> so far is the "node_cpu" name which is misleading in this case. You 
> should have renamed it to node_prev_cpu() instead as there is only one 
> caller.

One more thing. Unlike node->cpu, node->prev_cpu is subjected to change. 
So you should have use READ_ONCE() to read it to match the WRITE_ONCE() 
that modifies it.

Thanks,
Longman