[PATCH 1/3] clocksource/mips-gic-timer: Fix rcu_sched timeouts from multithreading

[PATCH 1/3] clocksource/mips-gic-timer: Fix rcu_sched timeouts from multithreading

[Matt Redfearn]

When the MIPS GIC clockevent code was written, it appears to have
inherited the 0x300 cycle min delta from the MIPS CPU timer driver. This
is suboptimal for two reasons.

Firstly, the CPU timer counts once every other cycle (i.e. half the
clock rate). The GIC counts once per clock. Assuming that the GIC and
CPU share the same clock this means the GIC is counting twice as fast,
and so the min delta should be (at least) doubled. Fix this by doubling
the min delta to 0x600.

Secondly, the fixed min delta ignores the fact that with MIPS
multithreading active, execution resource within a core is shared
between the hardware threads within that core. An inconvenienly timed
switch of executing thread within gic_next_event, between the read and
write of updated count, can result in the CPU writing an event in the
past, and subsequently not receiving a tick interrupt until the counter
wraps. This stalls the CPU from the RCU scheduler. Other CPUs detect
this and print rcu_sched timeout messages in  the kernel log. It can
lead to other issues as well if the CPU is holding locks or other
resources at the point at which it stalls. Fix this by scaling the min
delta for the timer based on the number of threads in the core
(smp_num_siblings). This accounts for the greater average runtime of
CPUs within a multithreading core.

Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com>
Fixes: b695d8e6ad6f ("clocksource: mips-gic: Use clockevents_config_and_register")
Cc: <stable@vger.kernel.org> # v3.19 +

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

 drivers/clocksource/mips-gic-timer.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
