[PATCH 1/3] cpufreq: stats: Remove redundant cpufreq_cpu_get() call

[PATCH 1/3] cpufreq: stats: Remove redundant cpufreq_cpu_get() call

[Saravana Kannan]

__cpufreq_stats_create_table always gets pass the valid and real policy
struct. So, there's no need to call cpufreq_cpu_get() to get the policy
again.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
---
 drivers/cpufreq/cpufreq_stats.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 5793e14..e4bd27f 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -185,7 +185,6 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy,
 {
 	unsigned int i, j, count = 0, ret = 0;
 	struct cpufreq_stats *stat;
-	struct cpufreq_policy *current_policy;
 	unsigned int alloc_size;
 	unsigned int cpu = policy->cpu;
 	if (per_cpu(cpufreq_stats_table, cpu))
@@ -194,13 +193,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy,
 	if ((stat) == NULL)
 		return -ENOMEM;
 
-	current_policy = cpufreq_cpu_get(cpu);
-	if (current_policy == NULL) {
-		ret = -EINVAL;
-		goto error_get_fail;
-	}
-
-	ret = sysfs_create_group(&current_policy->kobj, &stats_attr_group);
+	ret = sysfs_create_group(&policy->kobj, &stats_attr_group);
 	if (ret)
 		goto error_out;
 
@@ -243,11 +236,8 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy,
 	stat->last_time = get_jiffies_64();
 	stat->last_index = freq_table_get_index(stat, policy->cur);
 	spin_unlock(&cpufreq_stats_lock);
-	cpufreq_cpu_put(current_policy);
 	return 0;
 error_out:
-	cpufreq_cpu_put(current_policy);
-error_get_fail:
 	kfree(stat);
 	per_cpu(cpufreq_stats_table, cpu) = NULL;
 	return ret;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] cpufreq: Set policy to non-NULL only after all hotplug online work is done

[Rafael J. Wysocki]

On Tuesday, February 25, 2014 01:11:27 PM Saravana Kannan wrote:
> On 02/25/2014 05:04 AM, Rafael J. Wysocki wrote:
> > On Tuesday, February 25, 2014 02:20:57 PM Viresh Kumar wrote:
> >> On 25 February 2014 01:53, Saravana Kannan <skannan@codeaurora.org> wrote:
> >>> I was simplifying the scenario that causes it. We change the min/max using
> >>> ADJUST notifiers for multiple reasons -- thermal being one of them.
> >>>
> >>> thermal/cpu_cooling is one example of it.
> >>
> >> Just to understand the clear picture, you are actually hitting this bug? Or
> >> is this only a theoretical bug?
> >>
> This is a real bug. But the actual caller of cpufreq_update_policy() is 
> a driver that's local to our tree. I'm just giving examples of upstream 
> code that act in a similar way.
> 
> >>> So, cpufreq_update_policy() can be called on any CPU. If that races with
> >>> someone offlining a CPU and onlining it, you'll get this crash.
> >>
> >> Then shouldn't that be fixed by locks? I think yes. That makes me agree with
> >> Srivatsa more here.
> >>
> >> Though I would say that your argument was also valid that 'policy' shouldn't be
> >> up for sale unless it is prepared to. And for that reason only I
> >> floated that question
> >> earlier: What exactly we need to make sure is initialized in policy? Because
> >> policy might keep changing in future as well and that needs locks to protect
> >> that stuff. Like min/max/governor/ etc..
> >
> > Well, that depends on what the current users expect it to look like initially.
> > It should be initialized to the point in which all of them can handle it
> > correctly.
> 
> Yes, so let's not make it available until all of it is initialized.

And is "fully initialized" actually well defined?

> I don't like the piece meal check. 6 months down the lane someone making 
> changes might not remember this. The problem also applies for drivers 
> that might not be upstreamed, etc.

Please don't bring up out-of-the-tree drivers argument in mainline discussions,
it is meaningless here.

> >> So, probably a solution here might be a mix of both. Initialize policy to this
> >> minimum level and then make sure locking is used correctly..
> >
> > Yes.
> 
> Rafael, It's not clear what you mean by the yes. Do you want to 
> initialize it partly and make it available. I think that's always wrong.

So what actually is your porposal?

> >>> The idea would exist, but we can just call cpufreq_generic_get() and pass it
> >>> policy->clk if it is not NULL. Does that work for you?
> >>
> >> No. Not all drivers implement clk interface. And so clk doesn't look to be the
> >> right parameter. I thought maybe 'policy' can be the right parameter and
> >> then people can get use policy->cpu to get cpu id out of it.
> >>
> >> But even that doesn't look to be a great idea. X86 drivers may share policy
> >> structure for CPUs that don't actually share a clock line. And so they do need
> >> right CPU number as parameter instead of policy. As they might be doing
> >> some tricky stuff there. Also, we need to make sure that ->get() returns
> >> the frequency at which CPU x is running.
> >
> > That's not going to work in at least some cases anyway, because for some types
> > of HW we simply can't retrieve the current frequency in a non-racy way.
> 
> I think there's been a misunderstanding of what I'm proposing. The 
> reference to policy->clk is only to get rid of the dependency that 
> cpufreq_generic_get() has on the per cpu policy variable being filled. 
> You can do that by just removing calls to get _IF_ clk is filled in.

