From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f170.google.com (mail-pl1-f170.google.com [209.85.214.170]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D2EE785626 for ; Wed, 25 Dec 2024 03:15:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.170 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735096520; cv=none; b=Ary0VAtEmbado795flau/ey2c3KODIJqAnZAux66sDLAZZp4vUBhIlB1bFcrW04A7DqZn1J+lUtpw2YCHoVpsFpmZ7Egffp3IMn2Kpmd7DqIC78fZb+KTztTmLdaIAi8hmXbV33CPEgj/szRNZoZfocfazsZJfIPrAZk5pyqHy0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735096520; c=relaxed/simple; bh=/92SAJ2xoS31xu0oikEq/kDOCH63ladMUpHrgXrXIm8=; h=Message-ID:Date:MIME-Version:To:Cc:References:Subject:From: In-Reply-To:Content-Type; b=GVnTp6DS2szzfvLwT6/F6lhhfgNmMk9TOY4JvU4nXsbCBpgmdr+1dBTewidhzBRW0KHVptypZeazZKSIe7hImKeLnONs2x2QU4NkuvwS/Fzo/ty9PriWvGAPK1lCpOjPuVMo0nlSHFx7t+XMW91vbj1xz2KCFXglJmv8rIrlMp8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=HSZ/Rqeo; arc=none smtp.client-ip=209.85.214.170 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="HSZ/Rqeo" Received: by mail-pl1-f170.google.com with SMTP id d9443c01a7336-21634338cfdso84245775ad.2 for ; Tue, 24 Dec 2024 19:15:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1735096518; x=1735701318; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language:subject :references:cc:to:user-agent:mime-version:date:message-id:from:to:cc :subject:date:message-id:reply-to; bh=yXn8OOfjMShbKMjUir58/xwL0wFaBWSs8hWXOk5Ctqk=; b=HSZ/RqeotRD1/bV0+TPH9N+BQxCxr8OMLoaSCJW4PYtFUuLCY6OiWoDQX4tft0qi9E /behliAswL+gWhKxLk3PW1heemmfFgP6KKJ1MlEzKqk6syIeLD+3sZFD5ZaU3LpDEx6I L9/4IAVo9Oy0Za3FkZfqrYEvhqr20bKKUwstFIqMpANwfJYu4tc52bB2GYojT/0WiB07 6lZiB3Dudrs1gCFyd/utKzE+5DV0FxqrMqkQeHMQFppRzLNsUU57RRzNehhiXGYemTEC hP72xEvw6SeqbK7H4BOSRZnV/yC89+zWgpq4tnjI/N28ybUfcUrhbrpbhufOPob12C3s mzsA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1735096518; x=1735701318; h=content-transfer-encoding:in-reply-to:from:content-language:subject :references:cc:to:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=yXn8OOfjMShbKMjUir58/xwL0wFaBWSs8hWXOk5Ctqk=; b=ZiAbh8mBdg2oBHKl1rKqsnhNziqUFiU2iHgiSr2N46d4xnmXExBePx/Ttdhl5Yywud WJEQmPYWC9/6b9ifpWDPtiAbQq/a45bxHzZDHRQivGVjEcrJ2MsHUB2s8VFLMPGptQMY NcE4FBNH0CHf/kBNs44evxiTUcW836WrpOZU7nRmPjz7aCyQ9WOprT9UCqQzXusHTAh6 BRhuypFlpk3S1EExP7d2hZzhyPYDyGegokR+y/esj9m8hwCm1D9Kqg6zTJOWEhcDmd3q Elzk6fnKEJSzmY1GshF1MEDdCivt0HmZeDXQqZAyK0FGLyCb9/5iUc6rCL3ZYHKdp4cn SnnQ== X-Forwarded-Encrypted: i=1; AJvYcCXA0eNWUQDLabJJN7eiNbHFGoBR+M2ZNH9HNVfwFybRELNOoz7uMyWKFxagBEnpLA1gn6NK8N/O8+a0jeg=@vger.kernel.org X-Gm-Message-State: AOJu0Yy0ahDin2xxKpnH65nGXsHaI6qus+Z5zZkyc9suRZyNRHGAb5ri H03i1NbM2+TVbU8U3jU/snQB524EdbEzBDPuSkIM4toKB4JXJPFI X-Gm-Gg: ASbGncs3OayIkTcgNllsM2mxF0zaEmWeNAFySBH0vxps2MG8ySSbw69BQuLleBYhyKd oKQuqGMxPtHsyMLJUvLIDPIPqzax8B8jBrqHJcYOTdIi4vbWjEg7nQQ923ITisT8c/V2Q1tl7eV uEn4s1cQi1Gc2KFjmy39Z5MxCpLViYR9kFQr26d0jZySEBNwsH68+B2D4ql/IkB7sVSMCbdt9Qj PXaqDp5k6n1YzXL8PPSTo55ZRVj9m4mvG3q+9OyTR5RZvu6UZniEgdUonKrM4K5GWSwf/QvRuRE 0aXUnYhkqvX+4ACLgEBC+b0= X-Google-Smtp-Source: AGHT+IHJgiMfFr1jRIREVg0eHf33f8TQWLsqX1Wy+j41cBrm84Ey7BULAcpi9rgpOea27hL1Rz+PrQ== X-Received: by 2002:a17:903:186:b0:215:7719:24f6 with SMTP id d9443c01a7336-219e6ebb722mr225505945ad.23.1735096517871; Tue, 24 Dec 2024 19:15:17 -0800 (PST) Received: from [10.0.2.15] (KD106167137155.ppp-bb.dion.ne.jp. [106.167.137.155]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-219dca02acdsm96120575ad.262.2024.12.24.19.15.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 24 Dec 2024 19:15:17 -0800 (PST) Message-ID: <1c9cb6b4-f7ba-4480-b94c-dc2fa0f11949@gmail.com> Date: Wed, 25 Dec 2024 12:15:15 +0900 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: maobibo@loongson.cn Cc: boqun.feng@gmail.com, linux-kernel@vger.kernel.org, longman@redhat.com, mingo@redhat.com, will@kernel.org, Akira Yokosawa References: <20241223074704.18857-1-maobibo@loongson.cn> Subject: Re: [PATCH] locking/pvqspinlock: Use try_cmpxchg() in pv_unhash Content-Language: en-US From: Akira Yokosawa In-Reply-To: <20241223074704.18857-1-maobibo@loongson.cn> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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. I'm expecting Will and/or Boqun chiming in after holiday break. Thanks, Akira > > base-commit: 48f506ad0b683d3e7e794efa60c5785c4fdc86fa > -- > 2.39.3 >