WFE for long delays

PATCH Superseded Julien Thierry 3 participants 15 messages Sep 29, 2017 – Oct 12, 2017

Subsystems (4)

Arm (32-bit) arm64 Clocksource, clockevent drivers Arm architected timer driver

lore

Share & tools

lore

Julien Thierry

author

2017-09-29 10:52 UTC #0 lore raw

v3 cover WFE for long delays

It was pointed out that there are some windows where the event stream might not be configured, meaning we cannot safely use WFE (e.g. when using PSCI CPU_SUSPEND for idle, PSCI might reset CPU state).

The first patch provide a function to check whether the task is in state where it can expect events from the event stream.

Second patch actually makes use of WFE in delays.

Changes since v1:
- Keep track of event stream state
- Do not use WFE when event stream might be misconfigured

Changes since v2:
- Use cpumask to track event stream state instead of percpu variable
- Tidy a bit code in arm64/lib/delay
- Do not factor asm-generic/delay code

Julien Thierry (2):
arm_arch_timer: Expose event stream status
arm64: use WFE for long delays

 arch/arm/include/asm/arch_timer.h    |  1 +
 arch/arm64/include/asm/arch_timer.h  |  1 +
 arch/arm64/lib/delay.c               | 23 +++++++++++++++++++----
 drivers/clocksource/arm_arch_timer.c | 20 ++++++++++++++++++--
 include/clocksource/arm_arch_timer.h | 10 +++++++++-
 5 files changed, 48 insertions(+), 7 deletions(-)

--
1.9.1

Julien Thierry

author

2017-09-29 10:52 UTC #1 lore raw

PATCH v3 1/2 arm_arch_timer: Expose event stream status

The arch timer configuration for a CPU might get reset after suspending said CPU.

In order to reliably use the event stream in the kernel (e.g. for delays), we keep track of the state where we can safely concider the event stream as properly configured.

Signed-off-by Julien Thierry <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Russell King <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
 arch/arm/include/asm/arch_timer.h    |  1 +
 arch/arm64/include/asm/arch_timer.h  |  1 +
 drivers/clocksource/arm_arch_timer.c | 20 ++++++++++++++++++--
 include/clocksource/arm_arch_timer.h |  6 ++++++
 4 files changed, 26 insertions(+), 2 deletions(-)

arch/arm/include/asm/arch_timer.h +1 -0

index d4ebf56..0b6e104 100644
@@ -106,6 +106,7 @@ static inline u32 arch_timer_get_cntkctl(void)
 static inline void arch_timer_set_cntkctl(u32 cntkctl)
 {
 	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
+	isb();
 }
 
 #endif

arch/arm64/include/asm/arch_timer.h +1 -0

index a652ce0..bdedd8f 100644
@@ -144,6 +144,7 @@ static inline u32 arch_timer_get_cntkctl(void)
 static inline void arch_timer_set_cntkctl(u32 cntkctl)
 {
 	write_sysreg(cntkctl, cntkctl_el1);
+	isb();
 }
 
 static inline u64 arch_counter_get_cntpct(void)

drivers/clocksource/arm_arch_timer.c +18 -2

index fd4b7f6..47cf15e 100644
@@ -77,6 +77,7 @@ struct arch_timer {
 static bool arch_counter_suspend_stop;
 static bool vdso_default = true;
 
+static cpumask_t evtstrm_available;
 static bool evtstrm_enable = IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM);
 
 static int __init early_evtstrm_cfg(char *buf)

@@ -740,6 +741,7 @@ static void arch_timer_evtstrm_enable(int divider)
 #ifdef CONFIG_COMPAT
 	compat_elf_hwcap |= COMPAT_HWCAP_EVTSTRM;
 #endif
+	cpumask_set_cpu(smp_processor_id(), &evtstrm_available);
 }
 
 static void arch_timer_configure_evtstream(void)

@@ -864,6 +866,11 @@ u32 arch_timer_get_rate(void)
 	return arch_timer_rate;
 }
 
+bool arch_timer_evtstrm_available(void)
+{
+	return !!cpumask_test_cpu(smp_processor_id(), &evtstrm_available);
+}
+
 static u64 arch_counter_get_cntvct_mem(void)
 {
 	u32 vct_lo, vct_hi, tmp_hi;

@@ -929,6 +936,8 @@ static int arch_timer_dying_cpu(unsigned int cpu)
 {
 	struct clock_event_device *clk = this_cpu_ptr(arch_timer_evt);
 
+	cpumask_clear_cpu(smp_processor_id(), &evtstrm_available);
+
 	arch_timer_stop(clk);
 	return 0;
 }

@@ -938,10 +947,16 @@ static int arch_timer_dying_cpu(unsigned int cpu)
 static int arch_timer_cpu_pm_notify(struct notifier_block *self,
 				    unsigned long action, void *hcpu)
 {
-	if (action == CPU_PM_ENTER)
+	if (action == CPU_PM_ENTER) {
 		__this_cpu_write(saved_cntkctl, arch_timer_get_cntkctl());
-	else if (action == CPU_PM_ENTER_FAILED || action == CPU_PM_EXIT)
+
+		cpumask_clear_cpu(smp_processor_id(), &evtstrm_available);
+	} else if (action == CPU_PM_ENTER_FAILED || action == CPU_PM_EXIT) {
 		arch_timer_set_cntkctl(__this_cpu_read(saved_cntkctl));
+
+		if (elf_hwcap & HWCAP_EVTSTRM)
+			cpumask_set_cpu(smp_processor_id(), &evtstrm_available);
+	}
 	return NOTIFY_OK;
 }

@@ -1017,6 +1032,7 @@ static int __init arch_timer_register(void)
 	if (err)
 		goto out_unreg_notify;
 
+	cpumask_clear(&evtstrm_available);
 
 	/* Register and immediately configure the timer on the boot CPU */
 	err = cpuhp_setup_state(CPUHP_AP_ARM_ARCH_TIMER_STARTING,

include/clocksource/arm_arch_timer.h +6 -0

index cc805b7..4e28283 100644
@@ -93,6 +93,7 @@ struct arch_timer_mem {
 extern u32 arch_timer_get_rate(void);
 extern u64 (*arch_timer_read_counter)(void);
 extern struct arch_timer_kvm_info *arch_timer_get_kvm_info(void);
+extern bool arch_timer_evtstrm_available(void);
 
 #else

@@ -106,6 +107,11 @@ static inline u64 arch_timer_read_counter(void)
 	return 0;
 }
 
+static inline bool arch_timer_evtstrm_available(void)
+{
+	return false;
+}
+
 #endif
 
 #endif

--
1.9.1

Will Deacon

2017-10-11 15:14 UTC #2 lore raw

PATCH v3 1/2 arm_arch_timer: Expose event stream status

Hi Julien,

Quoted 13 lines · Julien Thierry

The arch timer configuration for a CPU might get reset after suspending said CPU.
In order to reliably use the event stream in the kernel (e.g. for delays), we keep track of the state where we can safely concider the event stream as properly configured.
Signed-off-by Julien Thierry <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Russell King <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>

Quoted 6 lines

---
 arch/arm/include/asm/arch_timer.h    |  1 +
 arch/arm64/include/asm/arch_timer.h  |  1 +
 drivers/clocksource/arm_arch_timer.c | 20 ++++++++++++++++++--
 include/clocksource/arm_arch_timer.h |  6 ++++++
 4 files changed, 26 insertions(+), 2 deletions(-)

The arch-timer bits needs an ack from Marc or Mark.

Quoted 45 lines

arch/arm/include/asm/arch_timer.h +1 -0

index d4ebf56..0b6e104 100644
@@ -106,6 +106,7 @@ static inline u32 arch_timer_get_cntkctl(void)
 static inline void arch_timer_set_cntkctl(u32 cntkctl)
 {
 	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
+	isb();
 }
 
 #endif

arch/arm64/include/asm/arch_timer.h +1 -0

index a652ce0..bdedd8f 100644
@@ -144,6 +144,7 @@ static inline u32 arch_timer_get_cntkctl(void)
 static inline void arch_timer_set_cntkctl(u32 cntkctl)
 {
 	write_sysreg(cntkctl, cntkctl_el1);
+	isb();
 }
 
 static inline u64 arch_counter_get_cntpct(void)

drivers/clocksource/arm_arch_timer.c +2 -0

index fd4b7f6..47cf15e 100644
@@ -77,6 +77,7 @@ struct arch_timer {
 static bool arch_counter_suspend_stop;
 static bool vdso_default = true;
 
+static cpumask_t evtstrm_available;
 static bool evtstrm_enable = IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM);
 
 static int __init early_evtstrm_cfg(char *buf)

@@ -740,6 +741,7 @@ static void arch_timer_evtstrm_enable(int divider)
 #ifdef CONFIG_COMPAT
 	compat_elf_hwcap |= COMPAT_HWCAP_EVTSTRM;
 #endif
+	cpumask_set_cpu(smp_processor_id(), &evtstrm_available);
 }
 
 static void arch_timer_configure_evtstream(void)

drivers/clocksource/arm_arch_timer.c u32 arch_timer_get_rate(void) · line 872

Quoted 7 lines

drivers/clocksource/arm_arch_timer.c +4 -0

@@ -864,6 +866,11 @@ u32 arch_timer_get_rate(void)
 	return arch_timer_rate;
 }
 
