Re: [PATCH v5 2/5] tick: Add freeze timer events

[Rafael J. Wysocki]

On Tue, Jul 18, 2017 at 2:30 AM, dbasehore . <dbasehore@xxxxxxxxxxxx> wrote:
> On Sat, Jul 15, 2017 at 5:39 AM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
>> On Thursday, July 13, 2017 03:58:53 PM dbasehore . wrote:
>>> On Thu, Jul 13, 2017 at 8:09 AM, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>>> > On Thu, Jul 13, 2017 at 9:32 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>>> >> On Fri, Jul 07, 2017 at 05:03:00PM -0700, Derek Basehore wrote:
>>> >>> Adds a new feature to tick to schedule wakeups on a CPU during freeze.
>>> >>> This won't fully wake up the system (devices are not resumed), but
>>> >>> allow simple platform functionality to be run during freeze with
>>> >>> little power impact.
>>> >>>
>>> >>> This implementation allows an idle driver to setup a timer event with
>>> >>> the clock event device when entering freeze by calling
>>> >>> tick_set_freeze_event. Only one caller should exist for the function.
>>> >>>
>>> >>> tick_freeze_event_expired is used to check if the timer went off when
>>> >>> the CPU wakes.
>>> >>>
>>> >>> The event is cleared by tick_clear_freeze_event.
>>> >>
>>> >> Why? What's wrong with using the RTC stuff? RTC should be able to wake
>>> >> suspended systems, see RTCWAKE(8).
>>> >
>>> > The RTC interrupt is an SCI (on ACPI systems) and we don't handle it
>>> > at this point, so we don't know what woke us up until we re-enable
>>> > interrupt handlers and run the one for the SCI.
>>>
>>> To add to that point, RTC wake ups are valid for fully waking up the
>>> system. The clock event wake up wasn't used for waking up the system
>>> before, so we know that we don't have to check if it should wake up
>>> the system entirely. The way rtc timers work right now, I think that
>>> we'd have to go all the way through resume devices to figure out if we
>>> should resume to user space or freeze again.
>>
>> Actually, that's not exactly the case any more.
>>
>> After some changes that went in for 4.13-rc1 there is an additional decision
>> point in the resume path, after the noirq stage, where we can decide to go back
>> to sleep if that's the right thing to do.
>>
>> This means that in principle you might hack the CMOS RTC driver to do something
>> more sophisticated than just calling pm_wakeup_hard_event() in rtc_handler().
>>
>> That's ACPI-specific, but I think you have ACPI on all of the systems where the
>> residency counders are going to be checked anyway.
>
> This will take more power than the current implementation I have.
> While I'm fine with that since the power draw would probably be within
> 100uW to 1mW, it's worth noting. This is because of the added overhead
> of noirq suspend and resume which take a combined time of about 50 to
> 100 ms depending on the platform. The implementation that used the
> APIC uses about 3uW.

That's correct, but I'm not quite sure how much of a difference it
makes in practice.

> Rather than make the change in rtc_handler for the CMOS RTC driver,
> the change might be better in the drivers/rtc/interface.c code to
> better handle multiple RTC alarms. For example, there might be 2
> alarms set for the same time (one that won't wake the system and one
> that will) or 2 alarms 1 second apart. In the later case, it's
> possible that 1 second will pass before the second alarm is scheduled.
> We need to make sure that the RTC irq runs before calling
> dpm_suspend_noirq too.

Well, I guess the choice will depend on which patch looks more
straightforward. :-)

> If I remember correctly, I proposed using the RTC to wakeup for this
> check to you, but you recommended using the APIC instead. This was of
> course before the additional decision point was added to freeze.

Right.  That's why I recommended it at that time.

Thanks,
Rafael