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

[Jie Zhan]

Hi Viresh,

Many thanks for your sugguestion.

Happy with all the comments. Will send a v2 soon.

Jie

On 16/08/2023 14:27, Viresh Kumar wrote:
> 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@xxxxxxxxxxxxx>
> > Reviewed-by: Wei Xu <xuwei5@xxxxxxxxxxxxx>
> > ---
> >   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