index ae3167c28b12..6c94a74682a2 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -72,6 +72,9 @@ struct irqaction gic_compare_irqaction = {
 static void gic_clockevent_cpu_init(unsigned int cpu,
 				    struct clock_event_device *cd)
 {
+	unsigned long max_d = 0x7fffffff;
+	unsigned long min_d = 0x600;
+
 	cd->name		= "MIPS GIC";
 	cd->features		= CLOCK_EVT_FEAT_ONESHOT |
 				  CLOCK_EVT_FEAT_C3STOP;
@@ -81,7 +84,15 @@ static void gic_clockevent_cpu_init(unsigned int cpu,
 	cd->cpumask		= cpumask_of(cpu);
 	cd->set_next_event	= gic_next_event;
 
-	clockevents_config_and_register(cd, gic_frequency, 0x300, 0x7fffffff);
+	/*
+	 * The min_delta is sensitive to the number of hardware threads in
+	 * the core. With more threads each thread will, on average, get
+	 * less instructions executed per clock. To account for this, we
+	 * scale the min delta based on the number of threads per core.
+	 */
+	min_d *= smp_num_siblings;
+
+	clockevents_config_and_register(cd, gic_frequency, min_d, max_d);
 
 	enable_percpu_irq(gic_timer_irq, IRQ_TYPE_NONE);
 }
-- 
2.7.4

[PATCH 1/3] clocksource/mips-gic-timer: Fix rcu_sched timeouts from multithreading

[Matt Redfearn]

When the MIPS GIC clockevent code was written, it appears to have
inherited the 0x300 cycle min delta from the MIPS CPU timer driver. This
is suboptimal for two reasons.

Firstly, the CPU timer counts once every other cycle (i.e. half the
clock rate). The GIC counts once per clock. Assuming that the GIC and
CPU share the same clock this means the GIC is counting twice as fast,
and so the min delta should be (at least) doubled. Fix this by doubling
the min delta to 0x600.

Secondly, the fixed min delta ignores the fact that with MIPS
multithreading active, execution resource within a core is shared
between the hardware threads within that core. An inconvenienly timed
switch of executing thread within gic_next_event, between the read and
write of updated count, can result in the CPU writing an event in the
past, and subsequently not receiving a tick interrupt until the counter
wraps. This stalls the CPU from the RCU scheduler. Other CPUs detect
this and print rcu_sched timeout messages in  the kernel log. It can
lead to other issues as well if the CPU is holding locks or other
resources at the point at which it stalls. Fix this by scaling the min
delta for the timer based on the number of threads in the core
(smp_num_siblings). This accounts for the greater average runtime of
CPUs within a multithreading core.

Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com>
Fixes: b695d8e6ad6f ("clocksource: mips-gic: Use clockevents_config_and_register")
Cc: <stable@vger.kernel.org> # v3.19 +

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

 drivers/clocksource/mips-gic-timer.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
index ae3167c28b12..6c94a74682a2 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -72,6 +72,9 @@ struct irqaction gic_compare_irqaction = {
 static void gic_clockevent_cpu_init(unsigned int cpu,
 				    struct clock_event_device *cd)
 {
+	unsigned long max_d = 0x7fffffff;
+	unsigned long min_d = 0x600;
+
 	cd->name		= "MIPS GIC";
 	cd->features		= CLOCK_EVT_FEAT_ONESHOT |
 				  CLOCK_EVT_FEAT_C3STOP;
@@ -81,7 +84,15 @@ static void gic_clockevent_cpu_init(unsigned int cpu,
 	cd->cpumask		= cpumask_of(cpu);
 	cd->set_next_event	= gic_next_event;
 
-	clockevents_config_and_register(cd, gic_frequency, 0x300, 0x7fffffff);
+	/*
+	 * The min_delta is sensitive to the number of hardware threads in
+	 * the core. With more threads each thread will, on average, get
+	 * less instructions executed per clock. To account for this, we
+	 * scale the min delta based on the number of threads per core.
+	 */
+	min_d *= smp_num_siblings;
+
+	clockevents_config_and_register(cd, gic_frequency, min_d, max_d);
 
 	enable_percpu_irq(gic_timer_irq, IRQ_TYPE_NONE);
 }
-- 
2.7.4

[PATCH 2/3] clocksource/mips-gic-timer: Ensure IRQs disabled for read-update-write

[Matt Redfearn]

The sequence of reading the current count, adding an offset and writing
back to the compare register is timing critical.
Commit e07127a077c7 ("clocksource: mips-gic-timer: Use new GIC accessor
functions") added a local_irq_save / local_irq_restore to protect the
mapping of another VPs registers through the VP redirect region, but
given the timing critical nature of this code it just feels right to
protect the whole read-update-write section, so move the local_irq_save
before the current count is read.

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

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

diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
index 6c94a74682a2..dadac9f3e9c4 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -43,9 +43,9 @@ static int gic_next_event(unsigned long delta, struct clock_event_device *evt)
 	u64 cnt;
 	int res;
 
+	local_irq_save(flags);
 	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);
-- 
2.7.4

[PATCH 2/3] clocksource/mips-gic-timer: Ensure IRQs disabled for read-update-write

[Matt Redfearn]

The sequence of reading the current count, adding an offset and writing
back to the compare register is timing critical.
Commit e07127a077c7 ("clocksource: mips-gic-timer: Use new GIC accessor
functions") added a local_irq_save / local_irq_restore to protect the
mapping of another VPs registers through the VP redirect region, but
given the timing critical nature of this code it just feels right to
protect the whole read-update-write section, so move the local_irq_save
before the current count is read.

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

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

diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
index 6c94a74682a2..dadac9f3e9c4 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -43,9 +43,9 @@ static int gic_next_event(unsigned long delta, struct clock_event_device *evt)
 	u64 cnt;
 	int res;
 
+	local_irq_save(flags);
 	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);
-- 
2.7.4

[PATCH 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 dadac9f3e9c4..ca334431b0bc 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -39,6 +39,7 @@ 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;
@@ -46,8 +47,12 @@ static int gic_next_event(unsigned long delta, struct clock_event_device *evt)
 	local_irq_save(flags);
 	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);
+	}
 	local_irq_restore(flags);
 	res = ((int)(gic_read_count() - cnt) >= 0) ? -ETIME : 0;
 	return res;
-- 
2.7.4

[PATCH 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 dadac9f3e9c4..ca334431b0bc 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -39,6 +39,7 @@ 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;
@@ -46,8 +47,12 @@ static int gic_next_event(unsigned long delta, struct clock_event_device *evt)
 	local_irq_save(flags);
 	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);
+	}
 	local_irq_restore(flags);
 	res = ((int)(gic_read_count() - cnt) >= 0) ? -ETIME : 0;
 	return res;
-- 
2.7.4

Re: [PATCH 1/3] clocksource/mips-gic-timer: Fix rcu_sched timeouts from multithreading

[Daniel Lezcano]

On 11/10/2017 16:01, Matt Redfearn wrote:
> When the MIPS GIC clockevent code was written, it appears to have
> inherited the 0x300 cycle min delta from the MIPS CPU timer driver. This
> is suboptimal for two reasons.
> 
> Firstly, the CPU timer counts once every other cycle (i.e. half the
> clock rate). The GIC counts once per clock. Assuming that the GIC and
> CPU share the same clock this means the GIC is counting twice as fast,
> and so the min delta should be (at least) doubled. Fix this by doubling
> the min delta to 0x600.
> 
> Secondly, the fixed min delta ignores the fact that with MIPS
> multithreading active, execution resource within a core is shared
> between the hardware threads within that core. An inconvenienly timed
> switch of executing thread within gic_next_event, between the read and
> write of updated count, can result in the CPU writing an event in the
> past, and subsequently not receiving a tick interrupt until the counter
> wraps. This stalls the CPU from the RCU scheduler. Other CPUs detect
> this and print rcu_sched timeout messages in  the kernel log. It can
> lead to other issues as well if the CPU is holding locks or other
> resources at the point at which it stalls. Fix this by scaling the min
> delta for the timer based on the number of threads in the core
> (smp_num_siblings). This accounts for the greater average runtime of
> CPUs within a multithreading core.
> 
> Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com>
> Fixes: b695d8e6ad6f ("clocksource: mips-gic: Use clockevents_config_and_register")
> Cc: <stable@vger.kernel.org> # v3.19 +
> 
> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
Hi Matt,

I applied the 3 patches for 4.15.

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] clocksource/mips-gic-timer: Fix rcu_sched timeouts from multithreading

[Thomas Gleixner]

On Wed, 11 Oct 2017, Matt Redfearn wrote:

> When the MIPS GIC clockevent code was written, it appears to have
> inherited the 0x300 cycle min delta from the MIPS CPU timer driver. This
> is suboptimal for two reasons.
> 
> Firstly, the CPU timer counts once every other cycle (i.e. half the
> clock rate). The GIC counts once per clock. Assuming that the GIC and
> CPU share the same clock this means the GIC is counting twice as fast,
> and so the min delta should be (at least) doubled. Fix this by doubling
> the min delta to 0x600.
> 
> Secondly, the fixed min delta ignores the fact that with MIPS
> multithreading active, execution resource within a core is shared
> between the hardware threads within that core. An inconvenienly timed
> switch of executing thread within gic_next_event, between the read and
> write of updated count, can result in the CPU writing an event in the
> past, and subsequently not receiving a tick interrupt until the counter
> wraps. This stalls the CPU from the RCU scheduler. Other CPUs detect
> this and print rcu_sched timeout messages in  the kernel log. It can
> lead to other issues as well if the CPU is holding locks or other
> resources at the point at which it stalls. Fix this by scaling the min
> delta for the timer based on the number of threads in the core
> (smp_num_siblings). This accounts for the greater average runtime of
> CPUs within a multithreading core.

I don't understand why this is not catched by the check at the end of the
next_event() function:

        res = ((int)(gic_read_count() - cnt) >= 0) ? -ETIME : 0;

Btw, the local_irq_save() in this function is pointless as this function is
always called with interrupts disabled from the core code.

Thanks,

	tglx

Re: [PATCH 1/3] clocksource/mips-gic-timer: Fix rcu_sched timeouts from multithreading

[Matt Redfearn]

On 18/10/17 21:34, Thomas Gleixner wrote:
> On Wed, 11 Oct 2017, Matt Redfearn wrote:
>
>> When the MIPS GIC clockevent code was written, it appears to have
>> inherited the 0x300 cycle min delta from the MIPS CPU timer driver. This
>> is suboptimal for two reasons.
>>
>> Firstly, the CPU timer counts once every other cycle (i.e. half the
>> clock rate). The GIC counts once per clock. Assuming that the GIC and
>> CPU share the same clock this means the GIC is counting twice as fast,
>> and so the min delta should be (at least) doubled. Fix this by doubling
>> the min delta to 0x600.
>>
>> Secondly, the fixed min delta ignores the fact that with MIPS
>> multithreading active, execution resource within a core is shared
>> between the hardware threads within that core. An inconvenienly timed
>> switch of executing thread within gic_next_event, between the read and
>> write of updated count, can result in the CPU writing an event in the
>> past, and subsequently not receiving a tick interrupt until the counter
>> wraps. This stalls the CPU from the RCU scheduler. Other CPUs detect
>> this and print rcu_sched timeout messages in  the kernel log. It can
>> lead to other issues as well if the CPU is holding locks or other
>> resources at the point at which it stalls. Fix this by scaling the min
>> delta for the timer based on the number of threads in the core
>> (smp_num_siblings). This accounts for the greater average runtime of
>> CPUs within a multithreading core.
> I don't understand why this is not catched by the check at the end of the
> next_event() function:
>
>          res = ((int)(gic_read_count() - cnt) >= 0) ? -ETIME : 0;
>
> Btw, the local_irq_save() in this function is pointless as this function is
> always called with interrupts disabled from the core code.
>
> Thanks,
>
> 	tglx
>
>

Hi tglx,

This is an issue because in some cases (hrtimer_reprogram -> 
clockevents_program_event -> clockevents_program_min_delta, when 
CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST=n) there is no retry performed in 
the case of -ETIME. There has been a patch pending for some time 
https://patchwork.kernel.org/patch/8909491/ which ought to address this 
and retry in the case of an event in the past on this call path. But in 
the meantime this patch vastly improves the situation.

Thanks,
Matt

Re: [PATCH 1/3] clocksource/mips-gic-timer: Fix rcu_sched timeouts from multithreading

[Matt Redfearn]

On 18/10/17 21:34, Thomas Gleixner wrote:
> On Wed, 11 Oct 2017, Matt Redfearn wrote:
>
>> When the MIPS GIC clockevent code was written, it appears to have
>> inherited the 0x300 cycle min delta from the MIPS CPU timer driver. This
>> is suboptimal for two reasons.
>>
>> Firstly, the CPU timer counts once every other cycle (i.e. half the
>> clock rate). The GIC counts once per clock. Assuming that the GIC and
>> CPU share the same clock this means the GIC is counting twice as fast,
>> and so the min delta should be (at least) doubled. Fix this by doubling
>> the min delta to 0x600.
>>
>> Secondly, the fixed min delta ignores the fact that with MIPS
>> multithreading active, execution resource within a core is shared
>> between the hardware threads within that core. An inconvenienly timed
>> switch of executing thread within gic_next_event, between the read and
>> write of updated count, can result in the CPU writing an event in the
>> past, and subsequently not receiving a tick interrupt until the counter
>> wraps. This stalls the CPU from the RCU scheduler. Other CPUs detect
>> this and print rcu_sched timeout messages in  the kernel log. It can
>> lead to other issues as well if the CPU is holding locks or other
>> resources at the point at which it stalls. Fix this by scaling the min
>> delta for the timer based on the number of threads in the core
>> (smp_num_siblings). This accounts for the greater average runtime of
>> CPUs within a multithreading core.
> I don't understand why this is not catched by the check at the end of the
> next_event() function:
>
>          res = ((int)(gic_read_count() - cnt) >= 0) ? -ETIME : 0;
>
> Btw, the local_irq_save() in this function is pointless as this function is
> always called with interrupts disabled from the core code.
>
> Thanks,
>
> 	tglx
>
>

Hi tglx,

This is an issue because in some cases (hrtimer_reprogram -> 
clockevents_program_event -> clockevents_program_min_delta, when 
CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST=n) there is no retry performed in 
the case of -ETIME. There has been a patch pending for some time 
https://patchwork.kernel.org/patch/8909491/ which ought to address this 
and retry in the case of an event in the past on this call path. But in 
the meantime this patch vastly improves the situation.

Thanks,
Matt

Re: [PATCH 1/3] clocksource/mips-gic-timer: Fix rcu_sched timeouts from multithreading

[Thomas Gleixner]

On Thu, 19 Oct 2017, Matt Redfearn wrote:
> On 18/10/17 21:34, Thomas Gleixner wrote:
> > On Wed, 11 Oct 2017, Matt Redfearn wrote:
> > > Secondly, the fixed min delta ignores the fact that with MIPS
> > > multithreading active, execution resource within a core is shared
> > > between the hardware threads within that core. An inconvenienly timed
> > > switch of executing thread within gic_next_event, between the read and
> > > write of updated count, can result in the CPU writing an event in the
> > > past, and subsequently not receiving a tick interrupt until the counter
> > > wraps. This stalls the CPU from the RCU scheduler. Other CPUs detect
> > > this and print rcu_sched timeout messages in  the kernel log. It can
> > > lead to other issues as well if the CPU is holding locks or other
> > > resources at the point at which it stalls. Fix this by scaling the min
> > > delta for the timer based on the number of threads in the core
> > > (smp_num_siblings). This accounts for the greater average runtime of
> > > CPUs within a multithreading core.
> >
> > I don't understand why this is not catched by the check at the end of the
> > next_event() function:
> > 
> >          res = ((int)(gic_read_count() - cnt) >= 0) ? -ETIME : 0;
> > 
> > Btw, the local_irq_save() in this function is pointless as this function is
> > always called with interrupts disabled from the core code.
> 
> This is an issue because in some cases (hrtimer_reprogram ->
> clockevents_program_event -> clockevents_program_min_delta, when
> CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST=n) there is no retry performed in the
> case of -ETIME. There has been a patch pending for some time
> https://patchwork.kernel.org/patch/8909491/ which ought to address this and
> retry in the case of an event in the past on this call path. But in the
> meantime this patch vastly improves the situation.

I somehow missed that one. Care to repost so we get that solved at the
place where it should be solved.

Thanks,

	tglx

Re: [PATCH 1/3] clocksource/mips-gic-timer: Fix rcu_sched timeouts from multithreading

[Daniel Lezcano]

On 11/10/2017 16:01, Matt Redfearn wrote:
> When the MIPS GIC clockevent code was written, it appears to have
> inherited the 0x300 cycle min delta from the MIPS CPU timer driver. This
> is suboptimal for two reasons.
> 
> Firstly, the CPU timer counts once every other cycle (i.e. half the
> clock rate). The GIC counts once per clock. Assuming that the GIC and
> CPU share the same clock this means the GIC is counting twice as fast,
> and so the min delta should be (at least) doubled. Fix this by doubling
> the min delta to 0x600.
> 
> Secondly, the fixed min delta ignores the fact that with MIPS
> multithreading active, execution resource within a core is shared
> between the hardware threads within that core. An inconvenienly timed
> switch of executing thread within gic_next_event, between the read and
> write of updated count, can result in the CPU writing an event in the
> past, and subsequently not receiving a tick interrupt until the counter
> wraps. This stalls the CPU from the RCU scheduler. Other CPUs detect
> this and print rcu_sched timeout messages in  the kernel log. It can
> lead to other issues as well if the CPU is holding locks or other
> resources at the point at which it stalls. Fix this by scaling the min
> delta for the timer based on the number of threads in the core
> (smp_num_siblings). This accounts for the greater average runtime of
> CPUs within a multithreading core.
> 
> Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com>
> Fixes: b695d8e6ad6f ("clocksource: mips-gic: Use clockevents_config_and_register")
> Cc: <stable@vger.kernel.org> # v3.19 +
> 
> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
> ---

Matt,

I'm dropping your series.

The first patch fails to compile because of the smp_num_siblings
variable undefined when compile testing on another arch (probably a
header is missing).

The issue the patch address could be fixed in the time framework as
stated by Thomas.

As spotted by Thomas also, the local_irq_save() is not needed in the
set_next_event() function, so the second patch is pointless and a patch
removing those local_irq_save()/restore() would make more sense.

The third patch does not longer apply after removing the two above.

 -- 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] clocksource/mips-gic-timer: Fix rcu_sched timeouts from multithreading

