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

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

[dirk.brandewie]

From: Dirk Brandewie <dirk.j.brandewie@intel.com>

Some drivers (intel_pstate) need to modify state on a core before it
is completely offline.  The ->exit() callback is executed during the
CPU_POST_DEAD phase of the cpu offline process which is too late to
change the state of the core.

Patch 1 adds an optional callback function to the cpufreq_driver
interface which is called by the core during the CPU_DOWN_PREPARE
phase of cpu offline in __cpufreq_remove_dev_prepare().

Patch 2 changes intel_pstate to use the ->exit_prepare callback to do
its cleanup during cpu offline. 

Dirk Brandewie (2):
  cpufreq: Add exit_prepare callback to cpufreq_driver interface
  intel_pstate: Set core to min P state during core offline

 Documentation/cpu-freq/cpu-drivers.txt |  8 +++++++-
 drivers/cpufreq/cpufreq.c              |  3 +++
 drivers/cpufreq/intel_pstate.c         | 20 +++++++++++++-------
 include/linux/cpufreq.h                |  1 +
 4 files changed, 24 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] cpufreq: Add exit_prepare callback to cpufreq_driver interface

[dirk.brandewie]

From: Dirk Brandewie <dirk.j.brandewie@intel.com>

This callback allows the driver to do clean up before the CPU is
completely down and its state cannot be modified.  This is used
by the intel_pstate driver to reduce the requested P state prior to
the core going away.  This is required because the requested P state
of the offline core is used to select the package P state. This
effectively sets the floor package P state to the requested P state on
the offline core.

Signed-off-by: Dirk Brandewie <dirk.j.brandewie@intel.com>
---
 Documentation/cpu-freq/cpu-drivers.txt | 8 +++++++-
 drivers/cpufreq/cpufreq.c              | 3 +++
 include/linux/cpufreq.h                | 1 +
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/Documentation/cpu-freq/cpu-drivers.txt b/Documentation/cpu-freq/cpu-drivers.txt
index 8b1a445..935f274 100644
--- a/Documentation/cpu-freq/cpu-drivers.txt
+++ b/Documentation/cpu-freq/cpu-drivers.txt
@@ -61,7 +61,13 @@ target_index		-	See below on the differences.
 
 And optionally
 
-cpufreq_driver.exit -		A pointer to a per-CPU cleanup function.
+cpufreq_driver.exit -		A pointer to a per-CPU cleanup
+		    		function called during CPU_POST_DEAD
+		    		phase of cpu hotplug process.
+
+cpufreq_driver.exit_prepare -	A pointer to a per-CPU cleanup function 
+			    	called during CPU_DOWN_PREPARE phase of 
+				cpu hotplug process.
 
 cpufreq_driver.resume -		A pointer to a per-CPU resume function
 				which is called with interrupts disabled
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index cf485d9..5c9bbfa 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1338,6 +1338,9 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
 		}
 	}
 
+	if (cpufreq_driver->exit_prepare)
+		cpufreq_driver->exit_prepare(policy);
+
 	return 0;
 }
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 4d89e0e..5fa94ad 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -224,6 +224,7 @@ struct cpufreq_driver {
 	int	(*bios_limit)	(int cpu, unsigned int *limit);
 
 	int	(*exit)		(struct cpufreq_policy *policy);
+	int	(*exit_prepare)	(struct cpufreq_policy *policy);
 	int	(*suspend)	(struct cpufreq_policy *policy);
 	int	(*resume)	(struct cpufreq_policy *policy);
 	struct freq_attr	**attr;
-- 
1.8.3.1

Re: [PATCH 1/2] cpufreq: Add exit_prepare callback to cpufreq_driver interface

[Srivatsa S. Bhat]

On 03/13/2014 11:06 PM, dirk.brandewie@gmail.com wrote:
> From: Dirk Brandewie <dirk.j.brandewie@intel.com>
> 
> This callback allows the driver to do clean up before the CPU is
> completely down and its state cannot be modified.  This is used
> by the intel_pstate driver to reduce the requested P state prior to
> the core going away.  This is required because the requested P state
> of the offline core is used to select the package P state. This
> effectively sets the floor package P state to the requested P state on
> the offline core.
> 
> Signed-off-by: Dirk Brandewie <dirk.j.brandewie@intel.com>
> ---
>  Documentation/cpu-freq/cpu-drivers.txt | 8 +++++++-
>  drivers/cpufreq/cpufreq.c              | 3 +++
>  include/linux/cpufreq.h                | 1 +
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/cpu-freq/cpu-drivers.txt b/Documentation/cpu-freq/cpu-drivers.txt
> index 8b1a445..935f274 100644
> --- a/Documentation/cpu-freq/cpu-drivers.txt
> +++ b/Documentation/cpu-freq/cpu-drivers.txt
> @@ -61,7 +61,13 @@ target_index		-	See below on the differences.
> 
>  And optionally
> 
> -cpufreq_driver.exit -		A pointer to a per-CPU cleanup function.
> +cpufreq_driver.exit -		A pointer to a per-CPU cleanup
> +		    		function called during CPU_POST_DEAD
> +		    		phase of cpu hotplug process.
> +
> +cpufreq_driver.exit_prepare -	A pointer to a per-CPU cleanup function 
> +			    	called during CPU_DOWN_PREPARE phase of 
> +				cpu hotplug process.
> 
>  cpufreq_driver.resume -		A pointer to a per-CPU resume function
>  				which is called with interrupts disabled
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index cf485d9..5c9bbfa 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1338,6 +1338,9 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
>  		}
>  	}
> 
> +	if (cpufreq_driver->exit_prepare)
> +		cpufreq_driver->exit_prepare(policy);
> +

