| Message ID | 20240830222844.1601170-10-ankur.a.arora@oracle.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Enable haltpoll on arm64 | expand |
On Fri, Aug 30, 2024 at 03:28:43PM -0700, Ankur Arora wrote: > Add architectural support for cpuidle-haltpoll driver by defining > arch_haltpoll_*(). > > Also define ARCH_CPUIDLE_HALTPOLL to allow cpuidle-haltpoll to be > selected, and given that we have an optimized polling mechanism > in smp_cond_load*(), select ARCH_HAS_OPTIMIZED_POLL. > > smp_cond_load*() are implemented via LDXR, WFE, with LDXR loading > a memory region in exclusive state and the WFE waiting for any > stores to it. > > In the edge case -- no CPU stores to the waited region and there's no > interrupt -- the event-stream will provide the terminating condition > ensuring we don't wait forever, but because the event-stream runs at > a fixed frequency (configured at 10kHz) we might spend more time in > the polling stage than specified by cpuidle_poll_time(). > > This would only happen in the last iteration, since overshooting the > poll_limit means the governor moves out of the polling stage. > > Tested-by: Haris Okanovic <harisokn@amazon.com> > Tested-by: Misono Tomohiro <misono.tomohiro@fujitsu.com> > Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com> > --- > arch/arm64/Kconfig | 10 ++++++++++ > arch/arm64/include/asm/cpuidle_haltpoll.h | 10 ++++++++++ > arch/arm64/kernel/Makefile | 1 + > arch/arm64/kernel/cpuidle_haltpoll.c | 22 ++++++++++++++++++++++ > 4 files changed, 43 insertions(+) > create mode 100644 arch/arm64/include/asm/cpuidle_haltpoll.h > create mode 100644 arch/arm64/kernel/cpuidle_haltpoll.c > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index a2f8ff354ca6..9bd93ce2f9d9 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -36,6 +36,7 @@ config ARM64 > select ARCH_HAS_MEMBARRIER_SYNC_CORE > select ARCH_HAS_NMI_SAFE_THIS_CPU_OPS > select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE > + select ARCH_HAS_OPTIMIZED_POLL > select ARCH_HAS_PTE_DEVMAP > select ARCH_HAS_PTE_SPECIAL > select ARCH_HAS_HW_PTE_YOUNG > @@ -2385,6 +2386,15 @@ config ARCH_HIBERNATION_HEADER > config ARCH_SUSPEND_POSSIBLE > def_bool y > > +config ARCH_CPUIDLE_HALTPOLL > + bool "Enable selection of the cpuidle-haltpoll driver" > + default n nit: this 'default n' line is redundant. > + help > + cpuidle-haltpoll allows for adaptive polling based on > + current load before entering the idle state. > + > + Some virtualized workloads benefit from using it. nit: This sentence is meaningless ^^. > + > endmenu # "Power management options" > > menu "CPU Power Management" > diff --git a/arch/arm64/include/asm/cpuidle_haltpoll.h b/arch/arm64/include/asm/cpuidle_haltpoll.h > new file mode 100644 > index 000000000000..ed615a99803b > --- /dev/null > +++ b/arch/arm64/include/asm/cpuidle_haltpoll.h > @@ -0,0 +1,10 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#ifndef _ARCH_HALTPOLL_H > +#define _ARCH_HALTPOLL_H > + > +static inline void arch_haltpoll_enable(unsigned int cpu) { } > +static inline void arch_haltpoll_disable(unsigned int cpu) { } > + > +bool arch_haltpoll_want(bool force); > +#endif > diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile > index 2b112f3b7510..bbfb57eda2f1 100644 > --- a/arch/arm64/kernel/Makefile > +++ b/arch/arm64/kernel/Makefile > @@ -70,6 +70,7 @@ obj-$(CONFIG_ARM64_PTR_AUTH) += pointer_auth.o > obj-$(CONFIG_ARM64_MTE) += mte.o > obj-y += vdso-wrap.o > obj-$(CONFIG_COMPAT_VDSO) += vdso32-wrap.o > +obj-$(CONFIG_ARCH_CPUIDLE_HALTPOLL) += cpuidle_haltpoll.o > > # Force dependency (vdso*-wrap.S includes vdso.so through incbin) > $(obj)/vdso-wrap.o: $(obj)/vdso/vdso.so > diff --git a/arch/arm64/kernel/cpuidle_haltpoll.c b/arch/arm64/kernel/cpuidle_haltpoll.c > new file mode 100644 > index 000000000000..63fc5ebca79b > --- /dev/null > +++ b/arch/arm64/kernel/cpuidle_haltpoll.c > @@ -0,0 +1,22 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#include <linux/kernel.h> > +#include <clocksource/arm_arch_timer.h> > +#include <asm/cpuidle_haltpoll.h> > + > +bool arch_haltpoll_want(bool force) > +{ > + /* > + * Enabling haltpoll requires two things: > + * > + * - Event stream support to provide a terminating condition to the > + * WFE in the poll loop. > + * > + * - KVM support for arch_haltpoll_enable(), arch_haltpoll_disable(). > + * > + * Given that the second is missing, allow haltpoll to only be force > + * loaded. > + */ > + return (arch_timer_evtstrm_available() && false) || force; > +} > +EXPORT_SYMBOL_GPL(arch_haltpoll_want); This seems a bit over-the-top to justify a new C file. Just have a static inline in the header which returns 'force'. The '&& false' is misleading and unnecessary with the comment. Will
Will Deacon <will@kernel.org> writes: > On Fri, Aug 30, 2024 at 03:28:43PM -0700, Ankur Arora wrote: >> Add architectural support for cpuidle-haltpoll driver by defining >> arch_haltpoll_*(). >> >> Also define ARCH_CPUIDLE_HALTPOLL to allow cpuidle-haltpoll to be >> selected, and given that we have an optimized polling mechanism >> in smp_cond_load*(), select ARCH_HAS_OPTIMIZED_POLL. >> >> smp_cond_load*() are implemented via LDXR, WFE, with LDXR loading >> a memory region in exclusive state and the WFE waiting for any >> stores to it. >> >> In the edge case -- no CPU stores to the waited region and there's no >> interrupt -- the event-stream will provide the terminating condition >> ensuring we don't wait forever, but because the event-stream runs at >> a fixed frequency (configured at 10kHz) we might spend more time in >> the polling stage than specified by cpuidle_poll_time(). >> >> This would only happen in the last iteration, since overshooting the >> poll_limit means the governor moves out of the polling stage. >> >> Tested-by: Haris Okanovic <harisokn@amazon.com> >> Tested-by: Misono Tomohiro <misono.tomohiro@fujitsu.com> >> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com> >> --- >> arch/arm64/Kconfig | 10 ++++++++++ >> arch/arm64/include/asm/cpuidle_haltpoll.h | 10 ++++++++++ >> arch/arm64/kernel/Makefile | 1 + >> arch/arm64/kernel/cpuidle_haltpoll.c | 22 ++++++++++++++++++++++ >> 4 files changed, 43 insertions(+) >> create mode 100644 arch/arm64/include/asm/cpuidle_haltpoll.h >> create mode 100644 arch/arm64/kernel/cpuidle_haltpoll.c >> >> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig >> index a2f8ff354ca6..9bd93ce2f9d9 100644 >> --- a/arch/arm64/Kconfig >> +++ b/arch/arm64/Kconfig >> @@ -36,6 +36,7 @@ config ARM64 >> select ARCH_HAS_MEMBARRIER_SYNC_CORE >> select ARCH_HAS_NMI_SAFE_THIS_CPU_OPS >> select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE >> + select ARCH_HAS_OPTIMIZED_POLL >> select ARCH_HAS_PTE_DEVMAP >> select ARCH_HAS_PTE_SPECIAL >> select ARCH_HAS_HW_PTE_YOUNG >> @@ -2385,6 +2386,15 @@ config ARCH_HIBERNATION_HEADER >> config ARCH_SUSPEND_POSSIBLE >> def_bool y >> >> +config ARCH_CPUIDLE_HALTPOLL >> + bool "Enable selection of the cpuidle-haltpoll driver" >> + default n > > nit: this 'default n' line is redundant. > >> + help >> + cpuidle-haltpoll allows for adaptive polling based on >> + current load before entering the idle state. >> + >> + Some virtualized workloads benefit from using it. > > nit: This sentence is meaningless ^^. Yeah. Yeah I think I added it to take care of a checkpatch warning. But clearly it doesn't add anything useful. Will fix. >> + >> endmenu # "Power management options" >> >> menu "CPU Power Management" >> diff --git a/arch/arm64/include/asm/cpuidle_haltpoll.h b/arch/arm64/include/asm/cpuidle_haltpoll.h >> new file mode 100644 >> index 000000000000..ed615a99803b >> --- /dev/null >> +++ b/arch/arm64/include/asm/cpuidle_haltpoll.h >> @@ -0,0 +1,10 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> + >> +#ifndef _ARCH_HALTPOLL_H >> +#define _ARCH_HALTPOLL_H >> + >> +static inline void arch_haltpoll_enable(unsigned int cpu) { } >> +static inline void arch_haltpoll_disable(unsigned int cpu) { } >> + >> +bool arch_haltpoll_want(bool force); >> +#endif >> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile >> index 2b112f3b7510..bbfb57eda2f1 100644 >> --- a/arch/arm64/kernel/Makefile >> +++ b/arch/arm64/kernel/Makefile >> @@ -70,6 +70,7 @@ obj-$(CONFIG_ARM64_PTR_AUTH) += pointer_auth.o >> obj-$(CONFIG_ARM64_MTE) += mte.o >> obj-y += vdso-wrap.o >> obj-$(CONFIG_COMPAT_VDSO) += vdso32-wrap.o >> +obj-$(CONFIG_ARCH_CPUIDLE_HALTPOLL) += cpuidle_haltpoll.o >> >> # Force dependency (vdso*-wrap.S includes vdso.so through incbin) >> $(obj)/vdso-wrap.o: $(obj)/vdso/vdso.so >> diff --git a/arch/arm64/kernel/cpuidle_haltpoll.c b/arch/arm64/kernel/cpuidle_haltpoll.c >> new file mode 100644 >> index 000000000000..63fc5ebca79b >> --- /dev/null >> +++ b/arch/arm64/kernel/cpuidle_haltpoll.c >> @@ -0,0 +1,22 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> + >> +#include <linux/kernel.h> >> +#include <clocksource/arm_arch_timer.h> >> +#include <asm/cpuidle_haltpoll.h> >> + >> +bool arch_haltpoll_want(bool force) >> +{ >> + /* >> + * Enabling haltpoll requires two things: >> + * >> + * - Event stream support to provide a terminating condition to the >> + * WFE in the poll loop. >> + * >> + * - KVM support for arch_haltpoll_enable(), arch_haltpoll_disable(). >> + * >> + * Given that the second is missing, allow haltpoll to only be force >> + * loaded. >> + */ >> + return (arch_timer_evtstrm_available() && false) || force; >> +} >> +EXPORT_SYMBOL_GPL(arch_haltpoll_want); > > This seems a bit over-the-top to justify a new C file. Just have a static > inline in the header which returns 'force'. The '&& false' is misleading > and unnecessary with the comment. So, the only reason for doing it this way was that I wanted to encode the arch_timer_evtstrm_available() dependency. But you are right that the comment suffices since the check itself is not operative. Will fix. Thanks for reviewing. -- ankur
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index a2f8ff354ca6..9bd93ce2f9d9 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -36,6 +36,7 @@ config ARM64 select ARCH_HAS_MEMBARRIER_SYNC_CORE select ARCH_HAS_NMI_SAFE_THIS_CPU_OPS select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE + select ARCH_HAS_OPTIMIZED_POLL select ARCH_HAS_PTE_DEVMAP select ARCH_HAS_PTE_SPECIAL select ARCH_HAS_HW_PTE_YOUNG @@ -2385,6 +2386,15 @@ config ARCH_HIBERNATION_HEADER config ARCH_SUSPEND_POSSIBLE def_bool y +config ARCH_CPUIDLE_HALTPOLL + bool "Enable selection of the cpuidle-haltpoll driver" + default n + help + cpuidle-haltpoll allows for adaptive polling based on + current load before entering the idle state. + + Some virtualized workloads benefit from using it. + endmenu # "Power management options" menu "CPU Power Management" diff --git a/arch/arm64/include/asm/cpuidle_haltpoll.h b/arch/arm64/include/asm/cpuidle_haltpoll.h new file mode 100644 index 000000000000..ed615a99803b --- /dev/null +++ b/arch/arm64/include/asm/cpuidle_haltpoll.h @@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef _ARCH_HALTPOLL_H +#define _ARCH_HALTPOLL_H + +static inline void arch_haltpoll_enable(unsigned int cpu) { } +static inline void arch_haltpoll_disable(unsigned int cpu) { } + +bool arch_haltpoll_want(bool force); +#endif diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index 2b112f3b7510..bbfb57eda2f1 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -70,6 +70,7 @@ obj-$(CONFIG_ARM64_PTR_AUTH) += pointer_auth.o obj-$(CONFIG_ARM64_MTE) += mte.o obj-y += vdso-wrap.o obj-$(CONFIG_COMPAT_VDSO) += vdso32-wrap.o +obj-$(CONFIG_ARCH_CPUIDLE_HALTPOLL) += cpuidle_haltpoll.o # Force dependency (vdso*-wrap.S includes vdso.so through incbin) $(obj)/vdso-wrap.o: $(obj)/vdso/vdso.so diff --git a/arch/arm64/kernel/cpuidle_haltpoll.c b/arch/arm64/kernel/cpuidle_haltpoll.c new file mode 100644 index 000000000000..63fc5ebca79b --- /dev/null +++ b/arch/arm64/kernel/cpuidle_haltpoll.c @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/kernel.h> +#include <clocksource/arm_arch_timer.h> +#include <asm/cpuidle_haltpoll.h> + +bool arch_haltpoll_want(bool force) +{ + /* + * Enabling haltpoll requires two things: + * + * - Event stream support to provide a terminating condition to the + * WFE in the poll loop. + * + * - KVM support for arch_haltpoll_enable(), arch_haltpoll_disable(). + * + * Given that the second is missing, allow haltpoll to only be force + * loaded. + */ + return (arch_timer_evtstrm_available() && false) || force; +} +EXPORT_SYMBOL_GPL(arch_haltpoll_want);