+bool arch_timer_evtstrm_available(void)
+{
+	return !!cpumask_test_cpu(smp_processor_id(), &evtstrm_available);
+}

I don't think you need the '!!' prefix here.

Quoted 33 lines

drivers/clocksource/arm_arch_timer.c +11 -2

+
 static u64 arch_counter_get_cntvct_mem(void)
 {
 	u32 vct_lo, vct_hi, tmp_hi;

@@ -929,6 +936,8 @@ static int arch_timer_dying_cpu(unsigned int cpu)
 {
 	struct clock_event_device *clk = this_cpu_ptr(arch_timer_evt);
 
+	cpumask_clear_cpu(smp_processor_id(), &evtstrm_available);
+
 	arch_timer_stop(clk);
 	return 0;
 }

@@ -938,10 +947,16 @@ static int arch_timer_dying_cpu(unsigned int cpu)
 static int arch_timer_cpu_pm_notify(struct notifier_block *self,
 				    unsigned long action, void *hcpu)
 {
-	if (action == CPU_PM_ENTER)
+	if (action == CPU_PM_ENTER) {
 		__this_cpu_write(saved_cntkctl, arch_timer_get_cntkctl());
-	else if (action == CPU_PM_ENTER_FAILED || action == CPU_PM_EXIT)
+
+		cpumask_clear_cpu(smp_processor_id(), &evtstrm_available);
+	} else if (action == CPU_PM_ENTER_FAILED || action == CPU_PM_EXIT) {
 		arch_timer_set_cntkctl(__this_cpu_read(saved_cntkctl));
+
+		if (elf_hwcap & HWCAP_EVTSTRM)
+			cpumask_set_cpu(smp_processor_id(), &evtstrm_available);
+	}
 	return NOTIFY_OK;
 }

drivers/clocksource/arm_arch_timer.c static int __init arch_timer_register(void) · line 1035

Quoted 4 lines

drivers/clocksource/arm_arch_timer.c +1 -0

@@ -1017,6 +1032,7 @@ static int __init arch_timer_register(void)
 	if (err)
 		goto out_unreg_notify;
 
+	cpumask_clear(&evtstrm_available);

This should already be ok by virtue of the mask being static, no? Alternatively, you could use the CPU_MASK_NONE static initialiser.

Will

Julien Thierry

author

2017-10-12 08:38 UTC #3 lore raw

PATCH v3 1/2 arm_arch_timer: Expose event stream status

Hi Will,

Quoted 73 lines · Will Deacon

Hi Julien,

Quoted 13 lines · Julien Thierry

The arch timer configuration for a CPU might get reset after suspending said CPU.
In order to reliably use the event stream in the kernel (e.g. for delays), we keep track of the state where we can safely concider the event stream as properly configured.
Signed-off-by Julien Thierry <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Russell King <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>

Quoted 6 lines

---
  arch/arm/include/asm/arch_timer.h    |  1 +
  arch/arm64/include/asm/arch_timer.h  |  1 +
  drivers/clocksource/arm_arch_timer.c | 20 ++++++++++++++++++--
  include/clocksource/arm_arch_timer.h |  6 ++++++
  4 files changed, 26 insertions(+), 2 deletions(-)

The arch-timer bits needs an ack from Marc or Mark.

Quoted 48 lines

arch/arm/include/asm/arch_timer.h +1 -0

index d4ebf56..0b6e104 100644
@@ -106,6 +106,7 @@ static inline u32 arch_timer_get_cntkctl(void)
  static inline void arch_timer_set_cntkctl(u32 cntkctl)
  {
  	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
+	isb();
  }
 
  #endif

arch/arm64/include/asm/arch_timer.h +1 -0

index a652ce0..bdedd8f 100644
@@ -144,6 +144,7 @@ static inline u32 arch_timer_get_cntkctl(void)
  static inline void arch_timer_set_cntkctl(u32 cntkctl)
  {
  	write_sysreg(cntkctl, cntkctl_el1);
+	isb();
  }
 
  static inline u64 arch_counter_get_cntpct(void)

drivers/clocksource/arm_arch_timer.c +2 -0

index fd4b7f6..47cf15e 100644
@@ -77,6 +77,7 @@ struct arch_timer {
  static bool arch_counter_suspend_stop;
  static bool vdso_default = true;
 
+static cpumask_t evtstrm_available;
  static bool evtstrm_enable = IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM);
 
  static int __init early_evtstrm_cfg(char *buf)

@@ -740,6 +741,7 @@ static void arch_timer_evtstrm_enable(int divider)
  #ifdef CONFIG_COMPAT
  	compat_elf_hwcap |= COMPAT_HWCAP_EVTSTRM;
  #endif
+	cpumask_set_cpu(smp_processor_id(), &evtstrm_available);
  }
 
  static void arch_timer_configure_evtstream(void)

