From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 04BEE13A26F; Wed, 6 Nov 2024 19:55:53 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730922954; cv=none; b=ONo6FVBVI/rGJ3HVFn/3iSpm/OKaJGrXtnoDCc2Suggnk5ra7HdkJuFSoA2qO5C+iuKxTFFUuNxyrxV0Jgn78sHRe5Nlpm8A6BTwYrPr3RDY0KyQBo3ZjOhO6iyBbKUn8aE3dToAgd76ZyVtqp6MsPBHVVHkp4JT/QWZwWLTqak= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730922954; c=relaxed/simple; bh=XPs6E4peSoALY20kBmu4SRgS9pGyv22f5TO51cGafaU=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Yzo7GMDx+F/2M+3S9K9qsvMwl8TdOn/gzmvI96Ijhf9inZRsEu2TclhfdeeBFPCFdmtezVAZFTyAGhZNXc3HY4zg56Qvj6z+OH62jDNB1yAlpo5lKi8l8pOTz/fOjusjb8QdaEcgaFyMCNgZqpXH3NFsAmYWsZSd4wkOHhMs9pA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 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> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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