| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 29 Mar 2016 14:31:29 +0200
From: "Rafael J. Wysocki" <rjw@...ysocki.net>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: Linux PM list <linux-pm@...r.kernel.org>,
Juri Lelli <juri.lelli@....com>,
Steve Muckle <steve.muckle@...aro.org>,
ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Peter Zijlstra <peterz@...radead.org>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
Vincent Guittot <vincent.guittot@...aro.org>,
Michael Turquette <mturquette@...libre.com>,
Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH v6 6/7][Resend] cpufreq: Support for fast frequency switching
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
Powered by blists - more mailing lists