[PATCH] cpufreq: Add user_min/max_freq

[PATCH] cpufreq: Add user_min/max_freq

[Leonard Crestez]

Current values in scaling_min_freq and scaling_max freq can change on
the fly due to event such as thermal monitoring. This behavior is
confusing for userspace and because once an userspace limit is written
to scaling_min/max_freq it is not possible to read it back.

Introduce two new user_min/max_freq files which only contain the limits
imposed by userspace, without any aggregation.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

---
This was motivated by these discussions:

* https://patchwork.kernel.org/patch/11078475/#22805379
* https://patchwork.kernel.org/patch/11171817/#22917099

Those threads are about devfreq but same issue applies to cpufreq as
well. Let me know if this solution seems reasonable?

An alternative would be to make scaling_min/max_freq always read back
the configured value and introduce new effective_min/max_freq files for
the aggregate values. That might break existing users (though I'm not
familiar with any).

 Documentation/admin-guide/pm/cpufreq.rst | 27 ++++++++++++++++++------
 drivers/cpufreq/cpufreq.c                | 19 +++++++++++++++++
 include/linux/pm_qos.h                   |  4 ++++
 3 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/Documentation/admin-guide/pm/cpufreq.rst b/Documentation/admin-guide/pm/cpufreq.rst
index 0c74a7784964..734c01c1040e 100644
--- a/Documentation/admin-guide/pm/cpufreq.rst
+++ b/Documentation/admin-guide/pm/cpufreq.rst
@@ -309,21 +309,36 @@ are the following:
 
 ``scaling_max_freq``
 	Maximum frequency the CPUs belonging to this policy are allowed to be
 	running at (in kHz).
 
-	This attribute is read-write and writing a string representing an
-	integer to it will cause a new limit to be set (it must not be lower
-	than the value of the ``scaling_min_freq`` attribute).
+	This attribute is read-write: writing an integer will set a new limit
+	(just like ``user_max_freq``) while reading will show the current
+	limit (potentially affected by other system contraints such as thermal
+	throttling).
 
 ``scaling_min_freq``
 	Minimum frequency the CPUs belonging to this policy are allowed to be
 	running at (in kHz).
 
-	This attribute is read-write and writing a string representing a
-	non-negative integer to it will cause a new limit to be set (it must not
-	be higher than the value of the ``scaling_max_freq`` attribute).
+	This attribute is read-write: writing an integer will set a new limit
+	(just like ``user_min_freq``) while reading will show the current
+	limit (potentially affected by other system contraints).
+
+``user_max_freq``
+	Userspace contraint for the maximum frequency the CPUs belonging to
+	this policy are allowed to be running at (in kHz).
+
+	This attribute is read-write: writing an integer will set a new limit
+	and reading will show the last limit set by userspace.
+
+``user_min_freq``
+	Userspace contraint for minimum frequency the CPUs belonging to this
+	policy are allowed to be running at (in kHz).
+
+	This attribute is read-write: writing an integer will set a new limit
+	and reading will show the last limit set by userspace.
 
 ``scaling_setspeed``
 	This attribute is functional only if the `userspace`_ scaling governor
 	is attached to the given policy.
 
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 48a224a6b178..caefed0dac43 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -722,13 +722,28 @@ static ssize_t store_##file_name					\
 									\
 	ret = freq_qos_update_request(policy->object##_freq_req, val);\
 	return ret >= 0 ? count : ret;					\
 }
 
+store_one(user_min_freq, min);
+store_one(user_max_freq, max);
 store_one(scaling_min_freq, min);
 store_one(scaling_max_freq, max);
 
