linux-kernel - Re: [PATCH V3 3/3] cpufreq: Specify default governor on command line

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200629020843.erntkwfprgi5ugqu@vireshk-i7>
Date:   Mon, 29 Jun 2020 07:38:43 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Quentin Perret <qperret@...gle.com>
Cc:     Rafael Wysocki <rjw@...ysocki.net>,
        Jonathan Corbet <corbet@....net>, linux-pm@...r.kernel.org,
        Vincent Guittot <vincent.guittot@...aro.org>,
        kernel-team@...roid.com, tkjos@...gle.com, adharmap@...eaurora.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V3 3/3] cpufreq: Specify default governor on command line

On 26-06-20, 16:57, Quentin Perret wrote:
> On Friday 26 Jun 2020 at 09:21:44 (+0530), Viresh Kumar wrote:
> > index e798a1193bdf..93c6399c1a42 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -50,6 +50,9 @@ static LIST_HEAD(cpufreq_governor_list);
> >  #define for_each_governor(__governor)				\
> >  	list_for_each_entry(__governor, &cpufreq_governor_list, governor_list)
> >  
> > +static char cpufreq_param_governor[CPUFREQ_NAME_LEN];
> > +static char default_governor[CPUFREQ_NAME_LEN];
> > +
> >  /**
> >   * The "cpufreq driver" - the arch- or hardware-dependent low
> >   * level driver of CPUFreq support, and its spinlock. This lock
> > @@ -1061,7 +1064,6 @@ __weak struct cpufreq_governor *cpufreq_default_governor(void)
> >  
> >  static int cpufreq_init_policy(struct cpufreq_policy *policy)
> >  {
> > -	struct cpufreq_governor *def_gov = cpufreq_default_governor();
> >  	struct cpufreq_governor *gov = NULL;
> >  	unsigned int pol = CPUFREQ_POLICY_UNKNOWN;
> >  	bool put_governor = false;
> > @@ -1071,22 +1073,29 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
> >  		/* Update policy governor to the one used before hotplug. */
> >  		gov = get_governor(policy->last_governor);
> >  		if (gov) {
> > -			put_governor = true;
> >  			pr_debug("Restoring governor %s for cpu %d\n",
> > -				 policy->governor->name, policy->cpu);
> > -		} else if (def_gov) {
> > -			gov = def_gov;
> > +				 gov->name, policy->cpu);
> >  		} else {
> > -			return -ENODATA;
> > +			gov = get_governor(default_governor);
> > +		}
> > +
> > +		if (gov) {
> > +			put_governor = true;
> > +		} else {
> > +			gov = cpufreq_default_governor();
> > +			if (!gov)
> > +				return -ENODATA;
> >  		}
> 
> As mentioned on patch 01, doing put_module() below if gov != NULL would
> avoid this dance with put_governor, but this works too, so no strong
> opinion.

I did it this way because the code looks buggy otherwise, even though
it isn't as put_module() handles it just fine. And so I would like to
keep it this way, unless there are two votes against mine :)

> > +
> >  	} else {
> > +
> >  		/* Use the default policy if there is no last_policy. */
> >  		if (policy->last_policy) {
> >  			pol = policy->last_policy;
> > -		} else if (def_gov) {
> > -			pol = cpufreq_parse_policy(def_gov->name);
> > +		} else {
> > +			pol = cpufreq_parse_policy(default_governor);
> >  			/*
> > -			 * In case the default governor is neiter "performance"
> > +			 * In case the default governor is neither "performance"
> >  			 * nor "powersave", fall back to the initial policy
> >  			 * value set by the driver.
> >  			 */
> > @@ -2796,13 +2805,22 @@ EXPORT_SYMBOL_GPL(cpufreq_unregister_driver);
> >  
> >  static int __init cpufreq_core_init(void)
> >  {
> > +	struct cpufreq_governor *gov = cpufreq_default_governor();
> > +	char *name = gov->name;
> > +
> >  	if (cpufreq_disabled())
> >  		return -ENODEV;
> >  
> >  	cpufreq_global_kobject = kobject_create_and_add("cpufreq", &cpu_subsys.dev_root->kobj);
> >  	BUG_ON(!cpufreq_global_kobject);
> >  
> > +	if (strlen(cpufreq_param_governor))
> > +		name = cpufreq_param_governor;
> > +
> > +	strncpy(default_governor, name, CPUFREQ_NAME_LEN);
> 
> Do we need both cpufreq_param_governor and default_governor?
> Could we move everything to only one of them? Something a little bit
> like that maybe?

No because we want to fallback to the default governor when the
governor shown by the cpufreq_param_governor is valid but missing.

> Also, one thing to keep in mind with this version (or the one you
> suggested) is that if the command line parameter is not valid, we will
> not fallback on the builtin default for the ->setpolicy() case. But I
> suppose one might argue this is a reasonable behaviour, so no objection
> from me.

Right, I did that on purpose.

> Otherwise, apart from these nits, I gave this a go on my setup, with
> builtin and modular governors & drivers, and everything worked exactly
> as expected.

Thanks for testing it out Quentin.

-- 
viresh