@@ -864,6 +866,11 @@ u32 arch_timer_get_rate(void)
  	return arch_timer_rate;
  }

Quoted 6 lines

drivers/clocksource/arm_arch_timer.c u32 arch_timer_get_rate(void) · line 872
Quoted 4 lines
drivers/clocksource/arm_arch_timer.c +4 -0
+bool arch_timer_evtstrm_available(void)
+{
+	return !!cpumask_test_cpu(smp_processor_id(), &evtstrm_available);
+}
I don't think you need the '!!' prefix here.

True, I'll remove that.

Quoted 33 lines

drivers/clocksource/arm_arch_timer.c +11 -2

+
  static u64 arch_counter_get_cntvct_mem(void)
  {
  	u32 vct_lo, vct_hi, tmp_hi;

@@ -929,6 +936,8 @@ static int arch_timer_dying_cpu(unsigned int cpu)
  {
  	struct clock_event_device *clk = this_cpu_ptr(arch_timer_evt);
 
+	cpumask_clear_cpu(smp_processor_id(), &evtstrm_available);
+
  	arch_timer_stop(clk);
  	return 0;
  }

@@ -938,10 +947,16 @@ static int arch_timer_dying_cpu(unsigned int cpu)
  static int arch_timer_cpu_pm_notify(struct notifier_block *self,
  				    unsigned long action, void *hcpu)
  {
-	if (action == CPU_PM_ENTER)
+	if (action == CPU_PM_ENTER) {
  		__this_cpu_write(saved_cntkctl, arch_timer_get_cntkctl());
-	else if (action == CPU_PM_ENTER_FAILED || action == CPU_PM_EXIT)
+
+		cpumask_clear_cpu(smp_processor_id(), &evtstrm_available);
+	} else if (action == CPU_PM_ENTER_FAILED || action == CPU_PM_EXIT) {
  		arch_timer_set_cntkctl(__this_cpu_read(saved_cntkctl));
+
+		if (elf_hwcap & HWCAP_EVTSTRM)
+			cpumask_set_cpu(smp_processor_id(), &evtstrm_available);
+	}
  	return NOTIFY_OK;
  }

Quoted 8 lines

drivers/clocksource/arm_arch_timer.c static int __init arch_timer_register(void) · line 1035
Quoted 5 lines
drivers/clocksource/arm_arch_timer.c +1 -0
@@ -1017,6 +1032,7 @@ static int __init arch_timer_register(void)
  	if (err)
  		goto out_unreg_notify;
 
+	cpumask_clear(&evtstrm_available);
This should already be ok by virtue of the mask being static, no? Alternatively, you could use the CPU_MASK_NONE static initialiser.

Yes, I just prefer explicit initialisations. But you're right, I'll use CPU_MASK_NONE unless it is encouraged to rely on implicit intilisation.

Thanks,

Signature · 1 line

Julien Thierry

Julien Thierry

author

2017-10-12 08:58 UTC #4 lore raw

PATCH v3 1/2 arm_arch_timer: Expose event stream status

In reply to Will Deacon

[resending as I don't see it on the ml]

Hi Will,

Quoted 73 lines · Will Deacon

Hi Julien,

Quoted 13 lines · Julien Thierry

The arch timer configuration for a CPU might get reset after suspending said CPU.
In order to reliably use the event stream in the kernel (e.g. for delays), we keep track of the state where we can safely concider the event stream as properly configured.
Signed-off-by Julien Thierry <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Russell King <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>

Quoted 6 lines

---
  arch/arm/include/asm/arch_timer.h    |  1 +
  arch/arm64/include/asm/arch_timer.h  |  1 +
  drivers/clocksource/arm_arch_timer.c | 20 ++++++++++++++++++--
  include/clocksource/arm_arch_timer.h |  6 ++++++
  4 files changed, 26 insertions(+), 2 deletions(-)

The arch-timer bits needs an ack from Marc or Mark.

Quoted 48 lines

arch/arm/include/asm/arch_timer.h +1 -0

index d4ebf56..0b6e104 100644
@@ -106,6 +106,7 @@ static inline u32 arch_timer_get_cntkctl(void)
  static inline void arch_timer_set_cntkctl(u32 cntkctl)
  {
  	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
+	isb();
  }
 
  #endif

arch/arm64/include/asm/arch_timer.h +1 -0

index a652ce0..bdedd8f 100644
@@ -144,6 +144,7 @@ static inline u32 arch_timer_get_cntkctl(void)
  static inline void arch_timer_set_cntkctl(u32 cntkctl)
  {
  	write_sysreg(cntkctl, cntkctl_el1);
+	isb();
  }
 
  static inline u64 arch_counter_get_cntpct(void)

drivers/clocksource/arm_arch_timer.c +2 -0

index fd4b7f6..47cf15e 100644
@@ -77,6 +77,7 @@ struct arch_timer {
  static bool arch_counter_suspend_stop;
  static bool vdso_default = true;
 
+static cpumask_t evtstrm_available;
  static bool evtstrm_enable = IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM);
 
  static int __init early_evtstrm_cfg(char *buf)

@@ -740,6 +741,7 @@ static void arch_timer_evtstrm_enable(int divider)
  #ifdef CONFIG_COMPAT
  	compat_elf_hwcap |= COMPAT_HWCAP_EVTSTRM;
  #endif
+	cpumask_set_cpu(smp_processor_id(), &evtstrm_available);
  }
 
  static void arch_timer_configure_evtstream(void)

@@ -864,6 +866,11 @@ u32 arch_timer_get_rate(void)
  	return arch_timer_rate;
  }

Quoted 6 lines

drivers/clocksource/arm_arch_timer.c u32 arch_timer_get_rate(void) · line 872
Quoted 4 lines
drivers/clocksource/arm_arch_timer.c +4 -0
+bool arch_timer_evtstrm_available(void)
+{
+	return !!cpumask_test_cpu(smp_processor_id(), &evtstrm_available);
+}
I don't think you need the '!!' prefix here.

True, I'll remove that.

Quoted 33 lines

drivers/clocksource/arm_arch_timer.c +11 -2