The placement of this hunk doesn't feel right. IMHO we should place it
right next to the code which stops the governor.

Something like this:

        if (has_target()) {
                ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
                if (ret) {
                        pr_err("%s: Failed to stop governor\n", __func__);
                        return ret; 
                }    
        } else if (cpufreq_driver->setpolicy) {
		if (cpufreq_driver->exit_prepare)
			cpufreq_driver->exit_prepare(policy);
	}

This makes it clear that GOV_STOP is used to stop managing the CPUs
for platforms that have ->target defined, and ->exit_prepare() is used
for similar purposes for platforms that have ->setpolicy() defined.

By the way, I like the name ->stop more than ->exit_prepare, because:
->exit() is done once per policy, which implies that ->exit_prepare
also shares similar semantics. However, what we really want the new
callback to do is to provide a way for the driver to stop managing the
CPU that is going offline, just like GOV_STOP. So naturally, this
new callback should be invoked during every CPU offline, and not just
once per policy. Hence the name "stop" (this CPU) makes perfect sense
for that IMHO. 

[Of course, I understand that GOV_STOP actually stops the entire
policy for all affected cpus and then we use GOV_START in
_remove_dev_finish() to restart the governor for the other online
CPUs in that policy. This is somewhat round-about, but conceptually
this is equivalent to asking the governor to let go control of only
the CPU going offline. The new ->stop callback should have the same
"stop only this CPU" semantics.]

>  	return 0;
>  }
> 
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 4d89e0e..5fa94ad 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -224,6 +224,7 @@ struct cpufreq_driver {
>  	int	(*bios_limit)	(int cpu, unsigned int *limit);
> 
>  	int	(*exit)		(struct cpufreq_policy *policy);
> +	int	(*exit_prepare)	(struct cpufreq_policy *policy);
>  	int	(*suspend)	(struct cpufreq_policy *policy);
>  	int	(*resume)	(struct cpufreq_policy *policy);
>  	struct freq_attr	**attr;
> 

[PATCH 2/2] intel_pstate: Set core to min P state during core offline

[dirk.brandewie]

From: Dirk Brandewie <dirk.j.brandewie@intel.com>

Change to use ->exit_prepare() callback to do clean up during CPU
hotplug. The requested P state for an offline core will be used by the
hardware coordination function to select the package P state. If the
core is under load when it is offlined it will fix the package P state
floor to the requested P state of offline core.

Reported-by: Patrick Marlier <patrick.marlier@gmail.com>
Signed-off-by: Dirk Brandewie <dirk.j.brandewie@intel.com>
---
 drivers/cpufreq/intel_pstate.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 2cd36b9..fe32a8c 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -447,7 +447,7 @@ static void core_set_pstate(struct cpudata *cpudata, int pstate)
 	if (limits.no_turbo)
 		val |= (u64)1 << 32;
 
-	wrmsrl(MSR_IA32_PERF_CTL, val);
+	wrmsrl_on_cpu(cpudata->cpu, MSR_IA32_PERF_CTL, val);
 }
 
 static struct cpu_defaults core_params = {
@@ -773,16 +773,22 @@ static int intel_pstate_verify_policy(struct cpufreq_policy *policy)
 	return 0;
 }
 
-static int intel_pstate_cpu_exit(struct cpufreq_policy *policy)
+static int intel_pstate_cpu_exit_prepare(struct cpufreq_policy *policy)
 {
-	int cpu = policy->cpu;
+	int cpu_num = policy->cpu;
+	struct cpudata *cpu = all_cpu_data[cpu_num];
+
+	pr_info("intel_pstate CPU %d exiting\n", cpu_num);
+
+	del_timer(&all_cpu_data[cpu_num]->timer);	
+	intel_pstate_set_pstate(cpu, cpu->pstate.min_pstate);
+	kfree(all_cpu_data[cpu_num]);
+	all_cpu_data[cpu_num] = NULL;
 
-	del_timer(&all_cpu_data[cpu]->timer);
-	kfree(all_cpu_data[cpu]);
-	all_cpu_data[cpu] = NULL;
 	return 0;
 }
 
