[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <efa0646d397294aa413a46cfb7f8e1d9e1f327b1.camel@amazon.com>
Date: Wed, 6 Nov 2024 20:31:25 +0000
From: "Okanovic, Haris" <harisokn@...zon.com>
To: "catalin.marinas@....com" <catalin.marinas@....com>
CC: "joao.m.martins@...cle.com" <joao.m.martins@...cle.com>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>, "mtosatti@...hat.com"
<mtosatti@...hat.com>, "boris.ostrovsky@...cle.com"
<boris.ostrovsky@...cle.com>, "mark.rutland@....com" <mark.rutland@....com>,
"ankur.a.arora@...cle.com" <ankur.a.arora@...cle.com>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
"konrad.wilk@...cle.com" <konrad.wilk@...cle.com>, "cl@...two.org"
<cl@...two.org>, "wanpengli@...cent.com" <wanpengli@...cent.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"mingo@...hat.com" <mingo@...hat.com>, "maobibo@...ngson.cn"
<maobibo@...ngson.cn>, "pbonzini@...hat.com" <pbonzini@...hat.com>,
"tglx@...utronix.de" <tglx@...utronix.de>, "misono.tomohiro@...itsu.com"
<misono.tomohiro@...itsu.com>, "daniel.lezcano@...aro.org"
<daniel.lezcano@...aro.org>, "arnd@...db.de" <arnd@...db.de>,
"lenb@...nel.org" <lenb@...nel.org>, "will@...nel.org" <will@...nel.org>,
"hpa@...or.com" <hpa@...or.com>, "peterz@...radead.org"
<peterz@...radead.org>, "vkuznets@...hat.com" <vkuznets@...hat.com>,
"sudeep.holla@....com" <sudeep.holla@....com>, "Okanovic, Haris"
<harisokn@...zon.com>, "rafael@...nel.org" <rafael@...nel.org>,
"bp@...en8.de" <bp@...en8.de>, "linux-pm@...r.kernel.org"
<linux-pm@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>,
"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 1/5] asm-generic: add smp_vcond_load_relaxed()
On Wed, 2024-11-06 at 19:55 +0000, Catalin Marinas wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> 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.
I see what you mean now - I glossed over the use of __cmpwait_relaxed()
in smp_cond_load_relaxed(). I'll post another rev with the fix, similar
to the above "reduced" code.
>
> --
> Catalin
Powered by blists - more mailing lists