+
  static u64 arch_counter_get_cntvct_mem(void)
  {
  	u32 vct_lo, vct_hi, tmp_hi;

@@ -929,6 +936,8 @@ static int arch_timer_dying_cpu(unsigned int cpu)
  {
  	struct clock_event_device *clk = this_cpu_ptr(arch_timer_evt);
 
+	cpumask_clear_cpu(smp_processor_id(), &evtstrm_available);
+
  	arch_timer_stop(clk);
  	return 0;
  }

@@ -938,10 +947,16 @@ static int arch_timer_dying_cpu(unsigned int cpu)
  static int arch_timer_cpu_pm_notify(struct notifier_block *self,
  				    unsigned long action, void *hcpu)
  {
-	if (action == CPU_PM_ENTER)
+	if (action == CPU_PM_ENTER) {
  		__this_cpu_write(saved_cntkctl, arch_timer_get_cntkctl());
-	else if (action == CPU_PM_ENTER_FAILED || action == CPU_PM_EXIT)
+
+		cpumask_clear_cpu(smp_processor_id(), &evtstrm_available);
+	} else if (action == CPU_PM_ENTER_FAILED || action == CPU_PM_EXIT) {
  		arch_timer_set_cntkctl(__this_cpu_read(saved_cntkctl));
+
+		if (elf_hwcap & HWCAP_EVTSTRM)
+			cpumask_set_cpu(smp_processor_id(), &evtstrm_available);
+	}
  	return NOTIFY_OK;
  }

Quoted 8 lines

drivers/clocksource/arm_arch_timer.c static int __init arch_timer_register(void) · line 1035
Quoted 5 lines
drivers/clocksource/arm_arch_timer.c +1 -0
@@ -1017,6 +1032,7 @@ static int __init arch_timer_register(void)
  	if (err)
  		goto out_unreg_notify;
 
+	cpumask_clear(&evtstrm_available);
This should already be ok by virtue of the mask being static, no? Alternatively, you could use the CPU_MASK_NONE static initialiser.

Yes, I just prefer explicit initialisations. But you're right, I'll use CPU_MASK_NONE unless it is encouraged to rely on implicit intilisation.

Thanks,

Signature · 1 line

Julien Thierry

Suzuki K Poulose

2017-10-12 09:27 UTC #5 lore raw

PATCH v3 1/2 arm_arch_timer: Expose event stream status

In reply to Julien Thierry

Quoted 5 lines · Julien Thierry

The arch timer configuration for a CPU might get reset after suspending said CPU.
In order to reliably use the event stream in the kernel (e.g. for delays), we keep track of the state where we can safely concider the event stream as

nit: consider

Quoted 60 lines

properly configured.

Signed-off-by Julien Thierry <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Russell King <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
  arch/arm/include/asm/arch_timer.h    |  1 +
  arch/arm64/include/asm/arch_timer.h  |  1 +
  drivers/clocksource/arm_arch_timer.c | 20 ++++++++++++++++++--
  include/clocksource/arm_arch_timer.h |  6 ++++++
  4 files changed, 26 insertions(+), 2 deletions(-)

arch/arm/include/asm/arch_timer.h +1 -0

index d4ebf56..0b6e104 100644
@@ -106,6 +106,7 @@ static inline u32 arch_timer_get_cntkctl(void)
  static inline void arch_timer_set_cntkctl(u32 cntkctl)
  {
  	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
+	isb();
  }
 
  #endif

arch/arm64/include/asm/arch_timer.h +1 -0

index a652ce0..bdedd8f 100644
@@ -144,6 +144,7 @@ static inline u32 arch_timer_get_cntkctl(void)
  static inline void arch_timer_set_cntkctl(u32 cntkctl)
  {
  	write_sysreg(cntkctl, cntkctl_el1);
+	isb();
  }
 
  static inline u64 arch_counter_get_cntpct(void)

drivers/clocksource/arm_arch_timer.c +2 -0

index fd4b7f6..47cf15e 100644
@@ -77,6 +77,7 @@ struct arch_timer {
  static bool arch_counter_suspend_stop;
  static bool vdso_default = true;
 
+static cpumask_t evtstrm_available;
  static bool evtstrm_enable = IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM);
 
  static int __init early_evtstrm_cfg(char *buf)

@@ -740,6 +741,7 @@ static void arch_timer_evtstrm_enable(int divider)
  #ifdef CONFIG_COMPAT
  	compat_elf_hwcap |= COMPAT_HWCAP_EVTSTRM;
  #endif
+	cpumask_set_cpu(smp_processor_id(), &evtstrm_available);
  }
 
  static void arch_timer_configure_evtstream(void)

drivers/clocksource/arm_arch_timer.c u32 arch_timer_get_rate(void) · line 873

Quoted 8 lines

drivers/clocksource/arm_arch_timer.c +5 -0

@@ -864,6 +866,11 @@ u32 arch_timer_get_rate(void)
  	return arch_timer_rate;
  }
 
+bool arch_timer_evtstrm_available(void)
+{
+	return !!cpumask_test_cpu(smp_processor_id(), &evtstrm_available);
+}
+

This could live in the header file as static inlin, if the evtstrm_available is made global and could avoid a function call where it is used.

Suzuki

Will Deacon

2017-10-12 09:30 UTC #6 lore raw

PATCH v3 1/2 arm_arch_timer: Expose event stream status

Quoted 72 lines · Suzuki K Poulose

Quoted 5 lines · Julien Thierry

The arch timer configuration for a CPU might get reset after suspending said CPU.
In order to reliably use the event stream in the kernel (e.g. for delays), we keep track of the state where we can safely concider the event stream as

nit: consider

Quoted 63 lines

properly configured.

Signed-off-by Julien Thierry <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Russell King <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
 arch/arm/include/asm/arch_timer.h    |  1 +
 arch/arm64/include/asm/arch_timer.h  |  1 +
 drivers/clocksource/arm_arch_timer.c | 20 ++++++++++++++++++--
 include/clocksource/arm_arch_timer.h |  6 ++++++
 4 files changed, 26 insertions(+), 2 deletions(-)

arch/arm/include/asm/arch_timer.h +1 -0

index d4ebf56..0b6e104 100644
@@ -106,6 +106,7 @@ static inline u32 arch_timer_get_cntkctl(void)
 static inline void arch_timer_set_cntkctl(u32 cntkctl)
 {
 	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
+	isb();
 }
 
 #endif

arch/arm64/include/asm/arch_timer.h +1 -0

index a652ce0..bdedd8f 100644
@@ -144,6 +144,7 @@ static inline u32 arch_timer_get_cntkctl(void)
 static inline void arch_timer_set_cntkctl(u32 cntkctl)
 {
 	write_sysreg(cntkctl, cntkctl_el1);
+	isb();
 }
 
 static inline u64 arch_counter_get_cntpct(void)

drivers/clocksource/arm_arch_timer.c +2 -0

index fd4b7f6..47cf15e 100644
@@ -77,6 +77,7 @@ struct arch_timer {
 static bool arch_counter_suspend_stop;
 static bool vdso_default = true;
 
+static cpumask_t evtstrm_available;
 static bool evtstrm_enable = IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM);
 
 static int __init early_evtstrm_cfg(char *buf)

@@ -740,6 +741,7 @@ static void arch_timer_evtstrm_enable(int divider)
 #ifdef CONFIG_COMPAT
 	compat_elf_hwcap |= COMPAT_HWCAP_EVTSTRM;
 #endif
+	cpumask_set_cpu(smp_processor_id(), &evtstrm_available);
 }
 
 static void arch_timer_configure_evtstream(void)

@@ -864,6 +866,11 @@ u32 arch_timer_get_rate(void)
 	return arch_timer_rate;
 }

Quoted 8 lines

