From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.loongson.cn (mail.loongson.cn [114.242.206.163]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 56A83DDD3 for ; Fri, 27 Dec 2024 02:02:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=114.242.206.163 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735264943; cv=none; b=QaijGSzAzuqJMC9xfJUUC1Gb1Vjq7vAVfxBfPGt47p4QVLhqomT1CiDGRBm536ZHh33Hj3KM0NxrYupjp1L6nI9e0KthlUTHXn2s41//pPFVSVUYOzXY5eDRS9l+AG2KgZ6R6MdlOcBU85KyJ2qStvDFciSLVqje7ne5rbzwIms= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735264943; c=relaxed/simple; bh=1MC7XNfZLp6ajqvDcPCQ0sIpESh9yc1QgC7knrURN20=; h=Subject:To:Cc:References:From:Message-ID:Date:MIME-Version: In-Reply-To:Content-Type; b=ZdepfNB1d9yWdyL8+coP4Xwj0wT2k5LBuTlAGqtMUefu7LsdPBgG6uTydUwMKZIgOI0JGQ1ZnugCw0HxzjN5mrh2GqWfr1UGTJw+Xxo5uTOUKlADp9C0n2A7yJ6ipLafteqP2pvwMHtC7saSjBBpOqEG9ILbXaRHXYZ9rALDxBw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=loongson.cn; spf=pass smtp.mailfrom=loongson.cn; arc=none smtp.client-ip=114.242.206.163 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=loongson.cn Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=loongson.cn Received: from loongson.cn (unknown [10.20.42.62]) by gateway (Coremail) with SMTP id _____8DxGeFZCm5nsc9aAA--.47687S3; Fri, 27 Dec 2024 10:00:58 +0800 (CST) Received: from [10.20.42.62] (unknown [10.20.42.62]) by front1 (Coremail) with SMTP id qMiowMDxfcdWCm5nIXwKAA--.55757S3; Fri, 27 Dec 2024 10:00:56 +0800 (CST) Subject: Re: [PATCH] locking/pvqspinlock: Use try_cmpxchg() in pv_unhash To: Waiman Long , Ingo Molnar , Will Deacon Cc: Boqun Feng , linux-kernel@vger.kernel.org, Akira Yokosawa References: <20241223074704.18857-1-maobibo@loongson.cn> <7e2daa4d-6f96-4189-9d86-644614f895e1@redhat.com> From: bibo mao Message-ID: <9b76aa09-532f-e2c5-8cf8-c732a079c5eb@loongson.cn> Date: Fri, 27 Dec 2024 10:00:07 +0800 User-Agent: Mozilla/5.0 (X11; Linux loongarch64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 In-Reply-To: <7e2daa4d-6f96-4189-9d86-644614f895e1@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-CM-TRANSID:qMiowMDxfcdWCm5nIXwKAA--.55757S3 X-CM-SenderInfo: xpdruxter6z05rqj20fqof0/ X-Coremail-Antispam: 1Uk129KBj93XoWxAw1kCryxKF1DZF1UCw4fCrX_yoWruryDpr y8tFW7GrWkJr1kZw1Dt3WUZryUXw1DAa1UGrn7XF1UAr1UJr1jgryUXr1j9r1UAr4xGr1U GFyUXrn0vr1DXwcCm3ZEXasCq-sJn29KB7ZKAUJUUUUr529EdanIXcx71UUUUU7KY7ZEXa sCq-sGcSsGvfJ3Ic02F40EFcxC0VAKzVAqx4xG6I80ebIjqfuFe4nvWSU5nxnvy29KBjDU 0xBIdaVrnRJUUUPYb4IE77IF4wAFF20E14v26r1j6r4UM7CY07I20VC2zVCF04k26cxKx2 IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rwA2F7IY1VAKz4vEj48v e4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_JFI_Gr1l84ACjcxK6xIIjxv20xvEc7CjxVAFwI 0_Jr0_Gr1l84ACjcxK6I8E87Iv67AKxVW8JVWxJwA2z4x0Y4vEx4A2jsIEc7CjxVAFwI0_ Gr0_Gr1UM2kKe7AKxVWUXVWUAwAS0I0E0xvYzxvE52x082IY62kv0487Mc804VCY07AIYI kI8VC2zVCFFI0UMc02F40EFcxC0VAKzVAqx4xG6I80ewAv7VC0I7IYx2IY67AKxVWUXVWU AwAv7VC2z280aVAFwI0_Jr0_Gr1lOx8S6xCaFVCjc4AY6r1j6r4UM4x0Y48IcVAKI48JMx k0xIA0c2IEe2xFo4CEbIxvr21lc7CjxVAaw2AFwI0_JF0_Jw1l42xK82IYc2Ij64vIr41l 4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1l4IxYO2xFxVAFwI0_Jrv_JF1lx2IqxVAqx4xG67AKxV WUJVWUGwC20s026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v26r126r1DMIIYrxkI 7VAKI48JMIIF0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2IY6xkF7I0E14v26r 1j6r4UMIIF0xvE42xK8VAvwI8IcIk0rVWUJVWUCwCI42IY6I8E87Iv67AKxVWUJVW8JwCI 42IY6I8E87Iv6xkF7I0E14v26r1j6r4UYxBIdaVFxhVjvjDU0xZFpf9x07j1tC7UUUUU= On 2024/12/26 上午12:04, Waiman Long wrote: > On 12/23/24 2:47 AM, Bibo Mao wrote: >> We ported pv spinlock to old linux kernel on LoongArch platform, there is > What old kernel are you using? Race condition like that should be hard > to reproduce. Do you hit this bug only once? >> error with some stress tests. The error report is something like this for >> short: >>   kernel BUG at kernel/locking/qspinlock_paravirt.h:261! >>   Oops - BUG[#1]: >>   CPU: 1 PID: 6613 Comm: pidof Not tainted 4.19.190+ #43 >>   Hardware name: Loongson KVM, BIOS 0.0.0 02/06/2015 >>      ra: 9000000000509cfc do_task_stat+0x29c/0xaf0 >>     ERA: 9000000000291308 __pv_queued_spin_unlock_slowpath+0xf8/0x100 >>    CRMD: 000000b0 (PLV0 -IE -DA +PG DACF=CC DACM=CC -WE) >>    PRMD: 00000000 (PPLV0 -PIE -PWE) >>           ... >>   Call Trace: >>   [<9000000000291308>] __pv_queued_spin_unlock_slowpath+0xf8/0x100 >>   [<9000000000509cf8>] do_task_stat+0x298/0xaf0 >>   [<9000000000502570>] proc_single_show+0x60/0xe0 >> >> The problem is that memory accessing is out of order on LoongArch >> platform, there is contension between pv_unhash() and pv_hash(). >> >> CPU0 pv_unhash:                                CPU1 pv_hash: >> >>    for_each_hash_entry(he, offset, hash) { >> for_each_hash_entry(he, offset, hash) { >>      if (READ_ONCE(he->lock) == lock) {             struct qspinlock >> *old = NULL; >>        node = READ_ONCE(he->node); >>        WRITE_ONCE(he->lock, NULL); >> >> On LoongArch platform which is out of order, the execution order may be >> switched like this: >>>       WRITE_ONCE(he->lock, NULL); >>                                                      if >> (try_cmpxchg(&he->lock, &old, lock)) { >> >> WRITE_ONCE(he->node, node); >>                                                        return &he->lock; >> >> CPU1 pv_hash() is executing and watch that lock is set with NULL. Write >> he->node with node of new lock. >>>       node = READ_ONCE(he->node); >> READ_ONCE(he->node) on CPU0 will return node of new lock rather than >> itself. > The pv_hash_entry structure is supposed to be cacheline aligned. The > use  of READ_ONCE/WRITE_ONCE will ensure that compiler won't rearrange > the ordering. If the CPU can rearrange read/write ordering on the same > cacheline like that, it may be some advance optimization technique that > I don't  quite understand. Can you check if the buffer returned by > alloc_large_system_hash() is really properly aligned? Yes, you are right. After communicating with HW guys, WRITE_ONCE will not surpass over READ_ONCE, WRITE_ONCE can only surpass WRITE_ONCE. And buffer allocated by alloc_large_system_hash() is cache aligned also, the problem may be LoongArch VM specific, I am investigating the detailed reason. >> >> Here READ_ONCE/WRITE_ONCE is replaced with try_cmpxchg(). >> >> Signed-off-by: Bibo Mao >> --- >>   kernel/locking/qspinlock_paravirt.h | 5 +++-- >>   1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/locking/qspinlock_paravirt.h >> b/kernel/locking/qspinlock_paravirt.h >> index dc1cb90e3644..cc4eabce092d 100644 >> --- a/kernel/locking/qspinlock_paravirt.h >> +++ b/kernel/locking/qspinlock_paravirt.h >> @@ -240,9 +240,10 @@ static struct pv_node *pv_unhash(struct qspinlock >> *lock) >>       struct pv_node *node; >>       for_each_hash_entry(he, offset, hash) { >> -        if (READ_ONCE(he->lock) == lock) { >> +        struct qspinlock *old = lock; >> + >> +        if (try_cmpxchg(&he->lock, &old, NULL)) { >>               node = READ_ONCE(he->node); >> -            WRITE_ONCE(he->lock, NULL); >>               return node; >>           } >>       } > > Anyway, this change isn't quite right as suggested by Akira. This change is abuse usage about try_cmpxchg(), the origin code maybe has no problem, I am testing again. Sorry for the noise and happy new year holiday :) Regards Bibo Mao > > Cheers, > Longman > >> >> base-commit: 48f506ad0b683d3e7e794efa60c5785c4fdc86fa