+
 static int intel_pstate_cpu_init(struct cpufreq_policy *policy)
 {
 	struct cpudata *cpu;
@@ -818,7 +824,7 @@ static struct cpufreq_driver intel_pstate_driver = {
 	.setpolicy	= intel_pstate_set_policy,
 	.get		= intel_pstate_get,
 	.init		= intel_pstate_cpu_init,
-	.exit		= intel_pstate_cpu_exit,
+	.exit_prepare	= intel_pstate_cpu_exit_prepare,
 	.name		= "intel_pstate",
 };
 
-- 
1.8.3.1

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

[Viresh Kumar]

On Thu, Mar 13, 2014 at 11:06 PM,  <dirk.brandewie@gmail.com> wrote:
> From: Dirk Brandewie <dirk.j.brandewie@intel.com>
>
> Some drivers (intel_pstate) need to modify state on a core before it
> is completely offline.  The ->exit() callback is executed during the
> CPU_POST_DEAD phase of the cpu offline process which is too late to
> change the state of the core.
>
> Patch 1 adds an optional callback function to the cpufreq_driver
> interface which is called by the core during the CPU_DOWN_PREPARE
> phase of cpu offline in __cpufreq_remove_dev_prepare().
>
> Patch 2 changes intel_pstate to use the ->exit_prepare callback to do
> its cleanup during cpu offline.

Copying stuff from other mail thread here so that we can discuss on a
single mail chain.

On 14 March 2014 03:09, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, March 13, 2014 12:56:02 PM Viresh Kumar wrote:
>> On 13 March 2014 08:07, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Wednesday, March 12, 2014 02:27:07 PM Dirk Brandewie wrote:
>>
>> >> > I see two possibilities:
>> >> >   1. Move the exit() callback to __cpufreq_remove_dev_prepare().  I don't
>> >> >      have a good understanding of what carnage this would cause in the core
>> >> >      or other scaling drivers.
>> >> >
>> >> >   2. Add another callback to the cpufreq_driver interface that would be call
>> >> >      from __cpufreq_remove_dev_prepare() if the callback was set.
>> >
>> > I prefer 2, the reason being that it pretty much is guaranteed not to break
>> > anything.  For the record, I'm not a fan of adding new cpufreq driver callbacks,
>> > but in this particular case it seems we can't really do anything better.
>>
>> I haven't thought a lot about which one of these two looks better, probably
>> Rafael might be correct. But I can see another way out here as this is very
>> much driver specific. Why can't we do a register_hotcpu_notifier() from
>> pstate driver alone?
>
> Why would that be better than adding a new callback?

Because its becoming more and more confusing. Probably we got the problem
right but have wrong solutions for it.

But having considered this issue in detail now, I have more inputs. All Dirk and
Patrick wanted is to set core to min P-state before it gets offlined. We already
have some infrastructure in core which is called this today:
cpufreq_generic_suspend(). We can rework on that to get a more ideal solution
for both the problems.

Over that I don't think Dirk's solution is going to work if we twist
the systems a
bit. For example, Dirk probably wanted to set P-STATE of every core to MIN
when it goes down. But his solution probably doesn't do that right now.

As exit() is called only when the policy expires or all the CPUs of that policy
are down. Suppose only one CPU out of 4 goes down from a policy, then
pstate driver would never know that happened. And that core wouldn't go
to min state.

I think we have two solutions here:
- If its just about setting core a particular freq when it goes down, I think it
looks a generic enough problem and so better fix core for that. Probably with
help of flags field/suspend-freq (maybe renamed) and without calling drivers
exit() at all..

- If this is highly driver specific (which doesn't look like if all we
have to do
is setting freq to MIN), then better have something like
register_hotcpu_notifier() with priority set to -1, so that it gets called after
cpufreq.

--
viresh

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

[Dirk Brandewie]

On 03/13/2014 09:59 PM, Viresh Kumar wrote:
> On Thu, Mar 13, 2014 at 11:06 PM,  <dirk.brandewie@gmail.com> wrote:
>> From: Dirk Brandewie <dirk.j.brandewie@intel.com>
>>
>> Some drivers (intel_pstate) need to modify state on a core before it
>> is completely offline.  The ->exit() callback is executed during the
>> CPU_POST_DEAD phase of the cpu offline process which is too late to
>> change the state of the core.
>>
>> Patch 1 adds an optional callback function to the cpufreq_driver
>> interface which is called by the core during the CPU_DOWN_PREPARE
>> phase of cpu offline in __cpufreq_remove_dev_prepare().
>>
>> Patch 2 changes intel_pstate to use the ->exit_prepare callback to do
>> its cleanup during cpu offline.
>
> Copying stuff from other mail thread here so that we can discuss on a
> single mail chain.
>
> On 14 March 2014 03:09, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Thursday, March 13, 2014 12:56:02 PM Viresh Kumar wrote:
>>> On 13 March 2014 08:07, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>> On Wednesday, March 12, 2014 02:27:07 PM Dirk Brandewie wrote:
>>>
>>>>>> I see two possibilities:
>>>>>>    1. Move the exit() callback to __cpufreq_remove_dev_prepare().  I don't
>>>>>>       have a good understanding of what carnage this would cause in the core
>>>>>>       or other scaling drivers.
>>>>>>
>>>>>>    2. Add another callback to the cpufreq_driver interface that would be call
>>>>>>       from __cpufreq_remove_dev_prepare() if the callback was set.
>>>>
>>>> I prefer 2, the reason being that it pretty much is guaranteed not to break
>>>> anything.  For the record, I'm not a fan of adding new cpufreq driver callbacks,
>>>> but in this particular case it seems we can't really do anything better.
>>>
>>> I haven't thought a lot about which one of these two looks better, probably
>>> Rafael might be correct. But I can see another way out here as this is very
>>> much driver specific. Why can't we do a register_hotcpu_notifier() from
>>> pstate driver alone?
>>
>> Why would that be better than adding a new callback?
>
> Because its becoming more and more confusing. Probably we got the problem
> right but have wrong solutions for it.
>
> But having considered this issue in detail now, I have more inputs. All Dirk and
> Patrick wanted is to set core to min P-state before it gets offlined. We already
> have some infrastructure in core which is called this today:
> cpufreq_generic_suspend(). We can rework on that to get a more ideal solution
> for both the problems.

Are you proposing adding cpufreq_generic_suspend() to the core I can not find
it in the mainline code.

>
> Over that I don't think Dirk's solution is going to work if we twist
> the systems a bit.

Could you explain what "twist the systems a bit" means?

>For example, Dirk probably wanted to set P-STATE of every core to MIN
> when it goes down. But his solution probably doesn't do that right now.
>

No, I wanted to set the core that was being off-lined to min P state.

> As exit() is called only when the policy expires or all the CPUs of that policy
> are down. Suppose only one CPU out of 4 goes down from a policy, then
> pstate driver would never know that happened. And that core wouldn't go
> to min state.

My patch does not change the semantics of exit() or when it is called.  For
intel_pstate their is one cpu per policy so I moved all the cleanup to
exit_prepare() if there were work I needed to do at CPU_POST_DEAD I would have
continued to export the *optional* exit callback.

The callback name exit_prepare() was probably unfortunate and might be causing
some confusion.  I will be changing the name per Rafael's feedback.
>
> I think we have two solutions here:
> - If its just about setting core a particular freq when it goes down, I think it
> looks a generic enough problem and so better fix core for that. Probably with
> help of flags field/suspend-freq (maybe renamed) and without calling drivers
> exit() at all..

ATM the only thing that needs to be done in this case is to allow intel_pstate
to set the P state on the core when it is going done.  My solution from the
cores point of view is more generic, it allows any driver that needs to do work
during CPU_DOWN_PREPARE to do it without adding any new logic to the core.

>
> - If this is highly driver specific (which doesn't look like if all we
> have to do is setting freq to MIN),

That is why intel_pstate is the *only* driver using exit_prepare().  If
other drivers need to do work during this phase of the cpu hotplug process
then they can export the callback otherwise it has no affect on existing
or future scaling drivers.

> then better have something like
> register_hotcpu_notifier() with priority set to -1, so that it gets called after
> cpufreq.

intel_pstate or any scaling driver having its own hotcpu_notifier makes possible
for the core and the driver to disagree on the state of the driver.  Having
the driver take hotplug notifications from two directions is a bad idea IMHO.

>
> --
> viresh
>

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

[Viresh Kumar]

On 14 March 2014 20:40, Dirk Brandewie <dirk.brandewie@gmail.com> wrote:
> Are you proposing adding cpufreq_generic_suspend() to the core I can not
> find
> it in the mainline code.

Its already there in linux-next. I am suggesting to reuse that
infrastructure with
some necessary modification to support both suspend and hotplug.

>> Over that I don't think Dirk's solution is going to work if we twist
>> the systems a bit.
>
> Could you explain what "twist the systems a bit" means?

The one I explained in the below paragraph.

>> For example, Dirk probably wanted to set P-STATE of every core to MIN
>> when it goes down. But his solution probably doesn't do that right now.
>>
>
> No, I wanted to set the core that was being off-lined to min P state.

Sorry, probably my words were a bit confusing. I meant exactly what
you just wrote. Core going down will set its freq to min.

>> As exit() is called only when the policy expires or all the CPUs of that
>> policy
>> are down. Suppose only one CPU out of 4 goes down from a policy, then
>> pstate driver would never know that happened. And that core wouldn't go
>> to min state.
>
> My patch does not change the semantics of exit() or when it is called.  For
> intel_pstate their is one cpu per policy so I moved all the cleanup to

I didn't knew that its guaranteed by pstate driver. I thought it would still be
hardware dependent as some cores might share clock line.

> exit_prepare() if there were work I needed to do at CPU_POST_DEAD I would
> have
> continued to export the *optional* exit callback.
>
> The callback name exit_prepare() was probably unfortunate and might be
> causing
> some confusion.  I will be changing the name per Rafael's feedback.

Don't know.. There is another problem here that exit_prepare() would be called
for each CPU whereas exit() would be called for each policy.

And I strongly feel that we shouldn't give another callback here but instead
just set core to a specific freq as mentioned by driver with some other field.

>> I think we have two solutions here:
>> - If its just about setting core a particular freq when it goes down, I
>> think it
>> looks a generic enough problem and so better fix core for that. Probably
>> with
>> help of flags field/suspend-freq (maybe renamed) and without calling
>> drivers
>> exit() at all..
>
>
> ATM the only thing that needs to be done in this case is to allow
> intel_pstate
> to set the P state on the core when it is going done.  My solution from the
> cores point of view is more generic, it allows any driver that needs to do
> work
> during CPU_DOWN_PREPARE to do it without adding any new logic to the core.

Yeah, do we really need to give that freedom right now? Would be better
to get this into core as that would be more generic and people looking to set
core to some freq at shutdown wouldn't be replicating that code.

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

[Dirk Brandewie]

On 03/14/2014 10:07 AM, Viresh Kumar wrote:
> On 14 March 2014 20:40, Dirk Brandewie <dirk.brandewie@gmail.com> wrote:
>> Are you proposing adding cpufreq_generic_suspend() to the core I can not
>> find
>> it in the mainline code.
>
> Its already there in linux-next. I am suggesting to reuse that
> infrastructure with
> some necessary modification to support both suspend and hotplug.

Suspend and hotplug are two very different things and if we start
crossing those wires bad things are going to happen IMHO.

In "normal" operation using the suspend path to do this work could
work in principal but doesn't handle the case where the user does
    echo 0 | sudo tee /sys/devices/system/cpu/cpuX/online

Trying force hotplug and suspend into a common mechanism would
lead to a bunch of special case code or a significant rework of the
core code IMHO.


>
>>> Over that I don't think Dirk's solution is going to work if we twist
>>> the systems a bit.
>>
>> Could you explain what "twist the systems a bit" means?
>
> The one I explained in the below paragraph.
>
>>> For example, Dirk probably wanted to set P-STATE of every core to MIN
>>> when it goes down. But his solution probably doesn't do that right now.
>>>
>>
>> No, I wanted to set the core that was being off-lined to min P state.
>
> Sorry, probably my words were a bit confusing. I meant exactly what
> you just wrote. Core going down will set its freq to min.
>
>>> As exit() is called only when the policy expires or all the CPUs of that
>>> policy
>>> are down. Suppose only one CPU out of 4 goes down from a policy, then
>>> pstate driver would never know that happened. And that core wouldn't go
>>> to min state.
>>
>> My patch does not change the semantics of exit() or when it is called.  For
>> intel_pstate their is one cpu per policy so I moved all the cleanup to
>
> I didn't knew that its guaranteed by pstate driver. I thought it would still be
> hardware dependent as some cores might share clock line.

This is guaranteed by the hardware.  Each core has its own MSR for P state
request.  Any coordination that is required between cores to select the
package P state is handled by the hardware.

>
>> exit_prepare() if there were work I needed to do at CPU_POST_DEAD I would
>> have
>> continued to export the *optional* exit callback.
>>
>> The callback name exit_prepare() was probably unfortunate and might be
>> causing
>> some confusion.  I will be changing the name per Rafael's feedback.
>
> Don't know.. There is another problem here that exit_prepare() would be called
> for each CPU whereas exit() would be called for each policy.

Granted but I don't see this as a problem in this case there is a 1:1
relationship.  If a driver chooses to use the *optional* exit_prepare() callback
and knows that there is a many:1 relationship between the policy and CPUs
then it would have to deal with it.

>
> And I strongly feel that we shouldn't give another callback here but instead
> just set core to a specific freq as mentioned by driver with some other field.
>
>>> I think we have two solutions here:
>>> - If its just about setting core a particular freq when it goes down, I
>>> think it
>>> looks a generic enough problem and so better fix core for that. Probably
>>> with
>>> help of flags field/suspend-freq (maybe renamed) and without calling
>>> drivers
>>> exit() at all..
>>
>>
>> ATM the only thing that needs to be done in this case is to allow
>> intel_pstate
>> to set the P state on the core when it is going done.  My solution from the
>> cores point of view is more generic, it allows any driver that needs to do
>> work
>> during CPU_DOWN_PREPARE to do it without adding any new logic to the core.
>
> Yeah, do we really need to give that freedom right now? Would be better
> to get this into core as that would be more generic and people looking to set
> core to some freq at shutdown wouldn't be replicating that code.

IMHO yes and it would be hard to be more generic, if your platform needs to
do architecture specific during the PREPARE phase of cpu hotplug use this
callback or not.

BTW now that you have added a path where the cpufreq_suspend() could fail
it return a value and be checked in dpm_suspend() instead of printing an
error and just continuing.

>

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

[Rafael J. Wysocki]

On Friday, March 14, 2014 11:29:04 AM Dirk Brandewie wrote:
> On 03/14/2014 10:07 AM, Viresh Kumar wrote:
> > On 14 March 2014 20:40, Dirk Brandewie <dirk.brandewie@gmail.com> wrote:
> >> Are you proposing adding cpufreq_generic_suspend() to the core I can not
> >> find
> >> it in the mainline code.
> >
> > Its already there in linux-next. I am suggesting to reuse that
> > infrastructure with
> > some necessary modification to support both suspend and hotplug.
> 
> Suspend and hotplug are two very different things and if we start
> crossing those wires bad things are going to happen IMHO.
> 
> In "normal" operation using the suspend path to do this work could
> work in principal but doesn't handle the case where the user does
>     echo 0 | sudo tee /sys/devices/system/cpu/cpuX/online
> 
> Trying force hotplug and suspend into a common mechanism would
> lead to a bunch of special case code or a significant rework of the
> core code IMHO.
> 
> 
> >
> >>> Over that I don't think Dirk's solution is going to work if we twist
> >>> the systems a bit.
> >>
> >> Could you explain what "twist the systems a bit" means?
> >
> > The one I explained in the below paragraph.
> >
> >>> For example, Dirk probably wanted to set P-STATE of every core to MIN
> >>> when it goes down. But his solution probably doesn't do that right now.
> >>>
> >>
> >> No, I wanted to set the core that was being off-lined to min P state.
> >
> > Sorry, probably my words were a bit confusing. I meant exactly what
> > you just wrote. Core going down will set its freq to min.
> >
> >>> As exit() is called only when the policy expires or all the CPUs of that
> >>> policy
> >>> are down. Suppose only one CPU out of 4 goes down from a policy, then
> >>> pstate driver would never know that happened. And that core wouldn't go
> >>> to min state.
> >>
> >> My patch does not change the semantics of exit() or when it is called.  For
> >> intel_pstate their is one cpu per policy so I moved all the cleanup to
> >
> > I didn't knew that its guaranteed by pstate driver. I thought it would still be
> > hardware dependent as some cores might share clock line.
> 
> This is guaranteed by the hardware.  Each core has its own MSR for P state
> request.  Any coordination that is required between cores to select the
> package P state is handled by the hardware.
> 
> >
> >> exit_prepare() if there were work I needed to do at CPU_POST_DEAD I would
> >> have
> >> continued to export the *optional* exit callback.
> >>
> >> The callback name exit_prepare() was probably unfortunate and might be
> >> causing
> >> some confusion.  I will be changing the name per Rafael's feedback.
> >
> > Don't know.. There is another problem here that exit_prepare() would be called
> > for each CPU whereas exit() would be called for each policy.
> 
> Granted but I don't see this as a problem in this case there is a 1:1
> relationship.  If a driver chooses to use the *optional* exit_prepare() callback
> and knows that there is a many:1 relationship between the policy and CPUs
> then it would have to deal with it.

Actually, I think we should make it clear that the new callback is for
->setpolicy drivers only, which will make things a bit clearer.

We seem to get caught by the difference between ->setpolicy and ->target
drivers on a regular basis, so it might be a good idea to make the distinction
more clear in the code.  I have an idea how to do that, but need some time
to prototype it.

> > And I strongly feel that we shouldn't give another callback here but instead
> > just set core to a specific freq as mentioned by driver with some other field.
> >
> >>> I think we have two solutions here:
> >>> - If its just about setting core a particular freq when it goes down, I
> >>> think it
> >>> looks a generic enough problem and so better fix core for that. Probably
> >>> with
> >>> help of flags field/suspend-freq (maybe renamed) and without calling
> >>> drivers
> >>> exit() at all..
> >>
> >>
> >> ATM the only thing that needs to be done in this case is to allow
> >> intel_pstate
> >> to set the P state on the core when it is going done.  My solution from the
> >> cores point of view is more generic, it allows any driver that needs to do
> >> work
> >> during CPU_DOWN_PREPARE to do it without adding any new logic to the core.
> >
> > Yeah, do we really need to give that freedom right now? Would be better
> > to get this into core as that would be more generic and people looking to set
> > core to some freq at shutdown wouldn't be replicating that code.

Question is if it needs to be more generic.

I honestly don't think that ->target drivers will ever do anything like it,
because they need the governor to "exit" before.  So we are talking about the
only two ->setpolicy drivers in the tree here.

> IMHO yes and it would be hard to be more generic, if your platform needs to
> do architecture specific during the PREPARE phase of cpu hotplug use this
> callback or not.
> 
> BTW now that you have added a path where the cpufreq_suspend() could fail
> it return a value and be checked in dpm_suspend() instead of printing an
> error and just continuing.

I'm not sure what you mean?  Are you saying that it might be a good idea
to allow cpufreq_suspend() to return error codes on failure?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Viresh Kumar]