drivers/clocksource/arm_arch_timer.c u32 arch_timer_get_rate(void) · line 873
Quoted 5 lines
drivers/clocksource/arm_arch_timer.c +5 -0
+bool arch_timer_evtstrm_available(void)
+{
+	return !!cpumask_test_cpu(smp_processor_id(), &evtstrm_available);
+}
+
This could live in the header file as static inlin, if the evtstrm_available is made global and could avoid a function call where it is used.

You might need an EXPORT_SYMBOl as well if you go down that route.

Will

Julien Thierry

author

2017-10-12 09:40 UTC #7 lore raw

PATCH v3 1/2 arm_arch_timer: Expose event stream status

Quoted 82 lines · Will Deacon

Quoted 72 lines · Suzuki K Poulose
Quoted 5 lines · Julien Thierry
The arch timer configuration for a CPU might get reset after suspending said CPU.
In order to reliably use the event stream in the kernel (e.g. for delays), we keep track of the state where we can safely concider the event stream as
nit: consider
Quoted 63 lines
properly configured.
Signed-off-by Julien Thierry <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Russell King <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
  arch/arm/include/asm/arch_timer.h    |  1 +
  arch/arm64/include/asm/arch_timer.h  |  1 +
  drivers/clocksource/arm_arch_timer.c | 20 ++++++++++++++++++--
  include/clocksource/arm_arch_timer.h |  6 ++++++
  4 files changed, 26 insertions(+), 2 deletions(-)
arch/arm/include/asm/arch_timer.h +1 -0
index d4ebf56..0b6e104 100644
@@ -106,6 +106,7 @@ static inline u32 arch_timer_get_cntkctl(void)
  static inline void arch_timer_set_cntkctl(u32 cntkctl)
  {
  	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
+	isb();
  }
 
  #endif
arch/arm64/include/asm/arch_timer.h +1 -0
index a652ce0..bdedd8f 100644
@@ -144,6 +144,7 @@ static inline u32 arch_timer_get_cntkctl(void)
  static inline void arch_timer_set_cntkctl(u32 cntkctl)
  {
  	write_sysreg(cntkctl, cntkctl_el1);
+	isb();
  }
 
  static inline u64 arch_counter_get_cntpct(void)
drivers/clocksource/arm_arch_timer.c +2 -0
index fd4b7f6..47cf15e 100644
@@ -77,6 +77,7 @@ struct arch_timer {
  static bool arch_counter_suspend_stop;
  static bool vdso_default = true;
 
+static cpumask_t evtstrm_available;
  static bool evtstrm_enable = IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM);
 
  static int __init early_evtstrm_cfg(char *buf)
@@ -740,6 +741,7 @@ static void arch_timer_evtstrm_enable(int divider)
  #ifdef CONFIG_COMPAT
  	compat_elf_hwcap |= COMPAT_HWCAP_EVTSTRM;
  #endif
+	cpumask_set_cpu(smp_processor_id(), &evtstrm_available);
  }
 
  static void arch_timer_configure_evtstream(void)
@@ -864,6 +866,11 @@ u32 arch_timer_get_rate(void)
  	return arch_timer_rate;
  }
 
Quoted 8 lines
drivers/clocksource/arm_arch_timer.c u32 arch_timer_get_rate(void) · line 873
Quoted 5 lines
drivers/clocksource/arm_arch_timer.c +5 -0
+bool arch_timer_evtstrm_available(void)
+{
+	return !!cpumask_test_cpu(smp_processor_id(), &evtstrm_available);
+}
+
This could live in the header file as static inlin, if the evtstrm_available is made global and could avoid a function call where it is used.

Quoted 1 line

You might need an EXPORT_SYMBOl as well if you go down that route.

Hmmm, I'm not fond of making the cpu mask global. I understand you can avoid the branch + ret of the function, but generally you want to check if the event stream is present to know if you can use wfe and wake up, so I don't think it might be used in a performance critical path.

So I'm wondering, is it really worth it?

Cheers,

Signature · 1 line

Julien Thierry

Will Deacon

2017-10-12 10:26 UTC #8 lore raw

PATCH v3 1/2 arm_arch_timer: Expose event stream status

Quoted 85 lines · Julien Thierry

Quoted 82 lines · Will Deacon
Quoted 80 lines · Suzuki K Poulose
Quoted 5 lines · Julien Thierry
The arch timer configuration for a CPU might get reset after suspending said CPU.
In order to reliably use the event stream in the kernel (e.g. for delays), we keep track of the state where we can safely concider the event stream as
nit: consider
Quoted 60 lines
properly configured.
Signed-off-by Julien Thierry <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Russell King <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm/include/asm/arch_timer.h    |  1 +
 arch/arm64/include/asm/arch_timer.h  |  1 +
 drivers/clocksource/arm_arch_timer.c | 20 ++++++++++++++++++--
 include/clocksource/arm_arch_timer.h |  6 ++++++
 4 files changed, 26 insertions(+), 2 deletions(-)
arch/arm/include/asm/arch_timer.h +1 -0
index d4ebf56..0b6e104 100644
@@ -106,6 +106,7 @@ static inline u32 arch_timer_get_cntkctl(void)
 static inline void arch_timer_set_cntkctl(u32 cntkctl)
 {
 	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
+	isb();
 }
 
 #endif
arch/arm64/include/asm/arch_timer.h +1 -0
index a652ce0..bdedd8f 100644
@@ -144,6 +144,7 @@ static inline u32 arch_timer_get_cntkctl(void)
 static inline void arch_timer_set_cntkctl(u32 cntkctl)
 {
 	write_sysreg(cntkctl, cntkctl_el1);
+	isb();
 }
 
 static inline u64 arch_counter_get_cntpct(void)
drivers/clocksource/arm_arch_timer.c +2 -0
index fd4b7f6..47cf15e 100644
@@ -77,6 +77,7 @@ struct arch_timer {
 static bool arch_counter_suspend_stop;
 static bool vdso_default = true;
 
+static cpumask_t evtstrm_available;
 static bool evtstrm_enable = IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM);
 
 static int __init early_evtstrm_cfg(char *buf)
@@ -740,6 +741,7 @@ static void arch_timer_evtstrm_enable(int divider)
 #ifdef CONFIG_COMPAT
 	compat_elf_hwcap |= COMPAT_HWCAP_EVTSTRM;
 #endif
+	cpumask_set_cpu(smp_processor_id(), &evtstrm_available);
 }
 
 static void arch_timer_configure_evtstream(void)
drivers/clocksource/arm_arch_timer.c u32 arch_timer_get_rate(void) · line 873
Quoted 8 lines
drivers/clocksource/arm_arch_timer.c +5 -0
@@ -864,6 +866,11 @@ u32 arch_timer_get_rate(void)
 	return arch_timer_rate;
 }
 
+bool arch_timer_evtstrm_available(void)
+{
+	return !!cpumask_test_cpu(smp_processor_id(), &evtstrm_available);
+}
+
This could live in the header file as static inlin, if the evtstrm_available is made global and could avoid a function call where it is used.

Quoted 8 lines

