[0/2] Add exit_prepare callback to the cpufreq_driver interface.

Commit Message

preeti Sept. 4, 2014, 9:10 a.m. UTC

Hi,

I went through the cpufreq code and the previous discussion in the
context of this patch and I propose the below patch. Please let me know
if it misses something that you all had discussed.

On 09/04/2014 11:38 AM, Viresh Kumar wrote:
> On 19 March 2014 19:49, Rafael J. Wysocki <[e-mail]> wrote:
> 
>> That said, for the intel_pstate case ->stop() as proposed by Dirk is demonstrably
>> sufficient and there are no other ->setpolicy drivers in sight wanting or needing
>> anything else.
>>
>> So to me, (1) the new ->stop() should *only* be called for ->setpolicy drivers,
>> because the purpose of it should be to "allow ->setpolicy drivers to do what the
>> GOV_STOP will do for regular drivers" as you put it above, and (2) some code in
>> the original intel_pstate's ->exit() may/should stay in there (instead of being
>> moved to the new ->stop()), which is the only possibly remaining issue here.
>>
>> The whole discussion about possibly re-using ->stop() for ->target drivers goes
>> in a totally wrong direction, because *if* ->target drivers need a new callback
>> to be executed around where ->stop() is called for ->setpolicy drivers, *then*
>> that has to be a *different* callback.
>>
>> And by the way, ->get() in fact has a different meaning for ->setpolicy drivers,
>> so it would be good to consider logical separation of ->setpolicy and ->target
>> drivers so that each kind has its own separate set of callbacks with no overlaps.
>> That would make it easier to avoid breakage resulting from changes made with
>> ->setpolicy drivers that also affect ->target drivers in unpredictable ways and
>> the other way around.
> 
> Okay, I have picked up a very old thread but it looks more sensible to start
> replying here..
> 
> Preeti (Cc'd) wants to do something similar, i.e. reduce freq of a
> core before it
> goes down. And the driver is probably: drivers/cpufreq/powernv-cpufreq.c, which
> is ->target() type.
> 
> Now should we reuse the same callback ->stop_cpu() or implement a new one?
> I don't know if adding a new callback would be a good idea here..
> 
> --
> viresh
> 

cpufreq: Allow stop CPU callback to be used by all cpufreq drivers
    
Commit 367dc4aa introduced the stop CPU callback for intel_pstate
drivers. During the CPU_DOWN_PREPARE stage, this callback is invoked
so that drivers can take some action on the pstate of the cpu
before it is taken offline. This callback was assumed to be useful
only for those drivers which have implemented the set_policy CPU
callback because they have no other way to take action about the
cpufreq of a CPU which is being hotplugged out except in the exit
callback which is called very late in the offline process.
    
The drivers which implement the target/target_index callbacks were
expected to take care of requirements like the ones that commit
367dc4aa addresses in the GOV_STOP notification event. But there
are disadvantages to restricting the usage of stop CPU callback
to cpufreq drivers that implement the set_policy callbacks and who
want to take explicit action on the setting the cpufreq during a
hotplug operation.
   
1.GOV_STOP gets called for every CPU offline and drivers would usually
  want to take action when the last cpu in the policy->cpus mask
  is taken offline. As long as there is more than one cpu in the
  policy->cpus mask, cpufreq core itself makes sure that the freq
  for the other cpus in this mask is set according to the maximum load.
  This is sensible and drivers which implement the target_index callback
  would mostly not want to modify that. However the cpufreq core leaves a
  loose end when the cpu in the policy->cpus mask is the last one to go offline;
  it does nothing explicit to the frequency of the core. Drivers may need
  a way to take some action here and stop CPU callback mechanism is the
  best way to do it today.
    
2.We cannot implement driver specific actions in the GOV_STOP mechanism.
  So we will need another driver callback which is invoked from here which is
  unnecessary.
    
  Therefore this patch extends the usage of stop CPU callback to be used
  by all cpufreq drivers as long as they have this callback implemented
  and irrespective of whether they are set_policy/target_index drivers.
  The assumption is if the drivers find the GOV_STOP path to be a suitable
  way of implementing what they want to do with the freq of the cpu
  going offine,they will not implement the stop CPU callback at all.
    
  Signed-off-by: Preeti U Murthy <[e-mail]>


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Viresh Kumar Sept. 4, 2014, 9:16 a.m. UTC | #1

On 4 September 2014 14:40, Preeti U Murthy <[e-mail]> wrote:
> cpufreq: Allow stop CPU callback to be used by all cpufreq drivers
>
> Commit 367dc4aa introduced the stop CPU callback for intel_pstate
> drivers. During the CPU_DOWN_PREPARE stage, this callback is invoked
> so that drivers can take some action on the pstate of the cpu
> before it is taken offline. This callback was assumed to be useful
> only for those drivers which have implemented the set_policy CPU
> callback because they have no other way to take action about the
> cpufreq of a CPU which is being hotplugged out except in the exit
> callback which is called very late in the offline process.
>
> The drivers which implement the target/target_index callbacks were
> expected to take care of requirements like the ones that commit
> 367dc4aa addresses in the GOV_STOP notification event. But there
> are disadvantages to restricting the usage of stop CPU callback
> to cpufreq drivers that implement the set_policy callbacks and who
> want to take explicit action on the setting the cpufreq during a
> hotplug operation.
>
> 1.GOV_STOP gets called for every CPU offline and drivers would usually
>   want to take action when the last cpu in the policy->cpus mask
>   is taken offline. As long as there is more than one cpu in the
>   policy->cpus mask, cpufreq core itself makes sure that the freq
>   for the other cpus in this mask is set according to the maximum load.
>   This is sensible and drivers which implement the target_index callback
>   would mostly not want to modify that. However the cpufreq core leaves a
>   loose end when the cpu in the policy->cpus mask is the last one to go offline;
>   it does nothing explicit to the frequency of the core. Drivers may need
>   a way to take some action here and stop CPU callback mechanism is the
>   best way to do it today.
>
> 2.We cannot implement driver specific actions in the GOV_STOP mechanism.
>   So we will need another driver callback which is invoked from here which is
>   unnecessary.
>
>   Therefore this patch extends the usage of stop CPU callback to be used
>   by all cpufreq drivers as long as they have this callback implemented
>   and irrespective of whether they are set_policy/target_index drivers.
>   The assumption is if the drivers find the GOV_STOP path to be a suitable
>   way of implementing what they want to do with the freq of the cpu
>   going offine,they will not implement the stop CPU callback at all.
>
>   Signed-off-by: Preeti U Murthy <[e-mail]>
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index d9fdedd..6463f35 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1380,7 +1380,7 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
>                 if (!cpufreq_suspended)
>                         pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
>                                  __func__, new_cpu, cpu);
> -       } else if (cpufreq_driver->stop_cpu && cpufreq_driver->setpolicy) {
> +       } else if (cpufreq_driver->stop_cpu) {
>                 cpufreq_driver->stop_cpu(policy);
>         }