On Sat, Mar 15, 2014 at 7:29 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> I honestly don't think that ->target drivers will ever do anything like it,
> because they need the governor to "exit" before.  So we are talking about the
> only two ->setpolicy drivers in the tree here.

I don't have a example platform which would need it but as I can see there are
platforms which want to do this when CPUs goes down from disable_nonboot_cpus(),
I would be in favor to keep this option available for them as well..
They might want
to do this when last CPU of a policy goes down, they might be able to save some
power over there.

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

[Srivatsa S. Bhat]

On 03/15/2014 07:29 AM, Rafael J. Wysocki wrote:
> On Friday, March 14, 2014 11:29:04 AM Dirk Brandewie wrote:
>> On 03/14/2014 10:07 AM, Viresh Kumar wrote:
>>> On 14 March 2014 20:40, Dirk Brandewie <dirk.brandewie@gmail.com> wrote:
>>>> Are you proposing adding cpufreq_generic_suspend() to the core I can not
>>>> find
>>>> it in the mainline code.
>>>
>>> Its already there in linux-next. I am suggesting to reuse that
>>> infrastructure with
>>> some necessary modification to support both suspend and hotplug.
>>
>> Suspend and hotplug are two very different things and if we start
>> crossing those wires bad things are going to happen IMHO.
>>
>> In "normal" operation using the suspend path to do this work could
>> work in principal but doesn't handle the case where the user does
>>     echo 0 | sudo tee /sys/devices/system/cpu/cpuX/online
>>
>> Trying force hotplug and suspend into a common mechanism would
>> lead to a bunch of special case code or a significant rework of the
>> core code IMHO.
>>
>>
>>>
>>>>> Over that I don't think Dirk's solution is going to work if we twist
>>>>> the systems a bit.
>>>>
>>>> Could you explain what "twist the systems a bit" means?
>>>
>>> The one I explained in the below paragraph.
>>>
>>>>> For example, Dirk probably wanted to set P-STATE of every core to MIN
>>>>> when it goes down. But his solution probably doesn't do that right now.
>>>>>
>>>>
>>>> No, I wanted to set the core that was being off-lined to min P state.
>>>
>>> Sorry, probably my words were a bit confusing. I meant exactly what
>>> you just wrote. Core going down will set its freq to min.
>>>
>>>>> As exit() is called only when the policy expires or all the CPUs of that
>>>>> policy
>>>>> are down. Suppose only one CPU out of 4 goes down from a policy, then
>>>>> pstate driver would never know that happened. And that core wouldn't go
>>>>> to min state.
>>>>
>>>> My patch does not change the semantics of exit() or when it is called.  For
>>>> intel_pstate their is one cpu per policy so I moved all the cleanup to
>>>
>>> I didn't knew that its guaranteed by pstate driver. I thought it would still be
>>> hardware dependent as some cores might share clock line.
>>
>> This is guaranteed by the hardware.  Each core has its own MSR for P state
>> request.  Any coordination that is required between cores to select the
>> package P state is handled by the hardware.
>>
>>>
>>>> exit_prepare() if there were work I needed to do at CPU_POST_DEAD I would
>>>> have
>>>> continued to export the *optional* exit callback.
>>>>
>>>> The callback name exit_prepare() was probably unfortunate and might be
>>>> causing
>>>> some confusion.  I will be changing the name per Rafael's feedback.
>>>
>>> Don't know.. There is another problem here that exit_prepare() would be called
>>> for each CPU whereas exit() would be called for each policy.
>>
>> Granted but I don't see this as a problem in this case there is a 1:1
>> relationship.  If a driver chooses to use the *optional* exit_prepare() callback
>> and knows that there is a many:1 relationship between the policy and CPUs
>> then it would have to deal with it.
> 
> Actually, I think we should make it clear that the new callback is for
> ->setpolicy drivers only, which will make things a bit clearer.
>