Quoted 1 line
You might need an EXPORT_SYMBOl as well if you go down that route.
Hmmm, I'm not fond of making the cpu mask global. I understand you can avoid the branch + ret of the function, but generally you want to check if the event stream is present to know if you can use wfe and wake up, so I don't think it might be used in a performance critical path.
So I'm wondering, is it really worth it?

I have no strong preference, but I think you'll need o EXPORT something either way so that delay and friends can be used from modules.

Will

Julien Thierry

author

2017-10-12 12:28 UTC #9 lore raw

PATCH v3 1/2 arm_arch_timer: Expose event stream status

Quoted 93 lines · Will Deacon

Quoted 92 lines · Julien Thierry
Quoted 82 lines · Will Deacon
Quoted 72 lines · Suzuki K Poulose
Quoted 5 lines · Julien Thierry
The arch timer configuration for a CPU might get reset after suspending said CPU.
In order to reliably use the event stream in the kernel (e.g. for delays), we keep track of the state where we can safely concider the event stream as
nit: consider
Quoted 63 lines
properly configured.
Signed-off-by Julien Thierry <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Russell King <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
  arch/arm/include/asm/arch_timer.h    |  1 +
  arch/arm64/include/asm/arch_timer.h  |  1 +
  drivers/clocksource/arm_arch_timer.c | 20 ++++++++++++++++++--
  include/clocksource/arm_arch_timer.h |  6 ++++++
  4 files changed, 26 insertions(+), 2 deletions(-)
arch/arm/include/asm/arch_timer.h +1 -0
index d4ebf56..0b6e104 100644
@@ -106,6 +106,7 @@ static inline u32 arch_timer_get_cntkctl(void)
  static inline void arch_timer_set_cntkctl(u32 cntkctl)
  {
  	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
+	isb();
  }
 
  #endif
arch/arm64/include/asm/arch_timer.h +1 -0
index a652ce0..bdedd8f 100644
@@ -144,6 +144,7 @@ static inline u32 arch_timer_get_cntkctl(void)
  static inline void arch_timer_set_cntkctl(u32 cntkctl)
  {
  	write_sysreg(cntkctl, cntkctl_el1);
+	isb();
  }
 
  static inline u64 arch_counter_get_cntpct(void)
drivers/clocksource/arm_arch_timer.c +2 -0
index fd4b7f6..47cf15e 100644
@@ -77,6 +77,7 @@ struct arch_timer {
  static bool arch_counter_suspend_stop;
  static bool vdso_default = true;
 
+static cpumask_t evtstrm_available;
  static bool evtstrm_enable = IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM);
 
  static int __init early_evtstrm_cfg(char *buf)
@@ -740,6 +741,7 @@ static void arch_timer_evtstrm_enable(int divider)
  #ifdef CONFIG_COMPAT
  	compat_elf_hwcap |= COMPAT_HWCAP_EVTSTRM;
  #endif
+	cpumask_set_cpu(smp_processor_id(), &evtstrm_available);
  }
 
  static void arch_timer_configure_evtstream(void)
@@ -864,6 +866,11 @@ u32 arch_timer_get_rate(void)
  	return arch_timer_rate;
  }
 
Quoted 8 lines
drivers/clocksource/arm_arch_timer.c u32 arch_timer_get_rate(void) · line 873
Quoted 5 lines
drivers/clocksource/arm_arch_timer.c +5 -0
+bool arch_timer_evtstrm_available(void)
+{
+	return !!cpumask_test_cpu(smp_processor_id(), &evtstrm_available);
+}
+
This could live in the header file as static inlin, if the evtstrm_available is made global and could avoid a function call where it is used.
Quoted 1 line
You might need an EXPORT_SYMBOl as well if you go down that route.
Hmmm, I'm not fond of making the cpu mask global. I understand you can avoid the branch + ret of the function, but generally you want to check if the event stream is present to know if you can use wfe and wake up, so I don't think it might be used in a performance critical path.

Quoted 5 lines

Quoted 1 line
So I'm wondering, is it really worth it?
I have no strong preference, but I think you'll need o EXPORT something either way so that delay and friends can be used from modules.

Turns out this is not the case. arch_timer_evtstrm_available is called by __delay which is itself exported and available for module.

So unless a module needs to check availability of the event stream there is no need to export it. I'll leave it like this for now unless there is a strong case to be able to inline or export arch_timer_evtstrm_available.

Thanks,

Signature · 1 line

Julien Thierry

Julien Thierry

author

2017-09-29 10:52 UTC #10 lore raw

PATCH v3 2/2 arm64: use WFE for long delays

In reply to Julien Thierry

The current delay implementation uses the yield instruction, which is a hint that it is beneficial to schedule another thread. As this is a hint, it may be implemented as a NOP, causing all delays to be busy loops. This is the case for many existing CPUs.

Taking advantage of the generic timer sending periodic events to all cores, we can use WFE during delays to reduce power consumption. This is beneficial only for delays longer than the period of the timer event stream.

If timer event stream is not enabled, delays will behave as yield/busy loops.

Signed-off-by Julien Thierry <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc Mark Rutland <[email protected]>

---
 arch/arm64/lib/delay.c               | 23 +++++++++++++++++++----
 include/clocksource/arm_arch_timer.h |  4 +++-
 2 files changed, 22 insertions(+), 5 deletions(-)

arch/arm64/lib/delay.c +19 -4

index dad4ec9..4dc27f3 100644
@@ -24,10 +24,28 @@
 #include <linux/module.h>
 #include <linux/timex.h>
 
+#include <clocksource/arm_arch_timer.h>
+
+#define USECS_TO_CYCLES(TIME_USECS)			\
+	xloops_to_cycles((TIME_USECS) * 0x10C7UL)
+
+static inline unsigned long xloops_to_cycles(unsigned long xloops)
+{
+	return (xloops * loops_per_jiffy * HZ) >> 32;
+}
+
 void __delay(unsigned long cycles)
 {
 	cycles_t start = get_cycles();
 
+	if (arch_timer_evtstrm_available()) {
+		const cycles_t timer_evt_period =
+			USECS_TO_CYCLES(ARCH_TIMER_EVT_STREAM_PERIOD_US);
+
+		while ((get_cycles() - start + timer_evt_period) < cycles)
+			wfe();
+	}
+
 	while ((get_cycles() - start) < cycles)
 		cpu_relax();
 }

@@ -35,10 +53,7 @@ void __delay(unsigned long cycles)
 
 inline void __const_udelay(unsigned long xloops)
 {
-	unsigned long loops;
-
-	loops = xloops * loops_per_jiffy * HZ;
-	__delay(loops >> 32);
+	__delay(xloops_to_cycles(xloops));
 }
 EXPORT_SYMBOL(__const_udelay);

include/clocksource/arm_arch_timer.h +3 -1

