linux-kernel - Re: [RFC PATCH] cpufreq: Support per-policy performance boost

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: <20230816062757.tocdrqh3isxcrt3r@vireshk-i7>
Date:   Wed, 16 Aug 2023 11:57:57 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Jie Zhan <zhanjie9@...ilicon.com>
Cc:     rafael@...nel.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org, linuxarm@...wei.com,
        xuwei5@...wei.com, wanghuiqiang@...wei.com,
        jonathan.cameron@...wei.com, wangxiongfeng2@...wei.com
Subject: Re: [RFC PATCH] cpufreq: Support per-policy performance boost

Hi Jie,

I am not sure how useful this per-policy boost is for your use case,
but I have no objection to the same if it is worth it.

On 24-07-23, 15:58, Jie Zhan wrote:
> The boost control currently applies to the whole system.  However, users
> may prefer to boost a subset of cores in order to provide prioritized
> performance to workloads running on the boosted cores.
> 
> Enable per-policy boost by adding a 'local_boost' sysfs interface.  This
> can be found at:
> 
> 	/sys/devices/system/cpu/cpufreq/policy<*>/local_boost

Just name it boost instead of local_boost, it is present in the
policy's directory, that is enough.

> 
> Same to the global boost switch, writing 1/0 to 'local_boost'
> enables/disables boost on a cpufreq policy respectively.
> 
> The user view of global and local boost controls should be:
> 
> 1. Enabling global boost initially enables local boost on all policies, and
> local boost can then be enabled or disabled individually on each policy,
> given that the platform does support so.
> 
> 2. Disabling global boost makes enabling local boost illegal, while writing
> 0 to 'local_boost' is fine but takes no effect.
> 
> A possible question could be: why not just limiting 'scaling_max_freq'?
> Well, the fundamental difference is that 'local_boost' could be more
> user-friendly.  When global boost is enabled, it is not straightforward to
> figure out the base frequency for setting 'scaling_max_freq' to a non-boost
> value. Also, this is supposed to take effect on the physical upper
> frequency limit, reflected through 'cpuinfo_max_freq'.
> 
> Signed-off-by: Jie Zhan <zhanjie9@...ilicon.com>
> Reviewed-by: Wei Xu <xuwei5@...ilicon.com>
> ---
>  drivers/cpufreq/cpufreq.c | 33 +++++++++++++++++++++++++++++++++
>  include/linux/cpufreq.h   |  3 +++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 6b52ebe5a890..14579c59f7ba 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -621,6 +621,35 @@ static ssize_t store_boost(struct kobject *kobj, struct kobj_attribute *attr,
>  }
>  define_one_global_rw(boost);
>  
> +static ssize_t show_local_boost(struct cpufreq_policy *policy, char *buf)
> +{
> +	return sysfs_emit(buf, "%d\n", policy->local_boost_enabled);
> +}
> +
> +static ssize_t store_local_boost(struct cpufreq_policy *policy,
> +				 const char *buf, size_t count)
> +{
> +	int ret, enable;
> +
> +	ret = kstrtoint(buf, 10, &enable);
> +	if (ret || enable < 0 || enable > 1)
> +		return -EINVAL;
> +
> +	if (enable && !cpufreq_driver->boost_enabled)
> +		return -EINVAL;

Not just while enabling it, you should check for boost_enabled all the
time.

Also, you can optimize here if the state is being set to existing
state and quit early. This is done in the case of global boost
already. Have a look.

> +
> +	cpus_read_lock();
> +	ret = cpufreq_driver->set_boost(policy, enable);
> +	cpus_read_unlock();
> +
> +	if (ret)
> +		return ret;
> +
> +	policy->local_boost_enabled = enable;

s/local_//

> +
> +	return count;
> +}
> +
>  static struct cpufreq_governor *find_governor(const char *str_governor)
>  {
>  	struct cpufreq_governor *t;
> @@ -931,6 +960,7 @@ cpufreq_freq_attr_rw(scaling_min_freq);
>  cpufreq_freq_attr_rw(scaling_max_freq);
>  cpufreq_freq_attr_rw(scaling_governor);
>  cpufreq_freq_attr_rw(scaling_setspeed);
> +cpufreq_freq_attr_rw(local_boost);
>  
>  static struct attribute *cpufreq_attrs[] = {
>  	&cpuinfo_min_freq.attr,
> @@ -944,6 +974,7 @@ static struct attribute *cpufreq_attrs[] = {
>  	&scaling_driver.attr,
>  	&scaling_available_governors.attr,
>  	&scaling_setspeed.attr,
> +	&local_boost.attr,

Making this file available if the global boost file isn't present is a
bit tricky I think. Maybe at least check for cpufreq_boost_supported()
before doing so.

>  	NULL
>  };
>  ATTRIBUTE_GROUPS(cpufreq);
> @@ -2716,6 +2747,8 @@ int cpufreq_boost_trigger_state(int state)
>  		ret = cpufreq_driver->set_boost(policy, state);
>  		if (ret)
>  			goto err_reset_state;
> +
> +		policy->local_boost_enabled = state;
>  	}
>  	cpus_read_unlock();
>  
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 26e2eb399484..a99a48ac9fdb 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -140,6 +140,9 @@ struct cpufreq_policy {
>  	 */
>  	bool			dvfs_possible_from_any_cpu;
>  
> +	/* Per policy boost enabled flag. */
> +	bool			local_boost_enabled;
> +
>  	 /* Cached frequency lookup from cpufreq_driver_resolve_freq. */
>  	unsigned int cached_target_freq;
>  	unsigned int cached_resolved_idx;
> -- 
> 2.30.0

-- 
viresh