Agreed. As far as I understand, for ->target drivers, today we use GOV_STOP
to stop managing the CPU going offline. And for ->setpolicy drivers, we will
use this new callback to achieve the same goal. 

Regards,
Srivatsa S. Bhat
 
> We seem to get caught by the difference between ->setpolicy and ->target
> drivers on a regular basis, so it might be a good idea to make the distinction
> more clear in the code.  I have an idea how to do that, but need some time
> to prototype it.
>  
>>> And I strongly feel that we shouldn't give another callback here but instead
>>> just set core to a specific freq as mentioned by driver with some other field.
>>>
>>>>> I think we have two solutions here:
>>>>> - If its just about setting core a particular freq when it goes down, I
>>>>> think it
>>>>> looks a generic enough problem and so better fix core for that. Probably
>>>>> with
>>>>> help of flags field/suspend-freq (maybe renamed) and without calling
>>>>> drivers
>>>>> exit() at all..
>>>>
>>>>
>>>> ATM the only thing that needs to be done in this case is to allow
>>>> intel_pstate
>>>> to set the P state on the core when it is going done.  My solution from the
>>>> cores point of view is more generic, it allows any driver that needs to do
>>>> work
>>>> during CPU_DOWN_PREPARE to do it without adding any new logic to the core.
>>>
>>> Yeah, do we really need to give that freedom right now? Would be better
>>> to get this into core as that would be more generic and people looking to set
>>> core to some freq at shutdown wouldn't be replicating that code.
> 
> Question is if it needs to be more generic.
> 
> I honestly don't think that ->target drivers will ever do anything like it,
> because they need the governor to "exit" before.  So we are talking about the
> only two ->setpolicy drivers in the tree here.
> 
>> IMHO yes and it would be hard to be more generic, if your platform needs to
>> do architecture specific during the PREPARE phase of cpu hotplug use this
>> callback or not.
>>
>> BTW now that you have added a path where the cpufreq_suspend() could fail
>> it return a value and be checked in dpm_suspend() instead of printing an
>> error and just continuing.
> 
> I'm not sure what you mean?  Are you saying that it might be a good idea
> to allow cpufreq_suspend() to return error codes on failure?
> 

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