[Daniel Lezcano]

On 18/10/2017 22:34, Thomas Gleixner wrote:
> On Wed, 11 Oct 2017, Matt Redfearn wrote:
> 
>> When the MIPS GIC clockevent code was written, it appears to have
>> inherited the 0x300 cycle min delta from the MIPS CPU timer driver. This
>> is suboptimal for two reasons.
>>
>> Firstly, the CPU timer counts once every other cycle (i.e. half the
>> clock rate). The GIC counts once per clock. Assuming that the GIC and
>> CPU share the same clock this means the GIC is counting twice as fast,
>> and so the min delta should be (at least) doubled. Fix this by doubling
>> the min delta to 0x600.
>>
>> Secondly, the fixed min delta ignores the fact that with MIPS
>> multithreading active, execution resource within a core is shared
>> between the hardware threads within that core. An inconvenienly timed
>> switch of executing thread within gic_next_event, between the read and
>> write of updated count, can result in the CPU writing an event in the
>> past, and subsequently not receiving a tick interrupt until the counter
>> wraps. This stalls the CPU from the RCU scheduler. Other CPUs detect
>> this and print rcu_sched timeout messages in  the kernel log. It can
>> lead to other issues as well if the CPU is holding locks or other
>> resources at the point at which it stalls. Fix this by scaling the min
>> delta for the timer based on the number of threads in the core
>> (smp_num_siblings). This accounts for the greater average runtime of
>> CPUs within a multithreading core.
> 
> I don't understand why this is not catched by the check at the end of the
> next_event() function:
> 
>         res = ((int)(gic_read_count() - cnt) >= 0) ? -ETIME : 0;
> 
> Btw, the local_irq_save() in this function is pointless as this function is
> always called with interrupts disabled from the core code.

