Received-SPF: pass (google.com: domain of linux-kernel+bounces-150461-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240418114942.52770-1-luxu.kernel@bytedance.com>
 <20240418114942.52770-2-luxu.kernel@bytedance.com> <20240418-dove-deferral-2b01100e13ca@spud>
In-Reply-To: <20240418-dove-deferral-2b01100e13ca@spud>
From: Xu Lu <luxu.kernel@bytedance.com>
Date: Fri, 19 Apr 2024 00:14:47 +0800
Message-ID: <CAPYmKFsddST1n2ZU+5=c_yPqAKTjQ4X1mKVFMZbhfo8xiB+OTA@mail.gmail.com>
Subject: Re: [External] Re: [RFC 1/2] riscv: process: Introduce idle thread
 using Zawrs extension
To: Conor Dooley <conor@kernel.org>
Cc: paul.walmsley@sifive.com, palmer@dabbelt.com, aou@eecs.berkeley.edu, 
	andy.chiu@sifive.com, guoren@kernel.org, linux-riscv@lists.infradead.org, 
	linux-kernel@vger.kernel.org, lihangjing@bytedance.com, 
	dengliang.1214@bytedance.com, xieyongji@bytedance.com, 
	chaiwen.cc@bytedance.com, Andrew Jones <ajones@ventanamicro.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 18, 2024 at 11:06=E2=80=AFPM Conor Dooley <conor@kernel.org> wr=
ote:
>
> + Drew,
>
> On Thu, Apr 18, 2024 at 07:49:41PM +0800, Xu Lu wrote:
> > The Zawrs extension introduces a new instruction WRS.NTO, which will
> > register a reservation set and causes the hart to temporarily stall
> > execution in a low-power state until a store occurs to the reservation
> > set or an interrupt is observed.
> >
> > This commit implements new version of idle thread for RISC-V via Zawrs
> > extension.
> >
> > Signed-off-by: Xu Lu <luxu.kernel@bytedance.com>
> > Reviewed-by: Hangjing Li <lihangjing@bytedance.com>
> > Reviewed-by: Liang Deng <dengliang.1214@bytedance.com>
> > Reviewed-by: Wen Chai <chaiwen.cc@bytedance.com>
> > ---
> >  arch/riscv/Kconfig                 | 24 +++++++++++++++++
> >  arch/riscv/include/asm/cpuidle.h   | 11 +-------
> >  arch/riscv/include/asm/hwcap.h     |  1 +
> >  arch/riscv/include/asm/processor.h | 17 +++++++++++++
> >  arch/riscv/kernel/cpu.c            |  5 ++++
> >  arch/riscv/kernel/cpufeature.c     |  1 +
> >  arch/riscv/kernel/process.c        | 41 +++++++++++++++++++++++++++++-
> >  7 files changed, 89 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index be09c8836d56..a0d344e9803f 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -19,6 +19,7 @@ config RISCV
> >       select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
> >       select ARCH_ENABLE_THP_MIGRATION if TRANSPARENT_HUGEPAGE
> >       select ARCH_HAS_BINFMT_FLAT
> > +     select ARCH_HAS_CPU_FINALIZE_INIT
> >       select ARCH_HAS_CURRENT_STACK_POINTER
> >       select ARCH_HAS_DEBUG_VIRTUAL if MMU
> >       select ARCH_HAS_DEBUG_VM_PGTABLE
> > @@ -525,6 +526,20 @@ config RISCV_ISA_SVPBMT
> >
> >          If you don't know what to do here, say Y.
> >
> > +config RISCV_ISA_ZAWRS
> > +     bool "Zawrs extension support for wait-on-reservation-set instruc=
tions"
> > +     depends on RISCV_ALTERNATIVE
> > +     default y
> > +     help
> > +        Adds support to dynamically detect the presence of the Zawrs
> > +        extension and enable its usage.
>
> Drew, could you, in your update, use the wording:
>            Add support for enabling optimisations in the kernel when the
>            Zawrs extension is detected at boot.
>
> There was some confusion recently about what these options were actually
> for, because this option doesn't control "dynamic detection" as the
> ACPI or DT detection is compiled at all times. I had written a patch for
> this wording in other options at the time but had forgotten to properly
> send it:
> https://lore.kernel.org/linux-riscv/20240418-stable-railway-7cce07e1e440@=
spud/T/#u
>
> > +
> > +        The Zawrs extension defines a pair of instructions to be used
> > +        in polling loops that allows a core to enter a low-power state
> > +        and wait on a store to a memory location.
> > +
> > +        If you don't know what to do here, say Y.
> > +
> >  config TOOLCHAIN_HAS_V
> >       bool
> >       default y
> > @@ -1075,6 +1090,15 @@ endmenu # "Power management options"
> >
> >  menu "CPU Power Management"
> >
> > +config RISCV_ZAWRS_IDLE
> > +     bool "Idle thread using ZAWRS extensions"
> > +     depends on RISCV_ISA_ZAWRS
> > +     default y
> > +     help
> > +             Adds support to implement idle thread using ZAWRS extensi=
on.
> > +
> > +             If you don't know what to do here, say Y.
>
> I don't think this second option is needed, why would we not always want
> to use the Zawrs version of this when it is available? Can we do it
> unconditionally when RISCV_ISA_ZAWRS is set and the extension is
> detected at runtime?
>
> Cheers,
> Conor.

Indeed, we can always choose WRS.NTO when entering idle.

This config is introduced for the second commit in this patch series.
In the second commit, we detect whether the target cpu is idle when
sending IPI and write IPI info to the reserve set of idle cpu so as to
avoid sending a physical IPI. Besides, the target idle cpu need not to
go through traditional interrupt handling routine. However, if all
cpus are busy and hardly enter idle, this commit may introduce
performance overhead of extra instructions when sending IPI. Thus we
introduce this config just in case.

Regards,
Xu Lu

>
>