From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755334AbbJ1DAt (ORCPT ); Tue, 27 Oct 2015 23:00:49 -0400 Received: from mail-pa0-f46.google.com ([209.85.220.46]:33554 "EHLO mail-pa0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754858AbbJ1DAr (ORCPT ); Tue, 27 Oct 2015 23:00:47 -0400 Date: Wed, 28 Oct 2015 08:30:42 +0530 From: Viresh Kumar To: "Rafael J. Wysocki" Cc: =?iso-8859-1?Q?B=E1lint?= Czobor , linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, Mike Chan , Todd Poynor , Peter Zijlstra , Ingo Molnar Subject: Re: [PATCH 01/70] cpufreq: interactive: New 'interactive' governor Message-ID: <20151028030042.GD13324@ubuntu> References: <1445967059-6897-1-git-send-email-czoborbalint@gmail.com> <4534884.dsz2GVUDrG@vostro.rjw.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4534884.dsz2GVUDrG@vostro.rjw.lan> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > > > 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 > > Signed-off-by: Todd Poynor > > Bug: 3152864 > > Signed-off-by: Bálint Czobor > > 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