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

Groups > linux.kernel > #1362266 > unrolled thread

[PATCH v6 6/7][Resend] cpufreq: Support for fast frequency switching

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency switching "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-03-22 03:00 +0100
    Re: [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency  switching Steve Muckle <steve.muckle@linaro.org> - 2016-03-26 02:20 +0100
      Re: [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency switching "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-26 02:50 +0100
        Re: [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency switching "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-27 03:30 +0200
        Re: [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency  switching Steve Muckle <steve.muckle@linaro.org> - 2016-03-28 18:50 +0200
          Re: [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency switching "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-03-29 14:10 +0200
    Re: [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency  switching Viresh Kumar <viresh.kumar@linaro.org> - 2016-03-28 08:30 +0200
      Re: [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency switching "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-03-29 14:30 +0200
    Re: [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency  switching Viresh Kumar <viresh.kumar@linaro.org> - 2016-03-28 09:10 +0200
      Re: [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency switching "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-03-29 14:10 +0200
        Re: [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency  switching Viresh Kumar <viresh.kumar@linaro.org> - 2016-03-29 16:30 +0200
    [Update][PATCH v7 6/7] cpufreq: Support for fast frequency switching "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-03-30 03:50 +0200
      Re: [Update][PATCH v7 6/7] cpufreq: Support for fast frequency  switching Viresh Kumar <viresh.kumar@linaro.org> - 2016-03-30 07:10 +0200
        Re: [Update][PATCH v7 6/7] cpufreq: Support for fast frequency switching "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-30 13:30 +0200

#1362266 — [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency switching

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Modify the ACPI cpufreq driver to provide a method for switching
CPU frequencies from interrupt context and update the cpufreq core
to support that method if available.

Introduce a new cpufreq driver callback, ->fast_switch, to be
invoked for frequency switching from interrupt context by (future)
governors supporting that feature via (new) helper function
cpufreq_driver_fast_switch().

Add two new policy flags, fast_switch_possible, to be set by the
cpufreq driver if fast frequency switching can be used for the
given policy and fast_switch_enabled, to be set by the governor
if it is going to use fast frequency switching for the given
policy.  Also add a helper for setting the latter.

Since fast frequency switching is inherently incompatible with
cpufreq transition notifiers, make it possible to set the
fast_switch_enabled only if there are no transition notifiers
already registered and make the registration of new transition
notifiers fail if fast_switch_enabled is set for at least one
policy.

Implement the ->fast_switch callback in the ACPI cpufreq driver
and make it set fast_switch_possible during policy initialization
as appropriate.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Changes from v5:
- cpufreq_enable_fast_switch() fixed to avoid printing a confusing message
  if fast_switch_possible is not set for the policy.
- Fixed a typo in that message.
- Removed the WARN_ON() from the (cpufreq_fast_switch_count > 0) check in
  cpufreq_register_notifier(), because it triggered false-positive warnings
  from the cpufreq_stats module (cpufreq_stats don't work with the fast
  switching, because it is based on notifiers).

Changes from v4:
- If cpufreq_enable_fast_switch() is about to fail, it will print the list
  of currently registered transition notifiers.
- Added lock_assert_held(&policy->rwsem) to cpufreq_enable_fast_switch().
- Added WARN_ON() to the (cpufreq_fast_switch_count > 0) check in
  cpufreq_register_notifier().
- Modified the kerneldoc comment of cpufreq_driver_fast_switch() to
  mention the RELATION_L expectation regarding the ->fast_switch callback.

Changes from v3:
- New fast_switch_enabled field in struct cpufreq_policy to help
  avoid affecting existing setups by setting the fast_switch_possible
  flag in the driver.
- __cpufreq_get() skips the policy->cur check if fast_switch_enabled is set.

Changes from v2:
- The driver ->fast_switch callback and cpufreq_driver_fast_switch()
  don't need the relation argument as they will always do RELATION_L now.
- New mechanism to make fast switch and cpufreq notifiers mutually
  exclusive.
- cpufreq_driver_fast_switch() doesn't do anything in addition to
  invoking the driver callback and returns its return value.

---
 drivers/cpufreq/acpi-cpufreq.c |   41 ++++++++++++
 drivers/cpufreq/cpufreq.c      |  130 ++++++++++++++++++++++++++++++++++++++---
 include/linux/cpufreq.h        |    9 ++
 3 files changed, 171 insertions(+), 9 deletions(-)

Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/acpi-cpufreq.c
+++ linux-pm/drivers/cpufreq/acpi-cpufreq.c
@@ -458,6 +458,43 @@ static int acpi_cpufreq_target(struct cp
 	return result;
 }
 
+unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
+				      unsigned int target_freq)
+{
+	struct acpi_cpufreq_data *data = policy->driver_data;
+	struct acpi_processor_performance *perf;
+	struct cpufreq_frequency_table *entry;
+	unsigned int next_perf_state, next_freq, freq;
+
+	/*
+	 * Find the closest frequency above target_freq.
+	 *
+	 * The table is sorted in the reverse order with respect to the
+	 * frequency and all of the entries are valid (see the initialization).
+	 */
+	entry = data->freq_table;
+	do {
+		entry++;
+		freq = entry->frequency;
+	} while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
+	entry--;
+	next_freq = entry->frequency;
+	next_perf_state = entry->driver_data;
+
+	perf = to_perf_data(data);
+	if (perf->state == next_perf_state) {
+		if (unlikely(data->resume))
+			data->resume = 0;
+		else
+			return next_freq;
+	}
+
+	data->cpu_freq_write(&perf->control_register,
+			     perf->states[next_perf_state].control);
+	perf->state = next_perf_state;
+	return next_freq;
+}
+
 static unsigned long
 acpi_cpufreq_guess_freq(struct acpi_cpufreq_data *data, unsigned int cpu)
 {
@@ -740,6 +777,9 @@ static int acpi_cpufreq_cpu_init(struct
 		goto err_unreg;
 	}
 
+	policy->fast_switch_possible = !acpi_pstate_strict &&
+		!(policy_is_shared(policy) && policy->shared_type != CPUFREQ_SHARED_TYPE_ANY);
+
 	data->freq_table = kzalloc(sizeof(*data->freq_table) *
 		    (perf->state_count+1), GFP_KERNEL);
 	if (!data->freq_table) {
@@ -874,6 +914,7 @@ static struct freq_attr *acpi_cpufreq_at
 static struct cpufreq_driver acpi_cpufreq_driver = {
 	.verify		= cpufreq_generic_frequency_table_verify,
 	.target_index	= acpi_cpufreq_target,
+	.fast_switch	= acpi_cpufreq_fast_switch,
 	.bios_limit	= acpi_processor_get_bios_limit,
 	.init		= acpi_cpufreq_cpu_init,
 	.exit		= acpi_cpufreq_cpu_exit,
Index: linux-pm/include/linux/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/cpufreq.h
+++ linux-pm/include/linux/cpufreq.h
@@ -102,6 +102,10 @@ struct cpufreq_policy {
 	 */
 	struct rw_semaphore	rwsem;
 
+	/* Fast switch flags */
+	bool			fast_switch_possible;	/* Set by the driver. */
+	bool			fast_switch_enabled;
+
 	/* Synchronization for frequency transitions */
 	bool			transition_ongoing; /* Tracks transition status */
 	spinlock_t		transition_lock;
@@ -156,6 +160,7 @@ int cpufreq_get_policy(struct cpufreq_po
 int cpufreq_update_policy(unsigned int cpu);
 bool have_governor_per_policy(void);
 struct kobject *get_governor_parent_kobj(struct cpufreq_policy *policy);
+void cpufreq_enable_fast_switch(struct cpufreq_policy *policy);
 #else
 static inline unsigned int cpufreq_get(unsigned int cpu)
 {
@@ -236,6 +241,8 @@ struct cpufreq_driver {
 				  unsigned int relation);	/* Deprecated */
 	int		(*target_index)(struct cpufreq_policy *policy,
 					unsigned int index);
+	unsigned int	(*fast_switch)(struct cpufreq_policy *policy,
+				       unsigned int target_freq);
 	/*
 	 * Only for drivers with target_index() and CPUFREQ_ASYNC_NOTIFICATION
 	 * unset.
@@ -464,6 +471,8 @@ struct cpufreq_governor {
 };
 
 /* Pass a target to the cpufreq driver */
+unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
+					unsigned int target_freq);
 int cpufreq_driver_target(struct cpufreq_policy *policy,
 				 unsigned int target_freq,
 				 unsigned int relation);
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -428,6 +428,57 @@ void cpufreq_freq_transition_end(struct
 }
 EXPORT_SYMBOL_GPL(cpufreq_freq_transition_end);
 
+/*
+ * Fast frequency switching status count.  Positive means "enabled", negative
+ * means "disabled" and 0 means "not decided yet".
+ */
+static int cpufreq_fast_switch_count;
+static DEFINE_MUTEX(cpufreq_fast_switch_lock);
+
+static void cpufreq_list_transition_notifiers(void)
+{
+	struct notifier_block *nb;
+
+	pr_info("cpufreq: Registered transition notifiers:\n");
+
+	mutex_lock(&cpufreq_transition_notifier_list.mutex);
+
+	for (nb = cpufreq_transition_notifier_list.head; nb; nb = nb->next)
+		pr_info("cpufreq: %pF\n", nb->notifier_call);
+
+	mutex_unlock(&cpufreq_transition_notifier_list.mutex);
+}
+
+/**
+ * cpufreq_enable_fast_switch - Enable fast frequency switching for policy.
+ * @policy: cpufreq policy to enable fast frequency switching for.
+ *
+ * Try to enable fast frequency switching for @policy.
+ *
+ * The attempt will fail if there is at least one transition notifier registered
+ * at this point, as fast frequency switching is quite fundamentally at odds
+ * with transition notifiers.  Thus if successful, it will make registration of
+ * transition notifiers fail going forward.
+ */
+void cpufreq_enable_fast_switch(struct cpufreq_policy *policy)
+{
+	lockdep_assert_held(&policy->rwsem);
+
+	if (!policy->fast_switch_possible)
+		return;
+
+	mutex_lock(&cpufreq_fast_switch_lock);
+	if (cpufreq_fast_switch_count >= 0) {
+		cpufreq_fast_switch_count++;
+		policy->fast_switch_enabled = true;
+	} else {
+		pr_warn("cpufreq: CPU%u: Fast frequency switching not enabled\n",
+			policy->cpu);
+		cpufreq_list_transition_notifiers();
+	}
+	mutex_unlock(&cpufreq_fast_switch_lock);
+}
+EXPORT_SYMBOL_GPL(cpufreq_enable_fast_switch);
 
 /*********************************************************************
  *                          SYSFS INTERFACE                          *
@@ -1083,6 +1134,24 @@ static void cpufreq_policy_free(struct c
 	kfree(policy);
 }
 
+static void cpufreq_driver_exit_policy(struct cpufreq_policy *policy)
+{
+	if (policy->fast_switch_enabled) {
+		mutex_lock(&cpufreq_fast_switch_lock);
+
+		policy->fast_switch_enabled = false;
+		if (!WARN_ON(cpufreq_fast_switch_count <= 0))
+			cpufreq_fast_switch_count--;
+
+		mutex_unlock(&cpufreq_fast_switch_lock);
+	}
+
+	if (cpufreq_driver->exit) {
+		cpufreq_driver->exit(policy);
+		policy->freq_table = NULL;
+	}
+}
+
 static int cpufreq_online(unsigned int cpu)
 {
 	struct cpufreq_policy *policy;
@@ -1236,8 +1305,7 @@ static int cpufreq_online(unsigned int c
 out_exit_policy:
 	up_write(&policy->rwsem);
 
-	if (cpufreq_driver->exit)
-		cpufreq_driver->exit(policy);
+	cpufreq_driver_exit_policy(policy);
 out_free_policy:
 	cpufreq_policy_free(policy, !new_policy);
 	return ret;
@@ -1334,10 +1402,7 @@ static void cpufreq_offline(unsigned int
 	 * since this is a core component, and is essential for the
 	 * subsequent light-weight ->init() to succeed.
 	 */
-	if (cpufreq_driver->exit) {
-		cpufreq_driver->exit(policy);
-		policy->freq_table = NULL;
-	}
+	cpufreq_driver_exit_policy(policy);
 
 unlock:
 	up_write(&policy->rwsem);
@@ -1452,8 +1517,12 @@ static unsigned int __cpufreq_get(struct
 
 	ret_freq = cpufreq_driver->get(policy->cpu);
 
-	/* Updating inactive policies is invalid, so avoid doing that. */
-	if (unlikely(policy_is_inactive(policy)))
+	/*
+	 * Updating inactive policies is invalid, so avoid doing that.  Also
+	 * if fast frequency switching is used with the given policy, the check
+	 * against policy->cur is pointless, so skip it in that case too.
+	 */
+	if (unlikely(policy_is_inactive(policy)) || policy->fast_switch_enabled)
 		return ret_freq;
 
 	if (ret_freq && policy->cur &&
@@ -1465,7 +1534,6 @@ static unsigned int __cpufreq_get(struct
 			schedule_work(&policy->update);
 		}
 	}
-
 	return ret_freq;
 }
 
@@ -1672,8 +1740,18 @@ int cpufreq_register_notifier(struct not
 
 	switch (list) {
 	case CPUFREQ_TRANSITION_NOTIFIER:
+		mutex_lock(&cpufreq_fast_switch_lock);
+
+		if (cpufreq_fast_switch_count > 0) {
+			mutex_unlock(&cpufreq_fast_switch_lock);
+			return -EBUSY;
+		}
 		ret = srcu_notifier_chain_register(
 				&cpufreq_transition_notifier_list, nb);
+		if (!ret)
+			cpufreq_fast_switch_count--;
+
+		mutex_unlock(&cpufreq_fast_switch_lock);
 		break;
 	case CPUFREQ_POLICY_NOTIFIER:
 		ret = blocking_notifier_chain_register(
@@ -1706,8 +1784,14 @@ int cpufreq_unregister_notifier(struct n
 
 	switch (list) {
 	case CPUFREQ_TRANSITION_NOTIFIER:
+		mutex_lock(&cpufreq_fast_switch_lock);
+
 		ret = srcu_notifier_chain_unregister(
 				&cpufreq_transition_notifier_list, nb);
+		if (!ret && !WARN_ON(cpufreq_fast_switch_count >= 0))
+			cpufreq_fast_switch_count++;
+
+		mutex_unlock(&cpufreq_fast_switch_lock);
 		break;
 	case CPUFREQ_POLICY_NOTIFIER:
 		ret = blocking_notifier_chain_unregister(
@@ -1726,6 +1810,34 @@ EXPORT_SYMBOL(cpufreq_unregister_notifie
  *                              GOVERNORS                            *
  *********************************************************************/
 
+/**
+ * cpufreq_driver_fast_switch - Carry out a fast CPU frequency switch.
+ * @policy: cpufreq policy to switch the frequency for.
+ * @target_freq: New frequency to set (may be approximate).
+ *
+ * Carry out a fast frequency switch from interrupt context.
+ *
+ * The driver's ->fast_switch() callback invoked by this function is expected to
+ * select the minimum available frequency greater than or equal to @target_freq
+ * (CPUFREQ_RELATION_L).
+ *
+ * This function must not be called if policy->fast_switch_enabled is unset.
+ *
+ * Governors calling this function must guarantee that it will never be invoked
+ * twice in parallel for the same policy and that it will never be called in
+ * parallel with either ->target() or ->target_index() for the same policy.
+ *
+ * If CPUFREQ_ENTRY_INVALID is returned by the driver's ->fast_switch()
+ * callback to indicate an error condition, the hardware configuration must be
+ * preserved.
+ */
+unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
+					unsigned int target_freq)
+{
+	return cpufreq_driver->fast_switch(policy, target_freq);
+}
+EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);
+
 /* Must set freqs->new to intermediate frequency */
 static int __target_intermediate(struct cpufreq_policy *policy,
 				 struct cpufreq_freqs *freqs, int index)

[toc] | [next] | [standalone]

#1364891 — Re: [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency switching

Hi Rafael,

On 03/21/2016 06:53 PM, Rafael J. Wysocki wrote:
> Add two new policy flags, fast_switch_possible, to be set by the
> cpufreq driver if fast frequency switching can be used for the
> given policy and fast_switch_enabled, to be set by the governor
> if it is going to use fast frequency switching for the given
> policy.  Also add a helper for setting the latter.
...
> @@ -740,6 +777,9 @@ static int acpi_cpufreq_cpu_init(struct
>  		goto err_unreg;
>  	}
>  
> +	policy->fast_switch_possible = !acpi_pstate_strict &&
> +		!(policy_is_shared(policy) && policy->shared_type != CPUFREQ_SHARED_TYPE_ANY);

Could the policy->fast_switch_possible flag be avoided by just checking
whether a driver has registered the .fast_switch callback?

...
> @@ -1726,6 +1810,34 @@ EXPORT_SYMBOL(cpufreq_unregister_notifie
>   *                              GOVERNORS                            *
>   *********************************************************************/
>  
> +/**
> + * cpufreq_driver_fast_switch - Carry out a fast CPU frequency switch.
> + * @policy: cpufreq policy to switch the frequency for.
> + * @target_freq: New frequency to set (may be approximate).
> + *
> + * Carry out a fast frequency switch from interrupt context.

I think that should say atomic rather than interrupt as this might not
be called from interrupt context.

thanks,
Steve

[toc] | [prev] | [next] | [standalone]

#1364893

On Sat, Mar 26, 2016 at 2:12 AM, Steve Muckle <steve.muckle@linaro.org> wrote:
> Hi Rafael,
>
> On 03/21/2016 06:53 PM, Rafael J. Wysocki wrote:
>> Add two new policy flags, fast_switch_possible, to be set by the
>> cpufreq driver if fast frequency switching can be used for the
>> given policy and fast_switch_enabled, to be set by the governor
>> if it is going to use fast frequency switching for the given
>> policy.  Also add a helper for setting the latter.
> ...
>> @@ -740,6 +777,9 @@ static int acpi_cpufreq_cpu_init(struct
>>               goto err_unreg;
>>       }
>>
>> +     policy->fast_switch_possible = !acpi_pstate_strict &&
>> +             !(policy_is_shared(policy) && policy->shared_type != CPUFREQ_SHARED_TYPE_ANY);
>
> Could the policy->fast_switch_possible flag be avoided by just checking
> whether a driver has registered the .fast_switch callback?

No, it couldn't.

As in this case, the driver has the ->fast_switch callback, but it
can't be used for policies that don't satisfy the above condition.  At
the same time it may be possible to use it for other policies on the
same system in principle.

> ...
>> @@ -1726,6 +1810,34 @@ EXPORT_SYMBOL(cpufreq_unregister_notifie
>>   *                              GOVERNORS                            *
>>   *********************************************************************/
>>
>> +/**
>> + * cpufreq_driver_fast_switch - Carry out a fast CPU frequency switch.
>> + * @policy: cpufreq policy to switch the frequency for.
>> + * @target_freq: New frequency to set (may be approximate).
>> + *
>> + * Carry out a fast frequency switch from interrupt context.
>
> I think that should say atomic rather than interrupt as this might not
> be called from interrupt context.

"Interrupt context" here means something like "context that cannot
sleep" and it's sort of a traditional way of calling that.  I
considered saying "atomic context" here, but then decided that it
might suggest too much.

Maybe something like "Carry out a fast frequency switch without
sleeping" would be better?

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1365035

On Sat, Mar 26, 2016 at 2:46 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Sat, Mar 26, 2016 at 2:12 AM, Steve Muckle <steve.muckle@linaro.org> wrote:
>> Hi Rafael,
>>
>> On 03/21/2016 06:53 PM, Rafael J. Wysocki wrote:
>>> Add two new policy flags, fast_switch_possible, to be set by the
>>> cpufreq driver if fast frequency switching can be used for the
>>> given policy and fast_switch_enabled, to be set by the governor
>>> if it is going to use fast frequency switching for the given
>>> policy.  Also add a helper for setting the latter.
>> ...
>>> @@ -740,6 +777,9 @@ static int acpi_cpufreq_cpu_init(struct
>>>               goto err_unreg;
>>>       }
>>>
>>> +     policy->fast_switch_possible = !acpi_pstate_strict &&
>>> +             !(policy_is_shared(policy) && policy->shared_type != CPUFREQ_SHARED_TYPE_ANY);
>>
>> Could the policy->fast_switch_possible flag be avoided by just checking
>> whether a driver has registered the .fast_switch callback?
>
> No, it couldn't.
>
> As in this case, the driver has the ->fast_switch callback, but it
> can't be used for policies that don't satisfy the above condition.  At
> the same time it may be possible to use it for other policies on the
> same system in principle.

In fact, for fast switching to be useful, the driver has to guarantee
that frequency can be updated on any of the policy CPUs (and it
doesn't matter which of them updates the frequency) and that's what
the fast_switch_possible flag is really for.  I guess I should add a
comment to that effect to its definition.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1365581 — Re: [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency switching

On 03/25/2016 06:46 PM, Rafael J. Wysocki wrote:
>>> @@ -1726,6 +1810,34 @@ EXPORT_SYMBOL(cpufreq_unregister_notifie
>>> >>   *                              GOVERNORS                            *
>>> >>   *********************************************************************/
>>> >>
>>> >> +/**
>>> >> + * cpufreq_driver_fast_switch - Carry out a fast CPU frequency switch.
>>> >> + * @policy: cpufreq policy to switch the frequency for.
>>> >> + * @target_freq: New frequency to set (may be approximate).
>>> >> + *
>>> >> + * Carry out a fast frequency switch from interrupt context.
>> >
>> > I think that should say atomic rather than interrupt as this might not
>> > be called from interrupt context.
>
> "Interrupt context" here means something like "context that cannot
> sleep" and it's sort of a traditional way of calling that.  I
> considered saying "atomic context" here, but then decided that it
> might suggest too much.
> 
> Maybe something like "Carry out a fast frequency switch without
> sleeping" would be better?

Yes I do think that's preferable. I also wonder if it makes sense to
state expectations of how long the operation should take - i.e. not only
will it not sleep, but it is expected to complete "quickly." However I
accept that it is not well defined what that means. Maybe a mention that
this may be called in scheduler hot paths.

[toc] | [prev] | [next] | [standalone]

#1366203

On Monday, March 28, 2016 09:47:53 AM Steve Muckle wrote:
> On 03/25/2016 06:46 PM, Rafael J. Wysocki wrote:
> >>> @@ -1726,6 +1810,34 @@ EXPORT_SYMBOL(cpufreq_unregister_notifie
> >>> >>   *                              GOVERNORS                            *
> >>> >>   *********************************************************************/
> >>> >>
> >>> >> +/**
> >>> >> + * cpufreq_driver_fast_switch - Carry out a fast CPU frequency switch.
> >>> >> + * @policy: cpufreq policy to switch the frequency for.
> >>> >> + * @target_freq: New frequency to set (may be approximate).
> >>> >> + *
> >>> >> + * Carry out a fast frequency switch from interrupt context.
> >> >
> >> > I think that should say atomic rather than interrupt as this might not
> >> > be called from interrupt context.
> >
> > "Interrupt context" here means something like "context that cannot
> > sleep" and it's sort of a traditional way of calling that.  I
> > considered saying "atomic context" here, but then decided that it
> > might suggest too much.
> > 
> > Maybe something like "Carry out a fast frequency switch without
> > sleeping" would be better?
> 
> Yes I do think that's preferable. I also wonder if it makes sense to
> state expectations of how long the operation should take - i.e. not only
> will it not sleep, but it is expected to complete "quickly." However I
> accept that it is not well defined what that means. Maybe a mention that
> this may be called in scheduler hot paths.

OK

[toc] | [prev] | [next] | [standalone]

#1365346 — Re: [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency switching

Sorry for jumping in late, was busy with other stuff and travel :(

On 22-03-16, 02:53, Rafael J. Wysocki wrote:
> Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/acpi-cpufreq.c
> +++ linux-pm/drivers/cpufreq/acpi-cpufreq.c
> @@ -458,6 +458,43 @@ static int acpi_cpufreq_target(struct cp
>  	return result;
>  }
>  
> +unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
> +				      unsigned int target_freq)
> +{
> +	struct acpi_cpufreq_data *data = policy->driver_data;
> +	struct acpi_processor_performance *perf;
> +	struct cpufreq_frequency_table *entry;
> +	unsigned int next_perf_state, next_freq, freq;
> +
> +	/*
> +	 * Find the closest frequency above target_freq.
> +	 *
> +	 * The table is sorted in the reverse order with respect to the
> +	 * frequency and all of the entries are valid (see the initialization).
> +	 */
> +	entry = data->freq_table;
> +	do {
> +		entry++;
> +		freq = entry->frequency;
> +	} while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
> +	entry--;
> +	next_freq = entry->frequency;
> +	next_perf_state = entry->driver_data;
> +
> +	perf = to_perf_data(data);
> +	if (perf->state == next_perf_state) {
> +		if (unlikely(data->resume))
> +			data->resume = 0;
> +		else
> +			return next_freq;
> +	}
> +
> +	data->cpu_freq_write(&perf->control_register,
> +			     perf->states[next_perf_state].control);
> +	perf->state = next_perf_state;
> +	return next_freq;
> +}
> +
>  static unsigned long
>  acpi_cpufreq_guess_freq(struct acpi_cpufreq_data *data, unsigned int cpu)
>  {
> @@ -740,6 +777,9 @@ static int acpi_cpufreq_cpu_init(struct
>  		goto err_unreg;
>  	}
>  
> +	policy->fast_switch_possible = !acpi_pstate_strict &&
> +		!(policy_is_shared(policy) && policy->shared_type != CPUFREQ_SHARED_TYPE_ANY);
> +
>  	data->freq_table = kzalloc(sizeof(*data->freq_table) *
>  		    (perf->state_count+1), GFP_KERNEL);
>  	if (!data->freq_table) {
> @@ -874,6 +914,7 @@ static struct freq_attr *acpi_cpufreq_at
>  static struct cpufreq_driver acpi_cpufreq_driver = {
>  	.verify		= cpufreq_generic_frequency_table_verify,
>  	.target_index	= acpi_cpufreq_target,
> +	.fast_switch	= acpi_cpufreq_fast_switch,
>  	.bios_limit	= acpi_processor_get_bios_limit,
>  	.init		= acpi_cpufreq_cpu_init,
>  	.exit		= acpi_cpufreq_cpu_exit,
> Index: linux-pm/include/linux/cpufreq.h
> ===================================================================
> --- linux-pm.orig/include/linux/cpufreq.h
> +++ linux-pm/include/linux/cpufreq.h
> @@ -102,6 +102,10 @@ struct cpufreq_policy {
>  	 */
>  	struct rw_semaphore	rwsem;
>  
> +	/* Fast switch flags */
> +	bool			fast_switch_possible;	/* Set by the driver. */
> +	bool			fast_switch_enabled;
> +
>  	/* Synchronization for frequency transitions */
>  	bool			transition_ongoing; /* Tracks transition status */
>  	spinlock_t		transition_lock;
> @@ -156,6 +160,7 @@ int cpufreq_get_policy(struct cpufreq_po
>  int cpufreq_update_policy(unsigned int cpu);
>  bool have_governor_per_policy(void);
>  struct kobject *get_governor_parent_kobj(struct cpufreq_policy *policy);
> +void cpufreq_enable_fast_switch(struct cpufreq_policy *policy);
>  #else
>  static inline unsigned int cpufreq_get(unsigned int cpu)
>  {
> @@ -236,6 +241,8 @@ struct cpufreq_driver {
>  				  unsigned int relation);	/* Deprecated */
>  	int		(*target_index)(struct cpufreq_policy *policy,
>  					unsigned int index);
> +	unsigned int	(*fast_switch)(struct cpufreq_policy *policy,
> +				       unsigned int target_freq);
>  	/*
>  	 * Only for drivers with target_index() and CPUFREQ_ASYNC_NOTIFICATION
>  	 * unset.
> @@ -464,6 +471,8 @@ struct cpufreq_governor {
>  };
>  
>  /* Pass a target to the cpufreq driver */
> +unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
> +					unsigned int target_freq);
>  int cpufreq_driver_target(struct cpufreq_policy *policy,
>  				 unsigned int target_freq,
>  				 unsigned int relation);
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -428,6 +428,57 @@ void cpufreq_freq_transition_end(struct
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_freq_transition_end);
>  
> +/*
> + * Fast frequency switching status count.  Positive means "enabled", negative
> + * means "disabled" and 0 means "not decided yet".
> + */
> +static int cpufreq_fast_switch_count;
> +static DEFINE_MUTEX(cpufreq_fast_switch_lock);
> +
> +static void cpufreq_list_transition_notifiers(void)
> +{
> +	struct notifier_block *nb;
> +
> +	pr_info("cpufreq: Registered transition notifiers:\n");
> +
> +	mutex_lock(&cpufreq_transition_notifier_list.mutex);
> +
> +	for (nb = cpufreq_transition_notifier_list.head; nb; nb = nb->next)
> +		pr_info("cpufreq: %pF\n", nb->notifier_call);
> +
> +	mutex_unlock(&cpufreq_transition_notifier_list.mutex);

This will get printed as:

cpufreq: cpufreq: Registered transition notifiers:
cpufreq: cpufreq: <func>+0x0/0x<address>
cpufreq: cpufreq: <func>+0x0/0x<address>
cpufreq: cpufreq: <func>+0x0/0x<address>

Maybe we want something like:
cpufreq: Registered transition notifiers:
  cpufreq: <func>+0x0/0x<address>
  cpufreq: <func>+0x0/0x<address>
  cpufreq: <func>+0x0/0x<address>

?

> +}
> +
> +/**
> + * cpufreq_enable_fast_switch - Enable fast frequency switching for policy.
> + * @policy: cpufreq policy to enable fast frequency switching for.
> + *
> + * Try to enable fast frequency switching for @policy.
> + *
> + * The attempt will fail if there is at least one transition notifier registered
> + * at this point, as fast frequency switching is quite fundamentally at odds
> + * with transition notifiers.  Thus if successful, it will make registration of
> + * transition notifiers fail going forward.
> + */
> +void cpufreq_enable_fast_switch(struct cpufreq_policy *policy)
> +{
> +	lockdep_assert_held(&policy->rwsem);
> +
> +	if (!policy->fast_switch_possible)
> +		return;
> +
> +	mutex_lock(&cpufreq_fast_switch_lock);
> +	if (cpufreq_fast_switch_count >= 0) {
> +		cpufreq_fast_switch_count++;
> +		policy->fast_switch_enabled = true;
> +	} else {
> +		pr_warn("cpufreq: CPU%u: Fast frequency switching not enabled\n",
> +			policy->cpu);
> +		cpufreq_list_transition_notifiers();
> +	}
> +	mutex_unlock(&cpufreq_fast_switch_lock);
> +}
> +EXPORT_SYMBOL_GPL(cpufreq_enable_fast_switch);

And, why don't we have support for disabling fast-switch support? What if we
switch to schedutil governor (from userspace) and then back to ondemand? We
don't call policy->exit for that.

>  /*********************************************************************
>   *                          SYSFS INTERFACE                          *
> @@ -1083,6 +1134,24 @@ static void cpufreq_policy_free(struct c
>  	kfree(policy);
>  }
>  
> +static void cpufreq_driver_exit_policy(struct cpufreq_policy *policy)
> +{
> +	if (policy->fast_switch_enabled) {

Shouldn't this be accessed from within lock as well ?

> +		mutex_lock(&cpufreq_fast_switch_lock);
> +
> +		policy->fast_switch_enabled = false;
> +		if (!WARN_ON(cpufreq_fast_switch_count <= 0))
> +			cpufreq_fast_switch_count--;

Shouldn't we make it more efficient and write it as:

		WARN_ON(cpufreq_fast_switch_count <= 0);
		policy->fast_switch_enabled = false;
                cpufreq_fast_switch_count--;

The WARN check will hold true only for a major bug somewhere in the core and we
shall *never* hit it.

> +		mutex_unlock(&cpufreq_fast_switch_lock);
> +	}
> +
> +	if (cpufreq_driver->exit) {
> +		cpufreq_driver->exit(policy);
> +		policy->freq_table = NULL;
> +	}
> +}
> +
>  static int cpufreq_online(unsigned int cpu)
>  {
>  	struct cpufreq_policy *policy;
> @@ -1236,8 +1305,7 @@ static int cpufreq_online(unsigned int c
>  out_exit_policy:
>  	up_write(&policy->rwsem);
>  
> -	if (cpufreq_driver->exit)
> -		cpufreq_driver->exit(policy);
> +	cpufreq_driver_exit_policy(policy);
>  out_free_policy:
>  	cpufreq_policy_free(policy, !new_policy);
>  	return ret;
> @@ -1334,10 +1402,7 @@ static void cpufreq_offline(unsigned int
>  	 * since this is a core component, and is essential for the
>  	 * subsequent light-weight ->init() to succeed.
>  	 */
> -	if (cpufreq_driver->exit) {
> -		cpufreq_driver->exit(policy);
> -		policy->freq_table = NULL;
> -	}
> +	cpufreq_driver_exit_policy(policy);
>  
>  unlock:
>  	up_write(&policy->rwsem);
> @@ -1452,8 +1517,12 @@ static unsigned int __cpufreq_get(struct
>  
>  	ret_freq = cpufreq_driver->get(policy->cpu);
>  
> -	/* Updating inactive policies is invalid, so avoid doing that. */
> -	if (unlikely(policy_is_inactive(policy)))
> +	/*
> +	 * Updating inactive policies is invalid, so avoid doing that.  Also
> +	 * if fast frequency switching is used with the given policy, the check
> +	 * against policy->cur is pointless, so skip it in that case too.
> +	 */
> +	if (unlikely(policy_is_inactive(policy)) || policy->fast_switch_enabled)
>  		return ret_freq;
>  
>  	if (ret_freq && policy->cur &&
> @@ -1465,7 +1534,6 @@ static unsigned int __cpufreq_get(struct
>  			schedule_work(&policy->update);
>  		}
>  	}
> -

Unrelated change ? And to me it looks better with the blank line ..

>  	return ret_freq;
>  }
>  
> @@ -1672,8 +1740,18 @@ int cpufreq_register_notifier(struct not
>  
>  	switch (list) {
>  	case CPUFREQ_TRANSITION_NOTIFIER:
> +		mutex_lock(&cpufreq_fast_switch_lock);
> +
> +		if (cpufreq_fast_switch_count > 0) {
> +			mutex_unlock(&cpufreq_fast_switch_lock);
> +			return -EBUSY;
> +		}
>  		ret = srcu_notifier_chain_register(
>  				&cpufreq_transition_notifier_list, nb);
> +		if (!ret)
> +			cpufreq_fast_switch_count--;
> +
> +		mutex_unlock(&cpufreq_fast_switch_lock);
>  		break;
>  	case CPUFREQ_POLICY_NOTIFIER:
>  		ret = blocking_notifier_chain_register(
> @@ -1706,8 +1784,14 @@ int cpufreq_unregister_notifier(struct n
>  
>  	switch (list) {
>  	case CPUFREQ_TRANSITION_NOTIFIER:
> +		mutex_lock(&cpufreq_fast_switch_lock);
> +
>  		ret = srcu_notifier_chain_unregister(
>  				&cpufreq_transition_notifier_list, nb);
> +		if (!ret && !WARN_ON(cpufreq_fast_switch_count >= 0))
> +			cpufreq_fast_switch_count++;

Again here, why shouldn't we write it as:

		WARN_ON(cpufreq_fast_switch_count >= 0);

		if (!ret)
			cpufreq_fast_switch_count++;

> +
> +		mutex_unlock(&cpufreq_fast_switch_lock);
>  		break;
>  	case CPUFREQ_POLICY_NOTIFIER:
>  		ret = blocking_notifier_chain_unregister(
> @@ -1726,6 +1810,34 @@ EXPORT_SYMBOL(cpufreq_unregister_notifie
>   *                              GOVERNORS                            *
>   *********************************************************************/
>  
> +/**
> + * cpufreq_driver_fast_switch - Carry out a fast CPU frequency switch.
> + * @policy: cpufreq policy to switch the frequency for.
> + * @target_freq: New frequency to set (may be approximate).
> + *
> + * Carry out a fast frequency switch from interrupt context.
> + *
> + * The driver's ->fast_switch() callback invoked by this function is expected to
> + * select the minimum available frequency greater than or equal to @target_freq
> + * (CPUFREQ_RELATION_L).
> + *
> + * This function must not be called if policy->fast_switch_enabled is unset.
> + *
> + * Governors calling this function must guarantee that it will never be invoked
> + * twice in parallel for the same policy and that it will never be called in
> + * parallel with either ->target() or ->target_index() for the same policy.
> + *
> + * If CPUFREQ_ENTRY_INVALID is returned by the driver's ->fast_switch()
> + * callback to indicate an error condition, the hardware configuration must be
> + * preserved.
> + */
> +unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
> +					unsigned int target_freq)
> +{
> +	return cpufreq_driver->fast_switch(policy, target_freq);
> +}
> +EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);
> +
>  /* Must set freqs->new to intermediate frequency */
>  static int __target_intermediate(struct cpufreq_policy *policy,
>  				 struct cpufreq_freqs *freqs, int index)

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1366212

On Monday, March 28, 2016 11:57:51 AM Viresh Kumar wrote:
> Sorry for jumping in late, was busy with other stuff and travel :(
> 

[cut]

> > +static void cpufreq_list_transition_notifiers(void)
> > +{
> > +	struct notifier_block *nb;
> > +
> > +	pr_info("cpufreq: Registered transition notifiers:\n");
> > +
> > +	mutex_lock(&cpufreq_transition_notifier_list.mutex);
> > +
> > +	for (nb = cpufreq_transition_notifier_list.head; nb; nb = nb->next)
> > +		pr_info("cpufreq: %pF\n", nb->notifier_call);
> > +
> > +	mutex_unlock(&cpufreq_transition_notifier_list.mutex);
> 
> This will get printed as:
> 
> cpufreq: cpufreq: Registered transition notifiers:
> cpufreq: cpufreq: <func>+0x0/0x<address>
> cpufreq: cpufreq: <func>+0x0/0x<address>
> cpufreq: cpufreq: <func>+0x0/0x<address>
> 
> Maybe we want something like:
> cpufreq: Registered transition notifiers:
>   cpufreq: <func>+0x0/0x<address>
>   cpufreq: <func>+0x0/0x<address>
>   cpufreq: <func>+0x0/0x<address>
> 
> ?

You seem to be saying that pr_fmt() already has "cpufreq: " in it.  Fair enough.

> > +}
> > +
> > +/**
> > + * cpufreq_enable_fast_switch - Enable fast frequency switching for policy.
> > + * @policy: cpufreq policy to enable fast frequency switching for.
> > + *
> > + * Try to enable fast frequency switching for @policy.
> > + *
> > + * The attempt will fail if there is at least one transition notifier registered
> > + * at this point, as fast frequency switching is quite fundamentally at odds
> > + * with transition notifiers.  Thus if successful, it will make registration of
> > + * transition notifiers fail going forward.
> > + */
> > +void cpufreq_enable_fast_switch(struct cpufreq_policy *policy)
> > +{
> > +	lockdep_assert_held(&policy->rwsem);
> > +
> > +	if (!policy->fast_switch_possible)
> > +		return;
> > +
> > +	mutex_lock(&cpufreq_fast_switch_lock);
> > +	if (cpufreq_fast_switch_count >= 0) {
> > +		cpufreq_fast_switch_count++;
> > +		policy->fast_switch_enabled = true;
> > +	} else {
> > +		pr_warn("cpufreq: CPU%u: Fast frequency switching not enabled\n",
> > +			policy->cpu);
> > +		cpufreq_list_transition_notifiers();
> > +	}
> > +	mutex_unlock(&cpufreq_fast_switch_lock);
> > +}
> > +EXPORT_SYMBOL_GPL(cpufreq_enable_fast_switch);
> 
> And, why don't we have support for disabling fast-switch support? What if we
> switch to schedutil governor (from userspace) and then back to ondemand? We
> don't call policy->exit for that.

Disabling fast switch can be automatic depending on whether or not
fast_switch_enabled is set, but I clearly forgot about the manual governor
switch case.

It should be fine to do it before calling cpufreq_governor(_EXIT) then.


> >  /*********************************************************************
> >   *                          SYSFS INTERFACE                          *
> > @@ -1083,6 +1134,24 @@ static void cpufreq_policy_free(struct c
> >  	kfree(policy);
> >  }
> >  
> > +static void cpufreq_driver_exit_policy(struct cpufreq_policy *policy)
> > +{
> > +	if (policy->fast_switch_enabled) {
> 
> Shouldn't this be accessed from within lock as well ?
> 
> > +		mutex_lock(&cpufreq_fast_switch_lock);
> > +
> > +		policy->fast_switch_enabled = false;
> > +		if (!WARN_ON(cpufreq_fast_switch_count <= 0))
> > +			cpufreq_fast_switch_count--;
> 
> Shouldn't we make it more efficient and write it as:
> 
> 		WARN_ON(cpufreq_fast_switch_count <= 0);
> 		policy->fast_switch_enabled = false;
>                 cpufreq_fast_switch_count--;
> 
> The WARN check will hold true only for a major bug somewhere in the core and we
> shall *never* hit it.

The point here is to avoid the decrementation if the WARN_ON() triggers too.

> > +		mutex_unlock(&cpufreq_fast_switch_lock);
> > +	}
> > +
> > +	if (cpufreq_driver->exit) {
> > +		cpufreq_driver->exit(policy);
> > +		policy->freq_table = NULL;
> > +	}
> > +}
> > +
> >  static int cpufreq_online(unsigned int cpu)
> >  {
> >  	struct cpufreq_policy *policy;
> > @@ -1236,8 +1305,7 @@ static int cpufreq_online(unsigned int c
> >  out_exit_policy:
> >  	up_write(&policy->rwsem);
> >  
> > -	if (cpufreq_driver->exit)
> > -		cpufreq_driver->exit(policy);
> > +	cpufreq_driver_exit_policy(policy);
> >  out_free_policy:
> >  	cpufreq_policy_free(policy, !new_policy);
> >  	return ret;
> > @@ -1334,10 +1402,7 @@ static void cpufreq_offline(unsigned int
> >  	 * since this is a core component, and is essential for the
> >  	 * subsequent light-weight ->init() to succeed.
> >  	 */
> > -	if (cpufreq_driver->exit) {
> > -		cpufreq_driver->exit(policy);
> > -		policy->freq_table = NULL;
> > -	}
> > +	cpufreq_driver_exit_policy(policy);
> >  
> >  unlock:
> >  	up_write(&policy->rwsem);
> > @@ -1452,8 +1517,12 @@ static unsigned int __cpufreq_get(struct
> >  
> >  	ret_freq = cpufreq_driver->get(policy->cpu);
> >  
> > -	/* Updating inactive policies is invalid, so avoid doing that. */
> > -	if (unlikely(policy_is_inactive(policy)))
> > +	/*
> > +	 * Updating inactive policies is invalid, so avoid doing that.  Also
> > +	 * if fast frequency switching is used with the given policy, the check
> > +	 * against policy->cur is pointless, so skip it in that case too.
> > +	 */
> > +	if (unlikely(policy_is_inactive(policy)) || policy->fast_switch_enabled)
> >  		return ret_freq;
> >  
> >  	if (ret_freq && policy->cur &&
> > @@ -1465,7 +1534,6 @@ static unsigned int __cpufreq_get(struct
> >  			schedule_work(&policy->update);
> >  		}
> >  	}
> > -
> 
> Unrelated change ? And to me it looks better with the blank line ..

Yes, it is unrelated.

> >  	return ret_freq;
> >  }
> >  
> > @@ -1672,8 +1740,18 @@ int cpufreq_register_notifier(struct not
> >  
> >  	switch (list) {
> >  	case CPUFREQ_TRANSITION_NOTIFIER:
> > +		mutex_lock(&cpufreq_fast_switch_lock);
> > +
> > +		if (cpufreq_fast_switch_count > 0) {
> > +			mutex_unlock(&cpufreq_fast_switch_lock);
> > +			return -EBUSY;
> > +		}
> >  		ret = srcu_notifier_chain_register(
> >  				&cpufreq_transition_notifier_list, nb);
> > +		if (!ret)
> > +			cpufreq_fast_switch_count--;
> > +
> > +		mutex_unlock(&cpufreq_fast_switch_lock);
> >  		break;
> >  	case CPUFREQ_POLICY_NOTIFIER:
> >  		ret = blocking_notifier_chain_register(
> > @@ -1706,8 +1784,14 @@ int cpufreq_unregister_notifier(struct n
> >  
> >  	switch (list) {
> >  	case CPUFREQ_TRANSITION_NOTIFIER:
> > +		mutex_lock(&cpufreq_fast_switch_lock);
> > +
> >  		ret = srcu_notifier_chain_unregister(
> >  				&cpufreq_transition_notifier_list, nb);
> > +		if (!ret && !WARN_ON(cpufreq_fast_switch_count >= 0))
> > +			cpufreq_fast_switch_count++;
> 
> Again here, why shouldn't we write it as:

And same here again, I don't want the incrementation to happen if the WARN_ON()
triggers.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1365373 — Re: [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency switching

forgot to review acpi update earlier ..

On 22-03-16, 02:53, Rafael J. Wysocki wrote:
> Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/acpi-cpufreq.c
> +++ linux-pm/drivers/cpufreq/acpi-cpufreq.c
> @@ -458,6 +458,43 @@ static int acpi_cpufreq_target(struct cp
>  	return result;
>  }
>  
> +unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
> +				      unsigned int target_freq)
> +{
> +	struct acpi_cpufreq_data *data = policy->driver_data;
> +	struct acpi_processor_performance *perf;
> +	struct cpufreq_frequency_table *entry;
> +	unsigned int next_perf_state, next_freq, freq;
> +
> +	/*
> +	 * Find the closest frequency above target_freq.
> +	 *
> +	 * The table is sorted in the reverse order with respect to the
> +	 * frequency and all of the entries are valid (see the initialization).
> +	 */
> +	entry = data->freq_table;
> +	do {
> +		entry++;
> +		freq = entry->frequency;
> +	} while (freq >= target_freq && freq != CPUFREQ_TABLE_END);

Consider this table:

11000
10000
9000

And a target-freq of 10000.

Wouldn't you end up selecting 11000 ? Or did I misread it ?

> +	entry--;
> +	next_freq = entry->frequency;
> +	next_perf_state = entry->driver_data;
> +
> +	perf = to_perf_data(data);
> +	if (perf->state == next_perf_state) {
> +		if (unlikely(data->resume))
> +			data->resume = 0;
> +		else
> +			return next_freq;
> +	}
> +
> +	data->cpu_freq_write(&perf->control_register,
> +			     perf->states[next_perf_state].control);
> +	perf->state = next_perf_state;
> +	return next_freq;
> +}
> +
>  static unsigned long
>  acpi_cpufreq_guess_freq(struct acpi_cpufreq_data *data, unsigned int cpu)
>  {
> @@ -740,6 +777,9 @@ static int acpi_cpufreq_cpu_init(struct
>  		goto err_unreg;
>  	}
>  
> +	policy->fast_switch_possible = !acpi_pstate_strict &&
> +		!(policy_is_shared(policy) && policy->shared_type != CPUFREQ_SHARED_TYPE_ANY);
> +
>  	data->freq_table = kzalloc(sizeof(*data->freq_table) *
>  		    (perf->state_count+1), GFP_KERNEL);
>  	if (!data->freq_table) {
> @@ -874,6 +914,7 @@ static struct freq_attr *acpi_cpufreq_at
>  static struct cpufreq_driver acpi_cpufreq_driver = {
>  	.verify		= cpufreq_generic_frequency_table_verify,
>  	.target_index	= acpi_cpufreq_target,
> +	.fast_switch	= acpi_cpufreq_fast_switch,
>  	.bios_limit	= acpi_processor_get_bios_limit,
>  	.init		= acpi_cpufreq_cpu_init,
>  	.exit		= acpi_cpufreq_cpu_exit,

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1366197

On Monday, March 28, 2016 12:33:41 PM Viresh Kumar wrote:
> forgot to review acpi update earlier ..
> 
> On 22-03-16, 02:53, Rafael J. Wysocki wrote:
> > Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpufreq/acpi-cpufreq.c
> > +++ linux-pm/drivers/cpufreq/acpi-cpufreq.c
> > @@ -458,6 +458,43 @@ static int acpi_cpufreq_target(struct cp
> >  	return result;
> >  }
> >  
> > +unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
> > +				      unsigned int target_freq)
> > +{
> > +	struct acpi_cpufreq_data *data = policy->driver_data;
> > +	struct acpi_processor_performance *perf;
> > +	struct cpufreq_frequency_table *entry;
> > +	unsigned int next_perf_state, next_freq, freq;
> > +
> > +	/*
> > +	 * Find the closest frequency above target_freq.
> > +	 *
> > +	 * The table is sorted in the reverse order with respect to the
> > +	 * frequency and all of the entries are valid (see the initialization).
> > +	 */
> > +	entry = data->freq_table;
> > +	do {
> > +		entry++;
> > +		freq = entry->frequency;
> > +	} while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
> 
> Consider this table:
> 
> 11000
> 10000
> 9000
> 
> And a target-freq of 10000.
> 
> Wouldn't you end up selecting 11000 ? Or did I misread it ?

In that case the loop will break for freq = 9000 (as per the above
freq >= freq_target check), so it looks like you've misread it.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1366337 — Re: [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency switching

On 29-03-16, 14:10, Rafael J. Wysocki wrote:
> In that case the loop will break for freq = 9000 (as per the above
> freq >= freq_target check), so it looks like you've misread it.

My bad ..

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1366846 — [Update][PATCH v7 6/7] cpufreq: Support for fast frequency switching

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Modify the ACPI cpufreq driver to provide a method for switching
CPU frequencies from interrupt context and update the cpufreq core
to support that method if available.

Introduce a new cpufreq driver callback, ->fast_switch, to be
invoked for frequency switching from interrupt context by (future)
governors supporting that feature via (new) helper function
cpufreq_driver_fast_switch().

Add two new policy flags, fast_switch_possible, to be set by the
cpufreq driver if fast frequency switching can be used for the
given policy and fast_switch_enabled, to be set by the governor
if it is going to use fast frequency switching for the given
policy.  Also add a helper for setting the latter.

Since fast frequency switching is inherently incompatible with
cpufreq transition notifiers, make it possible to set the
fast_switch_enabled only if there are no transition notifiers
already registered and make the registration of new transition
notifiers fail if fast_switch_enabled is set for at least one
policy.

Implement the ->fast_switch callback in the ACPI cpufreq driver
and make it set fast_switch_possible during policy initialization
as appropriate.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Changes from v6:
- Added cpufreq_exit_governor() that disables fast frequency switching
  before calling cpufreq_governor(_EXIT) and updated the callers of the
  latter to use the new function instead.
- Modified cpufreq_driver_fast_switch() to apply the limits to target_freq.
- Modified the changelog of cpufreq_driver_fast_switch() to mention the
  RCU-sched read-side critical sections requirement for ->fast_switch.
- Added a comment describing fast_switch_possible and fast_switch_enabled.
- Modified acpi-cpufreq to clear policy->fast_switch_possible when
  policy->driver_data is cleared.

Changes from v5:
- cpufreq_enable_fast_switch() fixed to avoid printing a confusing message
  if fast_switch_possible is not set for the policy.
- Fixed a typo in that message.
- Removed the WARN_ON() from the (cpufreq_fast_switch_count > 0) check in
  cpufreq_register_notifier(), because it triggered false-positive warnings
  from the cpufreq_stats module (cpufreq_stats don't work with the fast
  switching, because it is based on notifiers).

Changes from v4:
- If cpufreq_enable_fast_switch() is about to fail, it will print the list
  of currently registered transition notifiers.
- Added lock_assert_held(&policy->rwsem) to cpufreq_enable_fast_switch().
- Added WARN_ON() to the (cpufreq_fast_switch_count > 0) check in
  cpufreq_register_notifier().
- Modified the kerneldoc comment of cpufreq_driver_fast_switch() to
  mention the RELATION_L expectation regarding the ->fast_switch callback.

Changes from v3:
- New fast_switch_enabled field in struct cpufreq_policy to help
  avoid affecting existing setups by setting the fast_switch_possible
  flag in the driver.
- __cpufreq_get() skips the policy->cur check if fast_switch_enabled is set.

Changes from v2:
- The driver ->fast_switch callback and cpufreq_driver_fast_switch()
  don't need the relation argument as they will always do RELATION_L now.
- New mechanism to make fast switch and cpufreq notifiers mutually
  exclusive.
- cpufreq_driver_fast_switch() doesn't do anything in addition to
  invoking the driver callback and returns its return value.

---
 drivers/cpufreq/acpi-cpufreq.c |   42 +++++++++++++
 drivers/cpufreq/cpufreq.c      |  130 +++++++++++++++++++++++++++++++++++++++--
 include/linux/cpufreq.h        |   16 +++++
 3 files changed, 183 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/acpi-cpufreq.c
+++ linux-pm/drivers/cpufreq/acpi-cpufreq.c
@@ -458,6 +458,43 @@ static int acpi_cpufreq_target(struct cp
 	return result;
 }
 
+unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
+				      unsigned int target_freq)
+{
+	struct acpi_cpufreq_data *data = policy->driver_data;
+	struct acpi_processor_performance *perf;
+	struct cpufreq_frequency_table *entry;
+	unsigned int next_perf_state, next_freq, freq;
+
+	/*
+	 * Find the closest frequency above target_freq.
+	 *
+	 * The table is sorted in the reverse order with respect to the
+	 * frequency and all of the entries are valid (see the initialization).
+	 */
+	entry = data->freq_table;
+	do {
+		entry++;
+		freq = entry->frequency;
+	} while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
+	entry--;
+	next_freq = entry->frequency;
+	next_perf_state = entry->driver_data;
+
+	perf = to_perf_data(data);
+	if (perf->state == next_perf_state) {
+		if (unlikely(data->resume))
+			data->resume = 0;
+		else
+			return next_freq;
+	}
+
+	data->cpu_freq_write(&perf->control_register,
+			     perf->states[next_perf_state].control);
+	perf->state = next_perf_state;
+	return next_freq;
+}
+
 static unsigned long
 acpi_cpufreq_guess_freq(struct acpi_cpufreq_data *data, unsigned int cpu)
 {
@@ -821,6 +858,9 @@ static int acpi_cpufreq_cpu_init(struct
 	 */
 	data->resume = 1;
 
+	policy->fast_switch_possible = !acpi_pstate_strict &&
+		!(policy_is_shared(policy) && policy->shared_type != CPUFREQ_SHARED_TYPE_ANY);
+
 	return result;
 
 err_freqfree:
@@ -843,6 +883,7 @@ static int acpi_cpufreq_cpu_exit(struct
 	pr_debug("acpi_cpufreq_cpu_exit\n");
 
 	if (data) {
+		policy->fast_switch_possible = false;
 		policy->driver_data = NULL;
 		acpi_processor_unregister_performance(data->acpi_perf_cpu);
 		free_cpumask_var(data->freqdomain_cpus);
@@ -876,6 +917,7 @@ static struct freq_attr *acpi_cpufreq_at
 static struct cpufreq_driver acpi_cpufreq_driver = {
 	.verify		= cpufreq_generic_frequency_table_verify,
 	.target_index	= acpi_cpufreq_target,
+	.fast_switch	= acpi_cpufreq_fast_switch,
 	.bios_limit	= acpi_processor_get_bios_limit,
 	.init		= acpi_cpufreq_cpu_init,
 	.exit		= acpi_cpufreq_cpu_exit,
Index: linux-pm/include/linux/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/cpufreq.h
+++ linux-pm/include/linux/cpufreq.h
@@ -102,6 +102,17 @@ struct cpufreq_policy {
 	 */
 	struct rw_semaphore	rwsem;
 
+	/*
+	 * Fast switch flags:
+	 * - fast_switch_possible should be set by the driver if it can
+	 *   guarantee that frequency can be changed on any CPU sharing the
+	 *   policy and that the change will affect all of the policy CPUs then.
+	 * - fast_switch_enabled is to be set by governors that support fast
+	 *   freqnency switching with the help of cpufreq_enable_fast_switch().
+	 */
+	bool			fast_switch_possible;
+	bool			fast_switch_enabled;
+
 	/* Synchronization for frequency transitions */
 	bool			transition_ongoing; /* Tracks transition status */
 	spinlock_t		transition_lock;
@@ -156,6 +167,7 @@ int cpufreq_get_policy(struct cpufreq_po
 int cpufreq_update_policy(unsigned int cpu);
 bool have_governor_per_policy(void);
 struct kobject *get_governor_parent_kobj(struct cpufreq_policy *policy);
+void cpufreq_enable_fast_switch(struct cpufreq_policy *policy);
 #else
 static inline unsigned int cpufreq_get(unsigned int cpu)
 {
@@ -236,6 +248,8 @@ struct cpufreq_driver {
 				  unsigned int relation);	/* Deprecated */
 	int		(*target_index)(struct cpufreq_policy *policy,
 					unsigned int index);
+	unsigned int	(*fast_switch)(struct cpufreq_policy *policy,
+				       unsigned int target_freq);
 	/*
 	 * Only for drivers with target_index() and CPUFREQ_ASYNC_NOTIFICATION
 	 * unset.
@@ -464,6 +478,8 @@ struct cpufreq_governor {
 };
 
 /* Pass a target to the cpufreq driver */
+unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
+					unsigned int target_freq);
 int cpufreq_driver_target(struct cpufreq_policy *policy,
 				 unsigned int target_freq,
 				 unsigned int relation);
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -77,6 +77,7 @@ static inline bool has_target(void)
 static int cpufreq_governor(struct cpufreq_policy *policy, unsigned int event);
 static unsigned int __cpufreq_get(struct cpufreq_policy *policy);
 static int cpufreq_start_governor(struct cpufreq_policy *policy);
+static int cpufreq_exit_governor(struct cpufreq_policy *policy);
 
 /**
  * Two notifier lists: the "policy" list is involved in the
@@ -429,6 +430,68 @@ void cpufreq_freq_transition_end(struct
 }
 EXPORT_SYMBOL_GPL(cpufreq_freq_transition_end);
 
+/*
+ * Fast frequency switching status count.  Positive means "enabled", negative
+ * means "disabled" and 0 means "not decided yet".
+ */
+static int cpufreq_fast_switch_count;
+static DEFINE_MUTEX(cpufreq_fast_switch_lock);
+
+static void cpufreq_list_transition_notifiers(void)
+{
+	struct notifier_block *nb;
+
+	pr_info("Registered transition notifiers:\n");
+
+	mutex_lock(&cpufreq_transition_notifier_list.mutex);
+
+	for (nb = cpufreq_transition_notifier_list.head; nb; nb = nb->next)
+		pr_info("%pF\n", nb->notifier_call);
+
+	mutex_unlock(&cpufreq_transition_notifier_list.mutex);
+}
+
+/**
+ * cpufreq_enable_fast_switch - Enable fast frequency switching for policy.
+ * @policy: cpufreq policy to enable fast frequency switching for.
+ *
+ * Try to enable fast frequency switching for @policy.
+ *
+ * The attempt will fail if there is at least one transition notifier registered
+ * at this point, as fast frequency switching is quite fundamentally at odds
+ * with transition notifiers.  Thus if successful, it will make registration of
+ * transition notifiers fail going forward.
+ */
+void cpufreq_enable_fast_switch(struct cpufreq_policy *policy)
+{
+	lockdep_assert_held(&policy->rwsem);
+
+	if (!policy->fast_switch_possible)
+		return;
+
+	mutex_lock(&cpufreq_fast_switch_lock);
+	if (cpufreq_fast_switch_count >= 0) {
+		cpufreq_fast_switch_count++;
+		policy->fast_switch_enabled = true;
+	} else {
+		pr_warn("CPU%u: Fast frequency switching not enabled\n",
+			policy->cpu);
+		cpufreq_list_transition_notifiers();
+	}
+	mutex_unlock(&cpufreq_fast_switch_lock);
+}
+EXPORT_SYMBOL_GPL(cpufreq_enable_fast_switch);
+
+static void cpufreq_disable_fast_switch(struct cpufreq_policy *policy)
+{
+	mutex_lock(&cpufreq_fast_switch_lock);
+	if (policy->fast_switch_enabled) {
+		policy->fast_switch_enabled = false;
+		if (!WARN_ON(cpufreq_fast_switch_count <= 0))
+			cpufreq_fast_switch_count--;
+	}
+	mutex_unlock(&cpufreq_fast_switch_lock);
+}
 
 /*********************************************************************
  *                          SYSFS INTERFACE                          *
@@ -1319,7 +1382,7 @@ static void cpufreq_offline(unsigned int
 
 	/* If cpu is last user of policy, free policy */
 	if (has_target()) {
-		ret = cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
+		ret = cpufreq_exit_governor(policy);
 		if (ret)
 			pr_err("%s: Failed to exit governor\n", __func__);
 	}
@@ -1447,8 +1510,12 @@ static unsigned int __cpufreq_get(struct
 
 	ret_freq = cpufreq_driver->get(policy->cpu);
 
-	/* Updating inactive policies is invalid, so avoid doing that. */
-	if (unlikely(policy_is_inactive(policy)))
+	/*
+	 * Updating inactive policies is invalid, so avoid doing that.  Also
+	 * if fast frequency switching is used with the given policy, the check
+	 * against policy->cur is pointless, so skip it in that case too.
+	 */
+	if (unlikely(policy_is_inactive(policy)) || policy->fast_switch_enabled)
 		return ret_freq;
 
 	if (ret_freq && policy->cur &&
@@ -1672,8 +1739,18 @@ int cpufreq_register_notifier(struct not
 
 	switch (list) {
 	case CPUFREQ_TRANSITION_NOTIFIER:
+		mutex_lock(&cpufreq_fast_switch_lock);
+
+		if (cpufreq_fast_switch_count > 0) {
+			mutex_unlock(&cpufreq_fast_switch_lock);
+			return -EBUSY;
+		}
 		ret = srcu_notifier_chain_register(
 				&cpufreq_transition_notifier_list, nb);
+		if (!ret)
+			cpufreq_fast_switch_count--;
+
+		mutex_unlock(&cpufreq_fast_switch_lock);
 		break;
 	case CPUFREQ_POLICY_NOTIFIER:
 		ret = blocking_notifier_chain_register(
@@ -1706,8 +1783,14 @@ int cpufreq_unregister_notifier(struct n
 
 	switch (list) {
 	case CPUFREQ_TRANSITION_NOTIFIER:
+		mutex_lock(&cpufreq_fast_switch_lock);
+
 		ret = srcu_notifier_chain_unregister(
 				&cpufreq_transition_notifier_list, nb);
+		if (!ret && !WARN_ON(cpufreq_fast_switch_count >= 0))
+			cpufreq_fast_switch_count++;
+
+		mutex_unlock(&cpufreq_fast_switch_lock);
 		break;
 	case CPUFREQ_POLICY_NOTIFIER:
 		ret = blocking_notifier_chain_unregister(
@@ -1726,6 +1809,37 @@ EXPORT_SYMBOL(cpufreq_unregister_notifie
  *                              GOVERNORS                            *
  *********************************************************************/
 
+/**
+ * cpufreq_driver_fast_switch - Carry out a fast CPU frequency switch.
+ * @policy: cpufreq policy to switch the frequency for.
+ * @target_freq: New frequency to set (may be approximate).
+ *
+ * Carry out a fast frequency switch without sleeping.
+ *
+ * The driver's ->fast_switch() callback invoked by this function must be
+ * suitable for being called from within RCU-sched read-side critical sections
+ * and it is expected to select the minimum available frequency greater than or
+ * equal to @target_freq (CPUFREQ_RELATION_L).
+ *
+ * This function must not be called if policy->fast_switch_enabled is unset.
+ *
+ * Governors calling this function must guarantee that it will never be invoked
+ * twice in parallel for the same policy and that it will never be called in
+ * parallel with either ->target() or ->target_index() for the same policy.
+ *
+ * If CPUFREQ_ENTRY_INVALID is returned by the driver's ->fast_switch()
+ * callback to indicate an error condition, the hardware configuration must be
+ * preserved.
+ */
+unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
+					unsigned int target_freq)
+{
+	clamp_val(target_freq, policy->min, policy->max);
+
+	return cpufreq_driver->fast_switch(policy, target_freq);
+}
+EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);
+
 /* Must set freqs->new to intermediate frequency */
 static int __target_intermediate(struct cpufreq_policy *policy,
 				 struct cpufreq_freqs *freqs, int index)
@@ -1946,6 +2060,12 @@ static int cpufreq_start_governor(struct
 	return ret ? ret : cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
 }
 
+static int cpufreq_exit_governor(struct cpufreq_policy *policy)
+{
+	cpufreq_disable_fast_switch(policy);
+	return cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
+}
+
 int cpufreq_register_governor(struct cpufreq_governor *governor)
 {
 	int err;
@@ -2101,7 +2221,7 @@ static int cpufreq_set_policy(struct cpu
 			return ret;
 		}
 
-		ret = cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
+		ret = cpufreq_exit_governor(policy);
 		if (ret) {
 			pr_err("%s: Failed to Exit Governor: %s (%d)\n",
 			       __func__, old_gov->name, ret);
@@ -2118,7 +2238,7 @@ static int cpufreq_set_policy(struct cpu
 			pr_debug("cpufreq: governor change\n");
 			return 0;
 		}
-		cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
+		cpufreq_exit_governor(policy);
 	}
 
 	/* new governor failed, so re-start old one */

[toc] | [prev] | [next] | [standalone]

#1366895 — Re: [Update][PATCH v7 6/7] cpufreq: Support for fast frequency switching

On 30-03-16, 03:47, Rafael J. Wysocki wrote:
> Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c
> @@ -843,6 +883,7 @@ static int acpi_cpufreq_cpu_exit(struct
>  	pr_debug("acpi_cpufreq_cpu_exit\n");
>  
>  	if (data) {
> +		policy->fast_switch_possible = false;

Is this done just for keeping code symmetric or is there a logical advantage
of this? Just for my understanding, not saying that it is wrong.

Otherwise, it looks good

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1367116 — Re: [Update][PATCH v7 6/7] cpufreq: Support for fast frequency switching

On Wed, Mar 30, 2016 at 7:07 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 30-03-16, 03:47, Rafael J. Wysocki wrote:
>> Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c
>> @@ -843,6 +883,7 @@ static int acpi_cpufreq_cpu_exit(struct
>>       pr_debug("acpi_cpufreq_cpu_exit\n");
>>
>>       if (data) {
>> +             policy->fast_switch_possible = false;
>
> Is this done just for keeping code symmetric or is there a logical advantage
> of this? Just for my understanding, not saying that it is wrong.

It is not necessary for correctness today, as schedutil will be the
only governor using fast switch, but generally that prevents leaking
configuration information from one governor to another.

Thanks,
Rafael

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web