[Viresh Kumar]

On 18 March 2014 17:46, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> Agreed. As far as I understand, for ->target drivers, today we use GOV_STOP
> to stop managing the CPU going offline. And for ->setpolicy drivers, we will
> use this new callback to achieve the same goal.

So a better question would be: What's the purpose of ->stop() call for a policy?
Stop managing CPUs of that policy? Or even do something on CPUs of a policy
before CPUs are offlined?

Probably in the current solution Dirk is doing both these things..

And so I thought maybe its better not to restrict ->stop() to just
setpolicy() drivers.

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

[Srivatsa S. Bhat]

On 03/19/2014 10:33 AM, Viresh Kumar wrote:
> On 18 March 2014 17:46, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> Agreed. As far as I understand, for ->target drivers, today we use GOV_STOP
>> to stop managing the CPU going offline. And for ->setpolicy drivers, we will
>> use this new callback to achieve the same goal.
> 
> So a better question would be: What's the purpose of ->stop() call for a policy?

Ideally, it should remove the outgoing CPU from the policy and "stop managing
that CPU", whatever that means to the driver (for intel_pstate, it means
setting it to min P state and destroying the timer).

> Stop managing CPUs of that policy?

Stop managing only the particular CPU going offline. IOW, we should somehow
communicate to the ->stop() callback that we are taking CPU 'x' offline.

