2022-03-17 15:18:23

by Pierre Gondois

[permalink] [raw]
Subject: [PATCH v1 1/3] cpufreq: CPPC: Add cppc_cpufreq_search_cpu_data

cppc_cpufreq_get_cpu_data() allocates a new struct cppc_cpudata
for the input CPU at each call.

To search the struct associated with a cpu without allocating
a new one, add cppc_cpufreq_search_cpu_data().
Also add an early prototype.

This will be used in a later patch, when generating artificial
performance states to register an artificial Energy Model in the
cppc_cpufreq driver and enable the Energy Aware Scheduler for ACPI
based systems.

Signed-off-by: Pierre Gondois <[email protected]>
---
drivers/cpufreq/cppc_cpufreq.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 82d370ae6a4a..8f950fe72765 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -41,6 +41,8 @@
*/
static LIST_HEAD(cpu_data_list);

+static struct cppc_cpudata *cppc_cpufreq_search_cpu_data(unsigned int cpu);
+
static bool boost_supported;

struct cppc_workaround_oem_info {
@@ -479,6 +481,19 @@ static void cppc_cpufreq_put_cpu_data(struct cpufreq_policy *policy)
policy->driver_data = NULL;
}

+static inline struct cppc_cpudata *
+cppc_cpufreq_search_cpu_data(unsigned int cpu)
+{
+ struct cppc_cpudata *iter, *tmp;
+
+ list_for_each_entry_safe(iter, tmp, &cpu_data_list, node) {
+ if (cpumask_test_cpu(cpu, iter->shared_cpu_map))
+ return iter;
+ }
+
+ return NULL;
+}
+
static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
{
unsigned int cpu = policy->cpu;
--
2.25.1


2022-03-17 18:50:32

by Marc Zyngier

[permalink] [raw]
Subject: Re: [PATCH v1 1/3] cpufreq: CPPC: Add cppc_cpufreq_search_cpu_data

On 2022-03-17 13:34, Pierre Gondois wrote:
> cppc_cpufreq_get_cpu_data() allocates a new struct cppc_cpudata
> for the input CPU at each call.
>
> To search the struct associated with a cpu without allocating
> a new one, add cppc_cpufreq_search_cpu_data().
> Also add an early prototype.
>
> This will be used in a later patch, when generating artificial
> performance states to register an artificial Energy Model in the
> cppc_cpufreq driver and enable the Energy Aware Scheduler for ACPI
> based systems.
>
> Signed-off-by: Pierre Gondois <[email protected]>
> ---
> drivers/cpufreq/cppc_cpufreq.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c
> b/drivers/cpufreq/cppc_cpufreq.c
> index 82d370ae6a4a..8f950fe72765 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -41,6 +41,8 @@
> */
> static LIST_HEAD(cpu_data_list);
>
> +static struct cppc_cpudata *cppc_cpufreq_search_cpu_data(unsigned int
> cpu);
> +
> static bool boost_supported;
>
> struct cppc_workaround_oem_info {
> @@ -479,6 +481,19 @@ static void cppc_cpufreq_put_cpu_data(struct
> cpufreq_policy *policy)
> policy->driver_data = NULL;
> }
>
> +static inline struct cppc_cpudata *

Why the inline? This is hardly performance critical, and if
it is, you want something better than iterating over a list.

> +cppc_cpufreq_search_cpu_data(unsigned int cpu)
> +{
> + struct cppc_cpudata *iter, *tmp;
> +
> + list_for_each_entry_safe(iter, tmp, &cpu_data_list, node) {
> + if (cpumask_test_cpu(cpu, iter->shared_cpu_map))
> + return iter;
> + }
> +
> + return NULL;
> +}
> +
> static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
> {
> unsigned int cpu = policy->cpu;

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2022-03-17 19:46:37

by Marc Zyngier

[permalink] [raw]
Subject: Re: [PATCH v1 1/3] cpufreq: CPPC: Add cppc_cpufreq_search_cpu_data

On 2022-03-17 14:44, Pierre Gondois wrote:
> On 3/17/22 15:20, Marc Zyngier wrote:
>> On 2022-03-17 13:34, Pierre Gondois wrote:
>>> cppc_cpufreq_get_cpu_data() allocates a new struct cppc_cpudata
>>> for the input CPU at each call.
>>>
>>> To search the struct associated with a cpu without allocating
>>> a new one, add cppc_cpufreq_search_cpu_data().
>>> Also add an early prototype.
>>>
>>> This will be used in a later patch, when generating artificial
>>> performance states to register an artificial Energy Model in the
>>> cppc_cpufreq driver and enable the Energy Aware Scheduler for ACPI
>>> based systems.
>>>
>>> Signed-off-by: Pierre Gondois <[email protected]>
>>> ---
>>> drivers/cpufreq/cppc_cpufreq.c | 15 +++++++++++++++
>>> 1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c
>>> b/drivers/cpufreq/cppc_cpufreq.c
>>> index 82d370ae6a4a..8f950fe72765 100644
>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>> @@ -41,6 +41,8 @@
>>> */
>>> static LIST_HEAD(cpu_data_list);
>>>
>>> +static struct cppc_cpudata *cppc_cpufreq_search_cpu_data(unsigned
>>> int
>>> cpu);
>>> +
>>> static bool boost_supported;
>>>
>>> struct cppc_workaround_oem_info {
>>> @@ -479,6 +481,19 @@ static void cppc_cpufreq_put_cpu_data(struct
>>> cpufreq_policy *policy)
>>> policy->driver_data = NULL;
>>> }
>>>
>>> +static inline struct cppc_cpudata *
>>
>> Why the inline? This is hardly performance critical, and if
>> it is, you want something better than iterating over a list.
>
> This was made inline mainly because the function was small. The
> function
> is called only at boot, so it should not be performance critical. The
> 'inline' can be removed if necessary.
> Would letting it inlined have a negative impact ?

This is why we have a compiler. It is perfectly able to decide
on its own whether to inline or not, depending on how it can
optimise it. With modern compilers, 'inline' means nothing anyway,
and is ignored most of the time.

So dropping it will at least save 7 bytes of source code! ;-)

M.
--
Jazz is not dead. It just smells funny...

2022-03-17 20:09:07

by Pierre Gondois

[permalink] [raw]
Subject: Re: [PATCH v1 1/3] cpufreq: CPPC: Add cppc_cpufreq_search_cpu_data



On 3/17/22 15:20, Marc Zyngier wrote:
> On 2022-03-17 13:34, Pierre Gondois wrote:
>> cppc_cpufreq_get_cpu_data() allocates a new struct cppc_cpudata
>> for the input CPU at each call.
>>
>> To search the struct associated with a cpu without allocating
>> a new one, add cppc_cpufreq_search_cpu_data().
>> Also add an early prototype.
>>
>> This will be used in a later patch, when generating artificial
>> performance states to register an artificial Energy Model in the
>> cppc_cpufreq driver and enable the Energy Aware Scheduler for ACPI
>> based systems.
>>
>> Signed-off-by: Pierre Gondois <[email protected]>
>> ---
>> drivers/cpufreq/cppc_cpufreq.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c
>> b/drivers/cpufreq/cppc_cpufreq.c
>> index 82d370ae6a4a..8f950fe72765 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -41,6 +41,8 @@
>> */
>> static LIST_HEAD(cpu_data_list);
>>
>> +static struct cppc_cpudata *cppc_cpufreq_search_cpu_data(unsigned int
>> cpu);
>> +
>> static bool boost_supported;
>>
>> struct cppc_workaround_oem_info {
>> @@ -479,6 +481,19 @@ static void cppc_cpufreq_put_cpu_data(struct
>> cpufreq_policy *policy)
>> policy->driver_data = NULL;
>> }
>>
>> +static inline struct cppc_cpudata *
>
> Why the inline? This is hardly performance critical, and if
> it is, you want something better than iterating over a list.

This was made inline mainly because the function was small. The function
is called only at boot, so it should not be performance critical. The
'inline' can be removed if necessary.
Would letting it inlined have a negative impact ?


>
>> +cppc_cpufreq_search_cpu_data(unsigned int cpu)
>> +{
>> + struct cppc_cpudata *iter, *tmp;
>> +
>> + list_for_each_entry_safe(iter, tmp, &cpu_data_list, node) {
>> + if (cpumask_test_cpu(cpu, iter->shared_cpu_map))
>> + return iter;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>> {
>> unsigned int cpu = policy->cpu;
>
> Thanks,
>
> M.

Regards,
Pierre