index 4e28283..349e595 100644
@@ -67,7 +67,9 @@ enum arch_timer_spi_nr {
 #define ARCH_TIMER_USR_VT_ACCESS_EN	(1 << 8) /* virtual timer registers */
 #define ARCH_TIMER_USR_PT_ACCESS_EN	(1 << 9) /* physical timer registers */
 
-#define ARCH_TIMER_EVT_STREAM_FREQ	10000	/* 100us */
+#define ARCH_TIMER_EVT_STREAM_PERIOD_US	100
+#define ARCH_TIMER_EVT_STREAM_FREQ				\
+	(USEC_PER_SEC / ARCH_TIMER_EVT_STREAM_PERIOD_US)
 
 struct arch_timer_kvm_info {
 	struct timecounter timecounter;

--
1.9.1

Will Deacon

2017-10-11 15:13 UTC #11 lore raw

PATCH v3 2/2 arm64: use WFE for long delays

Hi Julien,

Quoted 27 lines · Julien Thierry

The current delay implementation uses the yield instruction, which is a hint that it is beneficial to schedule another thread. As this is a hint, it may be implemented as a NOP, causing all delays to be busy loops. This is the case for many existing CPUs.
Taking advantage of the generic timer sending periodic events to all cores, we can use WFE during delays to reduce power consumption. This is beneficial only for delays longer than the period of the timer event stream.
If timer event stream is not enabled, delays will behave as yield/busy loops.
Signed-off-by Julien Thierry <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
---
 arch/arm64/lib/delay.c               | 23 +++++++++++++++++++----
 include/clocksource/arm_arch_timer.h |  4 +++-
 2 files changed, 22 insertions(+), 5 deletions(-)
arch/arm64/lib/delay.c +0 -0
index dad4ec9..4dc27f3 100644

arch/arm64/lib/delay.c line 30

Quoted 7 lines

arch/arm64/lib/delay.c +4 -0

@@ -24,10 +24,28 @@
 #include <linux/module.h>
 #include <linux/timex.h>
 
+#include <clocksource/arm_arch_timer.h>
+
+#define USECS_TO_CYCLES(TIME_USECS)			\
+	xloops_to_cycles((TIME_USECS) * 0x10C7UL)

The macro parameter can be lower-case here.

arch/arm64/lib/delay.c line 41

Quoted 10 lines

arch/arm64/lib/delay.c +6 -0

+static inline unsigned long xloops_to_cycles(unsigned long xloops)
+{
+	return (xloops * loops_per_jiffy * HZ) >> 32;
+}
+
 void __delay(unsigned long cycles)
 {
 	cycles_t start = get_cycles();
 
+	if (arch_timer_evtstrm_available()) {

Hmm, is this never called in a context where preemption is enabled? Maybe arch_timer_evtstrm_available should be using raw_smp_processor_id() under the hood.

Quoted 27 lines

arch/arm64/lib/delay.c +8 -4

+		const cycles_t timer_evt_period =
+			USECS_TO_CYCLES(ARCH_TIMER_EVT_STREAM_PERIOD_US);
+
+		while ((get_cycles() - start + timer_evt_period) < cycles)
+			wfe();
+	}
+
 	while ((get_cycles() - start) < cycles)
 		cpu_relax();
 }

@@ -35,10 +53,7 @@ void __delay(unsigned long cycles)
 
 inline void __const_udelay(unsigned long xloops)
 {
-	unsigned long loops;
-
-	loops = xloops * loops_per_jiffy * HZ;
-	__delay(loops >> 32);
+	__delay(xloops_to_cycles(xloops));
 }
 EXPORT_SYMBOL(__const_udelay);

include/clocksource/arm_arch_timer.h +0 -0

index 4e28283..349e595 100644

include/clocksource/arm_arch_timer.h enum arch_timer_spi_nr { · line 72

Quoted 7 lines

include/clocksource/arm_arch_timer.h +3 -1

@@ -67,7 +67,9 @@ enum arch_timer_spi_nr {
 #define ARCH_TIMER_USR_VT_ACCESS_EN	(1 << 8) /* virtual timer registers */
 #define ARCH_TIMER_USR_PT_ACCESS_EN	(1 << 9) /* physical timer registers */
 
-#define ARCH_TIMER_EVT_STREAM_FREQ	10000	/* 100us */
+#define ARCH_TIMER_EVT_STREAM_PERIOD_US	100
+#define ARCH_TIMER_EVT_STREAM_FREQ				\
+	(USEC_PER_SEC / ARCH_TIMER_EVT_STREAM_PERIOD_US)

This needs an ack from Marc or Mark.

Will

Julien Thierry

author

2017-10-12 08:47 UTC #12 lore raw

PATCH v3 2/2 arm64: use WFE for long delays

Hi Will,

Quoted 33 lines · Will Deacon

Hi Julien,
Quoted 30 lines · Julien Thierry
The current delay implementation uses the yield instruction, which is a hint that it is beneficial to schedule another thread. As this is a hint, it may be implemented as a NOP, causing all delays to be busy loops. This is the case for many existing CPUs.
Taking advantage of the generic timer sending periodic events to all cores, we can use WFE during delays to reduce power consumption. This is beneficial only for delays longer than the period of the timer event stream.
If timer event stream is not enabled, delays will behave as yield/busy loops.
Signed-off-by Julien Thierry <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
---
  arch/arm64/lib/delay.c               | 23 +++++++++++++++++++----
  include/clocksource/arm_arch_timer.h |  4 +++-
  2 files changed, 22 insertions(+), 5 deletions(-)
arch/arm64/lib/delay.c +0 -0
index dad4ec9..4dc27f3 100644
@@ -24,10 +24,28 @@
  #include <linux/module.h>
  #include <linux/timex.h>
 

Quoted 7 lines

arch/arm64/lib/delay.c line 30
Quoted 4 lines
arch/arm64/lib/delay.c +4 -0
+#include <clocksource/arm_arch_timer.h>
+
+#define USECS_TO_CYCLES(TIME_USECS)			\
+	xloops_to_cycles((TIME_USECS) * 0x10C7UL)
The macro parameter can be lower-case here.

Noted, I'll change it.

Quoted 9 lines

arch/arm64/lib/delay.c +5 -0

+static inline unsigned long xloops_to_cycles(unsigned long xloops)
+{
+	return (xloops * loops_per_jiffy * HZ) >> 32;
+}
+
  void __delay(unsigned long cycles)
  {
  	cycles_t start = get_cycles();

Quoted 6 lines

arch/arm64/lib/delay.c line 41
Quoted 1 line
arch/arm64/lib/delay.c +1 -0
+	if (arch_timer_evtstrm_available()) {
Hmm, is this never called in a context where preemption is enabled? Maybe arch_timer_evtstrm_available should be using raw_smp_processor_id() under the hood.

This can be called from a preemptible context. But when it is, the event stream is either enabled both on the preemptible context and on the context where a preempted context can be resumed, or the event stream is just disabled in the whole system.

Does using raw_smp_processor_id solve an issue here?

Thanks,

Quoted 39 lines

Quoted 27 lines

arch/arm64/lib/delay.c +8 -4

+		const cycles_t timer_evt_period =
+			USECS_TO_CYCLES(ARCH_TIMER_EVT_STREAM_PERIOD_US);
+
+		while ((get_cycles() - start + timer_evt_period) < cycles)
+			wfe();
+	}
+
  	while ((get_cycles() - start) < cycles)
  		cpu_relax();
  }

@@ -35,10 +53,7 @@ void __delay(unsigned long cycles)
 
  inline void __const_udelay(unsigned long xloops)
  {
-	unsigned long loops;
-
-	loops = xloops * loops_per_jiffy * HZ;
-	__delay(loops >> 32);
+	__delay(xloops_to_cycles(xloops));
  }
  EXPORT_SYMBOL(__const_udelay);

include/clocksource/arm_arch_timer.h +0 -0

index 4e28283..349e595 100644

include/clocksource/arm_arch_timer.h enum arch_timer_spi_nr { · line 72

Quoted 7 lines

include/clocksource/arm_arch_timer.h +3 -1

@@ -67,7 +67,9 @@ enum arch_timer_spi_nr {
  #define ARCH_TIMER_USR_VT_ACCESS_EN	(1 << 8) /* virtual timer registers */
  #define ARCH_TIMER_USR_PT_ACCESS_EN	(1 << 9) /* physical timer registers */
 
-#define ARCH_TIMER_EVT_STREAM_FREQ	10000	/* 100us */
+#define ARCH_TIMER_EVT_STREAM_PERIOD_US	100
+#define ARCH_TIMER_EVT_STREAM_FREQ				\
+	(USEC_PER_SEC / ARCH_TIMER_EVT_STREAM_PERIOD_US)

This needs an ack from Marc or Mark.

Will

Signature · 1 line

Julien Thierry

Will Deacon

2017-10-12 08:52 UTC #13 lore raw

PATCH v3 2/2 arm64: use WFE for long delays

Quoted 62 lines · Julien Thierry

Hi Will,
Quoted 33 lines · Will Deacon
Hi Julien,
Quoted 30 lines · Julien Thierry
The current delay implementation uses the yield instruction, which is a hint that it is beneficial to schedule another thread. As this is a hint, it may be implemented as a NOP, causing all delays to be busy loops. This is the case for many existing CPUs.
Taking advantage of the generic timer sending periodic events to all cores, we can use WFE during delays to reduce power consumption. This is beneficial only for delays longer than the period of the timer event stream.
If timer event stream is not enabled, delays will behave as yield/busy loops.
Signed-off-by Julien Thierry <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
---
 arch/arm64/lib/delay.c               | 23 +++++++++++++++++++----
 include/clocksource/arm_arch_timer.h |  4 +++-
 2 files changed, 22 insertions(+), 5 deletions(-)
arch/arm64/lib/delay.c +0 -0
index dad4ec9..4dc27f3 100644
@@ -24,10 +24,28 @@
 #include <linux/module.h>
 #include <linux/timex.h>
 
Quoted 7 lines
arch/arm64/lib/delay.c line 30
Quoted 4 lines
arch/arm64/lib/delay.c +4 -0
+#include <clocksource/arm_arch_timer.h>
+
+#define USECS_TO_CYCLES(TIME_USECS)			\
+	xloops_to_cycles((TIME_USECS) * 0x10C7UL)
The macro parameter can be lower-case here.
Noted, I'll change it.
Quoted 9 lines
Quoted 9 lines
arch/arm64/lib/delay.c +5 -0
+static inline unsigned long xloops_to_cycles(unsigned long xloops)
+{
+	return (xloops * loops_per_jiffy * HZ) >> 32;
+}
+
 void __delay(unsigned long cycles)
 {
 	cycles_t start = get_cycles();
 
Quoted 6 lines
arch/arm64/lib/delay.c line 41
Quoted 1 line
arch/arm64/lib/delay.c +1 -0
+	if (arch_timer_evtstrm_available()) {
Hmm, is this never called in a context where preemption is enabled? Maybe arch_timer_evtstrm_available should be using raw_smp_processor_id() under the hood.

Quoted 6 lines

This can be called from a preemptible context. But when it is, the event stream is either enabled both on the preemptible context and on the context where a preempted context can be resumed, or the event stream is just disabled in the whole system.
Does using raw_smp_processor_id solve an issue here?

I thought that DEBUG_PREEMPT would splat if you called smp_processor_id() from preemptible context?

Will

Julien Thierry

author

2017-10-12 08:54 UTC #14 lore raw

PATCH v3 2/2 arm64: use WFE for long delays

Quoted 68 lines · Will Deacon

Quoted 67 lines · Julien Thierry
Hi Will,
Quoted 33 lines · Will Deacon
Hi Julien,
Quoted 30 lines · Julien Thierry
The current delay implementation uses the yield instruction, which is a hint that it is beneficial to schedule another thread. As this is a hint, it may be implemented as a NOP, causing all delays to be busy loops. This is the case for many existing CPUs.
Taking advantage of the generic timer sending periodic events to all cores, we can use WFE during delays to reduce power consumption. This is beneficial only for delays longer than the period of the timer event stream.
If timer event stream is not enabled, delays will behave as yield/busy loops.
Signed-off-by Julien Thierry <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
---
  arch/arm64/lib/delay.c               | 23 +++++++++++++++++++----
  include/clocksource/arm_arch_timer.h |  4 +++-
  2 files changed, 22 insertions(+), 5 deletions(-)
arch/arm64/lib/delay.c +0 -0
index dad4ec9..4dc27f3 100644
@@ -24,10 +24,28 @@
  #include <linux/module.h>
  #include <linux/timex.h>
 
Quoted 7 lines
arch/arm64/lib/delay.c line 30
Quoted 4 lines
arch/arm64/lib/delay.c +4 -0
+#include <clocksource/arm_arch_timer.h>
+
+#define USECS_TO_CYCLES(TIME_USECS)			\
+	xloops_to_cycles((TIME_USECS) * 0x10C7UL)
The macro parameter can be lower-case here.
Noted, I'll change it.
Quoted 9 lines
Quoted 9 lines
arch/arm64/lib/delay.c +5 -0
+static inline unsigned long xloops_to_cycles(unsigned long xloops)
+{
+	return (xloops * loops_per_jiffy * HZ) >> 32;
+}
+
  void __delay(unsigned long cycles)
  {
  	cycles_t start = get_cycles();
 
Quoted 6 lines
arch/arm64/lib/delay.c line 41
Quoted 1 line
arch/arm64/lib/delay.c +1 -0
+	if (arch_timer_evtstrm_available()) {
Hmm, is this never called in a context where preemption is enabled? Maybe arch_timer_evtstrm_available should be using raw_smp_processor_id() under the hood.
This can be called from a preemptible context. But when it is, the event stream is either enabled both on the preemptible context and on the context where a preempted context can be resumed, or the event stream is just disabled in the whole system.

Quoted 4 lines

Quoted 1 line
Does using raw_smp_processor_id solve an issue here?
I thought that DEBUG_PREEMPT would splat if you called smp_processor_id() from preemptible context?

Oh right, it will splat indeed. I'll use raw_smp_processor_id as suggested.

Thanks,

Signature · 1 line

Julien Thierry