[PATCH] cpufreq: schedutil: Always trace frequency if it does not change

[PATCH] cpufreq: schedutil: Always trace frequency if it does not change

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

sugov_update_commit() calls trace_cpu_frequency() to record the
current CPU frequency if it has not changed in the fast switch case
to prevent utilities from getting confused (they may report that the
CPU is idle if the frequency has not been recorded for too long, for
example).

However, the same problem may occur for a cpufreq driver that doesn't
support fast frequency switching and implements the ->target callback
(that is, it doesn't use frequency tables), but trace_cpu_frequency()
is not called if frequency updates are skipped in that case.

For this reason, modify sugov_update_commit() to always call
trace_cpu_frequency() when the new frequency to be set is equal to
the one that was set previously and reorganize the code in there to
reduce duplication somewhat.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 kernel/sched/cpufreq_schedutil.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
+++ linux-pm/kernel/sched/cpufreq_schedutil.c
@@ -98,22 +98,22 @@ static void sugov_update_commit(struct s
 {
 	struct cpufreq_policy *policy = sg_policy->policy;
 
+	if (sg_policy->next_freq == next_freq) {
+		trace_cpu_frequency(policy->cur, smp_processor_id());
+		return;
+	}
+
+	sg_policy->next_freq = next_freq;
+	sg_policy->last_freq_update_time = time;
+
 	if (policy->fast_switch_enabled) {
-		if (sg_policy->next_freq == next_freq) {
-			trace_cpu_frequency(policy->cur, smp_processor_id());
-			return;
-		}
-		sg_policy->next_freq = next_freq;
-		sg_policy->last_freq_update_time = time;
 		next_freq = cpufreq_driver_fast_switch(policy, next_freq);
 		if (next_freq == CPUFREQ_ENTRY_INVALID)
 			return;
 
 		policy->cur = next_freq;
 		trace_cpu_frequency(next_freq, smp_processor_id());
-	} else if (sg_policy->next_freq != next_freq) {
-		sg_policy->next_freq = next_freq;
-		sg_policy->last_freq_update_time = time;
+	} else {
 		sg_policy->work_in_progress = true;
 		irq_work_queue(&sg_policy->irq_work);
 	}

Re: [PATCH] cpufreq: schedutil: Always trace frequency if it does not change

[Viresh Kumar]

On 22-03-17, 01:56, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> sugov_update_commit() calls trace_cpu_frequency() to record the
> current CPU frequency if it has not changed in the fast switch case
> to prevent utilities from getting confused (they may report that the
> CPU is idle if the frequency has not been recorded for too long, for
> example).
> 
> However, the same problem may occur for a cpufreq driver that doesn't
> support fast frequency switching and implements the ->target callback
> (that is, it doesn't use frequency tables), but trace_cpu_frequency()
> is not called if frequency updates are skipped in that case.
> 
> For this reason, modify sugov_update_commit() to always call
> trace_cpu_frequency() when the new frequency to be set is equal to
> the one that was set previously and reorganize the code in there to
> reduce duplication somewhat.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  kernel/sched/cpufreq_schedutil.c |   18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

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

-- 
viresh

Re: [PATCH] cpufreq: schedutil: Always trace frequency if it does not change

[Peter Zijlstra]

On Wed, Mar 22, 2017 at 01:56:53AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> sugov_update_commit() calls trace_cpu_frequency() to record the
> current CPU frequency if it has not changed in the fast switch case
> to prevent utilities from getting confused (they may report that the
> CPU is idle if the frequency has not been recorded for too long, for
> example).

That seems like buggy tools; we should then fix the tools, not the
kernel to emit more superfluous information.

> However, the same problem may occur for a cpufreq driver that doesn't
> support fast frequency switching and implements the ->target callback
> (that is, it doesn't use frequency tables), but trace_cpu_frequency()
> is not called if frequency updates are skipped in that case.

I'm having trouble parsing that; am I right in understanding this is the
exact same scenario where only superfluous changes are omitted?

> For this reason, modify sugov_update_commit() to always call
> trace_cpu_frequency() when the new frequency to be set is equal to
> the one that was set previously and reorganize the code in there to
> reduce duplication somewhat.

So why not fix the tools?

Re: [PATCH] cpufreq: schedutil: Always trace frequency if it does not change

[Viresh Kumar]

On 22-03-17, 10:29, Peter Zijlstra wrote:
> On Wed, Mar 22, 2017 at 01:56:53AM +0100, Rafael J. Wysocki wrote:

> > However, the same problem may occur for a cpufreq driver that doesn't
> > support fast frequency switching and implements the ->target callback
> > (that is, it doesn't use frequency tables), but trace_cpu_frequency()
> > is not called if frequency updates are skipped in that case.
> 
> I'm having trouble parsing that; am I right in understanding this is the
> exact same scenario where only superfluous changes are omitted?

Yes. Just for a different driver type.

-- 
viresh

Re: [PATCH] cpufreq: schedutil: Always trace frequency if it does not change

[Rafael J. Wysocki]

On Wed, Mar 22, 2017 at 10:29 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Mar 22, 2017 at 01:56:53AM +0100, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> sugov_update_commit() calls trace_cpu_frequency() to record the
>> current CPU frequency if it has not changed in the fast switch case
>> to prevent utilities from getting confused (they may report that the
>> CPU is idle if the frequency has not been recorded for too long, for
>> example).
>
> That seems like buggy tools; we should then fix the tools, not the
> kernel to emit more superfluous information.
>
>> However, the same problem may occur for a cpufreq driver that doesn't
>> support fast frequency switching and implements the ->target callback
>> (that is, it doesn't use frequency tables), but trace_cpu_frequency()
>> is not called if frequency updates are skipped in that case.
>
> I'm having trouble parsing that; am I right in understanding this is the
> exact same scenario where only superfluous changes are omitted?

Yes, that's the same scenario basically, which is the point of the patch. :-)

>> For this reason, modify sugov_update_commit() to always call
>> trace_cpu_frequency() when the new frequency to be set is equal to
>> the one that was set previously and reorganize the code in there to
>> reduce duplication somewhat.
>
> So why not fix the tools?

Because I can't.

I just can't go and fix all of the tools binaries that people use out
there and I want them to use recent kernels at the same time.

But anyway, I think that the code with the patch looks better than
without it regardless and having a frivolous difference there between
the two cases is not useful.

Re: [PATCH] cpufreq: schedutil: Always trace frequency if it does not change

[Rafael J. Wysocki]

On Wed, Mar 22, 2017 at 1:52 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Mar 22, 2017 at 10:29 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> On Wed, Mar 22, 2017 at 01:56:53AM +0100, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> sugov_update_commit() calls trace_cpu_frequency() to record the
>>> current CPU frequency if it has not changed in the fast switch case
>>> to prevent utilities from getting confused (they may report that the
>>> CPU is idle if the frequency has not been recorded for too long, for
>>> example).
>>
>> That seems like buggy tools; we should then fix the tools, not the
>> kernel to emit more superfluous information.
>>
>>> However, the same problem may occur for a cpufreq driver that doesn't
>>> support fast frequency switching and implements the ->target callback
>>> (that is, it doesn't use frequency tables), but trace_cpu_frequency()
>>> is not called if frequency updates are skipped in that case.
>>
>> I'm having trouble parsing that; am I right in understanding this is the
>> exact same scenario where only superfluous changes are omitted?
>
> Yes, that's the same scenario basically, which is the point of the patch. :-)
>
>>> For this reason, modify sugov_update_commit() to always call
>>> trace_cpu_frequency() when the new frequency to be set is equal to
>>> the one that was set previously and reorganize the code in there to
>>> reduce duplication somewhat.
>>
>> So why not fix the tools?
>
> Because I can't.
>
> I just can't go and fix all of the tools binaries that people use out
> there and I want them to use recent kernels at the same time.
>
> But anyway, I think that the code with the patch looks better than
> without it regardless and having a frivolous difference there between
> the two cases is not useful.

Well, perhaps the changelog should just be

"Get rid of superfluous differences between two code branches in
sugov_update_commit()"

or similar. :-)

Re: [PATCH] cpufreq: schedutil: Always trace frequency if it does not change

[Peter Zijlstra]

On Wed, Mar 22, 2017 at 01:52:04PM +0100, Rafael J. Wysocki wrote:
> On Wed, Mar 22, 2017 at 10:29 AM, Peter Zijlstra <peterz@infradead.org> wrote:

> > So why not fix the tools?
> 
> Because I can't.
> 
> I just can't go and fix all of the tools binaries that people use out
> there and I want them to use recent kernels at the same time.
> 

Thing is; you're now letting random tracepoint user dictate kernel
implementation. That's a bad state to be in.

(and why I hate tracepoint to begin with)

Re: [PATCH] cpufreq: schedutil: Always trace frequency if it does not change

[Rafael J. Wysocki]

On Wed, Mar 22, 2017 at 3:21 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Mar 22, 2017 at 01:52:04PM +0100, Rafael J. Wysocki wrote:
>> On Wed, Mar 22, 2017 at 10:29 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
>> > So why not fix the tools?
>>
>> Because I can't.
>>
>> I just can't go and fix all of the tools binaries that people use out
>> there and I want them to use recent kernels at the same time.
>>
>
> Thing is; you're now letting random tracepoint user dictate kernel
> implementation. That's a bad state to be in.

Fair enough.

Admittedly, I was sort of divided on whether or not to drop the
trace_cpu_frequency() call entirely and now I see another reason to do
that.  There is a change queued up for 4.12 that will cause the
tracepoint to be triggered more often totally in vain which I don't
think is a good thing at all.  So I'll send a v2 dropping that call
and we'll see if anyone complains.

[PATCH v2] cpufreq: schedutil: Trace frequency only if it has changed

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

sugov_update_commit() calls trace_cpu_frequency() to record the
current CPU frequency if it has not changed in the fast switch case
to prevent utilities from getting confused (they may report that the
CPU is idle if the frequency has not been recorded for too long, for
example).

However, that may cause the tracepoint to be triggered quite often
for no real reason (if the frequency doesn't change, we will not
modify the last update time stamp and governor computations may
run again shortly when that happens), so don't do that (arguably, it
is done to work around a utilities bug anyway).

That allows code duplication in sugov_update_commit() to be reduced
somewhat too.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2:
Drop the trace_cpu_frequency() call in the sg_policy->next_freq == next_freq case.

---
 kernel/sched/cpufreq_schedutil.c |   16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
+++ linux-pm/kernel/sched/cpufreq_schedutil.c
@@ -98,22 +98,20 @@ static void sugov_update_commit(struct s
 {
 	struct cpufreq_policy *policy = sg_policy->policy;
 
+	if (sg_policy->next_freq == next_freq)
+		return;
+
+	sg_policy->next_freq = next_freq;
+	sg_policy->last_freq_update_time = time;
+
 	if (policy->fast_switch_enabled) {
-		if (sg_policy->next_freq == next_freq) {
-			trace_cpu_frequency(policy->cur, smp_processor_id());
-			return;
-		}
-		sg_policy->next_freq = next_freq;
-		sg_policy->last_freq_update_time = time;
 		next_freq = cpufreq_driver_fast_switch(policy, next_freq);
 		if (next_freq == CPUFREQ_ENTRY_INVALID)
 			return;
 
 		policy->cur = next_freq;
 		trace_cpu_frequency(next_freq, smp_processor_id());
-	} else if (sg_policy->next_freq != next_freq) {
-		sg_policy->next_freq = next_freq;
-		sg_policy->last_freq_update_time = time;
+	} else {
 		sg_policy->work_in_progress = true;
 		irq_work_queue(&sg_policy->irq_work);
 	}

Re: [PATCH v2] cpufreq: schedutil: Trace frequency only if it has changed

[Viresh Kumar]

On 22-03-17, 18:32, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> sugov_update_commit() calls trace_cpu_frequency() to record the
> current CPU frequency if it has not changed in the fast switch case
> to prevent utilities from getting confused (they may report that the
> CPU is idle if the frequency has not been recorded for too long, for
> example).
> 
> However, that may cause the tracepoint to be triggered quite often
> for no real reason (if the frequency doesn't change, we will not
> modify the last update time stamp and governor computations may
> run again shortly when that happens), so don't do that (arguably, it
> is done to work around a utilities bug anyway).
> 
> That allows code duplication in sugov_update_commit() to be reduced
> somewhat too.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> -> v2:
> Drop the trace_cpu_frequency() call in the sg_policy->next_freq == next_freq case.
> 
> ---
>  kernel/sched/cpufreq_schedutil.c |   16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)

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

-- 
viresh