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 72BB9C133 for ; Wed, 25 Dec 2024 16:04:49 +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=1735142691; cv=none; b=QNO0b120pjzlSVd1O9CKHEJNBGDkvkPjwbkE3ZRNm117HmZjXBEQayYLPC9MbwXvJJdp4V8tuB0ZwxJqKuk7YmaYrbODxkn9MOsHh1zfwIwQTHoOgyGKa5E8+7ywmr5k8uiQWbvHLy1pKB9ro7JPWJviV17DKSpP08kADW+loPU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735142691; c=relaxed/simple; bh=KsWu5yrXZw17mB37pRol3ZIZBQizmdUBY5Ue6F9zcEo=; h=From:Message-ID:Date:MIME-Version:Subject:To:Cc:References: In-Reply-To:Content-Type; b=Bc83O0TxGmGJs+IQBztUKBzoEBnScHz3bdzwghd/7OYqdC9OvNzG6LJo2+JErWB27yLdLj8c7nvA7hP73Iul8pBfIujkd2ytMR13dudL/mtMy/NyHi7GYBuBRNbTnsfSwNQUcF6QZQPknfnWHzMvgE9L4ITsr7X6fwfJBDTuvHM= 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=F8Y38qRw; 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="F8Y38qRw" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1735142688; 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=DMl/rn07LD/l+BHMBk6fej8Sz2DMDf9dk0PJfc7AIWI=; b=F8Y38qRwz9Vvh0iaqqyYFpjYYvdKyOgKlknNfgNkUU8bBvAe+kcF704OdJ5Z4Xwvch2Qi6 0kTIn+PfzjFB/1jJTpMnPmrKfM/vpAGI45EsC132DoMKB9cdZy9+yFRI/t6VUMmTyI5CRv Unw0P7xEIYLgD/CO9uf5TUIH7mx/bro= Received: from mail-qt1-f198.google.com (mail-qt1-f198.google.com [209.85.160.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-622-pNVzML3IOQalFIbTxLja9A-1; Wed, 25 Dec 2024 11:04:46 -0500 X-MC-Unique: pNVzML3IOQalFIbTxLja9A-1 X-Mimecast-MFC-AGG-ID: pNVzML3IOQalFIbTxLja9A Received: by mail-qt1-f198.google.com with SMTP id d75a77b69052e-467982f8816so131266481cf.1 for ; Wed, 25 Dec 2024 08:04:46 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1735142686; x=1735747486; 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=DMl/rn07LD/l+BHMBk6fej8Sz2DMDf9dk0PJfc7AIWI=; b=dSI0hOXddTJtbttfWu0cGSdkZREpEGpEXbSfKfvIGqX5DXYYmx176Q8mnNGd+nLM66 xwCyDllrA12qIf37d+/uxiMRKC109TsN2K/dWDcGY6D9WxUwgGK7ZLoN/WjPcaeAyTUM 9BMBcjEkzW+Jf+H73xH6M++eeTwaxWtdSbXD5IWWPADFjURaP+SdtPp2MFeIdliDnsEB xDRVShpS7Ul45BlnRG+JW7HaM4OJVc5f9k/4Fi/5u4bftknuWxFLheNLxT2NgSfZZWXN NWCX+yNXqmO1fuFOl4rtW0GyFCRyLw5vOq11fV0j8XPsID3aRp352d8uNx1uzYSluJFJ ohNA== X-Forwarded-Encrypted: i=1; AJvYcCUKr+XksJSXCEyEkk1Ue4h1n8fdjY/LJxUZ92h98aqdCvNiBVAJj4FgOXqFOiJbFZfQTBYC/X9MUUoHIvs=@vger.kernel.org X-Gm-Message-State: AOJu0YyBNsngGchPe11nLjLIrRMXrw07Jn+Gl1nnVOG8t6INOh8IsVR6 IjfPePsPH/+VcTvOnPCTyj024CzYHBj786sZRjSVyX+MyN8AGEoyWizakNSTQbNudecS7ddGogS QoVp5Wftahglkj3SGCxTKJXkDhxegy7iPMhFnLHvnqnGDwi4xcNXgFG8aiDg8KQ== X-Gm-Gg: ASbGncsxU7hB9TFh0nM2XnpD2kGcSs/ttUXalSFlDdX1la3AYYzuhU8aG0eZ6GT62zA oKoURWqlOPCbsu6RqNMEmDy6NKpGg1KlkxBtuupSnNbbSoG2/eYMkjnjMwO+fiPl1Vnrd8/MViI WTqQx3N6bhfmICPZ/tB5/lUJ2OrTkQd56Aj3kdXY1S7EUZsIl1OKe4TUH5ftPu9sB9glzUgxW9Z SGgA7TfvqP4PlT5R8TrWd3UksJ/BmHDRdKlqk630cPIre7Y7+ofRlb6pV6SLd0n/g5yUahTswim PHe3TRsq4fAxv3H3pAVhJfNk X-Received: by 2002:a05:622a:3c9:b0:467:58ae:b8de with SMTP id d75a77b69052e-46a4a9a61f8mr335012361cf.36.1735142686173; Wed, 25 Dec 2024 08:04:46 -0800 (PST) X-Google-Smtp-Source: AGHT+IEwvJn4njYocGP5SKNdSliV34gGY4CT5al5+72SFU3ZzQ4bHxLB2jBEYMPX2wQ+AOqqBFsvxQ== X-Received: by 2002:a05:622a:3c9:b0:467:58ae:b8de with SMTP id d75a77b69052e-46a4a9a61f8mr335011921cf.36.1735142685841; Wed, 25 Dec 2024 08:04:45 -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-46a3e6a107bsm63219771cf.37.2024.12.25.08.04.44 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 25 Dec 2024 08:04:45 -0800 (PST) From: Waiman Long X-Google-Original-From: Waiman Long Message-ID: <7e2daa4d-6f96-4189-9d86-644614f895e1@redhat.com> Date: Wed, 25 Dec 2024 11:04:43 -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: Bibo Mao , Ingo Molnar , Will Deacon Cc: Boqun Feng , linux-kernel@vger.kernel.org, Akira Yokosawa References: <20241223074704.18857-1-maobibo@loongson.cn> Content-Language: en-US In-Reply-To: <20241223074704.18857-1-maobibo@loongson.cn> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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? > > 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. Cheers, Longman > > base-commit: 48f506ad0b683d3e7e794efa60c5785c4fdc86fa