If adding a ->stop() callback in the cpufreq_driver is not the best way to
achieve it, then lets think of an alternative. The way I look at it, this
new mechanism what we want, should allow ->setpolicy drivers to do what the
GOV_STOP will do for regular drivers. That is, allow it to "shutdown the
CPU from a cpufreq perspective", whatever that means to the driver.
We can think of a completely different way of achieving it, if ->stop()
is not suitable for that purpose.

> Or even do something on CPUs of a policy
> before CPUs are offlined?
> 
> Probably in the current solution Dirk is doing both these things..
> 
> And so I thought maybe its better not to restrict ->stop() to just
> setpolicy() drivers.


Regards,
Srivatsa S. Bhat

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

[Rafael J. Wysocki]

On Wednesday, March 19, 2014 03:30:48 PM Srivatsa S. Bhat wrote:
> On 03/19/2014 10:33 AM, Viresh Kumar wrote:
> > On 18 March 2014 17:46, Srivatsa S. Bhat
> > <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> >> Agreed. As far as I understand, for ->target drivers, today we use GOV_STOP
> >> to stop managing the CPU going offline. And for ->setpolicy drivers, we will
> >> use this new callback to achieve the same goal.
> > 
> > So a better question would be: What's the purpose of ->stop() call for a policy?
> 
> Ideally, it should remove the outgoing CPU from the policy and "stop managing
> that CPU", whatever that means to the driver (for intel_pstate, it means
> setting it to min P state and destroying the timer).
> 
> > Stop managing CPUs of that policy?
> 
> Stop managing only the particular CPU going offline. IOW, we should somehow
> communicate to the ->stop() callback that we are taking CPU 'x' offline.
> 
> If adding a ->stop() callback in the cpufreq_driver is not the best way to
> achieve it, then lets think of an alternative. The way I look at it, this
> new mechanism what we want, should allow ->setpolicy drivers to do what the
> GOV_STOP will do for regular drivers. That is, allow it to "shutdown the
> CPU from a cpufreq perspective", whatever that means to the driver.
> We can think of a completely different way of achieving it, if ->stop()
> is not suitable for that purpose.

