[RFC v0 2/9] suspend: Add getter function to report if freezing is active

[RFC v0 2/9] suspend: Add getter function to report if freezing is active

[Daniel Wagner]

Instead encode the FREEZE state via the CPU state we allow the
interesting subsystems (MCE, microcode) to query the power
subsystem directly. Most notifiers are not interested at all
in this information so rather have explicit calls to freeze_active()
instead adding complexity to the rest of the users of the CPU
notifiers.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <len.brown@intel.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 include/linux/suspend.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 5efe743..5e15ade 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -216,6 +216,11 @@ static inline bool idle_should_freeze(void)
 	return unlikely(suspend_freeze_state == FREEZE_STATE_ENTER);
 }
 
+static inline bool freeze_active(void)
+{
+	return unlikely(suspend_freeze_state != FREEZE_STATE_NONE);
+}
+
 extern void freeze_set_ops(const struct platform_freeze_ops *ops);
 extern void freeze_wake(void);
 
@@ -244,6 +249,7 @@ extern int pm_suspend(suspend_state_t state);
 static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
 static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
 static inline bool idle_should_freeze(void) { return false; }
+static inline bool freeze_active(void) { return false; }
 static inline void freeze_set_ops(const struct platform_freeze_ops *ops) {}
 static inline void freeze_wake(void) {}
 #endif /* !CONFIG_SUSPEND */
-- 
2.4.3

Re: [RFC v0 2/9] suspend: Add getter function to report if freezing is active

[Rafael J. Wysocki]

On Friday, September 04, 2015 03:34:55 PM Daniel Wagner wrote:
> Instead encode the FREEZE state via the CPU state we allow the
> interesting subsystems (MCE, microcode) to query the power
> subsystem directly.

A use case, please.

> Most notifiers are not interested at all
> in this information so rather have explicit calls to freeze_active()
> instead adding complexity to the rest of the users of the CPU
> notifiers.

Why does it has anything to do with CPU notifiers?  We don't offline
CPUs for suspend-to-idle.


> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: linux-pm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  include/linux/suspend.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 5efe743..5e15ade 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -216,6 +216,11 @@ static inline bool idle_should_freeze(void)
>  	return unlikely(suspend_freeze_state == FREEZE_STATE_ENTER);
>  }
>  
> +static inline bool freeze_active(void)
> +{
> +	return unlikely(suspend_freeze_state != FREEZE_STATE_NONE);
> +}
> +
>  extern void freeze_set_ops(const struct platform_freeze_ops *ops);
>  extern void freeze_wake(void);
>  
> @@ -244,6 +249,7 @@ extern int pm_suspend(suspend_state_t state);
>  static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
>  static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
>  static inline bool idle_should_freeze(void) { return false; }
> +static inline bool freeze_active(void) { return false; }
>  static inline void freeze_set_ops(const struct platform_freeze_ops *ops) {}
>  static inline void freeze_wake(void) {}
>  #endif /* !CONFIG_SUSPEND */
> 

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

Re: [RFC v0 2/9] suspend: Add getter function to report if freezing is active

[Daniel Wagner]

On 09/05/2015 04:11 AM, Rafael J. Wysocki wrote:
> On Friday, September 04, 2015 03:34:55 PM Daniel Wagner wrote:
>> Instead encode the FREEZE state via the CPU state we allow the
>> interesting subsystems (MCE, microcode) to query the power
>> subsystem directly.
> 
> A use case, please.

The motivation for this change is to reduce the complexity in the
hotplug code. As tried to point out in the cover letter, the FROZEN
bits have only a bunch of users after all those years (2007). So it is
worth to have all the notifier users to handle the FROZEN state?

Don't know if that counts as use case.

>> Most notifiers are not interested at all
>> in this information so rather have explicit calls to freeze_active()
>> instead adding complexity to the rest of the users of the CPU
>> notifiers.
> 
> Why does it has anything to do with CPU notifiers?

cpu_{down|up} will call the notifiers with the CPU_TASK_FROZEN bit set
and so most notifiers are doing

	switch (actcion ~CPU_TASK_FROZEN)

to filter it out because they don't need to handle the system wide
ongoing freeze operations.

> We don't offline CPUs for suspend-to-idle.

Sure. As I said the motivation is to reduce the complexity in the
hotplug code.

Thanks,
Daniel

Re: [RFC v0 2/9] suspend: Add getter function to report if freezing is active

[Rafael J. Wysocki]

