[PATCH V2] cpufreq: Disallow ->resolve_freq() for drivers providing ->target_index()

[PATCH V2] cpufreq: Disallow ->resolve_freq() for drivers providing ->target_index()

[Viresh Kumar]

The handlers provided by cpufreq core are sufficient for resolving the
frequency for drivers providing ->target_index(), as the core already
has the frequency table and so ->resolve_freq() isn't required for such
platforms.

This patch disallows drivers with ->target_index() callback to use the
->resolve_freq() callback.

Also, it fixes a potential kernel crash for drivers providing ->target()
but no ->resolve_freq().

Fixes: e3c062360870 ("cpufreq: add cpufreq_driver_resolve_freq()")
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2:
- s/UINT_MAX/target_freq

 drivers/cpufreq/cpufreq.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index b696baeb249d..3ef9be3965ff 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -507,12 +507,20 @@ unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
 {
 	target_freq = clamp_val(target_freq, policy->min, policy->max);
 	policy->cached_target_freq = target_freq;
+
+	if (cpufreq_driver->target_index) {
+		int idx;
+
+		idx = cpufreq_frequency_table_target(policy, target_freq,
+						     CPUFREQ_RELATION_L);
+		policy->cached_resolved_idx = idx;
+		return policy->freq_table[idx].frequency;
+	}
+
 	if (cpufreq_driver->resolve_freq)
 		return cpufreq_driver->resolve_freq(policy, target_freq);
-	policy->cached_resolved_idx =
-		cpufreq_frequency_table_target(policy, target_freq,
-					       CPUFREQ_RELATION_L);
-	return policy->freq_table[policy->cached_resolved_idx].frequency;
+
+	return target_freq;
 }
 
 /*********************************************************************
-- 
2.7.4

Re: [PATCH V2] cpufreq: Disallow ->resolve_freq() for drivers providing ->target_index()

[Rafael J. Wysocki]

On Thursday, July 21, 2016 02:39:26 PM Viresh Kumar wrote:
> The handlers provided by cpufreq core are sufficient for resolving the
> frequency for drivers providing ->target_index(), as the core already
> has the frequency table and so ->resolve_freq() isn't required for such
> platforms.
> 
> This patch disallows drivers with ->target_index() callback to use the
> ->resolve_freq() callback.
> 
> Also, it fixes a potential kernel crash for drivers providing ->target()
> but no ->resolve_freq().
> 
> Fixes: e3c062360870 ("cpufreq: add cpufreq_driver_resolve_freq()")
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

OK, applied.

Thanks,
Rafael

Re: [PATCH V2] cpufreq: Disallow ->resolve_freq() for drivers providing ->target_index()

[Steve Muckle]

On Fri, Jul 22, 2016 at 01:22:22AM +0200, Rafael J. Wysocki wrote:
> OK, applied.

FWIW I do have a concern on this patch, I think it adds unnecessary
overhead.

Re: [PATCH V2] cpufreq: Disallow ->resolve_freq() for drivers providing ->target_index()

[Rafael J. Wysocki]

On Fri, Jul 22, 2016 at 1:22 AM, Steve Muckle <steve.muckle@linaro.org> wrote:
> On Fri, Jul 22, 2016 at 01:22:22AM +0200, Rafael J. Wysocki wrote:
>> OK, applied.
>
> FWIW I do have a concern on this patch, I think it adds unnecessary
> overhead.

It isn't unnecessary.  It prevents an otherwise possible kernel crash
from happening.

Re: [PATCH V2] cpufreq: Disallow ->resolve_freq() for drivers providing ->target_index()

[Steve Muckle]

On Fri, Jul 22, 2016 at 01:32:00AM +0200, Rafael J. Wysocki wrote:
> On Fri, Jul 22, 2016 at 1:22 AM, Steve Muckle <steve.muckle@linaro.org> wrote:
> > On Fri, Jul 22, 2016 at 01:22:22AM +0200, Rafael J. Wysocki wrote:
> >> OK, applied.
> >
> > FWIW I do have a concern on this patch, I think it adds unnecessary
> > overhead.
> 
> It isn't unnecessary.  It prevents an otherwise possible kernel crash
> from happening.

The logic may not be unecessary, but the overhead is. The crash could be
prevented in a way that doesn't require repeatedly checking a pointer
that doesn't change.

Re: [PATCH V2] cpufreq: Disallow ->resolve_freq() for drivers providing ->target_index()

[Rafael J. Wysocki]

On Fri, Jul 22, 2016 at 1:45 AM, Steve Muckle <steve.muckle@linaro.org> wrote:
> On Fri, Jul 22, 2016 at 01:32:00AM +0200, Rafael J. Wysocki wrote:
>> On Fri, Jul 22, 2016 at 1:22 AM, Steve Muckle <steve.muckle@linaro.org> wrote:
>> > On Fri, Jul 22, 2016 at 01:22:22AM +0200, Rafael J. Wysocki wrote:
>> >> OK, applied.
>> >
>> > FWIW I do have a concern on this patch, I think it adds unnecessary
>> > overhead.
>>
>> It isn't unnecessary.  It prevents an otherwise possible kernel crash
>> from happening.
>
> The logic may not be unecessary, but the overhead is. The crash could be
> prevented in a way that doesn't require repeatedly checking a pointer
> that doesn't change.

Well, you had the ->resolve_freq check in your patch, didn't you?

Viresh simply added a ->target_index check to it.

Now, you can argue that this is one check too many, but as long as
drivers are allowed to implement ->target without implementing
->resolve_freq, the *number* of checks in this routine cannot be
reduced.

There are three possible cases and two checks are required to
determine which case really takes place.

Re: [PATCH V2] cpufreq: Disallow ->resolve_freq() for drivers providing ->target_index()

[Steve Muckle]

On Fri, Jul 22, 2016 at 01:53:13AM +0200, Rafael J. Wysocki wrote:
> On Fri, Jul 22, 2016 at 1:45 AM, Steve Muckle <steve.muckle@linaro.org> wrote:
> > On Fri, Jul 22, 2016 at 01:32:00AM +0200, Rafael J. Wysocki wrote:
> >> On Fri, Jul 22, 2016 at 1:22 AM, Steve Muckle <steve.muckle@linaro.org> wrote:
> >> > On Fri, Jul 22, 2016 at 01:22:22AM +0200, Rafael J. Wysocki wrote:
> >> >> OK, applied.
> >> >
> >> > FWIW I do have a concern on this patch, I think it adds unnecessary
> >> > overhead.
> >>
> >> It isn't unnecessary.  It prevents an otherwise possible kernel crash
> >> from happening.
> >
> > The logic may not be unecessary, but the overhead is. The crash could be
> > prevented in a way that doesn't require repeatedly checking a pointer
> > that doesn't change.
> 
> Well, you had the ->resolve_freq check in your patch, didn't you?
> 
> Viresh simply added a ->target_index check to it.
> 
> Now, you can argue that this is one check too many, but as long as
> drivers are allowed to implement ->target without implementing
> ->resolve_freq, the *number* of checks in this routine cannot be
> reduced.
> 
> There are three possible cases and two checks are required to
> determine which case really takes place.

My thinking was that one of these two would be preferable:

- Forcing ->target() drivers to install a ->resolve_freq callback,
  enforcing this at cpufreq driver init time. My understanding is
  ->target() drivers are deprecated anyway and theren't aren't many of
  them, though I don't know offhand exactly how many or how hard it
  would be to do for each one.

- Forcing callers (schedutil in this case) to check that either
  ->target() or ->resolve_freq() is implemented. It means
  catching and scrutinizing future callers of resolve_freq.

But even if one of these is better than it could always be done on top
of this patch I suppose. I'm also not familiar with the platforms that use
->target() style drivers. So strictly speaking for my purposes it won't
matter since the number of tests is the same for them. 

Re: [PATCH V2] cpufreq: Disallow ->resolve_freq() for drivers providing ->target_index()

[Rafael J. Wysocki]

On Fri, Jul 22, 2016 at 2:09 AM, Steve Muckle <steve.muckle@linaro.org> wrote:
> On Fri, Jul 22, 2016 at 01:53:13AM +0200, Rafael J. Wysocki wrote:
>> On Fri, Jul 22, 2016 at 1:45 AM, Steve Muckle <steve.muckle@linaro.org> wrote:
>> > On Fri, Jul 22, 2016 at 01:32:00AM +0200, Rafael J. Wysocki wrote:
>> >> On Fri, Jul 22, 2016 at 1:22 AM, Steve Muckle <steve.muckle@linaro.org> wrote:
>> >> > On Fri, Jul 22, 2016 at 01:22:22AM +0200, Rafael J. Wysocki wrote:
>> >> >> OK, applied.
>> >> >
>> >> > FWIW I do have a concern on this patch, I think it adds unnecessary
>> >> > overhead.
>> >>
>> >> It isn't unnecessary.  It prevents an otherwise possible kernel crash
>> >> from happening.
>> >
>> > The logic may not be unecessary, but the overhead is. The crash could be
>> > prevented in a way that doesn't require repeatedly checking a pointer
>> > that doesn't change.
>>
>> Well, you had the ->resolve_freq check in your patch, didn't you?
>>
>> Viresh simply added a ->target_index check to it.
>>
>> Now, you can argue that this is one check too many, but as long as
>> drivers are allowed to implement ->target without implementing
>> ->resolve_freq, the *number* of checks in this routine cannot be
>> reduced.
>>
>> There are three possible cases and two checks are required to
>> determine which case really takes place.
>
> My thinking was that one of these two would be preferable:
>
> - Forcing ->target() drivers to install a ->resolve_freq callback,
>   enforcing this at cpufreq driver init time.

That would have been possible, but your series didn't do that.

>   My understanding is
>   ->target() drivers are deprecated anyway

No, they aren't.

There simply are cases in which frequency tables are not workable
(like the ACPI CPPC one).

>  and theren't aren't many of
>   them, though I don't know offhand exactly how many or how hard it
>   would be to do for each one.
>
> - Forcing callers (schedutil in this case) to check that either
>   ->target() or ->resolve_freq() is implemented. It means
>   catching and scrutinizing future callers of resolve_freq.

But that doesn't reduce the number of checks in cpufreq_driver_resolve_freq().

There still are three choices in there: return a frequency from the
table (if present), or call ->resolve_freq (if implemented), or return
target_freq (as the last resort).

> But even if one of these is better than it could always be done on top
> of this patch I suppose.

Right.

> I'm also not familiar with the platforms that use
> ->target() style drivers. So strictly speaking for my purposes it won't
> matter since the number of tests is the same for them.

OK

Re: [PATCH V2] cpufreq: Disallow ->resolve_freq() for drivers providing ->target_index()

[Steve Muckle]

On Fri, Jul 22, 2016 at 02:18:54AM +0200, Rafael J. Wysocki wrote:
> > My thinking was that one of these two would be preferable:
> >
> > - Forcing ->target() drivers to install a ->resolve_freq callback,
> >   enforcing this at cpufreq driver init time.
> 
> That would have been possible, but your series didn't do that.
> 
> >   My understanding is
> >   ->target() drivers are deprecated anyway
> 
> No, they aren't.

Ok. I didn't follow Documentation/cpu-freq/cpu-drivers.txt section 1.5
then - it suggests something about target() is deprecated, perhaps it's
out of date.

> There simply are cases in which frequency tables are not workable
> (like the ACPI CPPC one).

Sure that makes sense.

> >  and theren't aren't many of
> >   them, though I don't know offhand exactly how many or how hard it
> >   would be to do for each one.
> >
> > - Forcing callers (schedutil in this case) to check that either
> >   ->target() or ->resolve_freq() is implemented. It means
> >   catching and scrutinizing future callers of resolve_freq.
> 
> But that doesn't reduce the number of checks in cpufreq_driver_resolve_freq().
> 
> There still are three choices in there: return a frequency from the
> table (if present), or call ->resolve_freq (if implemented), or return
> target_freq (as the last resort).

Sorry, that should've been "check that either ->target_index() or
->resolve_freq() is implemented." 

Implementing resolve_freq for the target() drivers and requiring it at
driver init time is probably the better way to go though. Perhaps I can
work on this at some point.

Re: [PATCH V2] cpufreq: Disallow ->resolve_freq() for drivers providing ->target_index()

[Viresh Kumar]

On 21-07-16, 17:34, Steve Muckle wrote:
> On Fri, Jul 22, 2016 at 02:18:54AM +0200, Rafael J. Wysocki wrote:
> > > My thinking was that one of these two would be preferable:
> > >
> > > - Forcing ->target() drivers to install a ->resolve_freq callback,
> > >   enforcing this at cpufreq driver init time.
> > 
> > That would have been possible, but your series didn't do that.
> > 
> > >   My understanding is
> > >   ->target() drivers are deprecated anyway
> > 
> > No, they aren't.
> 
> Ok. I didn't follow Documentation/cpu-freq/cpu-drivers.txt section 1.5
> then - it suggests something about target() is deprecated, perhaps it's
> out of date.

They are kind of deprecated for the new uesrs, but we still have
handful of users of it.

> Sorry, that should've been "check that either ->target_index() or
> ->resolve_freq() is implemented." 
> 
> Implementing resolve_freq for the target() drivers and requiring it at
> driver init time is probably the better way to go though. Perhaps I can
> work on this at some point.

As I said earlier as well in one of the emails, if you are worried
about the extra 'if' check in the hot path, then wouldn't this fix it
for you?

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 3dd4884c6f9e..91d8ec4c8eb7 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -517,7 +517,7 @@ unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
                return policy->freq_table[idx].frequency;
        }
 
-       if (cpufreq_driver->resolve_freq)
+       if (likely(cpufreq_driver->resolve_freq))
                return cpufreq_driver->resolve_freq(policy, target_freq);
 
        return target_freq;

-- 
viresh

Re: [PATCH V2] cpufreq: Disallow ->resolve_freq() for drivers providing ->target_index()

[Rafael J. Wysocki]

On Friday, July 22, 2016 08:13:27 AM Viresh Kumar wrote:
> On 21-07-16, 17:34, Steve Muckle wrote:
> > On Fri, Jul 22, 2016 at 02:18:54AM +0200, Rafael J. Wysocki wrote:
> > > > My thinking was that one of these two would be preferable:
> > > >
> > > > - Forcing ->target() drivers to install a ->resolve_freq callback,
> > > >   enforcing this at cpufreq driver init time.
> > > 
> > > That would have been possible, but your series didn't do that.
> > > 
> > > >   My understanding is
> > > >   ->target() drivers are deprecated anyway
> > > 
> > > No, they aren't.
> > 
> > Ok. I didn't follow Documentation/cpu-freq/cpu-drivers.txt section 1.5
> > then - it suggests something about target() is deprecated, perhaps it's
> > out of date.
> 
> They are kind of deprecated for the new uesrs, but we still have
> handful of users of it.

No, they aren't deprecated, not even sort of.

Of course, stuff that can use frequency tables should implement ->target_index,
because there's no valid reason for it not to do that.

However, there are cases (and not legacy) where frequency tables are simply
impractical and those drivers have no choice but to implement ->target.

And if you want to try to force them into the frequency tables model
regardless, then think twice, because I'm not going to let you do that.

> > Sorry, that should've been "check that either ->target_index() or
> > ->resolve_freq() is implemented." 
> > 
> > Implementing resolve_freq for the target() drivers and requiring it at
> > driver init time is probably the better way to go though. Perhaps I can
> > work on this at some point.
> 
> As I said earlier as well in one of the emails, if you are worried
> about the extra 'if' check in the hot path, then wouldn't this fix it
> for you?
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 3dd4884c6f9e..91d8ec4c8eb7 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -517,7 +517,7 @@ unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
>                 return policy->freq_table[idx].frequency;
>         }
>  
> -       if (cpufreq_driver->resolve_freq)
> +       if (likely(cpufreq_driver->resolve_freq))
>                 return cpufreq_driver->resolve_freq(policy, target_freq);
>  
>         return target_freq;

A CPU with good enough branch prediction logic should be able to figure out
whether or not the test is "likely" after a few repetitions of it.

Thanks,
Rafael

Re: [PATCH V2] cpufreq: Disallow ->resolve_freq() for drivers providing ->target_index()

[Viresh Kumar]

On 22-07-16, 23:11, Rafael J. Wysocki wrote:
> No, they aren't deprecated, not even sort of.
> 
> Of course, stuff that can use frequency tables should implement ->target_index,
> because there's no valid reason for it not to do that.
> 
> However, there are cases (and not legacy) where frequency tables are simply
> impractical and those drivers have no choice but to implement ->target.
> 
> And if you want to try to force them into the frequency tables model
> regardless, then think twice, because I'm not going to let you do that.

No I am not :)

Perhaps this was just mis-worded in the Documentation then.

-- 
viresh