Re: [PATCH 01/70] cpufreq: interactive: New 'interactive' governor

From: Viresh Kumar
Date: Tue Oct 27 2015 - 23:00:52 EST


Hi Bálint,

On 28-10-15, 01:59, Rafael J. Wysocki wrote:
> On Tuesday, October 27, 2015 06:29:49 PM Bálint Czobor wrote:
> > From: Mike Chan <mike@xxxxxxxxxxx>
> >
> > This governor is designed for latency-sensitive workloads, such as
> > interactive user interfaces. The interactive governor aims to be
> > significantly more responsive to ramp CPU quickly up when CPU-intensive
> > activity begins.
> >
> > Existing governors sample CPU load at a particular rate, typically
> > every X ms. This can lead to under-powering UI threads for the period of
> > time during which the user begins interacting with a previously-idle system
> > until the next sample period happens.
> >
> > The 'interactive' governor uses a different approach. Instead of sampling
> > the CPU at a specified rate, the governor will check whether to scale the
> > CPU frequency up soon after coming out of idle. When the CPU comes out of
> > idle, a timer is configured to fire within 1-2 ticks. If the CPU is very
> > busy from exiting idle to when the timer fires then we assume the CPU is
> > underpowered and ramp to MAX speed.
> >
> > If the CPU was not sufficiently busy to immediately ramp to MAX speed, then
> > the governor evaluates the CPU load since the last speed adjustment,
> > choosing the highest value between that longer-term load or the short-term
> > load since idle exit to determine the CPU speed to ramp to.
> >
> > A realtime thread is used for scaling up, giving the remaining tasks the
> > CPU performance benefit, unlike existing governors which are more likely to
> > schedule rampup work to occur after your performance starved tasks have
> > completed.
> >
> > The tuneables for this governor are:
> > /sys/devices/system/cpu/cpufreq/interactive/min_sample_time:
> > The minimum amount of time to spend at the current frequency before
> > ramping down. This is to ensure that the governor has seen enough
> > historic CPU load data to determine the appropriate workload.
> > Default is 80000 uS.
> > /sys/devices/system/cpu/cpufreq/interactive/go_maxspeed_load
> > The CPU load at which to ramp to max speed. Default is 85.
> >
> > Change-Id: Ib2b362607c62f7c56d35f44a9ef3280f98c17585
> > Signed-off-by: Mike Chan <mike@xxxxxxxxxxx>
> > Signed-off-by: Todd Poynor <toddpoynor@xxxxxxxxxx>
> > Bug: 3152864
> > Signed-off-by: Bálint Czobor <czoborbalint@xxxxxxxxx>
>
> It's good to see that submitted, but it'll have to go through a detailed
> review which is going to take some time.

These are the last memories I have around upstreaming this governor:
http://marc.info/?l=linux-kernel&m=132867057910479&w=2

Has anything changed after that? Or we decided to go ahead with it and
upstream ?

> One my observation after a cursory look at it is that at least some later
> patches of the series modify drivers/cpufreq/cpufreq_interactive.c which is
> a new file added by the first patch. Is there any particular reason to
> avoid folding all of those patches into the first one?

Right, not to discourage you from upstreaming stuff, but there were
few things that went wrong here:

- I don't see the idle_notifier_register thing present in upstream
kernel for anything other than x86, and I am sure you were
targeting ARM here :)
- And I don't see the same stuff present in your patchset as well,
sorry if I am wrong. And if I am write, you didn't even base this of
a mainline release and compile tested it.
- 70 patches in a series are really really huge. I wouldn't expect
people to review any of that. Though to be fair enough, I must admit
of doing similar things in the past :)
- There is something called as a cover-letter (in case you aren't
aware about it), which should have been written here to give a
really nice summary about what you are trying to do here. There are
non-ARM people, who wouldn't have known about this stuff at all
until now.
- Mainline doesn't care about history of commits that are present in a
non-mainline repository, even if it is android. Yeah, you screw up a
bit here as people loose their credits for what they have worked on.
But it should really have been 1-3 patches ONLY for the governor.
And maybe a patchset of 5-7 patches, but it can be much lesser as
well.

Cc'ing Peter/Ingo for their inputs..

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