Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Rafael Wysocki <rjw@rjwysocki.net>,
	Linux PM <linux-pm@vger.kernel.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V2 2/5] cpufreq: Replace few CPUFREQ_CONST_LOOPS checks with has_target()
Date: Thu, 27 Jun 2019 11:52:04 +0200
Message-ID: <CAJZ5v0h+1eZGsxgJ5G5OfJiWcyd2GEqxbiU6v4yy9uMvzoAOvw@mail.gmail.com> (raw)
In-Reply-To: <20190627050048.b44kitdfuenxnzfi@vireshk-i7>

On Thu, Jun 27, 2019 at 7:00 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 20-06-19, 08:35, Viresh Kumar wrote:
> > > CPUFREQ_CONST_LOOPS was introduced in a very old commit from pre-2.6
> > > kernel release commit 6a4a93f9c0d5 ("[CPUFREQ] Fix 'out of sync'
> > > issue").
> > >
> > > Probably the initial idea was to just avoid these checks for set_policy
> > > type drivers and then things got changed over the years. And it is very
> > > unclear why these checks are there at all.
> > >
> > > Replace the CPUFREQ_CONST_LOOPS check with has_target(), which makes
> > > more sense now.
> > >
> > > cpufreq_notify_transition() is only called for has_target() type driver
> > > and not for set_policy type, and the check is simply redundant. Remove
> > > it as well.
> > >
> > > Also remove () around freq comparison statement as they aren't required
> > > and checkpatch also warns for them.
> > >
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > ---
> > >  drivers/cpufreq/cpufreq.c | 13 +++++--------
> > >  1 file changed, 5 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index 54befd775bd6..41ac701e324f 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -359,12 +359,10 @@ static void cpufreq_notify_transition(struct cpufreq_policy *policy,
> > >              * which is not equal to what the cpufreq core thinks is
> > >              * "old frequency".
> > >              */
> > > -           if (!(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
> > > -                   if (policy->cur && (policy->cur != freqs->old)) {
> > > -                           pr_debug("Warning: CPU frequency is %u, cpufreq assumed %u kHz\n",
> > > -                                    freqs->old, policy->cur);
> > > -                           freqs->old = policy->cur;
> > > -                   }
> > > +           if (policy->cur && policy->cur != freqs->old) {
> > > +                   pr_debug("Warning: CPU frequency is %u, cpufreq assumed %u kHz\n",
> > > +                            freqs->old, policy->cur);
> > > +                   freqs->old = policy->cur;
> > >             }
> > >
> > >             srcu_notifier_call_chain(&cpufreq_transition_notifier_list,
> > > @@ -1618,8 +1616,7 @@ static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
> > >     if (policy->fast_switch_enabled)
> > >             return ret_freq;
> > >
> > > -   if (ret_freq && policy->cur &&
> > > -           !(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
> > > +   if (has_target() && ret_freq && policy->cur) {
> > >             /* verify no discrepancy between actual and
> > >                                     saved value exists */
> > >             if (unlikely(ret_freq != policy->cur)) {
>
> @Rafael: Here are your comments from the IRC exchange we had
> yesterday:
>
> > <rafael>:
> >
> > so the problem is that, because of the CPUFREQ_CONST_LOOPS check in
> > __cpufreq_get(), it almost never does the cpufreq_out_of_sync() thing
> > now. Because many drivers set CPUFREQ_CONST_LOOPS most of the time,
> > some of them even unconditionally. This patch changes the code that
> > runs very rarely into code that runs relatively often.
>
> Right, we will do the frequency verification on has_target() platforms
> with CPUFREQ_CONST_LOOPS set after this patch. But why is it the wrong
> thing to do ?

Well, my point was exactly what I said.

The patch pretended to be a cleanup and changed the code in a
meaningful way (at least for some drivers).

> What we do here is that we verify that the cached value of current
> frequency is same as the real frequency the hardware is running at. It
> makes sense to not do this check for setpolicy type drivers as the
> cpufreq core isn't always aware of what the driver will end up doing
> with the frequency and so no verification.
>
> But for has_target() type drivers, cpufreq core caches the value with
> it and it should check it to make sure everything is fine. I don't see
> a correlation with CPUFREQ_CONST_LOOPS flag here, that's it. Either we
> do this verification or we don't, but there is no reason (as per my
> understanding) of skipping it using this flag.
>
> So if you look at the commit I pointed in the history git [1], it does
> two things:
> - It adds the verification code (which is quite similar today as
>   well).
> - And it sets the CPUFREQ_CONST_LOOPS flag only for setpolicy drivers,
>   rightly so.
>
> The problem happened when we started to use CPUFREQ_CONST_LOOPS for
> constant loops-per-jiffy thing as well and many has_target() drivers
> started using the same flag and unknowingly skipped the verification
> of frequency.
>
> So, I think the current code is doing the wrong thing by skipping the
> verification using CPUFREQ_CONST_LOOPS flag.

All right then, thanks for explaining it here.

The patch is a bug fix, not a cleanup, and it fixes the changes that
caused CPUFREQ_CONST_LOOPS to be used for a different purpose without
adjusting the original code accordingly.

I can agree with this rationale, but please fix the changelog.

  reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-20  3:05 [PATCH V2 0/5] cpufreq: cleanups Viresh Kumar
2019-06-20  3:05 ` [PATCH V2 1/5] cpufreq: Remove the redundant !setpolicy check Viresh Kumar
2019-06-27 21:52   ` Rafael J. Wysocki
2019-06-20  3:05 ` [PATCH V2 2/5] cpufreq: Replace few CPUFREQ_CONST_LOOPS checks with has_target() Viresh Kumar
2019-06-27  5:00   ` Viresh Kumar
2019-06-27  9:52     ` Rafael J. Wysocki [this message]
2019-06-28  5:16   ` [PATCH V3 2/5] cpufreq: Don't skip frequency validation for has_target() drivers Viresh Kumar
2019-06-20  3:05 ` [PATCH V2 3/5] cpufreq: Use has_target() instead of !setpolicy Viresh Kumar
2019-06-27 21:52   ` Rafael J. Wysocki
2019-06-20  3:05 ` [PATCH V2 4/5] cpufreq: Reuse cpufreq_update_current_freq() in __cpufreq_get() Viresh Kumar
2019-06-20  3:05 ` [PATCH V2 5/5] cpufreq: Avoid calling cpufreq_verify_current_freq() from handle_update() Viresh Kumar

Reply instructions:

You may reply publically 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=CAJZ5v0h+1eZGsxgJ5G5OfJiWcyd2GEqxbiU6v4yy9uMvzoAOvw@mail.gmail.com \
    --to=rafael@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=rjw@rjwysocki.net \
    --cc=vincent.guittot@linaro.org \
    --cc=viresh.kumar@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

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org linux-pm@archiver.kernel.org
	public-inbox-index linux-pm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox