[PATCH 1/3] clockevents: Retry programming min delta up to 10 times

[PATCH 1/3] clockevents: Retry programming min delta up to 10 times

[Matt Redfearn]

From: James Hogan <jhogan@kernel.org>

When CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST=n, the call path
hrtimer_reprogram -> clockevents_program_event ->
clockevents_program_min_delta will not retry if the clock event driver
returns -ETIME.

If the driver could not satisfy the program_min_delta for any reason,
the lack of a retry means the CPU may not receive a tick interrupt,
potentially until the counter does a full period. This leads to
rcu_sched timeout messages as the stalled CPU is detected by other CPUs,
and other issues if the CPU is holding locks or other resources at the
point at which it stalls.

There have been a couple of observed mechanisms through which a clock
event driver could not satisfy the requested min_delta and return
-ETIME.

With the MIPS GIC driver, the shared execution resource within MT cores
means inconventient latency due to execution of instructions from other
hardware threads in the core, within gic_next_event, can result in an
event being set in the past.

Additionally under virtualisation it is possible to get unexpected
latency during a clockevent device's set_next_event() callback which can
make it return -ETIME even for a delta based on min_delta_ns.

It isn't appropriate to use MIN_ADJUST in the virtualisation case as
occasional hypervisor induced high latency will cause min_delta_ns to
quickly increase to the maximum.

Instead, borrow the retry pattern from the MIN_ADJUST case, but without
making adjustments. We retry up to 10 times before giving up.

Signed-off-by: James Hogan <jhogan@kernel.org>
Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
---

 kernel/time/clockevents.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 4237e0744e26..9e5b7ed22dfd 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -281,16 +281,28 @@ static int clockevents_program_min_delta(struct clock_event_device *dev)
 {
 	unsigned long long clc;
 	int64_t delta;
+	int i;
 
-	delta = dev->min_delta_ns;
-	dev->next_event = ktime_add_ns(ktime_get(), delta);
+	for (i = 0;;) {
+		delta = dev->min_delta_ns;
+		dev->next_event = ktime_add_ns(ktime_get(), delta);
 
-	if (clockevent_state_shutdown(dev))
-		return 0;
+		if (clockevent_state_shutdown(dev))
+			return 0;
 
-	dev->retries++;
-	clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
-	return dev->set_next_event((unsigned long) clc, dev);
+		dev->retries++;
+		clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
+		if (dev->set_next_event((unsigned long) clc, dev) == 0)
+			return 0;
+
+		if (++i > 9) {
+			/*
+			 * We tried 10 times to program the device with the
+			 * given min_delta_ns. Get out of here.
+			 */
+			return -ETIME;
+		}
+	}
 }
 
 #endif /* CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST */
-- 
2.7.4

[PATCH 2/3] clocksource/mips-gic-timer: Remove pointless irq_save,restore

[Matt Redfearn]

gic_next_event is always called with interrupts disabled, so the save /
restore is pointless - remove it.

Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
---

 drivers/clocksource/mips-gic-timer.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
index ae3167c28b12..8e8e3aa25b3f 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -45,10 +45,8 @@ static int gic_next_event(unsigned long delta, struct clock_event_device *evt)
 
 	cnt = gic_read_count();
 	cnt += (u64)delta;
-	local_irq_save(flags);
 	write_gic_vl_other(mips_cm_vp_id(cpumask_first(evt->cpumask)));
 	write_gic_vo_compare(cnt);
-	local_irq_restore(flags);
 	res = ((int)(gic_read_count() - cnt) >= 0) ? -ETIME : 0;
 	return res;
 }
-- 
2.7.4

[PATCH V2 3/3] clocksource: mips-gic-timer: Add fastpath for local timer updates

[Matt Redfearn]

Always accessing the compare register via the CM redirect region is
(relatively) slow. If the timer being updated is the current CPUs
then this can be shortcutted by writing to the CM VP local region.

Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
---

 drivers/clocksource/mips-gic-timer.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