I still have a little problem understanding what you mean exactly.  At least
please explain the last sentence.

> Viresh,
> 
> I'll look at the patches later today and respond. Although, I would have 
> been nice you had helped me fix any issues with my patch than coming up 
> with new ones. Kinda removes to motivation for submitting patches upstream.

Everyone is free to post patches at any time during the discussion.  Viresh is
as well as you are.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 1/3] cpufreq: stats: Remove redundant cpufreq_cpu_get() call

[Saravana Kannan]

__cpufreq_stats_create_table always gets pass the valid and real policy
struct. So, there's no need to call cpufreq_cpu_get() to get the policy
again.

Change-Id: I0136b3e67018ee3af2335906407f55d8c6219f71
Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
---

Viresh/Rafael,

These 3 patches is the approximate code I have in mind.

Approximate because:
* I inserted one question as a comment into the code.
* If the patch doesn't have any bugs, the plan is to remove
  cpufreq_generic_get() and references to it.

This takes care of the "don't advertise before it's ready for use" rule.

Viresh,

I think the locking updates needs to be done in addition to this.

Regards,
Saravana

 drivers/cpufreq/cpufreq_stats.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 5793e14..e4bd27f 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -185,7 +185,6 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy,
 {
 	unsigned int i, j, count = 0, ret = 0;
 	struct cpufreq_stats *stat;
-	struct cpufreq_policy *current_policy;
 	unsigned int alloc_size;
 	unsigned int cpu = policy->cpu;
 	if (per_cpu(cpufreq_stats_table, cpu))
@@ -194,13 +193,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy,
 	if ((stat) == NULL)
 		return -ENOMEM;
 
-	current_policy = cpufreq_cpu_get(cpu);
-	if (current_policy == NULL) {
-		ret = -EINVAL;
-		goto error_get_fail;
-	}
-
-	ret = sysfs_create_group(&current_policy->kobj, &stats_attr_group);
+	ret = sysfs_create_group(&policy->kobj, &stats_attr_group);
 	if (ret)
 		goto error_out;
 
@@ -243,11 +236,8 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy,
 	stat->last_time = get_jiffies_64();
 	stat->last_index = freq_table_get_index(stat, policy->cur);
 	spin_unlock(&cpufreq_stats_lock);
-	cpufreq_cpu_put(current_policy);
 	return 0;
 error_out:
-	cpufreq_cpu_put(current_policy);
-error_get_fail:
 	kfree(stat);
 	per_cpu(cpufreq_stats_table, cpu) = NULL;
 	return ret;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 1/3] cpufreq: stats: Remove redundant cpufreq_cpu_get() call

[Viresh Kumar]

On 26 February 2014 09:08, Saravana Kannan <skannan@codeaurora.org> wrote:
> __cpufreq_stats_create_table always gets pass the valid and real policy
> struct. So, there's no need to call cpufreq_cpu_get() to get the policy
> again.
>
> Change-Id: I0136b3e67018ee3af2335906407f55d8c6219f71

??

> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> ---
>
> Viresh/Rafael,
>
> These 3 patches is the approximate code I have in mind.
>
> Approximate because:
> * I inserted one question as a comment into the code.
> * If the patch doesn't have any bugs, the plan is to remove
>   cpufreq_generic_get() and references to it.
>
> This takes care of the "don't advertise before it's ready for use" rule.
>
> Viresh,
>
> I think the locking updates needs to be done in addition to this.
>
> Regards,
> Saravana
>
>  drivers/cpufreq/cpufreq_stats.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index 5793e14..e4bd27f 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -185,7 +185,6 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy,
>  {
>         unsigned int i, j, count = 0, ret = 0;
>         struct cpufreq_stats *stat;
> -       struct cpufreq_policy *current_policy;
>         unsigned int alloc_size;
>         unsigned int cpu = policy->cpu;
>         if (per_cpu(cpufreq_stats_table, cpu))
> @@ -194,13 +193,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy,
>         if ((stat) == NULL)
>                 return -ENOMEM;
>
> -       current_policy = cpufreq_cpu_get(cpu);
> -       if (current_policy == NULL) {
> -               ret = -EINVAL;
> -               goto error_get_fail;
> -       }
> -
> -       ret = sysfs_create_group(&current_policy->kobj, &stats_attr_group);
> +       ret = sysfs_create_group(&policy->kobj, &stats_attr_group);
>         if (ret)
>                 goto error_out;
>
> @@ -243,11 +236,8 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy,
>         stat->last_time = get_jiffies_64();
>         stat->last_index = freq_table_get_index(stat, policy->cur);
>         spin_unlock(&cpufreq_stats_lock);
> -       cpufreq_cpu_put(current_policy);
>         return 0;
>  error_out:
> -       cpufreq_cpu_put(current_policy);
> -error_get_fail:
>         kfree(stat);
>         per_cpu(cpufreq_stats_table, cpu) = NULL;
>         return ret;

I was damn sure that this wasn't a waste of time. This was some meaningful
code when I visited it earlier. And we absolutely required a new
cpufreq_cpu_get()..

Reason: Earlier tables were created for this notifier: CPUFREQ_NOTIFY and
it used to come with another changed copy of 'policy' and so we were required
to get the real copy of policy to get to the right kobj.

But recently I have simplified stuff there and these tables are now added with
CPUFREQ_CREATE_POLICY and so this replication isn't required anymore.

So, Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

While you are at it please get this part into __cpufreq_stats_create_table()
routine:

table = cpufreq_frequency_get_table(cpu);
if (!table)
    return 0;

As it is replicated at two places currently.

Re: [PATCH 1/3] cpufreq: stats: Remove redundant cpufreq_cpu_get() call

[Saravana Kannan]

On 02/25/2014 09:06 PM, Viresh Kumar wrote:
> On 26 February 2014 09:08, Saravana Kannan <skannan@codeaurora.org> wrote:
>> __cpufreq_stats_create_table always gets pass the valid and real policy
>> struct. So, there's no need to call cpufreq_cpu_get() to get the policy
>> again.
>>
>> Change-Id: I0136b3e67018ee3af2335906407f55d8c6219f71
>
> ??
>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> ---
>>
>> Viresh/Rafael,
>>
>> These 3 patches is the approximate code I have in mind.
>>
>> Approximate because:
>> * I inserted one question as a comment into the code.
>> * If the patch doesn't have any bugs, the plan is to remove
>>    cpufreq_generic_get() and references to it.
>>
>> This takes care of the "don't advertise before it's ready for use" rule.
>>
>> Viresh,
>>
>> I think the locking updates needs to be done in addition to this.
>>
>> Regards,
>> Saravana
>>
>>   drivers/cpufreq/cpufreq_stats.c | 12 +-----------
>>   1 file changed, 1 insertion(+), 11 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
>> index 5793e14..e4bd27f 100644
>> --- a/drivers/cpufreq/cpufreq_stats.c
>> +++ b/drivers/cpufreq/cpufreq_stats.c
>> @@ -185,7 +185,6 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy,
>>   {
>>          unsigned int i, j, count = 0, ret = 0;
>>          struct cpufreq_stats *stat;
>> -       struct cpufreq_policy *current_policy;
>>          unsigned int alloc_size;
>>          unsigned int cpu = policy->cpu;
>>          if (per_cpu(cpufreq_stats_table, cpu))
>> @@ -194,13 +193,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy,
>>          if ((stat) == NULL)
>>                  return -ENOMEM;
>>
>> -       current_policy = cpufreq_cpu_get(cpu);
>> -       if (current_policy == NULL) {
>> -               ret = -EINVAL;
>> -               goto error_get_fail;
>> -       }
>> -
>> -       ret = sysfs_create_group(&current_policy->kobj, &stats_attr_group);
>> +       ret = sysfs_create_group(&policy->kobj, &stats_attr_group);
>>          if (ret)
>>                  goto error_out;
>>
>> @@ -243,11 +236,8 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy,
>>          stat->last_time = get_jiffies_64();
>>          stat->last_index = freq_table_get_index(stat, policy->cur);
>>          spin_unlock(&cpufreq_stats_lock);
>> -       cpufreq_cpu_put(current_policy);
>>          return 0;
>>   error_out:
>> -       cpufreq_cpu_put(current_policy);
>> -error_get_fail:
>>          kfree(stat);
>>          per_cpu(cpufreq_stats_table, cpu) = NULL;
>>          return ret;
>
> I was damn sure that this wasn't a waste of time. This was some meaningful
> code when I visited it earlier. And we absolutely required a new
> cpufreq_cpu_get()..
>
> Reason: Earlier tables were created for this notifier: CPUFREQ_NOTIFY and
> it used to come with another changed copy of 'policy' and so we were required
> to get the real copy of policy to get to the right kobj.
>
> But recently I have simplified stuff there and these tables are now added with
> CPUFREQ_CREATE_POLICY and so this replication isn't required anymore.

Agreed. I already knew it had a good reason. :) Just that it's not 
needed anymore.

> So, Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Thanks.

>
> While you are at it please get this part into __cpufreq_stats_create_table()
> routine:
>
> table = cpufreq_frequency_get_table(cpu);
> if (!table)
>      return 0;
>
> As it is replicated at two places currently.
>

Doing it as a separate patch since it's technically unrelated to these 
changes.

-Saravana

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation