From: Ankur Arora <ankur.a.arora@oracle.com> To: Mihai Carabas <mihai.carabas@oracle.com> Cc: Will Deacon <will@kernel.org>, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, catalin.marinas@arm.com, tglx@linutronix.de, mingo@redhat.com, bp@alien8.de, x86@kernel.org, hpa@zytor.com, pbonzini@redhat.com, wanpengli@tencent.com, vkuznets@redhat.com, rafael@kernel.org, daniel.lezcano@linaro.org, akpm@linux-foundation.org, pmladek@suse.com, peterz@infradead.org, dianders@chromium.org, npiggin@gmail.com, rick.p.edgecombe@intel.com, joao.m.martins@oracle.com, juerg.haefliger@canonical.com, mic@digikod.net, arnd@arndb.de, ankur.a.arora@oracle.com Subject: Re: [PATCH 7/7] cpuidle/poll_state: replace cpu_relax with smp_cond_load_relaxed Date: Mon, 05 Feb 2024 11:33:23 -0800 [thread overview] Message-ID: <87ttmmu2nw.fsf@oracle.com> (raw) In-Reply-To: <1b25b492-b9e7-4411-90d1-463d44084043@oracle.com> Mihai Carabas <mihai.carabas@oracle.com> writes: >>>>> cpu_relax on ARM64 does a simple "yield". Thus we replace it with >>>>> smp_cond_load_relaxed which basically does a "wfe". >>>>> >>>>> Suggested-by: Peter Zijlstra <peterz@infradead.org> >>>>> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com> >>>>> --- >>>>> drivers/cpuidle/poll_state.c | 14 +++++++++----- >>>>> 1 file changed, 9 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/drivers/cpuidle/poll_state.c b/drivers/cpuidle/poll_state.c >>>>> index 9b6d90a72601..440cd713e39a 100644 >>>>> --- a/drivers/cpuidle/poll_state.c >>>>> +++ b/drivers/cpuidle/poll_state.c >>>>> @@ -26,12 +26,16 @@ static int __cpuidle poll_idle(struct cpuidle_device *dev, >>>>> limit = cpuidle_poll_time(drv, dev); >>>>> - while (!need_resched()) { >>>>> - cpu_relax(); >>>>> - if (loop_count++ < POLL_IDLE_RELAX_COUNT) >>>>> - continue; >>>>> - >>>>> + for (;;) { >>>>> loop_count = 0; >>>>> + >>>>> + smp_cond_load_relaxed(¤t_thread_info()->flags, >>>>> + (VAL & _TIF_NEED_RESCHED) || >>>>> + (loop_count++ >= POLL_IDLE_RELAX_COUNT)); >>>>> + >>>>> + if (loop_count < POLL_IDLE_RELAX_COUNT) >>>>> + break; >>>>> + >>>>> if (local_clock_noinstr() - time_start > limit) { >>>>> dev->poll_time_limit = true; >>>>> break; >>>> Doesn't this make ARCH_HAS_CPU_RELAX a complete misnomer? >>> This controls the build of poll_state.c and the generic definition of >>> smp_cond_load_relaxed (used by x86) is using cpu_relax(). Do you propose >>> other approach here? >> Give it a better name? Having ARCH_HAS_CPU_RELAX control a piece of code >> that doesn't use cpu_relax() doesn't make sense to me. > > The generic code for smp_cond_load_relaxed is using cpu_relax and this one is > used on x86 - so ARCH_HAS_CPU_RELAX is a prerequisite on x86 when using > haltpoll. Only on ARM64 this is overwritten. Moreover ARCH_HAS_CPU_RELAX is > controlling the function definition for cpuidle_poll_state_init (this is how it > was originally designed). I suspect Will's point is that the term ARCH_HAS_CPU_RELAX doesn't make a whole lot of sense when we are only indirectly using cpu_relax() in the series. Also, all archs define cpu_relax() (though some as just a barrier()) so ARCH_HAS_CPU_RELAX . Maybe an arch can instead just opt into polling in idle? Perhaps something like this trivial patch: -- diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 5edec175b9bf..d80c98c64fd4 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -367,7 +367,7 @@ config ARCH_MAY_HAVE_PC_FDC config GENERIC_CALIBRATE_DELAY def_bool y -config ARCH_HAS_CPU_RELAX +config ARCH_WANTS_IDLE_POLL def_bool y config ARCH_HIBERNATION_POSSIBLE diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 55437f5e0c3a..6a0a1f16a5c3 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -36,7 +36,7 @@ #include <asm/cpu.h> #endif -#define ACPI_IDLE_STATE_START (IS_ENABLED(CONFIG_ARCH_HAS_CPU_RELAX) ? 1 : 0) +#define ACPI_IDLE_STATE_START (IS_ENABLED(CONFIG_ARCH_WANTS_IDLE_POLL) ? 1 : 0) static unsigned int max_cstate __read_mostly = ACPI_PROCESSOR_MAX_POWER; module_param(max_cstate, uint, 0400); @@ -787,7 +787,7 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr) if (max_cstate == 0) max_cstate = 1; - if (IS_ENABLED(CONFIG_ARCH_HAS_CPU_RELAX)) { + if (IS_ENABLED(CONFIG_ARCH_WANTS_IDLE_POLL)) { cpuidle_poll_state_init(drv); count = 1; } else { diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile index d103342b7cfc..23f48d99f0f2 100644 --- a/drivers/cpuidle/Makefile +++ b/drivers/cpuidle/Makefile @@ -7,7 +7,7 @@ obj-y += cpuidle.o driver.o governor.o sysfs.o governors/ obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o obj-$(CONFIG_DT_IDLE_STATES) += dt_idle_states.o obj-$(CONFIG_DT_IDLE_GENPD) += dt_idle_genpd.o -obj-$(CONFIG_ARCH_HAS_CPU_RELAX) += poll_state.o +obj-$(CONFIG_ARCH_WANTS_IDLE_POLL) += poll_state.o obj-$(CONFIG_HALTPOLL_CPUIDLE) += cpuidle-haltpoll.o ################################################################################## diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 3183aeb7f5b4..53e55a91d55d 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -275,7 +275,7 @@ static inline void cpuidle_coupled_parallel_barrier(struct cpuidle_device *dev, } #endif -#if defined(CONFIG_CPU_IDLE) && defined(CONFIG_ARCH_HAS_CPU_RELAX) +#if defined(CONFIG_CPU_IDLE) && defined(CONFIG_ARCH_WANTS_IDLE_POLL) void cpuidle_poll_state_init(struct cpuidle_driver *drv); #else static inline void cpuidle_poll_state_init(struct cpuidle_driver *drv) {} _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
From: Ankur Arora <ankur.a.arora@oracle.com> To: Mihai Carabas <mihai.carabas@oracle.com> Cc: Will Deacon <will@kernel.org>, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, catalin.marinas@arm.com, tglx@linutronix.de, mingo@redhat.com, bp@alien8.de, x86@kernel.org, hpa@zytor.com, pbonzini@redhat.com, wanpengli@tencent.com, vkuznets@redhat.com, rafael@kernel.org, daniel.lezcano@linaro.org, akpm@linux-foundation.org, pmladek@suse.com, peterz@infradead.org, dianders@chromium.org, npiggin@gmail.com, rick.p.edgecombe@intel.com, joao.m.martins@oracle.com, juerg.haefliger@canonical.com, mic@digikod.net, arnd@arndb.de, ankur.a.arora@oracle.com Subject: Re: [PATCH 7/7] cpuidle/poll_state: replace cpu_relax with smp_cond_load_relaxed Date: Mon, 05 Feb 2024 11:33:23 -0800 [thread overview] Message-ID: <87ttmmu2nw.fsf@oracle.com> (raw) In-Reply-To: <1b25b492-b9e7-4411-90d1-463d44084043@oracle.com> Mihai Carabas <mihai.carabas@oracle.com> writes: >>>>> cpu_relax on ARM64 does a simple "yield". Thus we replace it with >>>>> smp_cond_load_relaxed which basically does a "wfe". >>>>> >>>>> Suggested-by: Peter Zijlstra <peterz@infradead.org> >>>>> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com> >>>>> --- >>>>> drivers/cpuidle/poll_state.c | 14 +++++++++----- >>>>> 1 file changed, 9 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/drivers/cpuidle/poll_state.c b/drivers/cpuidle/poll_state.c >>>>> index 9b6d90a72601..440cd713e39a 100644 >>>>> --- a/drivers/cpuidle/poll_state.c >>>>> +++ b/drivers/cpuidle/poll_state.c >>>>> @@ -26,12 +26,16 @@ static int __cpuidle poll_idle(struct cpuidle_device *dev, >>>>> limit = cpuidle_poll_time(drv, dev); >>>>> - while (!need_resched()) { >>>>> - cpu_relax(); >>>>> - if (loop_count++ < POLL_IDLE_RELAX_COUNT) >>>>> - continue; >>>>> - >>>>> + for (;;) { >>>>> loop_count = 0; >>>>> + >>>>> + smp_cond_load_relaxed(¤t_thread_info()->flags, >>>>> + (VAL & _TIF_NEED_RESCHED) || >>>>> + (loop_count++ >= POLL_IDLE_RELAX_COUNT)); >>>>> + >>>>> + if (loop_count < POLL_IDLE_RELAX_COUNT) >>>>> + break; >>>>> + >>>>> if (local_clock_noinstr() - time_start > limit) { >>>>> dev->poll_time_limit = true; >>>>> break; >>>> Doesn't this make ARCH_HAS_CPU_RELAX a complete misnomer? >>> This controls the build of poll_state.c and the generic definition of >>> smp_cond_load_relaxed (used by x86) is using cpu_relax(). Do you propose >>> other approach here? >> Give it a better name? Having ARCH_HAS_CPU_RELAX control a piece of code >> that doesn't use cpu_relax() doesn't make sense to me. > > The generic code for smp_cond_load_relaxed is using cpu_relax and this one is > used on x86 - so ARCH_HAS_CPU_RELAX is a prerequisite on x86 when using > haltpoll. Only on ARM64 this is overwritten. Moreover ARCH_HAS_CPU_RELAX is > controlling the function definition for cpuidle_poll_state_init (this is how it > was originally designed). I suspect Will's point is that the term ARCH_HAS_CPU_RELAX doesn't make a whole lot of sense when we are only indirectly using cpu_relax() in the series. Also, all archs define cpu_relax() (though some as just a barrier()) so ARCH_HAS_CPU_RELAX . Maybe an arch can instead just opt into polling in idle? Perhaps something like this trivial patch: -- diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 5edec175b9bf..d80c98c64fd4 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -367,7 +367,7 @@ config ARCH_MAY_HAVE_PC_FDC config GENERIC_CALIBRATE_DELAY def_bool y -config ARCH_HAS_CPU_RELAX +config ARCH_WANTS_IDLE_POLL def_bool y config ARCH_HIBERNATION_POSSIBLE diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 55437f5e0c3a..6a0a1f16a5c3 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -36,7 +36,7 @@ #include <asm/cpu.h> #endif -#define ACPI_IDLE_STATE_START (IS_ENABLED(CONFIG_ARCH_HAS_CPU_RELAX) ? 1 : 0) +#define ACPI_IDLE_STATE_START (IS_ENABLED(CONFIG_ARCH_WANTS_IDLE_POLL) ? 1 : 0) static unsigned int max_cstate __read_mostly = ACPI_PROCESSOR_MAX_POWER; module_param(max_cstate, uint, 0400); @@ -787,7 +787,7 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr) if (max_cstate == 0) max_cstate = 1; - if (IS_ENABLED(CONFIG_ARCH_HAS_CPU_RELAX)) { + if (IS_ENABLED(CONFIG_ARCH_WANTS_IDLE_POLL)) { cpuidle_poll_state_init(drv); count = 1; } else { diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile index d103342b7cfc..23f48d99f0f2 100644 --- a/drivers/cpuidle/Makefile +++ b/drivers/cpuidle/Makefile @@ -7,7 +7,7 @@ obj-y += cpuidle.o driver.o governor.o sysfs.o governors/ obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o obj-$(CONFIG_DT_IDLE_STATES) += dt_idle_states.o obj-$(CONFIG_DT_IDLE_GENPD) += dt_idle_genpd.o -obj-$(CONFIG_ARCH_HAS_CPU_RELAX) += poll_state.o +obj-$(CONFIG_ARCH_WANTS_IDLE_POLL) += poll_state.o obj-$(CONFIG_HALTPOLL_CPUIDLE) += cpuidle-haltpoll.o ################################################################################## diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 3183aeb7f5b4..53e55a91d55d 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -275,7 +275,7 @@ static inline void cpuidle_coupled_parallel_barrier(struct cpuidle_device *dev, } #endif -#if defined(CONFIG_CPU_IDLE) && defined(CONFIG_ARCH_HAS_CPU_RELAX) +#if defined(CONFIG_CPU_IDLE) && defined(CONFIG_ARCH_WANTS_IDLE_POLL) void cpuidle_poll_state_init(struct cpuidle_driver *drv); #else static inline void cpuidle_poll_state_init(struct cpuidle_driver *drv) {}
next prev parent reply other threads:[~2024-02-05 19:34 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-11-20 14:01 [PATCH v2] Enable haltpoll for arm64 Mihai Carabas 2023-11-20 14:01 ` Mihai Carabas 2023-11-20 14:01 ` [PATCH 1/7] x86: Move ARCH_HAS_CPU_RELAX to arch Mihai Carabas 2023-11-20 14:01 ` Mihai Carabas 2023-11-27 14:44 ` Petr Mladek 2023-11-27 14:44 ` Petr Mladek 2023-11-28 14:09 ` Mihai Carabas 2023-11-28 14:09 ` Mihai Carabas 2023-11-20 14:01 ` [PATCH 2/7] x86/kvm: Move haltpoll_want() to be arch defined Mihai Carabas 2023-11-20 14:01 ` Mihai Carabas 2023-11-29 20:55 ` Rafael J. Wysocki 2023-11-29 20:55 ` Rafael J. Wysocki 2023-11-20 14:01 ` [PATCH 3/7] governors/haltpoll: Drop kvm_para_available() check Mihai Carabas 2023-11-20 14:01 ` Mihai Carabas 2023-11-20 14:01 ` [PATCH 4/7] arm64: Select ARCH_HAS_CPU_RELAX Mihai Carabas 2023-11-20 14:01 ` Mihai Carabas 2023-11-20 14:01 ` [PATCH 5/7] arm64: Define TIF_POLLING_NRFLAG Mihai Carabas 2023-11-20 14:01 ` Mihai Carabas 2023-11-20 14:01 ` [PATCH 6/7] cpuidle-haltpoll: ARM64 support Mihai Carabas 2023-11-20 14:01 ` Mihai Carabas 2023-11-20 14:01 ` [PATCH 7/7] cpuidle/poll_state: replace cpu_relax with smp_cond_load_relaxed Mihai Carabas 2023-11-20 14:01 ` Mihai Carabas 2023-11-22 20:51 ` Christoph Lameter 2023-11-22 20:51 ` Christoph Lameter 2023-11-22 21:33 ` Mihai Carabas 2023-11-22 21:33 ` Mihai Carabas 2023-11-27 20:17 ` Christoph Lameter (Ampere) 2023-11-27 20:17 ` Christoph Lameter (Ampere) 2023-12-01 6:59 ` Ankur Arora 2023-12-01 6:59 ` Ankur Arora 2023-12-11 11:46 ` Will Deacon 2023-12-11 11:46 ` Will Deacon 2024-01-28 21:22 ` Mihai Carabas 2024-01-28 21:22 ` Mihai Carabas 2024-01-29 18:15 ` Will Deacon 2024-01-29 18:15 ` Will Deacon 2024-02-05 12:28 ` Mihai Carabas 2024-02-05 12:28 ` Mihai Carabas 2024-02-05 19:33 ` Ankur Arora [this message] 2024-02-05 19:33 ` Ankur Arora 2024-01-17 21:19 ` [PATCH v2] Enable haltpoll for arm64 Christoph Lameter (Ampere) 2024-01-17 21:19 ` Christoph Lameter (Ampere) 2024-01-25 14:39 ` Mihai Carabas 2024-01-25 14:39 ` Mihai Carabas 2024-01-25 15:16 ` Rafael J. Wysocki 2024-01-25 15:16 ` Rafael J. Wysocki
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87ttmmu2nw.fsf@oracle.com \ --to=ankur.a.arora@oracle.com \ --cc=akpm@linux-foundation.org \ --cc=arnd@arndb.de \ --cc=bp@alien8.de \ --cc=catalin.marinas@arm.com \ --cc=daniel.lezcano@linaro.org \ --cc=dianders@chromium.org \ --cc=hpa@zytor.com \ --cc=joao.m.martins@oracle.com \ --cc=juerg.haefliger@canonical.com \ --cc=kvm@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=mic@digikod.net \ --cc=mihai.carabas@oracle.com \ --cc=mingo@redhat.com \ --cc=npiggin@gmail.com \ --cc=pbonzini@redhat.com \ --cc=peterz@infradead.org \ --cc=pmladek@suse.com \ --cc=rafael@kernel.org \ --cc=rick.p.edgecombe@intel.com \ --cc=tglx@linutronix.de \ --cc=vkuznets@redhat.com \ --cc=wanpengli@tencent.com \ --cc=will@kernel.org \ --cc=x86@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.