On Monday, September 07, 2015 10:55:43 AM Daniel Wagner wrote:
> On 09/05/2015 04:11 AM, Rafael J. Wysocki wrote:
> > On Friday, September 04, 2015 03:34:55 PM Daniel Wagner wrote:
> >> Instead encode the FREEZE state via the CPU state we allow the
> >> interesting subsystems (MCE, microcode) to query the power
> >> subsystem directly.
> > 
> > A use case, please.
> 
> The motivation for this change is to reduce the complexity in the
> hotplug code. As tried to point out in the cover letter, the FROZEN
> bits have only a bunch of users after all those years (2007). So it is
> worth to have all the notifier users to handle the FROZEN state?
> 
> Don't know if that counts as use case.
> 
> >> Most notifiers are not interested at all
> >> in this information so rather have explicit calls to freeze_active()
> >> instead adding complexity to the rest of the users of the CPU
> >> notifiers.
> > 
> > Why does it has anything to do with CPU notifiers?
> 
> cpu_{down|up} will call the notifiers with the CPU_TASK_FROZEN bit set
> and so most notifiers are doing
> 
> 	switch (actcion ~CPU_TASK_FROZEN)
> 
> to filter it out because they don't need to handle the system wide
> ongoing freeze operations.
> 
> > We don't offline CPUs for suspend-to-idle.
> 
> Sure. As I said the motivation is to reduce the complexity in the
> hotplug code.

Well, it looks like I confused two things.

Let me look at this again.

Thanks,
Rafael

Re: [RFC v0 2/9] suspend: Add getter function to report if freezing is active

[Rafael J. Wysocki]

On Monday, September 07, 2015 10:55:43 AM Daniel Wagner wrote:
> On 09/05/2015 04:11 AM, Rafael J. Wysocki wrote:
> > On Friday, September 04, 2015 03:34:55 PM Daniel Wagner wrote:
> >> Instead encode the FREEZE state via the CPU state we allow the
> >> interesting subsystems (MCE, microcode) to query the power
> >> subsystem directly.
> > 
> > A use case, please.
> 
> The motivation for this change is to reduce the complexity in the
> hotplug code. As tried to point out in the cover letter, the FROZEN
> bits have only a bunch of users after all those years (2007). So it is
> worth to have all the notifier users to handle the FROZEN state?
> 
> Don't know if that counts as use case.

Well, the code you're changing has nothing to do with CPU hotplug and
CPU_TASKS_FROZEN.  It is about suspend-to-idle.

Please grep for suspend_freeze_state and see what it is used for.

There is some confusion in the naming, but that is about the freezing of
the whole system, while CPU_TASKS_FROZEN is about the freezing of user space.

> >> Most notifiers are not interested at all
> >> in this information so rather have explicit calls to freeze_active()
> >> instead adding complexity to the rest of the users of the CPU
> >> notifiers.
> > 
> > Why does it has anything to do with CPU notifiers?
> 
> cpu_{down|up} will call the notifiers with the CPU_TASK_FROZEN bit set
> and so most notifiers are doing
> 
> 	switch (actcion ~CPU_TASK_FROZEN)
> 
> to filter it out because they don't need to handle the system wide
> ongoing freeze operations.
> 
> > We don't offline CPUs for suspend-to-idle.
> 
> Sure. As I said the motivation is to reduce the complexity in the
> hotplug code.

You need to fine a different way to do that.

Thanks,
Rafael

Re: [RFC v0 2/9] suspend: Add getter function to report if freezing is active

[Daniel Wagner]

On 09/07/2015 11:44 PM, Rafael J. Wysocki wrote:
> On Monday, September 07, 2015 10:55:43 AM Daniel Wagner wrote:
>> On 09/05/2015 04:11 AM, Rafael J. Wysocki wrote:
>>> On Friday, September 04, 2015 03:34:55 PM Daniel Wagner wrote:
>>>> Instead encode the FREEZE state via the CPU state we allow the
>>>> interesting subsystems (MCE, microcode) to query the power
>>>> subsystem directly.
>>>
>>> A use case, please.
>>
>> The motivation for this change is to reduce the complexity in the
>> hotplug code. As tried to point out in the cover letter, the FROZEN
>> bits have only a bunch of users after all those years (2007). So it is
>> worth to have all the notifier users to handle the FROZEN state?
>>
>> Don't know if that counts as use case.
> 
> Well, the code you're changing has nothing to do with CPU hotplug and
> CPU_TASKS_FROZEN.  It is about suspend-to-idle.
> 
> Please grep for suspend_freeze_state and see what it is used for.
> 
> There is some confusion in the naming, but that is about the freezing of
> the whole system, while CPU_TASKS_FROZEN is about the freezing of user space.

You are right. I got confused by all those frozen/freezing naming scheme.

>>>> Most notifiers are not interested at all
>>>> in this information so rather have explicit calls to freeze_active()
>>>> instead adding complexity to the rest of the users of the CPU
>>>> notifiers.
>>>
>>> Why does it has anything to do with CPU notifiers?
>>
>> cpu_{down|up} will call the notifiers with the CPU_TASK_FROZEN bit set
>> and so most notifiers are doing
>>
>> 	switch (actcion ~CPU_TASK_FROZEN)
>>
>> to filter it out because they don't need to handle the system wide
>> ongoing freeze operations.
>>
>>> We don't offline CPUs for suspend-to-idle.
>>
>> Sure. As I said the motivation is to reduce the complexity in the
>> hotplug code.
> 
> You need to fine a different way to do that.

I'll try something else.

Thanks for taking the time explaining!

cheers,
Daniel