linux-kernel - Re: [PATCH RFC 2/5] sched/fair: Skip frequency update if CPU about to idle

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] [day] [month] [year] [list]
Date:   Tue, 7 Nov 2017 21:11:11 -0800
From:   Joel Fernandes <joelaf@...gle.com>
To:     Vincent Guittot <vincent.guittot@...aro.org>
Cc:     Steve Muckle <smuckle@...gle.com>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        "Cc: Srinivas Pandruvada" <srinivas.pandruvada@...ux.intel.com>,
        "Cc: Len Brown" <lenb@...nel.org>,
        "Cc: Juri Lelli" <juri.lelli@....com>,
        "Cc: Patrick Bellasi" <patrick.bellasi@....com>,
        "Cc: Brendan Jackman" <brendan.jackman@....com>,
        "Cc: Chris Redpath" <Chris.Redpath@....com>,
        "Cc: Atish Patra" <atish.patra@...cle.com>,
        "Cc: Dietmar Eggemann" <dietmar.eggemann@....com>,
        "Cc: Morten Ramussen" <morten.rasmussen@....com>,
        "Cc: Frederic Weisbecker" <fweisbec@...il.com>,
        "Cc: Thomas Gleixner" <tglx@...utronix.de>,
        "Cc: EAS Dev" <eas-dev@...ts.linaro.org>,
        "Cc: Android Kernel" <kernel-team@...roid.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        "Kannan, Saravana" <skannan@...cinc.com>
Subject: Re: [PATCH RFC 2/5] sched/fair: Skip frequency update if CPU about to idle

Hi Vincent,

On Mon, Nov 6, 2017 at 12:00 AM, Vincent Guittot
<vincent.guittot@...aro.org> wrote:
> Hi Joel,
>
> On 4 November 2017 at 06:44, Joel Fernandes <joelaf@...gle.com> wrote:
>> On Wed, Nov 1, 2017 at 12:35 PM, Steve Muckle <smuckle@...gle.com> wrote:
>>> On 10/30/2017 12:02 PM, Joel Fernandes wrote:
>>>>>
>>>>> Also, this more looks like a policy decision. Will it be better to
>>>>> put that directly into schedutil? Like this:
>>>>>
>>>>>          if (cpu_idle())
>>>>>                  "Don't change the freq";
>>>>>
>>>>> Will something like that work?
>>>>
>>>>
>>>> I thought about this and I think it wont work very well. In the
>>>> dequeue path we're still running the task being dequeued so the CPU is
>>>> not yet idle. What is needed here IMO is a notion that the CPU is
>>>> possibly about to idle and we can get predict that from the dequeue
>>>> path of the CFS class.
>>>>
>>>> Also just looking at whether the CPU is currently idle or not in the
>>>> governor doesn't help to differentiate between say the dequeue path /
>>>> tick path. Both of which can occur when the CPU is not idle.
>>>>
>>>> Any thoughts about this?
>>>
>>>
>>> Also if it really is the case that this bit of policy is universally
>>> desirable, I'd think it is better to do this in the scheduler and avoid a
>>> needless trip through a fn pointer out to schedutil for performance reasons.
>>
>> I agree.
>>
>> Peter, what do you think? Are you Ok with the approach of this patch
>> (preventing of the cpufreq update call during dequeue)?
>
> I'm not really convinced that we should do change OPP at dequeue.

You mean "should not do", right?

> Although i agree that it makes perfect sense to prevent increasing OPP
> just before going idle for mp3 playback, i'm not sure that this is
> always the case especially for performance oriented use case because
> we delay the OPP increase and as a result the responsiveness of the
> CPU

Actually I think another way to look at it is this is no worse
performance-wise than if the task was running all the time (didn't
sleep) - the next granularity for increasing the frequency would then
be the next Tick.. So, I am not sure practically it makes any
difference to the performance of the task. Even if sleep was short, we
would update frequency on next enqueue/tick so I think we're still
fine from a time-granularity standpoint.

> In  fact this decision really depends about how long we are going to
> sleep. If the cpu will wakes up in few ms, it's worth already
> increasing the frequency when utilization is above the threshold
> because it will not decreases enough to go back to lower OPP. At the
> opposite, if the cpu will not wake up shortly skipping OPP change can
> make sense.
>
> Regarding the reduction of the number of OPP changes, will the
> util_est feature provide the same amount of reduction by directly
> providing the estimated max utilization ?

Yes, I think util_est can help reduce the oscillations which cause
this issue but other than the fact that util_est is not currently
mainlined, I think util_est will still have the same issue if the
util_est's estimation itself oscillates so I think util_est is a
mitigation than a solution. Do you agree?

> Just to say that IMHO skipping or not OPP change at dequeue is a
> policy decision and not a generic one

Yes I agree, but also this means we have to expose scheduler internals
to the governor to detect this case. Are you suggesting if this was to
be implemented - that we pass a flag to governor and make it to do the
decision there based on policy?
As I was discussing in earlier thread with Viresh, simply checking if
CPU is idle in the governor isn't good enough and since this issue is
about the scheduler making cpufreq update call when it didn't need to,
avoiding such a call would make sense to me.

thanks,

- Joel