From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Cc: peterz@infradead.org, tglx@linutronix.de,
rafael.j.wysocki@intel.com, daniel.lezcano@linaro.org,
rlippert@google.com, linux-pm@vger.kernel.org,
linus.walleij@linaro.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, sudeep.holla@arm.com,
linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH V3] cpuidle: Handle tick_broadcast_enter() failure gracefully
Date: Sat, 09 May 2015 20:48:04 +0200 [thread overview]
Message-ID: <3489917.ZU0EhIItEx@vostro.rjw.lan> (raw)
In-Reply-To: <4263013.jBGApPfu4h@vostro.rjw.lan>
On Saturday, May 09, 2015 08:46:20 PM Rafael J. Wysocki wrote:
> On Saturday, May 09, 2015 11:19:16 AM Preeti U Murthy wrote:
> > Hi Rafael,
> >
> > On 05/08/2015 07:48 PM, Rafael J. Wysocki wrote:
> > >> +/*
> > >> + * find_tick_valid_state - select a state where tick does not stop
> > >> + * @dev: cpuidle device for this cpu
> > >> + * @drv: cpuidle driver for this cpu
> > >> + */
> > >> +static int find_tick_valid_state(struct cpuidle_device *dev,
> > >> + struct cpuidle_driver *drv)
> > >> +{
> > >> + int i, ret = -1;
> > >> +
> > >> + for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
> > >> + struct cpuidle_state *s = &drv->states[i];
> > >> + struct cpuidle_state_usage *su = &dev->states_usage[i];
> > >> +
> > >> + /*
> > >> + * We do not explicitly check for latency requirement
> > >> + * since it is safe to assume that only shallower idle
> > >> + * states will have the CPUIDLE_FLAG_TIMER_STOP bit
> > >> + * cleared and they will invariably meet the latency
> > >> + * requirement.
> > >> + */
> > >> + if (s->disabled || su->disable ||
> > >> + (s->flags & CPUIDLE_FLAG_TIMER_STOP))
> > >> + continue;
> > >> +
> > >> + ret = i;
> > >> + }
> > >> + return ret;
> > >> +}
> > >> +
> > >> /**
> > >> * cpuidle_enter_state - enter the state and update stats
> > >> * @dev: cpuidle device for this cpu
> > >> @@ -168,10 +199,17 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
> > >> * CPU as a broadcast timer, this call may fail if it is not available.
> > >> */
> > >> if (broadcast && tick_broadcast_enter()) {
> > >> - default_idle_call();
> > >> - return -EBUSY;
> > >> + index = find_tick_valid_state(dev, drv);
> > >
> > > Well, the new state needs to be deeper than the old one or you may violate the
> > > governor's choice and this doesn't guarantee that.
> >
> > The comment above in find_tick_valid_state() explains why we are bound
> > to choose a shallow idle state. I think its safe to assume that any
> > state deeper than this one, would have the CPUIDLE_FLAG_TIMER_STOP flag
> > set and hence would be skipped.
> >
> > Your patch relies on the assumption that the idle states are arranged in
> > the increasing order of exit_latency/in the order of shallow to deep.
> > This is not guaranteed, is it?
>
> No, it isn't, which is a good point. There's no reason to rely on that
> assumption, so appended is an updated version of the patch using a latency
> limit instead of an index limit.
And the patch *is* actually appended this time, sorry.
---
drivers/cpuidle/cpuidle.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -73,7 +73,10 @@ int cpuidle_play_dead(void)
}
static int find_deepest_state(struct cpuidle_driver *drv,
- struct cpuidle_device *dev, bool freeze)
+ struct cpuidle_device *dev,
+ unsigned int max_latency,
+ unsigned int forbidden_flags,
+ bool freeze)
{
unsigned int latency_req = 0;
int i, ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
@@ -83,6 +86,8 @@ static int find_deepest_state(struct cpu
struct cpuidle_state_usage *su = &dev->states_usage[i];
if (s->disabled || su->disable || s->exit_latency <= latency_req
+ || s->exit_latency > max_latency
+ || (s->flags & forbidden_flags)
|| (freeze && !s->enter_freeze))
continue;
@@ -100,7 +105,7 @@ static int find_deepest_state(struct cpu
int cpuidle_find_deepest_state(struct cpuidle_driver *drv,
struct cpuidle_device *dev)
{
- return find_deepest_state(drv, dev, false);
+ return find_deepest_state(drv, dev, UINT_MAX, 0, false);
}
static void enter_freeze_proper(struct cpuidle_driver *drv,
@@ -139,7 +144,7 @@ int cpuidle_enter_freeze(struct cpuidle_
* that interrupts won't be enabled when it exits and allows the tick to
* be frozen safely.
*/
- index = find_deepest_state(drv, dev, true);
+ index = find_deepest_state(drv, dev, UINT_MAX, 0, true);
if (index >= 0)
enter_freeze_proper(drv, dev, index);
@@ -168,8 +173,13 @@ int cpuidle_enter_state(struct cpuidle_d
* CPU as a broadcast timer, this call may fail if it is not available.
*/
if (broadcast && tick_broadcast_enter()) {
- default_idle_call();
- return -EBUSY;
+ index = find_deepest_state(drv, dev, target_state->exit_latency,
+ CPUIDLE_FLAG_TIMER_STOP, false);
+ if (index < 0) {
+ default_idle_call();
+ return -EBUSY;
+ }
+ target_state = &drv->states[index];
}
trace_cpu_idle_rcuidle(index, dev->cpu);
next prev parent reply other threads:[~2015-05-09 18:23 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-08 7:35 [PATCH V3] cpuidle: Handle tick_broadcast_enter() failure gracefully Preeti U Murthy
2015-05-08 12:43 ` Sudeep Holla
2015-05-08 14:18 ` Rafael J. Wysocki
2015-05-08 21:51 ` Rafael J. Wysocki
2015-05-09 5:49 ` Preeti U Murthy
2015-05-09 18:46 ` Rafael J. Wysocki
2015-05-09 18:48 ` Rafael J. Wysocki [this message]
2015-05-09 20:11 ` Rafael J. Wysocki
2015-05-09 20:33 ` Rafael J. Wysocki
2015-05-09 23:15 ` [PATCH 0/3] cpuidle: updates related to tick_broadcast_enter() failures Rafael J. Wysocki
2015-05-09 23:18 ` [PATCH 1/3] sched / idle: Call idle_set_state() from cpuidle_enter_state() Rafael J. Wysocki
2015-05-09 23:18 ` [PATCH 2/3] sched / idle: Call default_idle_call() " Rafael J. Wysocki
2015-05-09 23:19 ` [PATCH 3/3] cpuidle: Select a different state on tick_broadcast_enter() failures Rafael J. Wysocki
2015-05-11 3:48 ` [PATCH 0/3] cpuidle: updates related to " Preeti U Murthy
2015-05-11 5:21 ` Preeti U Murthy
2015-05-11 23:13 ` Rafael J. Wysocki
2015-05-11 15:13 ` Sudeep Holla
2015-05-11 23:14 ` Rafael J. Wysocki
2015-05-11 17:40 ` Daniel Lezcano
2015-05-11 23:31 ` Rafael J. Wysocki
2015-05-12 8:41 ` Daniel Lezcano
2015-05-12 13:23 ` Rafael J. Wysocki
2015-05-12 18:04 ` Daniel Lezcano
2015-05-13 22:59 ` Kevin Hilman
2015-05-14 0:16 ` Rafael J. Wysocki
2015-05-14 0:13 ` Kevin Hilman
2015-05-14 0:42 ` Rafael J. Wysocki
2015-05-14 0:31 ` Kevin Hilman
2015-05-14 3:59 ` Preeti U Murthy
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=3489917.ZU0EhIItEx@vostro.rjw.lan \
--to=rjw@rjwysocki.net \
--cc=daniel.lezcano@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=preeti@linux.vnet.ibm.com \
--cc=rafael.j.wysocki@intel.com \
--cc=rlippert@google.com \
--cc=sudeep.holla@arm.com \
--cc=tglx@linutronix.de \
/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).