I agree.

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.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Viresh Kumar]

On 19 March 2014 19:49, Rafael J. Wysocki <rjw@rjwysocki.net> 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

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

[Viresh Kumar]

Hi,

It was a long weekend in India due to some holidays and so couldn't reply.

On Fri, Mar 14, 2014 at 11:59 PM, Dirk Brandewie
<dirk.brandewie@gmail.com> wrote:
> On 03/14/2014 10:07 AM, Viresh Kumar wrote:
> Suspend and hotplug are two very different things and if we start
> crossing those wires bad things are going to happen IMHO.
>
> In "normal" operation using the suspend path to do this work could
> work in principal but doesn't handle the case where the user does
>    echo 0 | sudo tee /sys/devices/system/cpu/cpuX/online
>
> Trying force hotplug and suspend into a common mechanism would
> lead to a bunch of special case code or a significant rework of the
> core code IMHO.

What you said is correct, we shouldn't do it. But what I am asking for
is a bit different. The stuff we are doing in core on system suspend
isn't actually related to suspend but only CPU online/offline.

There are platforms which want to set CPUs to a particular frequency
before they are taken out by disable_nonboot_cpus. And then there
are platforms which want to do similar thing when CPUs are taken
down with help of sysfs files. But there is a common baseline there:
Set CPUs to a particular P-state before they are taken down.

And so I wanted to keep a common solution for both these requirements.

> This is guaranteed by the hardware.  Each core has its own MSR for P state
> request.  Any coordination that is required between cores to select the
> package P state is handled by the hardware.

I see.. Let me send some patches which I have in my mind and then we can
decide which set looks more reasonable :)

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

[Rafael J. Wysocki]

On Thursday, September 04, 2014 04:07:28 PM Viresh Kumar wrote:
> On 4 September 2014 15:33, Preeti U Murthy <preeti@linux.vnet.ibm.com> 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.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.