[PATCH 1/4] base/drivers/arch_topology: Remove useless check

[PATCH 1/4] base/drivers/arch_topology: Remove useless check

[Daniel Lezcano]

The function 'register_cpufreq_notifier' registers the
init_cpu_capacity_notifier() only if raw_capacity is not NULL.

Hence init_cpu_capacity_notifier() can not be called with raw_capacity
set to NULL, it is pointless to check it.

Remove the check.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/base/arch_topology.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index e7cb0c6..204ed10 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -181,9 +181,6 @@ init_cpu_capacity_callback(struct notifier_block *nb,
 	struct cpufreq_policy *policy = data;
 	int cpu;
 
-	if (!raw_capacity)
-		return 0;
-
 	if (val != CPUFREQ_NOTIFY)
 		return 0;
 
-- 
2.7.4

[PATCH 2/4] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE

[Daniel Lezcano]

The mutex protects a per_cpu variable access. The potential race can
happen only when the cpufreq governor module is loaded and at the same
time the cpu capacity is changed in the sysfs.

There is no real interest of using a mutex to protect a variable
assignation when there is no situation where a task can take the lock
and block.

Replace the mutex by READ_ONCE / WRITE_ONCE.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/base/arch_topology.c  | 7 +------
 include/linux/arch_topology.h | 2 +-
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 204ed10..b19d6d4 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -30,12 +30,11 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
 		per_cpu(freq_scale, i) = scale;
 }
 
-static DEFINE_MUTEX(cpu_scale_mutex);
 DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
 
 void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity)
 {
-	per_cpu(cpu_scale, cpu) = capacity;
+	WRITE_ONCE(per_cpu(cpu_scale, cpu), capacity);
 }
 
 static ssize_t cpu_capacity_show(struct device *dev,
@@ -67,10 +66,8 @@ static ssize_t cpu_capacity_store(struct device *dev,
 	if (new_capacity > SCHED_CAPACITY_SCALE)
 		return -EINVAL;
 
-	mutex_lock(&cpu_scale_mutex);
 	for_each_cpu(i, &cpu_topology[this_cpu].core_sibling)
 		topology_set_cpu_scale(i, new_capacity);
-	mutex_unlock(&cpu_scale_mutex);
 
 	return count;
 }
@@ -116,7 +113,6 @@ void topology_normalize_cpu_scale(void)
 		return;
 
 	pr_debug("cpu_capacity: capacity_scale=%u\n", capacity_scale);
-	mutex_lock(&cpu_scale_mutex);
 	for_each_possible_cpu(cpu) {
 		pr_debug("cpu_capacity: cpu=%d raw_capacity=%u\n",
 			 cpu, raw_capacity[cpu]);
@@ -126,7 +122,6 @@ void topology_normalize_cpu_scale(void)
 		pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
 			cpu, topology_get_cpu_scale(NULL, cpu));
 	}
-	mutex_unlock(&cpu_scale_mutex);
 }
 
 bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index 2b70941..7c0aaa9 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -19,7 +19,7 @@ struct sched_domain;
 static inline
 unsigned long topology_get_cpu_scale(struct sched_domain *sd, int cpu)
 {
-	return per_cpu(cpu_scale, cpu);
+	return READ_ONCE(per_cpu(cpu_scale, cpu));
 }
 
 void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity);
-- 
2.7.4

[PATCH 3/4] base/drivers/topology: Move instructions in the error path

[Daniel Lezcano]

When the function topology_parse_cpu_capacity() fails, we set the boolean
cap_parsing_failed to true and we free the raw_capacity. This is correct as
the function begins with a check against cap_parsing_failed thus protecting
the function to be re-entered.

However, even it is impossible that can happen with the current code, let's
move in the instructions:

 - cap_parsing_failed = true;
 - free_raw_capacity();

 ... in the 'else' block when the error is detected, that is more semantically
 correct.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/base/arch_topology.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index b19d6d4..7311641 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -155,9 +155,9 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
 			pr_err("cpu_capacity: missing %pOF raw capacity\n",
 				cpu_node);
 			pr_err("cpu_capacity: partial information: fallback to 1024 for all CPUs\n");
+			cap_parsing_failed = true;
+			free_raw_capacity();
 		}
-		cap_parsing_failed = true;
-		free_raw_capacity();
 	}
 
 	return !ret;
-- 
2.7.4

[PATCH 4/4] base/drivers/topology: Default dmpis-mhz if they are not set in DT

[Daniel Lezcano]

In the case of assymetric SoC with the same micro-architecture, we
have a group of CPUs with smaller OPPs than the other group. One
example is the 96boards dragonboard 820c. There is no dmips/MHz
difference between both groups, so no need to specify the values in
the DT. Unfortunately, without these defined, there is no scaling
capacity comutation triggered, so we need to write
'capacity-dmips-mhz' for each CPU with the same value in order to
force the scaled capacity computation.

Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no
'capacity-dmips-mhz' is defined in the DT.

This was tested on db820c:
 - specified values in the DT (correct results)
 - partial values defined in the DT (error + fallback to defaults)
 - no specified values in the DT (correct results)

correct results are:
  cat /sys/devices/system/cpu/cpu*/cpu_capacity
   758
   758
  1024
  1024

  ... respectively for CPU0, CPU1, CPU2 and CPU3.

That reflects the capacity for the max frequencies 1593600 and 2150400.

Cc: Chris Redpath <chris.redpath@linaro.org>
Cc: Quentin Perret <quentin.perret@linaro.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Amit Kucheria <amit.kucheria@linaro.org>
Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>
Cc: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/base/arch_topology.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 7311641..7d594a6 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -205,6 +205,21 @@ static struct notifier_block init_cpu_capacity_notifier = {
 	.notifier_call = init_cpu_capacity_callback,
 };
 
