Re: [PATCH 2/6] RISC-V: Enable cbo.zero in usermode

From: Andrew Jones <ajones@ventanamicro.com>
To: Conor Dooley <conor@kernel.org>
Cc: Evan Green <evan@rivosinc.com>,
	linux-riscv@lists.infradead.org,  paul.walmsley@sifive.com,
	palmer@dabbelt.com, aou@eecs.berkeley.edu,
	 conor.dooley@microchip.com, apatel@ventanamicro.com
Subject: Re: [PATCH 2/6] RISC-V: Enable cbo.zero in usermode
Date: Thu, 10 Aug 2023 09:31:54 +0200	[thread overview]
Message-ID: <20230810-30583d716fb7652e22c868ee@orel> (raw)
In-Reply-To: <20230809-disrupt-jersey-f2545c5903fe@spud>

On Wed, Aug 09, 2023 at 07:12:58PM +0100, Conor Dooley wrote:
> On Wed, Aug 09, 2023 at 06:58:15PM +0200, Andrew Jones wrote:
> > On Wed, Aug 09, 2023 at 09:00:35AM -0700, Evan Green wrote:
> > > On Wed, Aug 9, 2023 at 4:55 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> > ...
> > > > +static __always_inline bool riscv_this_cpu_has_extension_likely(const unsigned long ext)
> > > > +{
> > > > +       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
> > > > +               return true;
> > > > +
> > > > +       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
> > > > +}
> > > > +
> > > > +static __always_inline bool riscv_this_cpu_has_extension_unlikely(const unsigned long ext)
> > > > +{
> > > > +       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))
> > > > +               return true;
> > > > +
> > > > +       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
> > > > +}
> > > 
> > > Another way to do this would be to add a parameter to
> > > riscv_has_extension_*() (as there are very few users), then these new
> > > functions can turn around and call those with the new parameter set to
> > > hart_isa[smp_processor_id()].isa. It's a tossup, so up to you. The
> > > only advantage to it I can argue is it keeps the code flows more
> > > unified.
> > >
> > 
> > I like unification, but I think I'd prefer we create wrappers and
> > try to avoid callers needing to construct hart_isa[].isa parameters
> > themselves. I'm also not a big fan of the NULL parameter needed when
> > riscv_isa_extension_available() is invoked for the riscv_isa bitmap.
> > So we need:
> > 
> >   1. check if an extension is in riscv_isa
> >   2. check if an extension is in a bitmap provided by the caller
> >   3. check if an extension is in this cpu's isa bitmap
> >   4. check if an extension is in the isa bitmap of a cpu provided by the
> >      caller
> > 
> > The only one we can optimize with alternatives is (1), so it definitely
> > gets wrappers (riscv_has_extension_likely/unlikely()). (3) and (4) can
> > also get wrappers which first try the optimized (1), like I have above.
> > Actually (3)'s wrapper could be based on (4)'s, or only provide wrappers
> > for (4)
> > 
> >  static __always_inline bool riscv_cpu_has_extension_likely(int cpu, const unsigned long ext)
> >  {
> >      if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
> >          return true;
> > 
> >      return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);
> >  }
> > 
> >  static __always_inline bool riscv_cpu_has_extension_unlikely(int cpu, const unsigned long ext)
> >  {
> >      if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))
> 
> Why are you gating on CONFIG_RISCV_ALTERNATIVE here?

This ensures we remove the riscv_has_extension_[un]likely() call
when that call would end up using its
__riscv_isa_extension_available(NULL, ext) fallback. If that fallback
where to return false, then we'd still need to make the
__riscv_isa_extension_available(hart_isa[cpu].isa, ext) call, doubling
the cost. Whereas, when we gate on CONFIG_RISCV_ALTERNATIVE, we know that
riscv_has_extension_[un]likely() will use an alternative to check the
global set of extensions. When the extension is there, the compiler
ensures that everything vanishes. When it's not, we'll fallback to a
single search of the cpu's isa bitmap.

> 
> >          return true;
> > 
> >      return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);
> >  }
> > 
> > and then use smp_processor_id() directly in the callers that need
> > to check this_cpu's extensions.
> > 
> > For case (2), I'd advocate we rename __riscv_isa_extension_available() to
> > riscv_has_extension() and drop the riscv_isa_extension_available() macro
> > in order to avoid having some calls with RISCV_ISA_EXT_* spelled out and
> > others that rely on the pasting.
> 
> > And, ideally, we'd convert most
> > riscv_has_extension(NULL, ext) calls to riscv_has_extension_[un]likely().
> 
> > I'm also not a big fan of the NULL parameter needed when
> > riscv_isa_extension_available() is invoked for the riscv_isa bitmap
> 
> Rather than actually act on my concerns about
> __riscv_isa_extension_available(), I've been busy devoting my spare
> time to playing MMOs with the excuse of not wanting to fiddle further
> with cpufeature.c et al until Palmer merged the new DT property stuff,
> but splitting out your case 1 above seems like it would really help
> there. The NULL argument case is the one I think has the potential to
> be a footgun in the face of config options.
> Split out we can document that purpose of each function & hopefully
> have one set of functions that deals with "this extension was detected
> to be present in the hardware" and one that does "this extension was
> detected & supported by this particular kernel".

Sounds good to me!

> 
> I'll try to take a proper look at this series tomorrow :)
>

Thanks!
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv