From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Doug Smythies <dsmythies@telus.net>
Cc: Giovanni Gherdovich <ggherdovich@suse.cz>,
Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
Peter Zijlstra <peterz@infradead.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Frederic Weisbecker <frederic@kernel.org>,
Mel Gorman <mgorman@suse.de>,
Daniel Lezcano <daniel.lezcano@linaro.org>,
Linux PM <linux-pm@vger.kernel.org>
Subject: Re: [RFC/RFT][PATCH v6] cpuidle: New timer events oriented governor for tickless systems
Date: Tue, 04 Dec 2018 00:47:31 +0100 [thread overview]
Message-ID: <2107394.tME3olEdyg@aspire.rjw.lan> (raw)
In-Reply-To: <CAJZ5v0j6AEjztoFfpQAc3NgQ5JtsWgRqscoE_u1eespVM-Cnmw@mail.gmail.com>
On Friday, November 30, 2018 9:51:19 AM CET Rafael J. Wysocki wrote:
> Hi Doug,
>
> On Fri, Nov 30, 2018 at 8:49 AM Doug Smythies <dsmythies@telus.net> wrote:
> >
> > Hi Rafael,
> >
> > On 2018.11.23 02:36 Rafael J. Wysocki wrote:
> >
> > ... [snip]...
> >
> > > +/**
> > > + * teo_find_shallower_state - Find shallower idle state matching given duration.
> > > + * @drv: cpuidle driver containing state data.
> > > + * @dev: Target CPU.
> > > + * @state_idx: Index of the capping idle state.
> > > + * @duration_us: Idle duration value to match.
> > > + */
> > > +static int teo_find_shallower_state(struct cpuidle_driver *drv,
> > > + struct cpuidle_device *dev, int state_idx,
> > > + unsigned int duration_us)
> > > +{
> > > + int i;
> > > +
> > > + for (i = state_idx - 1; i > 0; i--) {
> > > + if (drv->states[i].disabled || dev->states_usage[i].disable)
> > > + continue;
> > > +
> > > + if (drv->states[i].target_residency <= duration_us)
> > > + break;
> > > + }
> > > + return i;
> > > +}
> >
> > I think this subroutine has a problem when idle state 0
> > is disabled.
>
> You are right, thanks!
>
> > Perhaps something like this might help:
> >
> > diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
> > index bc1c9a2..5b97639 100644
> > --- a/drivers/cpuidle/governors/teo.c
> > +++ b/drivers/cpuidle/governors/teo.c
> > @@ -196,7 +196,8 @@ static void teo_update(struct cpuidle_driver *drv, struct cpuidle_device *dev)
> > }
> >
> > /**
> > - * teo_find_shallower_state - Find shallower idle state matching given duration.
> > + * teo_find_shallower_state - Find shallower idle state matching given
> > + * duration, if possible.
> > * @drv: cpuidle driver containing state data.
> > * @dev: Target CPU.
> > * @state_idx: Index of the capping idle state.
> > @@ -208,13 +209,15 @@ static int teo_find_shallower_state(struct cpuidle_driver *drv,
> > {
> > int i;
> >
> > - for (i = state_idx - 1; i > 0; i--) {
> > + for (i = state_idx - 1; i >= 0; i--) {
> > if (drv->states[i].disabled || dev->states_usage[i].disable)
> > continue;
> >
> > if (drv->states[i].target_residency <= duration_us)
> > break;
> > }
> > + if (i < 0)
> > + i = state_idx;
> > return i;
> > }
>
> I'll do something slightly similar, but equivalent.
I actually ended up fixing it differently, as the above will cause state_idx
to be returned even if some states shallower than state_idx are enabled, but
their target residencies are higher than duration_us. In that case, though,
it still is more correct to return the shallowest enabled state rather than
state_idx.
> >
> > @@ -264,7 +267,6 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> > if (max_early_idx >= 0 &&
> > count < cpu_data->states[i].early_hits)
> > count = cpu_data->states[i].early_hits;
> > -
> > continue;
> > }
> >
> > There is an additional issue where if idle state 0 is disabled (with the above suggested code patch),
> > idle state usage seems to fall to deeper states than idle state 1.
> > This is not the expected behaviour.
>
> No, it isn't.
>
> > Kernel 4.20-rc3 works as expected.
> > I have not figured this issue out yet, in the code.
> >
> > Example (1 minute per sample. Number of entries/exits per state):
> > State 0 State 1 State 2 State 3 State 4 Watts
> > 28235143, 83, 26, 17, 837, 64.900
> > 5583238, 657079, 5884941, 8498552, 30986831, 62.433 << Transition sample, after idle state 0 disabled
> > 0, 793517, 7186099, 10559878, 38485721, 61.900 << ?? should have all gone into Idle state 1
> > 0, 795414, 7340703, 10553117, 38513456, 62.050
> > 0, 807028, 7288195, 10574113, 38523524, 62.167
> > 0, 814983, 7403534, 10575108, 38571228, 62.167
> > 0, 838302, 7747127, 10552289, 38556054, 62.183
> > 9664999, 544473, 4914512, 6942037, 25295361, 63.633 << Transition sample, after idle state 0 enabled
> > 27893504, 96, 40, 9, 912, 66.500
> > 26556343, 83, 29, 7, 814, 66.683
> > 27929227, 64, 20, 10, 931, 66.683
>
> I see.
>
> OK, I'll look into this too, thanks!
This probably is the artifact of the fix for the teo_find_shallower_state()
issue.
Anyway, I'm not able to reproduce this with the teo_find_shallower_state() issue
fixed differently.
Thanks,
Rafael
next prev parent reply other threads:[~2018-12-03 23:47 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-30 7:48 [RFC/RFT][PATCH v6] cpuidle: New timer events oriented governor for tickless systems Doug Smythies
2018-11-30 8:51 ` Rafael J. Wysocki
2018-12-03 23:47 ` Rafael J. Wysocki [this message]
2018-12-05 23:06 ` Doug Smythies
2018-12-06 9:11 ` Rafael J. Wysocki
-- strict thread matches above, loose matches on Subject: below --
2018-11-28 23:20 Doug Smythies
2018-11-29 9:42 ` Rafael J. Wysocki
2018-12-03 23:52 ` Rafael J. Wysocki
2018-11-23 10:35 Rafael J. Wysocki
2018-11-23 10:40 ` Rafael J. Wysocki
2018-12-01 14:18 ` Giovanni Gherdovich
2018-12-03 23:37 ` Rafael J. Wysocki
2018-12-03 16:23 ` Doug Smythies
2018-12-07 13:34 ` Mel Gorman
2018-12-08 10:23 ` Giovanni Gherdovich
2018-12-08 16:24 ` Doug Smythies
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=2107394.tME3olEdyg@aspire.rjw.lan \
--to=rjw@rjwysocki.net \
--cc=daniel.lezcano@linaro.org \
--cc=dsmythies@telus.net \
--cc=frederic@kernel.org \
--cc=ggherdovich@suse.cz \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=mgorman@suse.de \
--cc=peterz@infradead.org \
--cc=srinivas.pandruvada@linux.intel.com \
/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).