From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 413C3D59F6C for ; Wed, 6 Nov 2024 19:57:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=A3nmlVLGBXgCWtosT8kzqyaZ7aneitiCpEA6ZoYORSo=; b=HRtdUERsWflvfjr5UAhlZ5Cyc7 VFZN6alMBnBRhO2yv41APf01ngLN382pmOQgsbNvr+ftnsih51+J8b7q9N/RHAaRv5ZvxLNA/DV3D Ikh2sFr5lqda0n2e6m2FJb/1suDVQTT3bqT4kK1mw/dWqQryM232udlzvDRJXwI7sGxb/N44/k8rD Vv09MPe3QiZz2SQLbB6GeZPnRxY9h1G7JQPEPjab2El4OGDiiI0BY7dO2tkYH7sd/mO5nWL7ZHVkR aBqtUpWUrQEZSIBVdIf2EE96ZIMbSqUFdM9A2JUt8sbcMLqrRXrqcOYA372zmsfWofADj+Uq9oOan LBZOBYkg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1t8m9u-00000004crf-3gzv; Wed, 06 Nov 2024 19:57:42 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1t8m8A-00000004cW1-37xV for linux-arm-kernel@lists.infradead.org; Wed, 06 Nov 2024 19:55:56 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 4AB1D5C1B07; Wed, 6 Nov 2024 19:55:09 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id B876EC4CED5; Wed, 6 Nov 2024 19:55:48 +0000 (UTC) Date: Wed, 6 Nov 2024 19:55:46 +0000 From: Catalin Marinas To: "Okanovic, Haris" Cc: "kvm@vger.kernel.org" , "rafael@kernel.org" , "sudeep.holla@arm.com" , "joao.m.martins@oracle.com" , "ankur.a.arora@oracle.com" , "dave.hansen@linux.intel.com" , "konrad.wilk@oracle.com" , "wanpengli@tencent.com" , "cl@gentwo.org" , "linux-kernel@vger.kernel.org" , "mingo@redhat.com" , "maobibo@loongson.cn" , "pbonzini@redhat.com" , "tglx@linutronix.de" , "misono.tomohiro@fujitsu.com" , "daniel.lezcano@linaro.org" , "arnd@arndb.de" , "lenb@kernel.org" , "will@kernel.org" , "hpa@zytor.com" , "peterz@infradead.org" , "boris.ostrovsky@oracle.com" , "vkuznets@redhat.com" , "bp@alien8.de" , "linux-pm@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "mtosatti@redhat.com" , "x86@kernel.org" , "mark.rutland@arm.com" Subject: Re: [PATCH 1/5] asm-generic: add smp_vcond_load_relaxed() Message-ID: References: <20240925232425.2763385-1-ankur.a.arora@oracle.com> <20241105183041.1531976-1-harisokn@amazon.com> <20241105183041.1531976-2-harisokn@amazon.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241106_115554_897501_C2687AB9 X-CRM114-Status: GOOD ( 33.18 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Nov 06, 2024 at 06:13:35PM +0000, Okanovic, Haris wrote: > On Wed, 2024-11-06 at 11:08 +0000, Catalin Marinas wrote: > > On Tue, Nov 05, 2024 at 12:30:37PM -0600, Haris Okanovic wrote: > > > diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h > > > index d4f581c1e21d..112027eabbfc 100644 > > > --- a/include/asm-generic/barrier.h > > > +++ b/include/asm-generic/barrier.h > > > @@ -256,6 +256,31 @@ do { \ > > > }) > > > #endif > > > > > > +/** > > > + * smp_vcond_load_relaxed() - (Spin) wait until an expected value at address > > > + * with no ordering guarantees. Spins until `(*addr & mask) == val` or > > > + * `nsecs` elapse, and returns the last observed `*addr` value. > > > + * > > > + * @nsecs: timeout in nanoseconds > > > > FWIW, I don't mind the relative timeout, it makes the API easier to use. > > Yes, it may take longer in absolute time if the thread is scheduled out > > before local_clock_noinstr() is read but the same can happen in the > > caller anyway. It's similar to udelay(), it can take longer if the > > thread is scheduled out. > > > > > + * @addr: pointer to an integer > > > + * @mask: a bit mask applied to read values > > > + * @val: Expected value with mask > > > + */ > > > +#ifndef smp_vcond_load_relaxed > > > +#define smp_vcond_load_relaxed(nsecs, addr, mask, val) ({ \ > > > + const u64 __start = local_clock_noinstr(); \ > > > + u64 __nsecs = (nsecs); \ > > > + typeof(addr) __addr = (addr); \ > > > + typeof(*__addr) __mask = (mask); \ > > > + typeof(*__addr) __val = (val); \ > > > + typeof(*__addr) __cur; \ > > > + smp_cond_load_relaxed(__addr, ( \ > > > + (VAL & __mask) == __val || \ > > > + local_clock_noinstr() - __start > __nsecs \ > > > + )); \ > > > +}) > > > > The generic implementation has the same problem as Ankur's current > > series. smp_cond_load_relaxed() can't wait on anything other than the > > variable at __addr. If it goes into a WFE, there's nothing executed to > > read the timer and check for progress. Any generic implementation of > > such function would have to use cpu_relax() and polling. > > How would the caller enter wfe()? Can you give a specific scenario that > you're concerned about? Let's take the arm64 example with the event stream disabled. Without the subsequent patches implementing smp_vcond_load_relaxed(), just expand the arm64 smp_cond_load_relaxed() implementation in the above macro. If the timer check doesn't trigger an exit from the loop, __cmpwait_relaxed() only waits on the variable to change its value, nothing to do with the timer. > This code already reduces to a relaxed poll, something like this: > > ``` > start = clock(); > while((READ_ONCE(*addr) & mask) != val && (clock() - start) < nsecs) { > cpu_relax(); > } > ``` Well, that's if you also use the generic implementation of smp_cond_load_relaxed() but have you checked all the other architectures that don't do something similar to the arm64 wfe (riscv comes close)? Even if all other architectures just use a cpu_relax(), that's still abusing the smp_cond_load_relaxed() semantics. And what if one places another loop in their __cmpwait()? That's allowed because you are supposed to wait on a single variable to change not on multiple states. -- Catalin