+#undef show_one
+
+#define show_one(file_name, object)					\
+static ssize_t show_##file_name						\
+(struct cpufreq_policy *policy, char *buf)				\
+{									\
+	s32 val = freq_qos_get_request_value(policy->object##_freq_req);\
+	return sprintf(buf, "%d\n", val);				\
+}
+
+show_one(user_min_freq, min);
+show_one(user_max_freq, max);
+
 /**
  * show_cpuinfo_cur_freq - current CPU frequency as detected by hardware
  */
 static ssize_t show_cpuinfo_cur_freq(struct cpufreq_policy *policy,
 					char *buf)
@@ -906,10 +921,12 @@ cpufreq_freq_attr_ro(related_cpus);
 cpufreq_freq_attr_ro(affected_cpus);
 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(user_min_freq);
+cpufreq_freq_attr_rw(user_max_freq);
 
 static struct attribute *default_attrs[] = {
 	&cpuinfo_min_freq.attr,
 	&cpuinfo_max_freq.attr,
 	&cpuinfo_transition_latency.attr,
@@ -919,10 +936,12 @@ static struct attribute *default_attrs[] = {
 	&related_cpus.attr,
 	&scaling_governor.attr,
 	&scaling_driver.attr,
 	&scaling_available_governors.attr,
 	&scaling_setspeed.attr,
+	&user_min_freq.attr,
+	&user_max_freq.attr,
 	NULL
 };
 
 #define to_policy(k) container_of(k, struct cpufreq_policy, kobj)
 #define to_attr(a) container_of(a, struct freq_attr, attr)
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index e97c2e376889..90b147b7d7a3 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -310,7 +310,11 @@ int freq_qos_add_notifier(struct freq_constraints *qos,
 			  enum freq_qos_req_type type,
 			  struct notifier_block *notifier);
 int freq_qos_remove_notifier(struct freq_constraints *qos,
 			     enum freq_qos_req_type type,
 			     struct notifier_block *notifier);
+static inline s32 freq_qos_get_request_value(struct freq_qos_request *req)
+{
+	return req->pnode.prio;
+}
 
 #endif
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: Add user_min/max_freq

[Matthias Kaehlcke]

On Wed, Oct 30, 2019 at 02:41:49AM +0200, Leonard Crestez wrote:
> Current values in scaling_min_freq and scaling_max freq can change on
> the fly due to event such as thermal monitoring. This behavior is
> confusing for userspace and because once an userspace limit is written
> to scaling_min/max_freq it is not possible to read it back.

Yes, this is indeed confusing.

> Introduce two new user_min/max_freq files which only contain the limits
> imposed by userspace, without any aggregation.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> 
> ---
> This was motivated by these discussions:
> 
> * https://patchwork.kernel.org/patch/11078475/#22805379
> * https://patchwork.kernel.org/patch/11171817/#22917099
> 
> Those threads are about devfreq but same issue applies to cpufreq as
> well. Let me know if this solution seems reasonable?
> 
> An alternative would be to make scaling_min/max_freq always read back
> the configured value and introduce new effective_min/max_freq files for
> the aggregate values. That might break existing users (though I'm not
> familiar with any).

It seems there isn't really a perfect solution :(

This change creates a set of new, consistent attributes, but since we
can't make the current min/max attributes read-only userspace will keep
using them forever.

It's somewhat doubtful that userspace can do anything useful with the
current min/max values, since they might change just after being read.
Anything besides monitoring the limits (approximately) would be inherently
broken.

Having min/max return the configured value would be the expected behavior
(IMO), but obviously I also don't know for sure if there are userspace
components relying on the current behavior.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: Add user_min/max_freq

[Rafael J. Wysocki]

On Wednesday, October 30, 2019 1:41:49 AM CET Leonard Crestez wrote:
> Current values in scaling_min_freq and scaling_max freq can change on
> the fly due to event such as thermal monitoring.

Which is intentional.

> This behavior is confusing for userspace and because once an userspace
> limit is written to scaling_min/max_freq it is not possible to read it back.

That can be argued both ways.

It is also useful to know the effective constraints and arguably the ability
to read back the values that you have written is mostly needed for debugging
the code.

Also arguably, if there are multiple sources of frequency limits in user space,
there needs to be a user space arbiter deciding on which value to use and in
that case it needs to store the last value chosen by it anyway.

> Introduce two new user_min/max_freq files which only contain the limits
> imposed by userspace, without any aggregation.

I'm not sure how useful that is except for the debugging use case to be honest.

Do you have any specific use cases beyond debugging in mind?

> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
> This was motivated by these discussions:
> 
> * https://patchwork.kernel.org/patch/11078475/#22805379
> * https://patchwork.kernel.org/patch/11171817/#22917099
> 
> Those threads are about devfreq but same issue applies to cpufreq as
> well. Let me know if this solution seems reasonable?
> 
> An alternative would be to make scaling_min/max_freq always read back
> the configured value and introduce new effective_min/max_freq files for
> the aggregate values. That might break existing users (though I'm not
> familiar with any).
> 
>  Documentation/admin-guide/pm/cpufreq.rst | 27 ++++++++++++++++++------
>  drivers/cpufreq/cpufreq.c                | 19 +++++++++++++++++
>  include/linux/pm_qos.h                   |  4 ++++
>  3 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/admin-guide/pm/cpufreq.rst b/Documentation/admin-guide/pm/cpufreq.rst
> index 0c74a7784964..734c01c1040e 100644
> --- a/Documentation/admin-guide/pm/cpufreq.rst
> +++ b/Documentation/admin-guide/pm/cpufreq.rst
> @@ -309,21 +309,36 @@ are the following:
>  
>  ``scaling_max_freq``
>  	Maximum frequency the CPUs belonging to this policy are allowed to be
>  	running at (in kHz).
>  
> -	This attribute is read-write and writing a string representing an
> -	integer to it will cause a new limit to be set (it must not be lower
> -	than the value of the ``scaling_min_freq`` attribute).
> +	This attribute is read-write: writing an integer will set a new limit
> +	(just like ``user_max_freq``) while reading will show the current
> +	limit (potentially affected by other system contraints such as thermal
> +	throttling).
>  
>  ``scaling_min_freq``
>  	Minimum frequency the CPUs belonging to this policy are allowed to be
>  	running at (in kHz).
>  
> -	This attribute is read-write and writing a string representing a
> -	non-negative integer to it will cause a new limit to be set (it must not
> -	be higher than the value of the ``scaling_max_freq`` attribute).
> +	This attribute is read-write: writing an integer will set a new limit
> +	(just like ``user_min_freq``) while reading will show the current
> +	limit (potentially affected by other system contraints).
> +
> +``user_max_freq``
> +	Userspace contraint for the maximum frequency the CPUs belonging to
> +	this policy are allowed to be running at (in kHz).
> +
> +	This attribute is read-write: writing an integer will set a new limit
> +	and reading will show the last limit set by userspace.

Making these read-write is not useful IMO.  Make them read-only.

> +
> +``user_min_freq``
> +	Userspace contraint for minimum frequency the CPUs belonging to this
> +	policy are allowed to be running at (in kHz).
> +
> +	This attribute is read-write: writing an integer will set a new limit
> +	and reading will show the last limit set by userspace.
>  
>  ``scaling_setspeed``
>  	This attribute is functional only if the `userspace`_ scaling governor
>  	is attached to the given policy.
>  
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 48a224a6b178..caefed0dac43 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -722,13 +722,28 @@ static ssize_t store_##file_name					\
>  									\
>  	ret = freq_qos_update_request(policy->object##_freq_req, val);\
>  	return ret >= 0 ? count : ret;					\
>  }
>  
> +store_one(user_min_freq, min);
> +store_one(user_max_freq, max);
>  store_one(scaling_min_freq, min);
>  store_one(scaling_max_freq, max);

I don't agree with duplicating functionality like this.

>  
> +#undef show_one
> +
> +#define show_one(file_name, object)					\
> +static ssize_t show_##file_name						\
> +(struct cpufreq_policy *policy, char *buf)				\
> +{									\
> +	s32 val = freq_qos_get_request_value(policy->object##_freq_req);\
> +	return sprintf(buf, "%d\n", val);				\
> +}
> +
> +show_one(user_min_freq, min);
> +show_one(user_max_freq, max);
> +
>  /**
>   * show_cpuinfo_cur_freq - current CPU frequency as detected by hardware
>   */
>  static ssize_t show_cpuinfo_cur_freq(struct cpufreq_policy *policy,
>  					char *buf)
> @@ -906,10 +921,12 @@ cpufreq_freq_attr_ro(related_cpus);
>  cpufreq_freq_attr_ro(affected_cpus);
>  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(user_min_freq);
> +cpufreq_freq_attr_rw(user_max_freq);
>  
>  static struct attribute *default_attrs[] = {
>  	&cpuinfo_min_freq.attr,
>  	&cpuinfo_max_freq.attr,
>  	&cpuinfo_transition_latency.attr,
> @@ -919,10 +936,12 @@ static struct attribute *default_attrs[] = {
>  	&related_cpus.attr,
>  	&scaling_governor.attr,
>  	&scaling_driver.attr,
>  	&scaling_available_governors.attr,
>  	&scaling_setspeed.attr,
> +	&user_min_freq.attr,
> +	&user_max_freq.attr,
>  	NULL
>  };
>  
>  #define to_policy(k) container_of(k, struct cpufreq_policy, kobj)
>  #define to_attr(a) container_of(a, struct freq_attr, attr)
> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> index e97c2e376889..90b147b7d7a3 100644
> --- a/include/linux/pm_qos.h
> +++ b/include/linux/pm_qos.h
> @@ -310,7 +310,11 @@ int freq_qos_add_notifier(struct freq_constraints *qos,
>  			  enum freq_qos_req_type type,
>  			  struct notifier_block *notifier);
>  int freq_qos_remove_notifier(struct freq_constraints *qos,
>  			     enum freq_qos_req_type type,
>  			     struct notifier_block *notifier);
> +static inline s32 freq_qos_get_request_value(struct freq_qos_request *req)
> +{
> +	return req->pnode.prio;
> +}
>  
>  #endif
> 





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: Add user_min/max_freq

[Leonard Crestez]

On 31.10.2019 12:24, Rafael J. Wysocki wrote:
> On Wednesday, October 30, 2019 1:41:49 AM CET Leonard Crestez wrote:
>> Current values in scaling_min_freq and scaling_max freq can change on
>> the fly due to event such as thermal monitoring.
> 
> Which is intentional.
> 
>> This behavior is confusing for userspace and because once an userspace
>> limit is written to scaling_min/max_freq it is not possible to read it back.
> 
> That can be argued both ways.
> 
> It is also useful to know the effective constraints and arguably the ability
> to read back the values that you have written is mostly needed for debugging
> the code.
> 
> Also arguably, if there are multiple sources of frequency limits in user space,
> there needs to be a user space arbiter deciding on which value to use and in
> that case it needs to store the last value chosen by it anyway.

If an userspace tool needs to temporarily adjust min/max_freq it has no 
way of reliably restoring the old value.

>> Introduce two new user_min/max_freq files which only contain the limits
>> imposed by userspace, without any aggregation.
> 
> I'm not sure how useful that is except for the debugging use case to be honest.
> 
> Do you have any specific use cases beyond debugging in mind?

No. I guess it would be useful for userspace cpufreq daemons but I'm not 
familiar with any. Maybe somebody else could chime in?

Honestly it's not very useful for debugging: what you would want is a 
debugfs files which can enumerate all QoS requests in the system.

>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>> This was motivated by these discussions:
>>
>> * https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F11078475%2F%2322805379&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C9935807314f14a73eb9d08d75dec8695%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637081142759735597&amp;sdata=G0JDLEytUKMCDN2x7ccXmRHBFktPOJBbPsY52A0ppxY%3D&amp;reserved=0
>> * https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F11171817%2F%2322917099&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C9935807314f14a73eb9d08d75dec8695%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637081142759735597&amp;sdata=FLP0%2FoMMubhPMyDt2xTL%2F3xlLfR%2FK9CWIcDA6T14MFw%3D&amp;reserved=0
>>
>> Those threads are about devfreq but same issue applies to cpufreq as
>> well. Let me know if this solution seems reasonable?
>>
>> An alternative would be to make scaling_min/max_freq always read back
>> the configured value and introduce new effective_min/max_freq files for
>> the aggregate values. That might break existing users (though I'm not
>> familiar with any).
>>
>>   Documentation/admin-guide/pm/cpufreq.rst | 27 ++++++++++++++++++------
>>   drivers/cpufreq/cpufreq.c                | 19 +++++++++++++++++
>>   include/linux/pm_qos.h                   |  4 ++++
>>   3 files changed, 44 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/pm/cpufreq.rst b/Documentation/admin-guide/pm/cpufreq.rst
>> index 0c74a7784964..734c01c1040e 100644
>> --- a/Documentation/admin-guide/pm/cpufreq.rst
>> +++ b/Documentation/admin-guide/pm/cpufreq.rst
>> @@ -309,21 +309,36 @@ are the following:
>>   
>>   ``scaling_max_freq``
>>   	Maximum frequency the CPUs belonging to this policy are allowed to be
>>   	running at (in kHz).
>>   
>> -	This attribute is read-write and writing a string representing an
>> -	integer to it will cause a new limit to be set (it must not be lower
>> -	than the value of the ``scaling_min_freq`` attribute).
>> +	This attribute is read-write: writing an integer will set a new limit
>> +	(just like ``user_max_freq``) while reading will show the current
>> +	limit (potentially affected by other system contraints such as thermal
>> +	throttling).
>>   
>>   ``scaling_min_freq``
>>   	Minimum frequency the CPUs belonging to this policy are allowed to be
>>   	running at (in kHz).
>>   
>> -	This attribute is read-write and writing a string representing a
>> -	non-negative integer to it will cause a new limit to be set (it must not
>> -	be higher than the value of the ``scaling_max_freq`` attribute).
>> +	This attribute is read-write: writing an integer will set a new limit
>> +	(just like ``user_min_freq``) while reading will show the current
>> +	limit (potentially affected by other system contraints).
>> +
>> +``user_max_freq``
>> +	Userspace contraint for the maximum frequency the CPUs belonging to
>> +	this policy are allowed to be running at (in kHz).
>> +
>> +	This attribute is read-write: writing an integer will set a new limit
>> +	and reading will show the last limit set by userspace.
> 
> Making these read-write is not useful IMO.  Make them read-only.
> 
>> +
>> +``user_min_freq``
>> +	Userspace contraint for minimum frequency the CPUs belonging to this
>> +	policy are allowed to be running at (in kHz).
>> +
>> +	This attribute is read-write: writing an integer will set a new limit
>> +	and reading will show the last limit set by userspace.
>>   
>>   ``scaling_setspeed``
>>   	This attribute is functional only if the `userspace`_ scaling governor
>>   	is attached to the given policy.
>>   
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 48a224a6b178..caefed0dac43 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -722,13 +722,28 @@ static ssize_t store_##file_name					\
>>   									\
>>   	ret = freq_qos_update_request(policy->object##_freq_req, val);\
>>   	return ret >= 0 ? count : ret;					\
>>   }
>>   
>> +store_one(user_min_freq, min);
>> +store_one(user_max_freq, max);
>>   store_one(scaling_min_freq, min);
>>   store_one(scaling_max_freq, max);
> 
> I don't agree with duplicating functionality like this.

OK. My logic was that a tool which doesn't want to deal with the 
"effective" limits would switch to always reading/writing 
user_min/max_freq if they're present.

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: Add user_min/max_freq

[Rafael J. Wysocki]

On Thu, Oct 31, 2019 at 2:01 PM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> On 31.10.2019 12:24, Rafael J. Wysocki wrote:
> > On Wednesday, October 30, 2019 1:41:49 AM CET Leonard Crestez wrote:
> >> Current values in scaling_min_freq and scaling_max freq can change on
> >> the fly due to event such as thermal monitoring.
> >
> > Which is intentional.
> >
> >> This behavior is confusing for userspace and because once an userspace
> >> limit is written to scaling_min/max_freq it is not possible to read it back.
> >
> > That can be argued both ways.
> >
> > It is also useful to know the effective constraints and arguably the ability
> > to read back the values that you have written is mostly needed for debugging
> > the code.
> >
> > Also arguably, if there are multiple sources of frequency limits in user space,
> > there needs to be a user space arbiter deciding on which value to use and in
> > that case it needs to store the last value chosen by it anyway.
>
> If an userspace tool needs to temporarily adjust min/max_freq it has no
> way of reliably restoring the old value.

And the new attributes don't really help here AFAICS, because if the
old value was written by a user space task different from the one
updating it, that task may try to update it again in parallel with the
current writer, and so the current writer actually doesn't know
whether or not the value it has read is the most recent one (and even
so, whether or not writing it back is desirable anyway).

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: Add user_min/max_freq

[Matthias Kaehlcke]

On Thu, Oct 31, 2019 at 11:24:31AM +0100, Rafael J. Wysocki wrote:
> On Wednesday, October 30, 2019 1:41:49 AM CET Leonard Crestez wrote:
> > Current values in scaling_min_freq and scaling_max freq can change on
> > the fly due to event such as thermal monitoring.
> 
> Which is intentional.
> 
> > This behavior is confusing for userspace and because once an userspace
> > limit is written to scaling_min/max_freq it is not possible to read it back.
> 
> That can be argued both ways.
> 
> It is also useful to know the effective constraints and arguably the ability
> to read back the values that you have written is mostly needed for debugging
> the code.

Agreed that reading the values back is probably mostly useful for debugging.

Reading the effective constraints is a debugging use case as well, userspace
can't make any decisions based on values which might be in constant flux.

IMO the current interface is completely counterintuitive, I really hope we
wouldn't implement it the same way if given a chance to do it again. If there
is use for reading the effective constraints it should be exposed as a separate
read-only attribute. Keep it simple (when possible).

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: Add user_min/max_freq

[Rafael J. Wysocki]

On Thu, Oct 31, 2019 at 11:24 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Thu, Oct 31, 2019 at 11:24:31AM +0100, Rafael J. Wysocki wrote:
> > On Wednesday, October 30, 2019 1:41:49 AM CET Leonard Crestez wrote:
> > > Current values in scaling_min_freq and scaling_max freq can change on
> > > the fly due to event such as thermal monitoring.
> >
> > Which is intentional.
> >
> > > This behavior is confusing for userspace and because once an userspace
> > > limit is written to scaling_min/max_freq it is not possible to read it back.
> >
> > That can be argued both ways.
> >
> > It is also useful to know the effective constraints and arguably the ability
> > to read back the values that you have written is mostly needed for debugging
> > the code.
>
> Agreed that reading the values back is probably mostly useful for debugging.
>
> Reading the effective constraints is a debugging use case as well, userspace
> can't make any decisions based on values which might be in constant flux.

It sometimes helps to diagnose issues on production systems, though.
If you see those numbers change even without writing to the
attributes, for example, that indicates the existence of a source of
frequency constraints in the system (e.g. platform firmware) that
might have gone unnoticed otherwise.

> IMO the current interface is completely counterintuitive, I really hope we
> wouldn't implement it the same way if given a chance to do it again. If there
> is use for reading the effective constraints it should be exposed as a separate
> read-only attribute. Keep it simple (when possible).

I agree that such a design would have been better, but the existing
one cannot be changed this way now.  And I had not invented it. :-)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel