From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Len Brown <lenb@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
Linux PM <linux-pm@vger.kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Daniel Lezcano <daniel.lezcano@linaro.org>,
Doug Smythies <dsmythies@telus.net>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] cpuidle: Consolidate disabled state checks
Date: Mon, 18 Nov 2019 10:22:05 +0100 [thread overview]
Message-ID: <CAJZ5v0ifOQaOm-8n5gUgud0sCn-Y1KQWWhzhtzdm+exvMLgL7Q@mail.gmail.com> (raw)
In-Reply-To: <CAJvTdKn9wuoXkKecZxCJHPZAG7XK_BqAZT5=7k9Mi4zo4SBL0g@mail.gmail.com>
On Mon, Nov 18, 2019 at 5:46 AM Len Brown <lenb@kernel.org> wrote:
>
> On Mon, Nov 4, 2019 at 6:16 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > There are two reasons why CPU idle states may be disabled: either
> > because the driver has disabled them or because they have been
> > disabled by user space via sysfs.
> >
> > In the former case, the state's "disabled" flag is set once during
> > the initialization of the driver and it is never cleared later (it
> > is read-only effectively).
>
> for x86 (intel_idle and acpi_idle), no states with disabled=1 are registered
> with cpuidle. Instead, intel_idle (currently) skips them in the loop
> that registers states.
> (and acpi_idle never touches the disabled field)
>
> And so for x86, governors checking for drv->states[i].disabled is a NOP,
> and the condition described by CPUIDLE_STATE_DISABLED_BY_DRIVER
> does not (yet) exist.
OK
> Looking at the ARM code, it seems that cpuidle-imx6q.c and cpuidle-tegra20.c
> reach into the cpuidle states at run time and toggle the
> drv->states[i].disabled.
I might have overlooked that, let me check.
> It seems that this patch takes the initial value of
> drv->states->disabled, and sets the (per cpu)
> usage.disable=..BY_DRIVER,
> but that subsequent run-time toggles in drv->states[i]disabled by
> these drivers would be missed,
> because you're removed the run-time checking of drv->states->disabled?
If it is updated at run time, then yes, the updates will be missed, so
thanks for pointing that out.
> Finally, I'd like to change intel_idle so that it *can* register a
> state that is disabled, by default.
> If I change the driver to NOT skip registering disabled states, and
> the cpuidle copy has cpuidle_state.disabled=1,
> then the state is indeed, unused at run-time. But as you said,
> it is effectively read-only, and is not indicated in sysfs, and can
> not be changed via sysfs.
>
> One way to do this is to do what you do here and initialize
> usage.disabled to drv->state.disabled. (not distinguishing between
> DRIVER and USER)
> That way the user could later over-ride what a driver set, by clearing
> the disabled attribute.
>
> However, the ARM drivers, at least, seem to want to reserve the right
> to set and clear the drv->state.disabled,
> and to have them continue to have that right, we have to continue
> checking that field at run-time.
Alternatively, the drivers in question can be changed to update the
disable field in state_usage instead (maybe under a lock to prevent
them from racing with user space).
> And giving drivers the opportunity to do that disabling driver-wide,
> instead of per-cpu (usage) wide,
> seems to be something we may want to keep.
So it looks like you want me to revert this patch which is something
that I really don't want to do, because of the extra checks all over
the place which are simply pointless in the majority of cases.
Cheers,
Rafael
next prev parent reply other threads:[~2019-11-18 9:22 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-04 11:16 [PATCH] cpuidle: Consolidate disabled state checks Rafael J. Wysocki
2019-11-04 11:52 ` Peter Zijlstra
2019-11-18 4:45 ` Len Brown
2019-11-18 9:22 ` Rafael J. Wysocki [this message]
2019-11-18 11:26 ` Rafael J. Wysocki
2019-11-18 23:04 ` [RFC][PATCH 0/2] cpuidle: Allow states to be disabled by default (was: Re: [PATCH] cpuidle: Consolidate disabled state checks) Rafael J. Wysocki
2019-11-18 23:07 ` [RFC][PATCH 1/2] cpuidle: Drop disabled field from struct cpuidle_state Rafael J. Wysocki
2019-11-18 23:09 ` [RFC][PATCH 2/2] cpuidle: Allow idle states to be disabled by default Rafael J. Wysocki
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=CAJZ5v0ifOQaOm-8n5gUgud0sCn-Y1KQWWhzhtzdm+exvMLgL7Q@mail.gmail.com \
--to=rafael@kernel.org \
--cc=daniel.lezcano@linaro.org \
--cc=dsmythies@telus.net \
--cc=lenb@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=peterz@infradead.org \
--cc=rjw@rjwysocki.net \
/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).