+static int topology_set_default_capacity(void)
+{
+	int cpu;
+
+	raw_capacity = kzalloc(num_possible_cpus() * sizeof(*raw_capacity),
+			       GFP_KERNEL);
+	if (!raw_capacity)
+		return -ENOMEM;
+
+	for_each_possible_cpu(cpu)
+		raw_capacity[cpu] = SCHED_CAPACITY_SCALE;
+
+	return 0;
+}
+
 static int __init register_cpufreq_notifier(void)
 {
 	int ret;
@@ -214,9 +229,19 @@ static int __init register_cpufreq_notifier(void)
 	 * until we have the necessary code to parse the cpu capacity, so
 	 * skip registering cpufreq notifier.
 	 */
-	if (!acpi_disabled || !raw_capacity)
+	if (!acpi_disabled)
 		return -EINVAL;
 
+	if (!raw_capacity) {
+
+		pr_info("cpu_capacity: No capacity defined in DT, set default "
+		       "values to %ld\n", SCHED_CAPACITY_SCALE);
+
+		ret = topology_set_default_capacity();
+		if (ret)
+			return ret;
+	}
+
 	if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) {
 		pr_err("cpu_capacity: failed to allocate memory for cpus_to_visit\n");
 		return -ENOMEM;
-- 
2.7.4

Re: [PATCH 1/4] base/drivers/arch_topology: Remove useless check

[Viresh Kumar]

On Mon, Oct 29, 2018 at 9:56 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:

Would have been better if I was cc'd on all the patches since I was
looking at this
stuff actively this week :)

> The function 'register_cpufreq_notifier' registers the
> init_cpu_capacity_notifier() only if raw_capacity is not NULL.
>
> Hence init_cpu_capacity_notifier() can not be called with raw_capacity
> set to NULL, it is pointless to check it.

It isn't entirely pointless though.

It is possible for init_cpu_capacity_notifier() to get called after
free_raw_capacity()
is called from it as the notifier unregistration happens from a workqueue.

Re: [PATCH 2/4] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE

[Viresh Kumar]

On Mon, Oct 29, 2018 at 9:54 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> The mutex protects a per_cpu variable access. The potential race can
> happen only when the cpufreq governor module is loaded and at the same
> time the cpu capacity is changed in the sysfs.
>
> There is no real interest of using a mutex to protect a variable
> assignation when there is no situation where a task can take the lock
> and block.
>
> Replace the mutex by READ_ONCE / WRITE_ONCE.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/base/arch_topology.c  | 7 +------
>  include/linux/arch_topology.h | 2 +-
>  2 files changed, 2 insertions(+), 7 deletions(-)

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH 3/4] base/drivers/topology: Move instructions in the error path

[Viresh Kumar]

On Mon, Oct 29, 2018 at 9:56 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> When the function topology_parse_cpu_capacity() fails, we set the boolean
> cap_parsing_failed to true and we free the raw_capacity. This is correct as
> the function begins with a check against cap_parsing_failed thus protecting
> the function to be re-entered.
>
> However, even it is impossible that can happen with the current code, let's

Why impossible ?

> move in the instructions:
>
>  - cap_parsing_failed = true;
>  - free_raw_capacity();
>
>  ... in the 'else' block when the error is detected, that is more semantically
>  correct.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/base/arch_topology.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index b19d6d4..7311641 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -155,9 +155,9 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
>                         pr_err("cpu_capacity: missing %pOF raw capacity\n",
>                                 cpu_node);
>                         pr_err("cpu_capacity: partial information: fallback to 1024 for all CPUs\n");
> +                       cap_parsing_failed = true;
> +                       free_raw_capacity();
>                 }
> -               cap_parsing_failed = true;
> -               free_raw_capacity();

While it is fine to move free_raw_capacity(), it is not to move the
other line. With your
patch what will happen if the first CPU in DT doesn't have the
"capacity-dmips-mhz"
property set ? We will never set cap_parsing_failed and keep on
re-entering this routine
which wasn't required.

Note that the current implementation isn't written to always print an
error where this
property is only partially filled and the same wouldn't happen with
your patch as well.

--
viresh

Re: [PATCH 4/4] base/drivers/topology: Default dmpis-mhz if they are not set in DT

[Viresh Kumar]

On 29-10-18, 17:23, Daniel Lezcano wrote:
> In the case of assymetric SoC with the same micro-architecture, we
> have a group of CPUs with smaller OPPs than the other group. One
> example is the 96boards dragonboard 820c. There is no dmips/MHz
> difference between both groups, so no need to specify the values in
> the DT. Unfortunately, without these defined, there is no scaling
> capacity comutation triggered, so we need to write
> 'capacity-dmips-mhz' for each CPU with the same value in order to
> force the scaled capacity computation.
> 
> Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no
> 'capacity-dmips-mhz' is defined in the DT.
> 
> This was tested on db820c:
>  - specified values in the DT (correct results)
>  - partial values defined in the DT (error + fallback to defaults)
>  - no specified values in the DT (correct results)
> 
> correct results are:
>   cat /sys/devices/system/cpu/cpu*/cpu_capacity
>    758
>    758
>   1024
>   1024
> 
>   ... respectively for CPU0, CPU1, CPU2 and CPU3.
> 
> That reflects the capacity for the max frequencies 1593600 and 2150400.
> 
> Cc: Chris Redpath <chris.redpath@linaro.org>
> Cc: Quentin Perret <quentin.perret@linaro.org>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Amit Kucheria <amit.kucheria@linaro.org>
> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>
> Cc: Niklas Cassel <niklas.cassel@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/base/arch_topology.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 7311641..7d594a6 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -205,6 +205,21 @@ static struct notifier_block init_cpu_capacity_notifier = {
>  	.notifier_call = init_cpu_capacity_callback,
>  };
>  
> +static int topology_set_default_capacity(void)
> +{
> +	int cpu;
> +
> +	raw_capacity = kzalloc(num_possible_cpus() * sizeof(*raw_capacity),
> +			       GFP_KERNEL);

kmalloc ?

> +	if (!raw_capacity)
> +		return -ENOMEM;
> +
> +	for_each_possible_cpu(cpu)
> +		raw_capacity[cpu] = SCHED_CAPACITY_SCALE;

It makes it look as if it is important to set default values to
SCHED_CAPACITY_SCALE while that is not the case. Maybe add a comment
that all CPUs are assumed to have same capacity now. Maybe initialize
to 1 instead ? Not sure if that would be better or not.

> +
> +	return 0;
> +}
> +
>  static int __init register_cpufreq_notifier(void)
>  {
>  	int ret;
> @@ -214,9 +229,19 @@ static int __init register_cpufreq_notifier(void)
>  	 * until we have the necessary code to parse the cpu capacity, so
>  	 * skip registering cpufreq notifier.
>  	 */
> -	if (!acpi_disabled || !raw_capacity)
> +	if (!acpi_disabled)
>  		return -EINVAL;
>  
> +	if (!raw_capacity) {
> +
> +		pr_info("cpu_capacity: No capacity defined in DT, set default "
> +		       "values to %ld\n", SCHED_CAPACITY_SCALE);

So we will end up doing this also for the case where the DT is
partially filled with the capacity info and we already printed error
messages for it. Should we really do that ?

-- 
viresh

Re: [PATCH 1/4] base/drivers/arch_topology: Remove useless check

[Daniel Lezcano]

On 30/10/2018 06:50, Viresh Kumar wrote:
> On Mon, Oct 29, 2018 at 9:56 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> 
> Would have been better if I was cc'd on all the patches since I was
> looking at this
> stuff actively this week :)

ah, yes. Sorry for that.


>> The function 'register_cpufreq_notifier' registers the
>> init_cpu_capacity_notifier() only if raw_capacity is not NULL.
>>
>> Hence init_cpu_capacity_notifier() can not be called with raw_capacity
>> set to NULL, it is pointless to check it.
> 
> It isn't entirely pointless though.
> 
> It is possible for init_cpu_capacity_notifier() to get called after
> free_raw_capacity()
> is called from it as the notifier unregistration happens from a workqueue.

The workqueue is called from init_cpu_capacity_callback(). This one is
called in the notifier callback. IOW the notification callback
unregisters itself. But if it is not registered, it won't unregister,
hence it won't call the workqueue and init_cpu_capacity_notifier() is
not called.



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 3/4] base/drivers/topology: Move instructions in the error path

