linux-kernel - Re: [PATCH/RFC] clockevents: Ignore C3STOP when CPUIdle is disabled

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]

Message-ID: <CANqRtoTLvTKgSRHWOOW3WHZ7LcgS4xijmz3KN1tgSLCO532_Cw@mail.gmail.com>
Date:	Tue, 18 Jun 2013 17:49:23 +0900
From:	Magnus Damm <magnus.damm@...il.com>
To:	Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	Mark Rutland <mark.rutland@....com>,
	SH-Linux <linux-sh@...r.kernel.org>,
	Marc Zyngier <marc.zyngier@....com>,
	Catalin Marinas <catalin.marinas@....com>,
	"Simon Horman [Horms]" <horms@...ge.net.au>,
	John Stultz <john.stultz@...aro.org>,
	Shinya Kuribayashi <shinya.kuribayashi.px@...esas.com>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH/RFC] clockevents: Ignore C3STOP when CPUIdle is disabled

Hi Daniel,

On Tue, Jun 18, 2013 at 5:24 PM, Daniel Lezcano
<daniel.lezcano@...aro.org> wrote:
> On 06/18/2013 09:39 AM, Magnus Damm wrote:
>> On Tue, Jun 18, 2013 at 4:32 PM, Daniel Lezcano
>> <daniel.lezcano@...aro.org> wrote:
>>> On 06/18/2013 09:17 AM, Magnus Damm wrote:
>>>> From: Magnus Damm <damm@...nsource.se>
>>>>
>>>> Introduce the function tick_device_may_c3stop() that
>>>> ignores the C3STOP flag in case CPUIdle is disabled.
>>>>
>>>> The C3STOP flag tells the system that a clock event
>>>> device may be stopped during deep sleep, but if this
>>>> will happen or not depends on things like if CPUIdle
>>>> is enabled and if a CPUIdle driver is available.
>>>>
>>>> This patch assumes that if CPUIdle is disabled then
>>>> the sleep mode triggering C3STOP will never be entered.
>>>> So by ignoring C3STOP when CPUIdle is disabled then it
>>>> becomes possible to use high resolution timers with only
>>>> per-cpu local timers - regardless if they have the
>>>> C3STOP flag set or not.
>>>>
>>>> Observed on the r8a73a4 SoC that at this point only uses
>>>> ARM architected timers for clock event and clock sources.
>>>>
>>>> Without this patch high resolution timers are run time
>>>> disabled on the r8a73a4 SoC - this regardless of CPUIdle
>>>> is disabled or not.
>>>>
>>>> The less short term fix is to add support for more timers
>>>> on the r8a73a4 SoC, but until CPUIdle support is enabled
>>>> it must be possible to use high resoultion timers without
>>>> additional timers.
>>>>
>>>> I'd like to hear some feedback and also test this on more
>>>> systems before merging the code, see the non-SOB below.
>>>
>>> Do we need a broadcast timer when cpuidle is not compiled in the kernel ?
>>
>> Yes, if there is no per-cpu timer available. It depends on what the
>> SMP support code for a particular SoC or architecture happen to
>> enable.
>
> Ok thanks for the information.

No problem. Thanks for your comments!

> There is here a multiple occurrence of the information "the timer will
> stop when power is saved": CLOCK_EVT_FEAT_C3STOP and
> CPUIDLE_FLAG_TIMER_STOP, so I am wondering if some code simplification
> couldn't be done before your patch.

I'm sure it's possible to rearrange things in many ways, and the area
that you point out indeed seems to have some overlap. Somehow
describing which timers that stop during what CPUIdle sleep state
would be nice to have. Also, today clock event drivers simply state
C3STOP but there may be shallow sleep modes where the timer doesn't
have to stop. It all seems a bit coarse grained to me as-is.

> The function:
>
> tick_broadcast_oneshot_control is called from clockevents_notify. This
> one is called from the cpuidle framework or the back-end cpuidle driver.
> The caller knows the timer will be stop and this is why it is switching
> to the broadcast mode. But we have a sanity check in
> tick_broadcast_oneshot_control function:
>
>         if (!(dev->features & CLOCK_EVT_FEAT_C3STOP))
>                 return;
>
> In other words, CPUIDLE_FLAG_TIMER_STOP will tell the framework to call
> clockevents_notify and the tick broadcast code will re-check the device
> will effectively go down. IMHO, we can get rid of this check.
>
> The same happens for the tick_do_broadcast_on_off function.
>
> That reduces the number of C3STOP usage.

That may very well be the case. Care to hack up a patch? =)

The goal with this patch is simply to make it possible to use high
resolution timers if CPUIdle is disabled. Right now the ARM
architected timer is sort of optimized for power, so it sets the
C3STOP flag to say that on some SoCs during some sleep modes these
timers may stop. My point is that this flag doesn't matter as long as
CPUIdle is disabled.

Thanks,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/