Would it be worth to add some comment in include/linux/clockchips.h in
the structure definition for the different callbacks to tell which ones
are called with the irq disabled ?


-- 
 <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] clocksource/mips-gic-timer: Fix rcu_sched timeouts from multithreading

[Thomas Gleixner]

On Thu, 19 Oct 2017, Daniel Lezcano wrote:
> On 18/10/2017 22:34, Thomas Gleixner wrote:
> > On Wed, 11 Oct 2017, Matt Redfearn wrote:
> > 
> >> When the MIPS GIC clockevent code was written, it appears to have
> >> inherited the 0x300 cycle min delta from the MIPS CPU timer driver. This
> >> is suboptimal for two reasons.
> >>
> >> Firstly, the CPU timer counts once every other cycle (i.e. half the
> >> clock rate). The GIC counts once per clock. Assuming that the GIC and
> >> CPU share the same clock this means the GIC is counting twice as fast,
> >> and so the min delta should be (at least) doubled. Fix this by doubling
> >> the min delta to 0x600.
> >>
> >> Secondly, the fixed min delta ignores the fact that with MIPS
> >> multithreading active, execution resource within a core is shared
> >> between the hardware threads within that core. An inconvenienly timed
> >> switch of executing thread within gic_next_event, between the read and
> >> write of updated count, can result in the CPU writing an event in the
> >> past, and subsequently not receiving a tick interrupt until the counter
> >> wraps. This stalls the CPU from the RCU scheduler. Other CPUs detect
> >> this and print rcu_sched timeout messages in  the kernel log. It can
> >> lead to other issues as well if the CPU is holding locks or other
> >> resources at the point at which it stalls. Fix this by scaling the min
> >> delta for the timer based on the number of threads in the core
> >> (smp_num_siblings). This accounts for the greater average runtime of
> >> CPUs within a multithreading core.
> > 
> > I don't understand why this is not catched by the check at the end of the
> > next_event() function:
> > 
> >         res = ((int)(gic_read_count() - cnt) >= 0) ? -ETIME : 0;
> > 
> > Btw, the local_irq_save() in this function is pointless as this function is
> > always called with interrupts disabled from the core code.
> 
> Would it be worth to add some comment in include/linux/clockchips.h in
> the structure definition for the different callbacks to tell which ones
> are called with the irq disabled ?

Yes. IIRC all callbacks are invoked with interrupts disabled. Care to check
that and whip up a patch?

Thanks,

	tglx

Re: [PATCH 1/3] clocksource/mips-gic-timer: Fix rcu_sched timeouts from multithreading

[Matt Redfearn]

On 19/10/17 10:09, Daniel Lezcano wrote:
> On 11/10/2017 16:01, Matt Redfearn wrote:
>> When the MIPS GIC clockevent code was written, it appears to have
>> inherited the 0x300 cycle min delta from the MIPS CPU timer driver. This
>> is suboptimal for two reasons.
>>
>> Firstly, the CPU timer counts once every other cycle (i.e. half the
>> clock rate). The GIC counts once per clock. Assuming that the GIC and
>> CPU share the same clock this means the GIC is counting twice as fast,
>> and so the min delta should be (at least) doubled. Fix this by doubling
>> the min delta to 0x600.
>>
>> Secondly, the fixed min delta ignores the fact that with MIPS
>> multithreading active, execution resource within a core is shared
>> between the hardware threads within that core. An inconvenienly timed
>> switch of executing thread within gic_next_event, between the read and
>> write of updated count, can result in the CPU writing an event in the
>> past, and subsequently not receiving a tick interrupt until the counter
>> wraps. This stalls the CPU from the RCU scheduler. Other CPUs detect
>> this and print rcu_sched timeout messages in  the kernel log. It can
>> lead to other issues as well if the CPU is holding locks or other
>> resources at the point at which it stalls. Fix this by scaling the min
>> delta for the timer based on the number of threads in the core
>> (smp_num_siblings). This accounts for the greater average runtime of
>> CPUs within a multithreading core.
>>
>> Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com>
>> Fixes: b695d8e6ad6f ("clocksource: mips-gic: Use clockevents_config_and_register")
>> Cc: <stable@vger.kernel.org> # v3.19 +
>>
>> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
>> ---
> Matt,
>
> I'm dropping your series.
>
> The first patch fails to compile because of the smp_num_siblings
> variable undefined when compile testing on another arch (probably a
> header is missing).
>
> The issue the patch address could be fixed in the time framework as
> stated by Thomas.
>
> As spotted by Thomas also, the local_irq_save() is not needed in the
> set_next_event() function, so the second patch is pointless and a patch
> removing those local_irq_save()/restore() would make more sense.
>
> The third patch does not longer apply after removing the two above.
>
>   -- Daniel
>

Hi Daniel,

Yep, makes sense that's fine - thanks!

Matt

Re: [PATCH 1/3] clocksource/mips-gic-timer: Fix rcu_sched timeouts from multithreading

[Matt Redfearn]

On 19/10/17 10:09, Daniel Lezcano wrote:
> On 11/10/2017 16:01, Matt Redfearn wrote:
>> When the MIPS GIC clockevent code was written, it appears to have
>> inherited the 0x300 cycle min delta from the MIPS CPU timer driver. This
>> is suboptimal for two reasons.
>>
>> Firstly, the CPU timer counts once every other cycle (i.e. half the
>> clock rate). The GIC counts once per clock. Assuming that the GIC and
>> CPU share the same clock this means the GIC is counting twice as fast,
>> and so the min delta should be (at least) doubled. Fix this by doubling
>> the min delta to 0x600.
>>
>> Secondly, the fixed min delta ignores the fact that with MIPS
>> multithreading active, execution resource within a core is shared
>> between the hardware threads within that core. An inconvenienly timed
>> switch of executing thread within gic_next_event, between the read and
>> write of updated count, can result in the CPU writing an event in the
>> past, and subsequently not receiving a tick interrupt until the counter
>> wraps. This stalls the CPU from the RCU scheduler. Other CPUs detect
>> this and print rcu_sched timeout messages in  the kernel log. It can
>> lead to other issues as well if the CPU is holding locks or other
>> resources at the point at which it stalls. Fix this by scaling the min
>> delta for the timer based on the number of threads in the core
>> (smp_num_siblings). This accounts for the greater average runtime of
>> CPUs within a multithreading core.
>>
>> Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com>
>> Fixes: b695d8e6ad6f ("clocksource: mips-gic: Use clockevents_config_and_register")
>> Cc: <stable@vger.kernel.org> # v3.19 +
>>
>> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
>> ---
> Matt,
>
> I'm dropping your series.
>
> The first patch fails to compile because of the smp_num_siblings
> variable undefined when compile testing on another arch (probably a
> header is missing).
>
> The issue the patch address could be fixed in the time framework as
> stated by Thomas.
>
> As spotted by Thomas also, the local_irq_save() is not needed in the
> set_next_event() function, so the second patch is pointless and a patch
> removing those local_irq_save()/restore() would make more sense.
>
> The third patch does not longer apply after removing the two above.
>
>   -- Daniel
>

Hi Daniel,

Yep, makes sense that's fine - thanks!

Matt

Re: [PATCH 1/3] clocksource/mips-gic-timer: Fix rcu_sched timeouts from multithreading

[Daniel Lezcano]

On 19/10/2017 11:18, Thomas Gleixner wrote:
> On Thu, 19 Oct 2017, Daniel Lezcano wrote:
>> On 18/10/2017 22:34, Thomas Gleixner wrote:
>>> On Wed, 11 Oct 2017, Matt Redfearn wrote:
>>>
>>>> When the MIPS GIC clockevent code was written, it appears to have
>>>> inherited the 0x300 cycle min delta from the MIPS CPU timer driver. This
>>>> is suboptimal for two reasons.
>>>>
>>>> Firstly, the CPU timer counts once every other cycle (i.e. half the
>>>> clock rate). The GIC counts once per clock. Assuming that the GIC and
>>>> CPU share the same clock this means the GIC is counting twice as fast,
>>>> and so the min delta should be (at least) doubled. Fix this by doubling
>>>> the min delta to 0x600.
>>>>
>>>> Secondly, the fixed min delta ignores the fact that with MIPS
>>>> multithreading active, execution resource within a core is shared
>>>> between the hardware threads within that core. An inconvenienly timed
>>>> switch of executing thread within gic_next_event, between the read and
>>>> write of updated count, can result in the CPU writing an event in the
>>>> past, and subsequently not receiving a tick interrupt until the counter
>>>> wraps. This stalls the CPU from the RCU scheduler. Other CPUs detect
>>>> this and print rcu_sched timeout messages in  the kernel log. It can
>>>> lead to other issues as well if the CPU is holding locks or other
>>>> resources at the point at which it stalls. Fix this by scaling the min
>>>> delta for the timer based on the number of threads in the core
>>>> (smp_num_siblings). This accounts for the greater average runtime of
>>>> CPUs within a multithreading core.
>>>
>>> I don't understand why this is not catched by the check at the end of the
>>> next_event() function:
>>>
>>>         res = ((int)(gic_read_count() - cnt) >= 0) ? -ETIME : 0;
>>>
>>> Btw, the local_irq_save() in this function is pointless as this function is
>>> always called with interrupts disabled from the core code.
>>
>> Would it be worth to add some comment in include/linux/clockchips.h in
>> the structure definition for the different callbacks to tell which ones
>> are called with the irq disabled ?
> 
> Yes. IIRC all callbacks are invoked with interrupts disabled. Care to check
> that and whip up a patch?

Sure, no problem.


-- 
 <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