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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxx>
Fixes: b695d8e6ad6f ("clocksource: mips-gic: Use clockevents_config_and_register")
Cc: <stable@xxxxxxxxxxxxxxx> # v3.19 +

Signed-off-by: Matt Redfearn <matt.redfearn@xxxxxxxx>
---
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



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]