Rafael explicitly said earlier that he want to see a separate callback for
->target() drivers, don't know why..

It looks fine to me though.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

preeti Sept. 4, 2014, 10:03 a.m. UTC | #2

On 09/04/2014 02:46 PM, Viresh Kumar wrote:
> On 4 September 2014 14:40, Preeti U Murthy <[e-mail]> wrote:
>> cpufreq: Allow stop CPU callback to be used by all cpufreq drivers
>>
>> Commit 367dc4aa introduced the stop CPU callback for intel_pstate
>> drivers. During the CPU_DOWN_PREPARE stage, this callback is invoked
>> so that drivers can take some action on the pstate of the cpu
>> before it is taken offline. This callback was assumed to be useful
>> only for those drivers which have implemented the set_policy CPU
>> callback because they have no other way to take action about the
>> cpufreq of a CPU which is being hotplugged out except in the exit
>> callback which is called very late in the offline process.
>>
>> The drivers which implement the target/target_index callbacks were
>> expected to take care of requirements like the ones that commit
>> 367dc4aa addresses in the GOV_STOP notification event. But there
>> are disadvantages to restricting the usage of stop CPU callback
>> to cpufreq drivers that implement the set_policy callbacks and who
>> want to take explicit action on the setting the cpufreq during a
>> hotplug operation.
>>
>> 1.GOV_STOP gets called for every CPU offline and drivers would usually
>>   want to take action when the last cpu in the policy->cpus mask
>>   is taken offline. As long as there is more than one cpu in the
>>   policy->cpus mask, cpufreq core itself makes sure that the freq
>>   for the other cpus in this mask is set according to the maximum load.
>>   This is sensible and drivers which implement the target_index callback
>>   would mostly not want to modify that. However the cpufreq core leaves a
>>   loose end when the cpu in the policy->cpus mask is the last one to go offline;
>>   it does nothing explicit to the frequency of the core. Drivers may need
>>   a way to take some action here and stop CPU callback mechanism is the
>>   best way to do it today.
>>
>> 2.We cannot implement driver specific actions in the GOV_STOP mechanism.
>>   So we will need another driver callback which is invoked from here which is
>>   unnecessary.
>>
>>   Therefore this patch extends the usage of stop CPU callback to be used
>>   by all cpufreq drivers as long as they have this callback implemented
>>   and irrespective of whether they are set_policy/target_index drivers.
>>   The assumption is if the drivers find the GOV_STOP path to be a suitable
>>   way of implementing what they want to do with the freq of the cpu
>>   going offine,they will not implement the stop CPU callback at all.
>>
>>   Signed-off-by: Preeti U Murthy <[e-mail]>
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index d9fdedd..6463f35 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1380,7 +1380,7 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
>>                 if (!cpufreq_suspended)
>>                         pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
>>                                  __func__, new_cpu, cpu);
>> -       } else if (cpufreq_driver->stop_cpu && cpufreq_driver->setpolicy) {
>> +       } else if (cpufreq_driver->stop_cpu) {
>>                 cpufreq_driver->stop_cpu(policy);
>>         }
> 
> Rafael explicitly said earlier that he want to see a separate callback for
> ->target() drivers, don't know why..