[Daniel Lezcano]

On 30/10/2018 07:12, Viresh Kumar wrote:
> On Mon, Oct 29, 2018 at 9:56 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> When the function topology_parse_cpu_capacity() fails, we set the boolean
>> cap_parsing_failed to true and we free the raw_capacity. This is correct as
>> the function begins with a check against cap_parsing_failed thus protecting
>> the function to be re-entered.
>>
>> However, even it is impossible that can happen with the current code, let's
> 
> Why impossible ?
>> move in the instructions:
>>
>>  - cap_parsing_failed = true;
>>  - free_raw_capacity();
>>
>>  ... in the 'else' block when the error is detected, that is more semantically
>>  correct.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/base/arch_topology.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>> index b19d6d4..7311641 100644
>> --- a/drivers/base/arch_topology.c
>> +++ b/drivers/base/arch_topology.c
>> @@ -155,9 +155,9 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
>>                         pr_err("cpu_capacity: missing %pOF raw capacity\n",
>>                                 cpu_node);
>>                         pr_err("cpu_capacity: partial information: fallback to 1024 for all CPUs\n");
>> +                       cap_parsing_failed = true;
>> +                       free_raw_capacity();
>>                 }
>> -               cap_parsing_failed = true;
>> -               free_raw_capacity();
> 
> While it is fine to move free_raw_capacity(), it is not to move the
> other line. With your
> patch what will happen if the first CPU in DT doesn't have the
> "capacity-dmips-mhz"
> property set ? We will never set cap_parsing_failed and keep on
> re-entering this routine
> which wasn't required.

Ok.


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/4] base/drivers/arch_topology: Remove useless check

[Viresh Kumar]

On 30-10-18, 08:55, Daniel Lezcano wrote:
> The workqueue is called from init_cpu_capacity_callback(). This one is
> called in the notifier callback. IOW the notification callback
> unregisters itself. But if it is not registered, it won't unregister,
> hence it won't call the workqueue and init_cpu_capacity_notifier() is
> not called.

Sorry, couldn't understand most of your reply :)

Though let me try to explain the problem I was trying to show you..

cpufreq-notifier-callback
        -> init_cpu_capacity_callback()
                -> free raw capacity
                -> queue_work() to unregister the notifier
                -> return

        ->init_cpu_capacity_callback() called again before work was processed.
                -> If we don't check raw_capacity, we may end up using
                NULL pointer, as the notifier isn't unregistered yet.

-- 
viresh

Re: [PATCH 4/4] base/drivers/topology: Default dmpis-mhz if they are not set in DT

[Daniel Lezcano]