index 8e8e3aa25b3f..e8dee5491227 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -39,14 +39,19 @@ static u64 notrace gic_read_count(void)
 
 static int gic_next_event(unsigned long delta, struct clock_event_device *evt)
 {
+	int cpu = cpumask_first(evt->cpumask);
 	unsigned long flags;
 	u64 cnt;
 	int res;
 
 	cnt = gic_read_count();
 	cnt += (u64)delta;
-	write_gic_vl_other(mips_cm_vp_id(cpumask_first(evt->cpumask)));
-	write_gic_vo_compare(cnt);
+	if (cpu == raw_smp_processor_id()) {
+		write_gic_vl_compare(cnt);
+	} else {
+		write_gic_vl_other(mips_cm_vp_id(cpu));
+		write_gic_vo_compare(cnt);
+	}
 	res = ((int)(gic_read_count() - cnt) >= 0) ? -ETIME : 0;
 	return res;
 }
-- 
2.7.4

Re: [PATCH 1/3] clockevents: Retry programming min delta up to 10 times

[Thomas Gleixner]

On Thu, 19 Oct 2017, Matt Redfearn wrote:
>  	unsigned long long clc;
>  	int64_t delta;
> +	int i;
>  
> -	delta = dev->min_delta_ns;
> -	dev->next_event = ktime_add_ns(ktime_get(), delta);
> +	for (i = 0;;) {

Bah. What's wrong with

	for (i = 0; i < 10; i++) {

	    	....
		if (!(dev->set_next_event((unsigned long) clc, dev))
			return 0;
	}
	return -ETIME;

Hmm?

> +		delta = dev->min_delta_ns;
> +		dev->next_event = ktime_add_ns(ktime_get(), delta);
>  
> -	if (clockevent_state_shutdown(dev))
> -		return 0;
> +		if (clockevent_state_shutdown(dev))
> +			return 0;
>  
> -	dev->retries++;
> -	clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
> -	return dev->set_next_event((unsigned long) clc, dev);
> +		dev->retries++;
> +		clc = ((unsigned long long) delta * dev->mult) >> dev->shift;

I'd rather make that:

	delta = 0;
	for (i = 0; i < 10; i++) {
		delta += dev->min_delta_ns;
		dev->next_event = ktime_add_ns(ktime_get(), delta);
		clc = .....
	   	.....

That makes it more likely to succeed fast. Hmm?

Thanks,

	tglx

Re: [PATCH 1/3] clockevents: Retry programming min delta up to 10 times

[Matt Redfearn]

On 19/10/17 13:43, Thomas Gleixner wrote:
> On Thu, 19 Oct 2017, Matt Redfearn wrote:
>>   	unsigned long long clc;
>>   	int64_t delta;
>> +	int i;
>>   
>> -	delta = dev->min_delta_ns;
>> -	dev->next_event = ktime_add_ns(ktime_get(), delta);
>> +	for (i = 0;;) {
> Bah. What's wrong with
>
> 	for (i = 0; i < 10; i++) {
>
> 	    	....
> 		if (!(dev->set_next_event((unsigned long) clc, dev))
> 			return 0;
> 	}
> 	return -ETIME;
>
> Hmm?

Sure, can make it like that.

>> +		delta = dev->min_delta_ns;
>> +		dev->next_event = ktime_add_ns(ktime_get(), delta);
>>   
>> -	if (clockevent_state_shutdown(dev))
>> -		return 0;
>> +		if (clockevent_state_shutdown(dev))
>> +			return 0;
>>   
>> -	dev->retries++;
>> -	clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
>> -	return dev->set_next_event((unsigned long) clc, dev);
>> +		dev->retries++;
>> +		clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
> I'd rather make that:
>
> 	delta = 0;
> 	for (i = 0; i < 10; i++) {
> 		delta += dev->min_delta_ns;
> 		dev->next_event = ktime_add_ns(ktime_get(), delta);
> 		clc = .....
> 	   	.....
>
> That makes it more likely to succeed fast. Hmm?

That will set the target time to increasing multiples of min_delta_ns in 
the future, right? Sure, it should make it succeed faster - I'll make it 
like that. Are you OK with the arbitrarily chosen 10 retries?

Thanks,
Matt

>
> Thanks,
>
> 	tglx

Re: [PATCH 2/3] clocksource/mips-gic-timer: Remove pointless irq_save,restore

[Daniel Lezcano]

On 19/10/2017 13:55, Matt Redfearn wrote:
> gic_next_event is always called with interrupts disabled, so the save /
> restore is pointless - remove it.
> 
> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> ---

Applied for 4.15. I slightly changed the description and fixed:

Reported-by: Thomas Gleixner <tglx@linutronix.de>
Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Thanks!

  -- Daniel

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/3] clockevents: Retry programming min delta up to 10 times

[Thomas Gleixner]

On Thu, 19 Oct 2017, Matt Redfearn wrote:
> On 19/10/17 13:43, Thomas Gleixner wrote:
> > 	delta = 0;
> > 	for (i = 0; i < 10; i++) {
> > 		delta += dev->min_delta_ns;
> > 		dev->next_event = ktime_add_ns(ktime_get(), delta);
> > 		clc = .....
> > 	   	.....
> > 
> > That makes it more likely to succeed fast. Hmm?
> 
> That will set the target time to increasing multiples of min_delta_ns in the
> future, right?

Yes, but without fiddling with min_delta_ns itself.

> Sure, it should make it succeed faster - I'll make it like
> that. Are you OK with the arbitrarily chosen 10 retries?

I lost my crystalball so I have to trust yours :)

Thanks,

	tglx

Re: [PATCH V2 3/3] clocksource: mips-gic-timer: Add fastpath for local timer updates

[Daniel Lezcano]

On 19/10/2017 13:55, Matt Redfearn wrote:
> Always accessing the compare register via the CM redirect region is
> (relatively) slow. If the timer being updated is the current CPUs
> then this can be shortcutted by writing to the CM VP local region.
> 
> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
> ---

Applied this one for 4.15 also.

Thanks.

  -- Daniel


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH v2] clockevents: Retry programming min delta up to 10 times

[Matt Redfearn]

From: James Hogan <jhogan@kernel.org>

When CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST=n, the call path
hrtimer_reprogram -> clockevents_program_event ->
clockevents_program_min_delta will not retry if the clock event driver
returns -ETIME.

If the driver could not satisfy the program_min_delta for any reason,
the lack of a retry means the CPU may not receive a tick interrupt,
potentially until the counter does a full period. This leads to
rcu_sched timeout messages as the stalled CPU is detected by other CPUs,
and other issues if the CPU is holding locks or other resources at the
point at which it stalls.

There have been a couple of observed mechanisms through which a clock
event driver could not satisfy the requested min_delta and return
-ETIME.

With the MIPS GIC driver, the shared execution resource within MT cores
means inconventient latency due to execution of instructions from other
hardware threads in the core, within gic_next_event, can result in an
event being set in the past.

Additionally under virtualisation it is possible to get unexpected
latency during a clockevent device's set_next_event() callback which can
make it return -ETIME even for a delta based on min_delta_ns.

It isn't appropriate to use MIN_ADJUST in the virtualisation case as
occasional hypervisor induced high latency will cause min_delta_ns to
quickly increase to the maximum.

Instead, borrow the retry pattern from the MIN_ADJUST case, but without
making adjustments. We retry up to 10 times, each time increasing the
attempted delta by min_delta, before giving up.

Signed-off-by: James Hogan <jhogan@kernel.org>
Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>

---

Changes in v2:
Restructure for loop and retry with increasing multiples of min_delta.

 kernel/time/clockevents.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 4237e0744e26..84c2e3b57428 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -281,16 +281,22 @@ static int clockevents_program_min_delta(struct clock_event_device *dev)
 {
 	unsigned long long clc;
 	int64_t delta;
+	int i;
 
-	delta = dev->min_delta_ns;
-	dev->next_event = ktime_add_ns(ktime_get(), delta);
+	delta = 0;
+	for (i = 0; i < 10; i++) {
+		delta += dev->min_delta_ns;
+		dev->next_event = ktime_add_ns(ktime_get(), delta);
 
-	if (clockevent_state_shutdown(dev))
-		return 0;
+		if (clockevent_state_shutdown(dev))
+			return 0;
 
-	dev->retries++;
-	clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
-	return dev->set_next_event((unsigned long) clc, dev);
+		dev->retries++;
+		clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
+		if (dev->set_next_event((unsigned long) clc, dev) == 0)
+			return 0;
+	}
+	return -ETIME;
 }
 
 #endif /* CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST */
-- 
2.7.4

Re: [PATCH 1/3] clockevents: Retry programming min delta up to 10 times

[Martin Schwidefsky]

On Thu, 19 Oct 2017 15:29:28 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Thu, 19 Oct 2017, Matt Redfearn wrote:
> > On 19/10/17 13:43, Thomas Gleixner wrote:  
> > > 	delta = 0;
> > > 	for (i = 0; i < 10; i++) {
> > > 		delta += dev->min_delta_ns;
> > > 		dev->next_event = ktime_add_ns(ktime_get(), delta);
> > > 		clc = .....
> > > 	   	.....
> > > 
> > > That makes it more likely to succeed fast. Hmm?  
> > 
> > That will set the target time to increasing multiples of min_delta_ns in the
> > future, right?  
> 
> Yes, but without fiddling with min_delta_ns itself.

Grumpf, more extra code for yet another piece of broken hardware
I guess.
 
> > Sure, it should make it succeed faster - I'll make it like
> > that. Are you OK with the arbitrarily chosen 10 retries?  
> 
> I lost my crystalball so I have to trust yours :)

The alternative implementation would be to do the retries in
the clockevent driver itself. Then that particular driver can
choose the correct number of retries, no?

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH 1/3] clockevents: Retry programming min delta up to 10 times

[Thomas Gleixner]

On Thu, 19 Oct 2017, Martin Schwidefsky wrote:

> On Thu, 19 Oct 2017 15:29:28 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > On Thu, 19 Oct 2017, Matt Redfearn wrote:
> > > On 19/10/17 13:43, Thomas Gleixner wrote:  
> > > > 	delta = 0;
> > > > 	for (i = 0; i < 10; i++) {
> > > > 		delta += dev->min_delta_ns;
> > > > 		dev->next_event = ktime_add_ns(ktime_get(), delta);
> > > > 		clc = .....
> > > > 	   	.....
> > > > 
> > > > That makes it more likely to succeed fast. Hmm?  
> > > 
> > > That will set the target time to increasing multiples of min_delta_ns in the
> > > future, right?  
> > 
> > Yes, but without fiddling with min_delta_ns itself.
> 
> Grumpf, more extra code for yet another piece of broken hardware
> I guess.

and virtualization. Oh wait.. the virt is the ultimate reference for broken
hardware...

> > > Sure, it should make it succeed faster - I'll make it like
> > > that. Are you OK with the arbitrarily chosen 10 retries?  
> > 
> > I lost my crystalball so I have to trust yours :)
> 
> The alternative implementation would be to do the retries in
> the clockevent driver itself. Then that particular driver can
> choose the correct number of retries, no?

There is no correct number ever. All you can do is set an upper limit.

Thanks,

	tglx

[tip:timers/core] clockevents: Retry programming min delta up to 10 times

[tip-bot for James Hogan]

Commit-ID:  3a29ddb1c5986a6d3f941bfb1f434105203ce7f6
Gitweb:     https://git.kernel.org/tip/3a29ddb1c5986a6d3f941bfb1f434105203ce7f6
Author:     James Hogan <jhogan@kernel.org>
AuthorDate: Thu, 19 Oct 2017 15:17:23 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 19 Oct 2017 16:29:15 +0200

clockevents: Retry programming min delta up to 10 times

When CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST=n, the call path
hrtimer_reprogram -> clockevents_program_event ->
clockevents_program_min_delta will not retry if the clock event driver
returns -ETIME.

If the driver could not satisfy the program_min_delta for any reason, the
lack of a retry means the CPU may not receive a tick interrupt, potentially
until the counter does a full period. This leads to rcu_sched timeout
messages as the stalled CPU is detected by other CPUs, and other issues if
the CPU is holding locks or other resources at the point at which it
stalls.

There have been a couple of observed mechanisms through which a clock event
driver could not satisfy the requested min_delta and return -ETIME.

With the MIPS GIC driver, the shared execution resource within MT cores
means inconventient latency due to execution of instructions from other
hardware threads in the core, within gic_next_event, can result in an event
being set in the past.

Additionally under virtualisation it is possible to get unexpected latency
during a clockevent device's set_next_event() callback which can make it
return -ETIME even for a delta based on min_delta_ns.

It isn't appropriate to use MIN_ADJUST in the virtualisation case as
occasional hypervisor induced high latency will cause min_delta_ns to
quickly increase to the maximum.

Instead, borrow the retry pattern from the MIN_ADJUST case, but without
making adjustments. Retry up to 10 times, each time increasing the
attempted delta by min_delta, before giving up.

[ Matt: Reworked the loop and made retry increase the delta. ]

Signed-off-by: James Hogan <jhogan@kernel.org>
Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-mips@linux-mips.org
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "Martin Schwidefsky" <schwidefsky@de.ibm.com>
Cc: James Hogan <james.hogan@mips.com>
Link: https://lkml.kernel.org/r/1508422643-6075-1-git-send-email-matt.redfearn@mips.com

---
 kernel/time/clockevents.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 4237e07..16c027e 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -280,17 +280,22 @@ static int clockevents_program_min_delta(struct clock_event_device *dev)
 static int clockevents_program_min_delta(struct clock_event_device *dev)
 {
 	unsigned long long clc;
-	int64_t delta;
+	int64_t delta = 0;
+	int i;
 
-	delta = dev->min_delta_ns;
-	dev->next_event = ktime_add_ns(ktime_get(), delta);
+	for (i = 0; i < 10; i++) {
+		delta += dev->min_delta_ns;
+		dev->next_event = ktime_add_ns(ktime_get(), delta);
 
-	if (clockevent_state_shutdown(dev))
-		return 0;
+		if (clockevent_state_shutdown(dev))
+			return 0;
 
-	dev->retries++;
-	clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
-	return dev->set_next_event((unsigned long) clc, dev);
+		dev->retries++;
+		clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
+		if (dev->set_next_event((unsigned long) clc, dev) == 0)
+			return 0;
+	}
+	return -ETIME;
 }
 
 #endif /* CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST */

Re: [PATCH 2/3] clocksource/mips-gic-timer: Remove pointless irq_save,restore

[Daniel Lezcano]

On 19/10/2017 15:23, Daniel Lezcano wrote:
> On 19/10/2017 13:55, Matt Redfearn wrote:
>> gic_next_event is always called with interrupts disabled, so the save /
>> restore is pointless - remove it.
>>
>> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
>> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
>> ---
> 
> Applied for 4.15. I slightly changed the description and fixed:
> 
> Reported-by: Thomas Gleixner <tglx@linutronix.de>
> Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>

... and removed the unused variable 'flags' :/

https://git.linaro.org/people/daniel.lezcano/linux.git/commit/?h=clockevents/4.15&id=7957b07b559175500b2a03e8a39738c1b4a832fe


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog