From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Alex Shi <alex.shi@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
vincent.guittot@linaro.org, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org,
Ulf Hansson <ulf.hansson@linaro.org>,
Rasmus Villemoes <linux@rasmusvillemoes.dk>,
Arjan van de Ven <arjan@linux.intel.com>,
Rik van Riel <riel@redhat.com>
Subject: Re: [PATCH 3/3] cpuidle/menu: add per cpu pm_qos_resume_latency consideration
Date: Thu, 19 Jan 2017 11:21:58 +0100 [thread overview]
Message-ID: <20170119102158.GA1827@mai> (raw)
In-Reply-To: <01f9b016-0b7c-44ac-70e5-8cd9b8bd1500@linaro.org>
On Thu, Jan 19, 2017 at 05:25:37PM +0800, Alex Shi wrote:
>
> > That said, I have the feeling that is taking the wrong direction. Each time we
> > are entering idle, we check the latencies. Entering idle can be done thousand
> > of times per second. Wouldn't make sense to disable the states not fulfilling
> > the constraints at the moment the latencies are changed ? As the idle states
> > have increasing exit latencies, setting an idle state limit to disable all
> > states after that limit may be more efficient than checking again and again in
> > the idle path, no ?
>
> You'r right. save some checking is good thing to do.
Hi Alex,
I think you missed the point.
What I am proposing is to change the current approach by disabling all the
states after a specific latency.
We add a specific internal function:
static int cpuidle_set_latency(struct cpuidle_driver *drv,
struct cpuidle_device *dev,
int latency)
{
int i, idx;
for (i = 0, idx = 0; i < drv->state_count; i++) {
struct cpuidle_state *s = &drv->states[i];
if (s->latency > latency)
break;
idx = i;
}
dev->state_count = idx;
return 0;
}
This function is called from the notifier callback:
static int cpuidle_latency_notify(struct notifier_block *b,
unsigned long l, void *v)
{
- wake_up_all_idle_cpus();
+ struct cpuidle_device *dev;
+ struct cpuidle_driver *drv;
+
+ cpuidle_pause_and_lock();
+ for_each_possible_cpu(cpu) {
+ dev = &per_cpu(cpuidle_dev, cpu);
+ drv = = cpuidle_get_cpu_driver(dev);
+ cpuidle_set_latency(drv, dev, l)
+ }
+ cpuidle_resume_and_unlock();
+
return NOTIFY_OK;
}
-----------------------------------------------------------------------------
The menu governor becomes:
diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index bba3c2af..87e58e3 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -352,7 +352,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
* Find the idle state with the lowest power while satisfying
* our constraints.
*/
- for (i = data->last_state_idx + 1; i < drv->state_count; i++) {
+ for (i = data->last_state_idx + 1; i < dev->state_count; i++) {
struct cpuidle_state *s = &drv->states[i];
struct cpuidle_state_usage *su = &dev->states_usage[i];
... with a cleanup around latency_req.
-----------------------------------------------------------------------------
And the cpuidle_device structure is changed to:
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index b923c32..2fc966cb 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -88,6 +88,7 @@ struct cpuidle_device {
cpumask_t coupled_cpus;
struct cpuidle_coupled *coupled;
#endif
+ int state_count;
};
DECLARE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
At init time, the drv->state_count and all cpu's dev->state_count are the same.
Well, that is the rough idea: instead of reading the latency when entering
idle, let's disable/enable the idle states when we set a new latency.
I did not check how that fits with the per cpu latency, but I think it will be
cleaner to change the approach rather than spreading latencies dances around.
next prev parent reply other threads:[~2017-01-19 10:23 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-12 13:27 [PATCH v2 0/3] per cpu resume latency Alex Shi
2017-01-12 13:27 ` [PATCH 1/3] cpuidle/menu: stop seeking deeper idle if current state is too deep Alex Shi
2017-01-12 13:27 ` [PATCH v2 2/3] cpu: expose pm_qos_resume_latency for each cpu Alex Shi
2017-01-17 10:23 ` Daniel Lezcano
2017-01-19 8:18 ` Alex Shi
2017-01-12 13:27 ` [PATCH 3/3] cpuidle/menu: add per cpu pm_qos_resume_latency consideration Alex Shi
2017-01-12 20:03 ` Rik van Riel
2017-01-16 1:11 ` Alex Shi
2017-01-17 9:38 ` Daniel Lezcano
2017-01-19 9:25 ` Alex Shi
2017-01-19 10:21 ` Daniel Lezcano [this message]
2017-01-19 21:43 ` Rafael J. Wysocki
2017-01-20 8:35 ` Alex Shi
2017-01-20 10:54 ` Daniel Lezcano
2017-01-22 1:31 ` Alex Shi
2017-01-23 12:50 ` Daniel Lezcano
2017-01-23 14:58 ` Alex Shi
[not found] <1483630187-29622-1-git-send-email-alex.shi@linaro.org>
2017-01-05 15:29 ` Alex Shi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170119102158.GA1827@mai \
--to=daniel.lezcano@linaro.org \
--cc=alex.shi@linaro.org \
--cc=arjan@linux.intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux@rasmusvillemoes.dk \
--cc=rafael.j.wysocki@intel.com \
--cc=riel@redhat.com \
--cc=ulf.hansson@linaro.org \
--cc=vincent.guittot@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).