Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1409242

Re: [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to driver frequency

Show all headers | View raw

I clearly missed the !policy->fast_switch_enabled check in sugov_limit() and so
the confusion.

On 30-05-16, 16:25, Rafael J. Wysocki wrote:
> On Mon, May 30, 2016 at 12:18 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > Suppose this is the current range of frequencies supported by a
> > driver: 200, 400, 600, 800, 1000 (in MHz).
> >
> > And policy->cur = next_freq = 400 MHz.
> >
> > A.) Suppose that we change policy->min to 400 MHz from userspace.
> >     -> sugov_limits()
> >        This will find everything in order and simply set
> >        need_freq_update, without updating the frequency.
> >
> >     On next util-callback, we will forcefully return true from
> >     sugov_should_update_freq() and reach sugov_update_commit().
> >
> >     We calculate next_freq and that comes to 400 MHz again (that's the
> >     case we are trying to target with the above code).
> >
> >     With the current code, we will forcefully end up calling
> >     cpufreq_driver_fast_switch().
> >
> >     Because the new and current frequencies are same,
> >     cpufreq_driver->fast_switch() will simply return.
> >
> >     NOTE: I also think that cpufreq_driver_fast_switch() should have a
> >     check like (policy->cur == target_freq). I will add that too, in
> >     case you agree.
> >
> >     So, forcefully updating next_freq to UINT_MAX will end up wasting
> >     some cycles, but wouldn't do any useful stuff.
> 
> It will, but there's no way to distinguish this case from B in the
> governor with the current min/max synchronization mechanism.  That is,
> it only knows that something has changed, but checking what exactly
> has changed would be racy.
> 
> > B.) Suppose that we change policy->min to 600 MHz from userspace.
> >     -> sugov_limits()
> >        This will find that policy->cur is less than 600 and will set
> >        that to 600 MHz by calling __cpufreq_driver_target(). We will
> >        also set need_freq_update.
> >
> >        Note that next_freq and policy->cur are not in sync anymore and
> >        perhaps this is the most important case for the above code.
> 
> It is.
> 
> Moreover, please note that __cpufreq_driver_target() is only called in
> sugov_limits() when policy->fast_switch_enabled is unset.

Yep, I missed it.

I am not sure how harmful it can be, but we are returning from sugov_limits()
without making sure that policy->cur is in valid range currently. I also know
that you left it out because of the possible races with the util handler.

But this is something that is fundamentally broken for now. The user writes
updates the policy->max/min, we return the call to the user thinks that it has
successfully written to the file and everything is aligned. But we may be
running at an frequency from invalid range. Yes, that will happen very soon, but
its broken.

-- 
viresh

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v2 0/3] cpufreq: avoid redundant driver calls in schedutil Steve Muckle <steve.muckle@linaro.org> - 2016-05-26 05:00 +0200
  [PATCH v2 1/3] cpufreq: add resolve_freq driver callback Steve Muckle <steve.muckle@linaro.org> - 2016-05-26 05:00 +0200
    Re: [PATCH v2 1/3] cpufreq: add resolve_freq driver callback Viresh Kumar <viresh.kumar@linaro.org> - 2016-05-26 08:30 +0200
      Re: [PATCH v2 1/3] cpufreq: add resolve_freq driver callback Steve Muckle <steve.muckle@linaro.org> - 2016-05-30 17:40 +0200
        Re: [PATCH v2 1/3] cpufreq: add resolve_freq driver callback Viresh Kumar <viresh.kumar@linaro.org> - 2016-05-31 07:40 +0200
  [PATCH v2 2/3] cpufreq: acpi-cpufreq: add resolve_freq callback Steve Muckle <steve.muckle@linaro.org> - 2016-05-26 05:00 +0200
    Re: [PATCH v2 2/3] cpufreq: acpi-cpufreq: add resolve_freq callback Viresh Kumar <viresh.kumar@linaro.org> - 2016-05-26 08:50 +0200
      Re: [PATCH v2 2/3] cpufreq: acpi-cpufreq: add resolve_freq callback Steve Muckle <steve.muckle@linaro.org> - 2016-05-30 18:30 +0200
  [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to driver frequency Steve Muckle <steve.muckle@linaro.org> - 2016-05-26 05:00 +0200
    Re: [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to  driver frequency Viresh Kumar <viresh.kumar@linaro.org> - 2016-05-26 09:20 +0200
      Re: [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to  driver frequency "Rafael J. Wysocki" <rafael@kernel.org> - 2016-05-29 02:50 +0200
        Re: [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to  driver frequency Viresh Kumar <viresh.kumar@linaro.org> - 2016-05-30 12:20 +0200
          Re: [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to  driver frequency "Rafael J. Wysocki" <rafael@kernel.org> - 2016-05-30 16:30 +0200
            Re: [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to  driver frequency Viresh Kumar <viresh.kumar@linaro.org> - 2016-05-30 17:40 +0200
              Re: [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to  driver frequency "Rafael J. Wysocki" <rafael@kernel.org> - 2016-05-30 21:10 +0200
            Re: [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to  driver frequency Wanpeng Li <kernellwp@gmail.com> - 2016-05-31 03:50 +0200
      Re: [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to  driver frequency Steve Muckle <steve.muckle@linaro.org> - 2016-05-30 18:40 +0200
    Re: [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to  driver frequency Wanpeng Li <kernellwp@gmail.com> - 2016-05-27 07:50 +0200
      Re: [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to  driver frequency Steve Muckle <steve.muckle@linaro.org> - 2016-05-30 18:50 +0200

csiph-web