From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3AAB61DFE1 for ; Wed, 25 Dec 2024 20:53:10 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735159993; cv=none; b=QxLir0x2n2rKynKMLBRXK3yIki5KmZiE4Hd/obT8Ru0xkhL6LZJHYSOuITiRJf0olg00wgt9X+RY24jkkm+NjtzQygeZ3PK8naMjBnaUmMRBPByjH7mjGFnL+ULzsH6e1YRCsY3dcQQgSNVg3NImIQlc7TnkP83LLvbSYEunXOg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735159993; c=relaxed/simple; bh=EYPtwdTzDjwlgF3WYKrpqQN2Wi6Qr4lpsai0Gpm1E3Y=; h=From:Message-ID:Date:MIME-Version:Subject:To:Cc:References: In-Reply-To:Content-Type; b=Hb6JRxx6UCouWnk2qsS1CvCyj2XGsrGmHkQvcWCo8DNa/puAqhucj4MYFjkJbw0RZGB+49qDt92c6XAyINg2y+y6WxDTPtLIcozJtm5L+mEkYl9KO6iSAytomFOZP9mvmt2Y/zDBYIno67It01fbu5fAfFn2wTTCLDSfxnyXBe4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=cp39wHJL; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="cp39wHJL" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1735159989; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=18dYVVfxZoN0JMdWR7aqy0Ct2dRC7iui/Vd8YM+O8ms=; b=cp39wHJLDvETcJF7u3J4CFNYDuMKdJB3n4lOlAFdN5g6g0Fxp3asDw5Z/uIuQJpjPzMXhg ZXnrJa9x0pQQ4xN1PdvgbepeZHy7ubUe/CCV4BSHr47rOGmGVnyZpveCSVltnY8kzU00df Yh2iTmK7L6qOhHcYX1ChiQQUY9zyuAA= Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-62-wiyM_KDON7eNXCmIROw2IA-1; Wed, 25 Dec 2024 15:53:08 -0500 X-MC-Unique: wiyM_KDON7eNXCmIROw2IA-1 X-Mimecast-MFC-AGG-ID: wiyM_KDON7eNXCmIROw2IA Received: by mail-qt1-f199.google.com with SMTP id d75a77b69052e-467919046fbso145850171cf.0 for ; Wed, 25 Dec 2024 12:53:07 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1735159987; x=1735764787; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:user-agent:mime-version:date:message-id:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=18dYVVfxZoN0JMdWR7aqy0Ct2dRC7iui/Vd8YM+O8ms=; b=jowc6SegRMc+gBt0gyzRebEw/9VUO3SgB82O8r6qp3iGe5YWlAJuMftDfXTU6VVivj u0R9KuBCyBCoVDbmzSksAVsyRz7M3nNat2Y9u+xtkH1IutB/CbeWOTBknDev8EJSDouu 0yvi4Tpy02Pb9H5eHuufQrW23DFZLItve+ar0EJ3SPoZipxbbUem1DPSuRriQiWkkiOm haslGBknNVp7kPa0bX3dr4sK0C0OVhg8A7HPMNT4eblbeHcJSpdmpDv32eSxlXncKfOi 3Nv2mv9exgtrXTIM+lHZFPsw/C0He/qEkMF4X5YlRiMmB44dN0XcRKMWkuqEK8evqzLT 6i/g== X-Forwarded-Encrypted: i=1; AJvYcCUvkkE0boUX3dOETKHga5l83Kly8V484LpWTqnmYLgXTuFywsr5VUvFmfe3/7NX282ksUQZ++EARTt60qg=@vger.kernel.org X-Gm-Message-State: AOJu0YzMwTeeo9hOul9ERyC+f+jy/Cs5dfC+e4+oblQlIuX9KUpmOikz WyYOIkI7lrHzhZyT4TDsonJV+IiT2rMYvuydtMFGfgu9W7h2FvYikvbZBzrn6slDcdt8HFNmvR0 MAJMqa63qglZBX6aQGc3hC1QStPQssMO1xUJaiZYQ1l3b6UiO+PglwHKzOaT0CQ== X-Gm-Gg: ASbGncvu/v6MBcdXd6iza0pOqYAkOHFeqlLfg1vy4MfjrMnv4uxEKjnApZwiJbygcL3 NwuYwqxYIK/HR3zivzGr2bgEwv01IUPSPQmdxBEXU++Qz3MURkGOgMssndzCahCdH+Bgthxm1oh kg2ypgRuyqELQ/Mh+MMqpQBHfYF8gzy2fnRVaWU2i8LBsjVEmjn5ylo5Z/on5enj9u2BWhPT7Hq g8em5oX0UyQYujVeil7X1nyoeaDjCcOdt0fr/2wq3N2LfhbwbM6Jc/JxGx6H0fXSvtIgMjw02zn 0vpHVrFDBh8mo/8XBSvqGk5g X-Received: by 2002:ac8:5842:0:b0:467:45f2:58b with SMTP id d75a77b69052e-46a4b0d5ed5mr277516541cf.6.1735159987240; Wed, 25 Dec 2024 12:53:07 -0800 (PST) X-Google-Smtp-Source: AGHT+IH38UaMeVIzwCxeK3GXZk6y2Kr/vw4fi3Xboj5HifWQdGR9wXCmwGvEXmSsVlMDKsY2ACGtRg== X-Received: by 2002:ac8:5842:0:b0:467:45f2:58b with SMTP id d75a77b69052e-46a4b0d5ed5mr277516361cf.6.1735159986898; Wed, 25 Dec 2024 12:53:06 -0800 (PST) Received: from ?IPV6:2601:188:ca00:a00:f844:fad5:7984:7bd7? ([2601:188:ca00:a00:f844:fad5:7984:7bd7]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-46a3e653f2dsm65161221cf.4.2024.12.25.12.53.05 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 25 Dec 2024 12:53:06 -0800 (PST) From: Waiman Long X-Google-Original-From: Waiman Long Message-ID: Date: Wed, 25 Dec 2024 15:53:04 -0500 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] locking/pvqspinlock: Use try_cmpxchg() in pv_unhash To: Akira Yokosawa , maobibo@loongson.cn Cc: boqun.feng@gmail.com, linux-kernel@vger.kernel.org, mingo@redhat.com, will@kernel.org References: <20241223074704.18857-1-maobibo@loongson.cn> <1c9cb6b4-f7ba-4480-b94c-dc2fa0f11949@gmail.com> Content-Language: en-US In-Reply-To: <1c9cb6b4-f7ba-4480-b94c-dc2fa0f11949@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 12/24/24 10:15 PM, Akira Yokosawa wrote: > Hi, > > [With my LKMM reviewer hat on] > > On Mon, 23 Dec 2024 15:47:04 +0800, Bibo Mao wrote: >> We ported pv spinlock to old linux kernel on LoongArch platform, there is >> 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. >> >> 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; >> } >> } > But this change might delay load of he->node *after* he->lock returns to NULL. > > Let's get back to the current code (plus labeling ONCE accesses): > > for_each_hash_entry(he, offset, hash) { > if (READ_ONCE(he->lock) == lock) { /* A */ > node = READ_ONCE(he->node); /* B */ > WRITE_ONCE(he->lock, NULL); /* C */ > return node; > } > } > > It looks to me you want to guarantee the ordering of A -> B -> C. > > Your change effectively provides ordering of [A C] -> B. > [A C] is done in an atomic RMW op. > > For the ordering of A -> B -> C, I'd change the code to > > for_each_hash_entry(he, offset, hash) { > if (smp_load_acquire(&he->lock) == lock) { /* A */ > node = READ_ONCE(he->node); /* B */ > smp_store_release(&he->lock, NULL); /* C */ > return node; > } > } > > Note: A -> B is load-to-load ordering, and it is not provided by > control dependency. Upgrading A to ACQUIRE is a lightweight option. If Loongson CPU, like PowerPC, does not provide load-to-load ordering in a control dependency, the proper way is to provide its version of  smp_acquire__after_ctrl_dep() helper and use it to provide the acquire barrier instead of using smp_load_acquire() here. Cheers, Longman