linux-kernel - Re: [PATCH 1/3] clocksource/mips-gic-timer: Fix rcu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 19 Oct 2017 10:21:50 +0100
From:   Matt Redfearn <matt.redfearn@...s.com>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>
CC:     <linux-mips@...ux-mips.org>,
        Matt Redfearn <matt.redfearn@...tec.com>,
        "# v3 . 19 +" <stable@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] clocksource/mips-gic-timer: Fix rcu_sched timeouts
 from multithreading



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@...tec.com>
>> Fixes: b695d8e6ad6f ("clocksource: mips-gic: Use clockevents_config_and_register")
>> Cc: <stable@...r.kernel.org> # v3.19 +
>>
>> Signed-off-by: Matt Redfearn <matt.redfearn@...s.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