I think Rafael's point was that since no driver that had implemented the
target_index callback was using it at the time that this patch was
proposed, it was be best to couple the check on existence of stop_cpu
callback with the the check on the kind of cpufreq driver. However
powerpc is also in need of this today and we implement the target_index
callback and find it convenient to use the stop CPU callback.

Rafael, in which case would it not make sense to remove the check on
driver->setpolicy above?

Besides, I don't understand very well why we had this double check in
the first place. Only if the drivers are in need of the functionality
like stop_cpu, would they have implemented this callback right? If we
are to assume that the drivers which have implemented the target_index
callback should never be needing it, they would not have implemented the
stop CPU callback either. So what was that, which was blatantly wrong
with just having a check on stop_cpu? I did go through the discussion
but did not find a convincing answer to this.

Rafael?

Regards
Preeti U Murthy

> 
> It looks fine to me though.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Viresh Kumar Sept. 4, 2014, 10:37 a.m. UTC | #3

On 4 September 2014 15:33, Preeti U Murthy <[e-mail]> wrote:
> I think Rafael's point was that since no driver that had implemented the
> target_index callback was using it at the time that this patch was
> proposed, it was be best to couple the check on existence of stop_cpu
> callback with the the check on the kind of cpufreq driver. However
> powerpc is also in need of this today and we implement the target_index
> callback and find it convenient to use the stop CPU callback.

No, this is what he said..

"
So to me, (1) the new ->stop() should *only* be called for ->setpolicy drivers,
because the purpose of it should be to "allow ->setpolicy drivers to do what the
GOV_STOP will do for regular drivers"
"

> Rafael, in which case would it not make sense to remove the check on
> driver->setpolicy above?
>
> Besides, I don't understand very well why we had this double check in
> the first place. Only if the drivers are in need of the functionality
> like stop_cpu, would they have implemented this callback right? If we
> are to assume that the drivers which have implemented the target_index
> callback should never be needing it, they would not have implemented the
> stop CPU callback either. So what was that, which was blatantly wrong
> with just having a check on stop_cpu? I did go through the discussion
> but did not find a convincing answer to this.

The idea was to get something similar to GOV_STOP for setpolicy drivers.
But in the end we didn't get to that. What we do in GOV_STOP is stop
changing CPUs frequency, but here in stop_cpu() we can stop changing
CPUs frequency OR take it to minimum, whatever we want..

As I said earlier, probably we should just do what you did in your patch +
some documentation changes.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki Sept. 4, 2014, 7:56 p.m. UTC | #4

On Thursday, September 04, 2014 04:07:28 PM Viresh Kumar wrote:
> On 4 September 2014 15:33, Preeti U Murthy <[e-mail]> wrote:
> > I think Rafael's point was that since no driver that had implemented the
> > target_index callback was using it at the time that this patch was
> > proposed, it was be best to couple the check on existence of stop_cpu
> > callback with the the check on the kind of cpufreq driver. However
> > powerpc is also in need of this today and we implement the target_index
> > callback and find it convenient to use the stop CPU callback.
> 
> No, this is what he said..
> 
> "
> So to me, (1) the new ->stop() should *only* be called for ->setpolicy drivers,
> because the purpose of it should be to "allow ->setpolicy drivers to do what the
> GOV_STOP will do for regular drivers"
> "
> 
> > Rafael, in which case would it not make sense to remove the check on
> > driver->setpolicy above?
> >
> > Besides, I don't understand very well why we had this double check in
> > the first place. Only if the drivers are in need of the functionality
> > like stop_cpu, would they have implemented this callback right? If we
> > are to assume that the drivers which have implemented the target_index
> > callback should never be needing it, they would not have implemented the
> > stop CPU callback either. So what was that, which was blatantly wrong
> > with just having a check on stop_cpu? I did go through the discussion
> > but did not find a convincing answer to this.
> 
> The idea was to get something similar to GOV_STOP for setpolicy drivers.
> But in the end we didn't get to that. What we do in GOV_STOP is stop
> changing CPUs frequency, but here in stop_cpu() we can stop changing
> CPUs frequency OR take it to minimum, whatever we want..
> 
> As I said earlier, probably we should just do what you did in your patch +
> some documentation changes.

OK, if that works for everybody.  For one, I wouldn't like to end up with a
callback used for different things in every drvier implementing it.

Patch

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index d9fdedd..6463f35 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1380,7 +1380,7 @@  static int __cpufreq_remove_dev_prepare(struct device *dev,
                if (!cpufreq_suspended)
                        pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
                                 __func__, new_cpu, cpu);
-       } else if (cpufreq_driver->stop_cpu && cpufreq_driver->setpolicy) {
+       } else if (cpufreq_driver->stop_cpu) {
                cpufreq_driver->stop_cpu(policy);
        }