On 30/10/2018 08:13, Viresh Kumar wrote:
> On 29-10-18, 17:23, Daniel Lezcano wrote:
>> In the case of assymetric SoC with the same micro-architecture, we
>> have a group of CPUs with smaller OPPs than the other group. One
>> example is the 96boards dragonboard 820c. There is no dmips/MHz
>> difference between both groups, so no need to specify the values in
>> the DT. Unfortunately, without these defined, there is no scaling
>> capacity comutation triggered, so we need to write
>> 'capacity-dmips-mhz' for each CPU with the same value in order to
>> force the scaled capacity computation.
>>
>> Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no
>> 'capacity-dmips-mhz' is defined in the DT.
>>
>> This was tested on db820c:
>>  - specified values in the DT (correct results)
>>  - partial values defined in the DT (error + fallback to defaults)
>>  - no specified values in the DT (correct results)
>>
>> correct results are:
>>   cat /sys/devices/system/cpu/cpu*/cpu_capacity
>>    758
>>    758
>>   1024
>>   1024
>>
>>   ... respectively for CPU0, CPU1, CPU2 and CPU3.
>>
>> That reflects the capacity for the max frequencies 1593600 and 2150400.
>>
>> Cc: Chris Redpath <chris.redpath@linaro.org>
>> Cc: Quentin Perret <quentin.perret@linaro.org>
>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>> Cc: Amit Kucheria <amit.kucheria@linaro.org>
>> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>
>> Cc: Niklas Cassel <niklas.cassel@linaro.org>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/base/arch_topology.c | 27 ++++++++++++++++++++++++++-
>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>> index 7311641..7d594a6 100644
>> --- a/drivers/base/arch_topology.c
>> +++ b/drivers/base/arch_topology.c
>> @@ -205,6 +205,21 @@ static struct notifier_block init_cpu_capacity_notifier = {
>>  	.notifier_call = init_cpu_capacity_callback,
>>  };
>>  
>> +static int topology_set_default_capacity(void)
>> +{
>> +	int cpu;
>> +
>> +	raw_capacity = kzalloc(num_possible_cpus() * sizeof(*raw_capacity),
>> +			       GFP_KERNEL);
> 
> kmalloc ?
> 
>> +	if (!raw_capacity)
>> +		return -ENOMEM;
>> +
>> +	for_each_possible_cpu(cpu)
>> +		raw_capacity[cpu] = SCHED_CAPACITY_SCALE;
> 
> It makes it look as if it is important to set default values to
> SCHED_CAPACITY_SCALE while that is not the case. Maybe add a comment
> that all CPUs are assumed to have same capacity now. Maybe initialize
> to 1 instead ? Not sure if that would be better or not.

SCHED_CAPACITY_SCALE is the default value in this file.

eg.

DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE
...
pr_err("cpu_capacity: partial information: fallback to 1024 for all
CPUs\n");
...

So I prefer to use the SCHED_CAPACITY_SCALE for consistency.

>> +
>> +	return 0;
>> +}
>> +
>>  static int __init register_cpufreq_notifier(void)
>>  {
>>  	int ret;
>> @@ -214,9 +229,19 @@ static int __init register_cpufreq_notifier(void)
>>  	 * until we have the necessary code to parse the cpu capacity, so
>>  	 * skip registering cpufreq notifier.
>>  	 */
>> -	if (!acpi_disabled || !raw_capacity)
>> +	if (!acpi_disabled)
>>  		return -EINVAL;
>>  
>> +	if (!raw_capacity) {
>> +
>> +		pr_info("cpu_capacity: No capacity defined in DT, set default "
>> +		       "values to %ld\n", SCHED_CAPACITY_SCALE);
> 
> So we will end up doing this also for the case where the DT is
> partially filled with the capacity info and we already printed error
> messages for it. Should we really do that ?

Yes, it is the default behavior as stated in the documentation:

"capacity-dmips-mhz property is all-or-nothing: if it is specified for a
cpu node, it has to be specified for every other cpu nodes, or the
system will fall back to the default capacity value for every CPU."

and also in the error message:

pr_err("cpu_capacity: partial information: fallback to 1024 for all
CPUs\n");

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 4/4] base/drivers/topology: Default dmpis-mhz if they are not set in DT

[Viresh Kumar]

On 30-10-18, 09:39, Daniel Lezcano wrote:
> SCHED_CAPACITY_SCALE is the default value in this file.
> 
> eg.
> 
> DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE
> ...
> pr_err("cpu_capacity: partial information: fallback to 1024 for all
> CPUs\n");
> ...
> 
> So I prefer to use the SCHED_CAPACITY_SCALE for consistency.

> Yes, it is the default behavior as stated in the documentation:
> 
> "capacity-dmips-mhz property is all-or-nothing: if it is specified for a
> cpu node, it has to be specified for every other cpu nodes, or the
> system will fall back to the default capacity value for every CPU."
> 
> and also in the error message:
> 
> pr_err("cpu_capacity: partial information: fallback to 1024 for all
> CPUs\n");

Okay.

-- 
viresh

Re: [PATCH 4/4] base/drivers/topology: Default dmpis-mhz if they are not set in DT

[Viresh Kumar]

s/dmpis/dmips/ in $subject

On 29-10-18, 17:23, Daniel Lezcano wrote:
> In the case of assymetric SoC with the same micro-architecture, we

                 asymmetric ?

> have a group of CPUs with smaller OPPs than the other group. One
> example is the 96boards dragonboard 820c. There is no dmips/MHz
> difference between both groups, so no need to specify the values in
> the DT. Unfortunately, without these defined, there is no scaling
> capacity comutation triggered, so we need to write

           computation

> 'capacity-dmips-mhz' for each CPU with the same value in order to
> force the scaled capacity computation.
> 
> Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no
> 'capacity-dmips-mhz' is defined in the DT.
> 
> This was tested on db820c:
>  - specified values in the DT (correct results)
>  - partial values defined in the DT (error + fallback to defaults)
>  - no specified values in the DT (correct results)
> 
> correct results are:
>   cat /sys/devices/system/cpu/cpu*/cpu_capacity
>    758
>    758
>   1024
>   1024
> 
>   ... respectively for CPU0, CPU1, CPU2 and CPU3.
> 
> That reflects the capacity for the max frequencies 1593600 and 2150400.
> 
> Cc: Chris Redpath <chris.redpath@linaro.org>
> Cc: Quentin Perret <quentin.perret@linaro.org>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Amit Kucheria <amit.kucheria@linaro.org>
> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>
> Cc: Niklas Cassel <niklas.cassel@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/base/arch_topology.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 7311641..7d594a6 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -205,6 +205,21 @@ static struct notifier_block init_cpu_capacity_notifier = {
>  	.notifier_call = init_cpu_capacity_callback,
>  };
>  
> +static int topology_set_default_capacity(void)
> +{
> +	int cpu;
> +
> +	raw_capacity = kzalloc(num_possible_cpus() * sizeof(*raw_capacity),
> +			       GFP_KERNEL);
> +	if (!raw_capacity)
> +		return -ENOMEM;
> +
> +	for_each_possible_cpu(cpu)
> +		raw_capacity[cpu] = SCHED_CAPACITY_SCALE;

This isn't actually required as the value of raw_capacity isn't used
anymore after this point in code. Rather it is forcefully updated in
init_cpu_capacity_callback():

raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *
                    policy->cpuinfo.max_freq / 1000UL;

Maybe it is better to allocate raw_capacity once at boot and use
another global variable as flag (raw_capacity is used as a flag right
now at many places).

-- 
viresh

Re: [PATCH 1/4] base/drivers/arch_topology: Remove useless check

[Daniel Lezcano]

On 30/10/2018 09:33, Viresh Kumar wrote:
> On 30-10-18, 08:55, Daniel Lezcano wrote:
>> The workqueue is called from init_cpu_capacity_callback(). This one is
>> called in the notifier callback. IOW the notification callback
>> unregisters itself. But if it is not registered, it won't unregister,
>> hence it won't call the workqueue and init_cpu_capacity_notifier() is
>> not called.
> 
> Sorry, couldn't understand most of your reply :)

Logical, I didn't understand your initial comment :)

> Though let me try to explain the problem I was trying to show you..
> 
> cpufreq-notifier-callback
>         -> init_cpu_capacity_callback()
>                 -> free raw capacity
>                 -> queue_work() to unregister the notifier
>                 -> return
> 
>         ->init_cpu_capacity_callback() called again before work was processed.
>                 -> If we don't check raw_capacity, we may end up using
>                 NULL pointer, as the notifier isn't unregistered yet.

Oh, nice catch.

I'm wondering if having a statically per_cpu variable, even if it is not
free at the end, isn't worth regarding the twisted code we end up with
an allocation.




-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/4] base/drivers/arch_topology: Remove useless check

[Viresh Kumar]

On 30-10-18, 14:35, Daniel Lezcano wrote:
> I'm wondering if having a statically per_cpu variable, even if it is not
> free at the end, isn't worth regarding the twisted code we end up with
> an allocation.

Maybe yeah.

-- 
viresh

Re: [PATCH 4/4] base/drivers/topology: Default dmpis-mhz if they are not set in DT

[Daniel Lezcano]

On 30/10/2018 09:58, Viresh Kumar wrote:
> s/dmpis/dmips/ in $subject
> 
> On 29-10-18, 17:23, Daniel Lezcano wrote:
>> In the case of assymetric SoC with the same micro-architecture, we
> 
>                  asymmetric ?
> 
>> have a group of CPUs with smaller OPPs than the other group. One
>> example is the 96boards dragonboard 820c. There is no dmips/MHz
>> difference between both groups, so no need to specify the values in
>> the DT. Unfortunately, without these defined, there is no scaling
>> capacity comutation triggered, so we need to write
> 
>            computation
> 
>> 'capacity-dmips-mhz' for each CPU with the same value in order to
>> force the scaled capacity computation.
>>
>> Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no
>> 'capacity-dmips-mhz' is defined in the DT.
>>
>> This was tested on db820c:
>>  - specified values in the DT (correct results)
>>  - partial values defined in the DT (error + fallback to defaults)
>>  - no specified values in the DT (correct results)
>>
>> correct results are:
>>   cat /sys/devices/system/cpu/cpu*/cpu_capacity
>>    758
>>    758
>>   1024
>>   1024
>>
>>   ... respectively for CPU0, CPU1, CPU2 and CPU3.
>>
>> That reflects the capacity for the max frequencies 1593600 and 2150400.
>>
>> Cc: Chris Redpath <chris.redpath@linaro.org>
>> Cc: Quentin Perret <quentin.perret@linaro.org>
>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>> Cc: Amit Kucheria <amit.kucheria@linaro.org>
>> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>
>> Cc: Niklas Cassel <niklas.cassel@linaro.org>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/base/arch_topology.c | 27 ++++++++++++++++++++++++++-
>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>> index 7311641..7d594a6 100644
>> --- a/drivers/base/arch_topology.c
>> +++ b/drivers/base/arch_topology.c
>> @@ -205,6 +205,21 @@ static struct notifier_block init_cpu_capacity_notifier = {
>>  	.notifier_call = init_cpu_capacity_callback,
>>  };
>>  
>> +static int topology_set_default_capacity(void)
>> +{
>> +	int cpu;
>> +
>> +	raw_capacity = kzalloc(num_possible_cpus() * sizeof(*raw_capacity),
>> +			       GFP_KERNEL);
>> +	if (!raw_capacity)
>> +		return -ENOMEM;
>> +
>> +	for_each_possible_cpu(cpu)
>> +		raw_capacity[cpu] = SCHED_CAPACITY_SCALE;
> 
> This isn't actually required as the value of raw_capacity isn't used
> anymore after this point in code. Rather it is forcefully updated in
> init_cpu_capacity_callback():
> 
> raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *
>                     policy->cpuinfo.max_freq / 1000UL;
> 
> Maybe it is better to allocate raw_capacity once at boot and use
> another global variable as flag (raw_capacity is used as a flag right
> now at many places).

Can we keep the proposed change as is to simply fix the default value?

I want to do a separate change with a raw_capacity rewrite and remove
the workqueue freeing it.


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 4/4] base/drivers/topology: Default dmpis-mhz if they are not set in DT

[Viresh Kumar]

On 21-11-18, 23:12, Daniel Lezcano wrote:
> On 30/10/2018 09:58, Viresh Kumar wrote:
> > s/dmpis/dmips/ in $subject
> > 
> > On 29-10-18, 17:23, Daniel Lezcano wrote:
> >> In the case of assymetric SoC with the same micro-architecture, we
> > 
> >                  asymmetric ?
> > 
> >> have a group of CPUs with smaller OPPs than the other group. One
> >> example is the 96boards dragonboard 820c. There is no dmips/MHz
> >> difference between both groups, so no need to specify the values in
> >> the DT. Unfortunately, without these defined, there is no scaling
> >> capacity comutation triggered, so we need to write
> > 
> >            computation
> > 
> >> 'capacity-dmips-mhz' for each CPU with the same value in order to
> >> force the scaled capacity computation.
> >>
> >> Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no
> >> 'capacity-dmips-mhz' is defined in the DT.
> >>
> >> This was tested on db820c:
> >>  - specified values in the DT (correct results)
> >>  - partial values defined in the DT (error + fallback to defaults)
> >>  - no specified values in the DT (correct results)
> >>
> >> correct results are:
> >>   cat /sys/devices/system/cpu/cpu*/cpu_capacity
> >>    758
> >>    758
> >>   1024
> >>   1024
> >>
> >>   ... respectively for CPU0, CPU1, CPU2 and CPU3.
> >>
> >> That reflects the capacity for the max frequencies 1593600 and 2150400.
> >>
> >> Cc: Chris Redpath <chris.redpath@linaro.org>
> >> Cc: Quentin Perret <quentin.perret@linaro.org>
> >> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> >> Cc: Amit Kucheria <amit.kucheria@linaro.org>
> >> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>
> >> Cc: Niklas Cassel <niklas.cassel@linaro.org>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> ---
> >>  drivers/base/arch_topology.c | 27 ++++++++++++++++++++++++++-
> >>  1 file changed, 26 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> >> index 7311641..7d594a6 100644
> >> --- a/drivers/base/arch_topology.c
> >> +++ b/drivers/base/arch_topology.c
> >> @@ -205,6 +205,21 @@ static struct notifier_block init_cpu_capacity_notifier = {
> >>  	.notifier_call = init_cpu_capacity_callback,
> >>  };
> >>  
> >> +static int topology_set_default_capacity(void)
> >> +{
> >> +	int cpu;
> >> +
> >> +	raw_capacity = kzalloc(num_possible_cpus() * sizeof(*raw_capacity),
> >> +			       GFP_KERNEL);
> >> +	if (!raw_capacity)
> >> +		return -ENOMEM;
> >> +
> >> +	for_each_possible_cpu(cpu)
> >> +		raw_capacity[cpu] = SCHED_CAPACITY_SCALE;
> > 
> > This isn't actually required as the value of raw_capacity isn't used
> > anymore after this point in code. Rather it is forcefully updated in
> > init_cpu_capacity_callback():
> > 
> > raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *
> >                     policy->cpuinfo.max_freq / 1000UL;
> > 
> > Maybe it is better to allocate raw_capacity once at boot and use
> > another global variable as flag (raw_capacity is used as a flag right
> > now at many places).
> 
> Can we keep the proposed change as is to simply fix the default value?
> 
> I want to do a separate change with a raw_capacity rewrite and remove
> the workqueue freeing it.

Sure. But you still don't need to update raw_capacity[cpu] above as I pointed
out earlier. You can drop those lines at least.

-- 
viresh

Re: [PATCH 4/4] base/drivers/topology: Default dmpis-mhz if they are not set in DT

[Daniel Lezcano]

On 22/11/2018 05:29, Viresh Kumar wrote:
> On 21-11-18, 23:12, Daniel Lezcano wrote:
>> On 30/10/2018 09:58, Viresh Kumar wrote:
>>> s/dmpis/dmips/ in $subject
>>>
>>> On 29-10-18, 17:23, Daniel Lezcano wrote:
>>>> In the case of assymetric SoC with the same micro-architecture, we
>>>
>>>                  asymmetric ?
>>>
>>>> have a group of CPUs with smaller OPPs than the other group. One
>>>> example is the 96boards dragonboard 820c. There is no dmips/MHz
>>>> difference between both groups, so no need to specify the values in
>>>> the DT. Unfortunately, without these defined, there is no scaling
>>>> capacity comutation triggered, so we need to write
>>>
>>>            computation
>>>
>>>> 'capacity-dmips-mhz' for each CPU with the same value in order to
>>>> force the scaled capacity computation.
>>>>
>>>> Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no
>>>> 'capacity-dmips-mhz' is defined in the DT.
>>>>
>>>> This was tested on db820c:
>>>>  - specified values in the DT (correct results)
>>>>  - partial values defined in the DT (error + fallback to defaults)
>>>>  - no specified values in the DT (correct results)
>>>>
>>>> correct results are:
>>>>   cat /sys/devices/system/cpu/cpu*/cpu_capacity
>>>>    758
>>>>    758
>>>>   1024
>>>>   1024
>>>>
>>>>   ... respectively for CPU0, CPU1, CPU2 and CPU3.
>>>>
>>>> That reflects the capacity for the max frequencies 1593600 and 2150400.
>>>>
>>>> Cc: Chris Redpath <chris.redpath@linaro.org>
>>>> Cc: Quentin Perret <quentin.perret@linaro.org>
>>>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>>>> Cc: Amit Kucheria <amit.kucheria@linaro.org>
>>>> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>
>>>> Cc: Niklas Cassel <niklas.cassel@linaro.org>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> ---
>>>>  drivers/base/arch_topology.c | 27 ++++++++++++++++++++++++++-
>>>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>>>> index 7311641..7d594a6 100644
>>>> --- a/drivers/base/arch_topology.c
>>>> +++ b/drivers/base/arch_topology.c
>>>> @@ -205,6 +205,21 @@ static struct notifier_block init_cpu_capacity_notifier = {
>>>>  	.notifier_call = init_cpu_capacity_callback,
>>>>  };
>>>>  
>>>> +static int topology_set_default_capacity(void)
>>>> +{
>>>> +	int cpu;
>>>> +
>>>> +	raw_capacity = kzalloc(num_possible_cpus() * sizeof(*raw_capacity),
>>>> +			       GFP_KERNEL);
>>>> +	if (!raw_capacity)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	for_each_possible_cpu(cpu)
>>>> +		raw_capacity[cpu] = SCHED_CAPACITY_SCALE;
>>>
>>> This isn't actually required as the value of raw_capacity isn't used
>>> anymore after this point in code. Rather it is forcefully updated in
>>> init_cpu_capacity_callback():
>>>
>>> raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *
>>>                     policy->cpuinfo.max_freq / 1000UL;
>>>
>>> Maybe it is better to allocate raw_capacity once at boot and use
>>> another global variable as flag (raw_capacity is used as a flag right
>>> now at many places).
>>
>> Can we keep the proposed change as is to simply fix the default value?
>>
>> I want to do a separate change with a raw_capacity rewrite and remove
>> the workqueue freeing it.
> 
> Sure. But you still don't need to update raw_capacity[cpu] above as I pointed
> out earlier. You can drop those lines at least.

Oh ... actually raw_capacity is not needed at all!



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 4/4] base/drivers/topology: Default dmpis-mhz if they are not set in DT

[Viresh Kumar]

On 22-11-18, 11:29, Daniel Lezcano wrote:
> Oh ... actually raw_capacity is not needed at all!

It is required as another routine writes some values to it I believe :)

-- 
viresh

Re: [PATCH 4/4] base/drivers/topology: Default dmpis-mhz if they are not set in DT

[Daniel Lezcano]

On 22/11/2018 11:31, Viresh Kumar wrote:
> On 22-11-18, 11:29, Daniel Lezcano wrote:
>> Oh ... actually raw_capacity is not needed at all!
> 
> It is required as another routine writes some values to it I believe :)

Let's see if I can remove it.


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 4/4] base/drivers/topology: Default dmpis-mhz if they are not set in DT

[Daniel Lezcano]

On 22/11/2018 11:31, Viresh Kumar wrote:
> On 22-11-18, 11:29, Daniel Lezcano wrote:
>> Oh ... actually raw_capacity is not needed at all!
> 
> It is required as another routine writes some values to it I believe :)

Well actually it is accessed 'later' by topology_normalize_cpu_scale()
by a call from arch/arm/kernel/topology.c and arch/arm64/kernel/topology.c

This code deserves a bit of rework, I don't have the time to take care
of it, so I will resend with your suggestion and do some cleanup later.

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/4] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE

[Sudeep Holla]

On Mon, Oct 29, 2018 at 05:23:18PM +0100, Daniel Lezcano wrote:
> The mutex protects a per_cpu variable access. The potential race can
> happen only when the cpufreq governor module is loaded and at the same
> time the cpu capacity is changed in the sysfs.
>

I wonder if we really need that sysfs entry to be writable. For some
reason, I had assumed it's read only, obviously it's not. I prefer to
make it RO if there's no strong reason other than debug purposes.

--
Regards,
Sudeep

Re: [PATCH 2/4] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE

[Daniel Lezcano]

On 23/11/2018 14:58, Sudeep Holla wrote:
> On Mon, Oct 29, 2018 at 05:23:18PM +0100, Daniel Lezcano wrote:
>> The mutex protects a per_cpu variable access. The potential race can
>> happen only when the cpufreq governor module is loaded and at the same
>> time the cpu capacity is changed in the sysfs.
>>
> 
> I wonder if we really need that sysfs entry to be writable. For some
> reason, I had assumed it's read only, obviously it's not. I prefer to
> make it RO if there's no strong reason other than debug purposes.

Are you suggesting to remove the READ_ONCE/WRITE_ONCE patch and set the
sysfs file read-only ?


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/4] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE

[Sudeep Holla]

On Fri, Nov 23, 2018 at 05:04:18PM +0100, Daniel Lezcano wrote:
> On 23/11/2018 14:58, Sudeep Holla wrote:
> > On Mon, Oct 29, 2018 at 05:23:18PM +0100, Daniel Lezcano wrote:
> >> The mutex protects a per_cpu variable access. The potential race can
> >> happen only when the cpufreq governor module is loaded and at the same
> >> time the cpu capacity is changed in the sysfs.
> >>
> >
> > I wonder if we really need that sysfs entry to be writable. For some
> > reason, I had assumed it's read only, obviously it's not. I prefer to
> > make it RO if there's no strong reason other than debug purposes.
>
> Are you suggesting to remove the READ_ONCE/WRITE_ONCE patch and set the
> sysfs file read-only ?
>

Just to be sure, if we retain RW capability we still need to fix the
race you are pointing out.

However I just don't see the need for RW cpu_capacity sysfs and hence
asking the reason here. IIRC I had pointed this out long back(not sure
internally or externally) but seemed to have missed the version that got
merged. So I am just asking if we really need write capability given that
it has known issues.

If user-space starts writing the value to influence the scheduler, then
it makes it difficult for kernel to change the way it uses the
cpu_capacity in future.

Sorry if there's valid usecase and I am just making noise here.

--
Regards,
Sudeep

Re: [PATCH 2/4] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE

[Daniel Lezcano]

On 23/11/2018 17:20, Sudeep Holla wrote:
> On Fri, Nov 23, 2018 at 05:04:18PM +0100, Daniel Lezcano wrote:
>> On 23/11/2018 14:58, Sudeep Holla wrote:
>>> On Mon, Oct 29, 2018 at 05:23:18PM +0100, Daniel Lezcano wrote:
>>>> The mutex protects a per_cpu variable access. The potential race can
>>>> happen only when the cpufreq governor module is loaded and at the same
>>>> time the cpu capacity is changed in the sysfs.
>>>>
>>>
>>> I wonder if we really need that sysfs entry to be writable. For some
>>> reason, I had assumed it's read only, obviously it's not. I prefer to
>>> make it RO if there's no strong reason other than debug purposes.
>>
>> Are you suggesting to remove the READ_ONCE/WRITE_ONCE patch and set the
>> sysfs file read-only ?
>>
> 
> Just to be sure, if we retain RW capability we still need to fix the
> race you are pointing out.
> 
> However I just don't see the need for RW cpu_capacity sysfs and hence
> asking the reason here. IIRC I had pointed this out long back(not sure
> internally or externally) but seemed to have missed the version that got
> merged. So I am just asking if we really need write capability given that
> it has known issues.
> 
> If user-space starts writing the value to influence the scheduler, then
> it makes it difficult for kernel to change the way it uses the
> cpu_capacity in future.
> 
> Sorry if there's valid usecase and I am just making noise here.

It's ok [added Juri Lelli]

I've been through the history:

commit be8f185d8af4dbd450023a42a48c6faa8cbcdfe6
Author: Juri Lelli <juri.lelli@arm.com>
Date:   Thu Nov 3 05:40:18 2016 +0000

    arm64: add sysfs cpu_capacity attribute

    Add a sysfs cpu_capacity attribute with which it is possible to read and
    write (thus over-writing default values) CPUs capacity. This might be
    useful in situations where values needs changing after boot.

    The new attribute shows up as:

     /sys/devices/system/cpu/cpu*/cpu_capacity

    Cc: Will Deacon <will.deacon@arm.com>
    Cc: Mark Brown <broonie@kernel.org>
    Cc: Sudeep Holla <sudeep.holla@arm.com>
    Signed-off-by: Juri Lelli <juri.lelli@arm.com>
    Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>

Juri do you have a use case where we want to override the capacity?

Shall we switch the sysfs attribute to read-only?


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/4] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE

[Juri Lelli]

Hi,

On 23/11/18 17:54, Daniel Lezcano wrote:
> On 23/11/2018 17:20, Sudeep Holla wrote:
> > On Fri, Nov 23, 2018 at 05:04:18PM +0100, Daniel Lezcano wrote:
> >> On 23/11/2018 14:58, Sudeep Holla wrote:
> >>> On Mon, Oct 29, 2018 at 05:23:18PM +0100, Daniel Lezcano wrote:
> >>>> The mutex protects a per_cpu variable access. The potential race can
> >>>> happen only when the cpufreq governor module is loaded and at the same
> >>>> time the cpu capacity is changed in the sysfs.
> >>>>
> >>>
> >>> I wonder if we really need that sysfs entry to be writable. For some
> >>> reason, I had assumed it's read only, obviously it's not. I prefer to
> >>> make it RO if there's no strong reason other than debug purposes.
> >>
> >> Are you suggesting to remove the READ_ONCE/WRITE_ONCE patch and set the
> >> sysfs file read-only ?
> >>
> > 
> > Just to be sure, if we retain RW capability we still need to fix the
> > race you are pointing out.
> > 
> > However I just don't see the need for RW cpu_capacity sysfs and hence
> > asking the reason here. IIRC I had pointed this out long back(not sure
> > internally or externally) but seemed to have missed the version that got
> > merged. So I am just asking if we really need write capability given that
> > it has known issues.
> > 
> > If user-space starts writing the value to influence the scheduler, then
> > it makes it difficult for kernel to change the way it uses the
> > cpu_capacity in future.
> > 
> > Sorry if there's valid usecase and I am just making noise here.
> 
> It's ok [added Juri Lelli]
> 
> I've been through the history:
> 
> commit be8f185d8af4dbd450023a42a48c6faa8cbcdfe6
> Author: Juri Lelli <juri.lelli@arm.com>
> Date:   Thu Nov 3 05:40:18 2016 +0000
> 
>     arm64: add sysfs cpu_capacity attribute
> 
>     Add a sysfs cpu_capacity attribute with which it is possible to read and
>     write (thus over-writing default values) CPUs capacity. This might be
>     useful in situations where values needs changing after boot.
> 
>     The new attribute shows up as:
> 
>      /sys/devices/system/cpu/cpu*/cpu_capacity
> 
>     Cc: Will Deacon <will.deacon@arm.com>
>     Cc: Mark Brown <broonie@kernel.org>
>     Cc: Sudeep Holla <sudeep.holla@arm.com>
>     Signed-off-by: Juri Lelli <juri.lelli@arm.com>
>     Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> 
> Juri do you have a use case where we want to override the capacity?
> 
> Shall we switch the sysfs attribute to read-only?

So, I spent a bit of time researching patchset history and IIRC the
point of having a RW cpu_capacity was to help in situations where one
wants to change values after boot, because she/he now has "better"
numbers (remember we advocate to use Dhrystone to populate DTs, but that
is highly debatable). I also seem to remember that there might also be
cases where DT values cannot be changed at all for a (new?) platform
that happens to be using DTs shipped with an old revision; something
along these lines was mentioned (by Mark?) during the review process,
but exact details escape my mind ATM, apologies.

Best,

- Juri

Re: [PATCH 2/4] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE

[Daniel Lezcano]

Hi Juri,

On 26/11/2018 09:27, Juri Lelli wrote:
> Hi,
> 
> On 23/11/18 17:54, Daniel Lezcano wrote:
>> On 23/11/2018 17:20, Sudeep Holla wrote:
>>> On Fri, Nov 23, 2018 at 05:04:18PM +0100, Daniel Lezcano wrote:
>>>> On 23/11/2018 14:58, Sudeep Holla wrote:
>>>>> On Mon, Oct 29, 2018 at 05:23:18PM +0100, Daniel Lezcano wrote:
>>>>>> The mutex protects a per_cpu variable access. The potential race can
>>>>>> happen only when the cpufreq governor module is loaded and at the same
>>>>>> time the cpu capacity is changed in the sysfs.
>>>>>>
>>>>>
>>>>> I wonder if we really need that sysfs entry to be writable. For some
>>>>> reason, I had assumed it's read only, obviously it's not. I prefer to
>>>>> make it RO if there's no strong reason other than debug purposes.
>>>>
>>>> Are you suggesting to remove the READ_ONCE/WRITE_ONCE patch and set the
>>>> sysfs file read-only ?
>>>>
>>>
>>> Just to be sure, if we retain RW capability we still need to fix the
>>> race you are pointing out.
>>>
>>> However I just don't see the need for RW cpu_capacity sysfs and hence
>>> asking the reason here. IIRC I had pointed this out long back(not sure
>>> internally or externally) but seemed to have missed the version that got
>>> merged. So I am just asking if we really need write capability given that
>>> it has known issues.
>>>
>>> If user-space starts writing the value to influence the scheduler, then
>>> it makes it difficult for kernel to change the way it uses the
>>> cpu_capacity in future.
>>>
>>> Sorry if there's valid usecase and I am just making noise here.
>>
>> It's ok [added Juri Lelli]
>>
>> I've been through the history:
>>
>> commit be8f185d8af4dbd450023a42a48c6faa8cbcdfe6
>> Author: Juri Lelli <juri.lelli@arm.com>
>> Date:   Thu Nov 3 05:40:18 2016 +0000
>>
>>     arm64: add sysfs cpu_capacity attribute
>>
>>     Add a sysfs cpu_capacity attribute with which it is possible to read and
>>     write (thus over-writing default values) CPUs capacity. This might be
>>     useful in situations where values needs changing after boot.
>>
>>     The new attribute shows up as:
>>
>>      /sys/devices/system/cpu/cpu*/cpu_capacity
>>
>>     Cc: Will Deacon <will.deacon@arm.com>
>>     Cc: Mark Brown <broonie@kernel.org>
>>     Cc: Sudeep Holla <sudeep.holla@arm.com>
>>     Signed-off-by: Juri Lelli <juri.lelli@arm.com>
>>     Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
>>
>> Juri do you have a use case where we want to override the capacity?
>>
>> Shall we switch the sysfs attribute to read-only?
> 
> So, I spent a bit of time researching patchset history and IIRC the
> point of having a RW cpu_capacity was to help in situations where one
> wants to change values after boot, because she/he now has "better"
> numbers (remember we advocate to use Dhrystone to populate DTs, but that
> is highly debatable). I also seem to remember that there might also be
> cases where DT values cannot be changed at all for a (new?) platform
> that happens to be using DTs shipped with an old revision; something
> along these lines was mentioned (by Mark?) during the review process,
> but exact details escape my mind ATM, apologies.

Ok, so I guess it makes sense to keep it RW then.

Thanks for the feedback.

  -- Daniel



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/4] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

On Mon, Nov 26, 2018 at 09:27:02AM +0100, Juri Lelli wrote:

> is highly debatable). I also seem to remember that there might also be
> cases where DT values cannot be changed at all for a (new?) platform
> that happens to be using DTs shipped with an old revision; something
> along these lines was mentioned (by Mark?) during the review process,
> but exact details escape my mind ATM, apologies.

Yeah, DTs might be in firmware which is either non-updatable or which
people are reluctant to update due to it not being recoverable if the
update goes wrong.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]