Re: [PATCH v6 08/11] cpufreq/schedutil: take into account interrupt

2018-06-12 Thread Vincent Guittot
On 12 June 2018 at 11:20, Quentin Perret  wrote:
> On Tuesday 12 Jun 2018 at 11:16:56 (+0200), Vincent Guittot wrote:
>> The time spent under interrupt can be significant but it is not reflected
>> in the utilization of CPU when deciding to choose an OPP. Now that we have
>> access to this metric, schedutil can take it into account when selecting
>> the OPP for a CPU.
>> rqs utilization don't see the time spend under interrupt context and report
>> their value in the normal context time window. We need to compensate this 
>> when
>> adding interrupt utilization
>>
>> The CPU utilization is :
>>   irq util_avg + (1 - irq util_avg / max capacity ) * /Sum rq util_avg
>>
>> A test with iperf on hikey (octo arm64) gives:
>> iperf -c server_address -r -t 5
>>
>> w/o patch w/ patch
>> Tx 276 Mbits/sec304 Mbits/sec +10%
>> Rx 299 Mbits/sec328 Mbits/sec +09%
>>
>> 8 iterations
>> stdev is lower than 1%
>> Only WFI idle state is enable (shallowest diel state)
> 
> nit: s/diel/idle
>
> And, out of curiosity, what happens if you leave the idle states
> untouched ? Do you still see an improvement ? Or is it lost in the
> noise ?

the result are less stable because c-state wake up time impact
performance and cpuidle is not good to select the right idle state in
such case. Normally, an app should use qos dma latency or a driver per
device resume latency

>
> Thanks,
> Quentin


Re: [PATCH v6 08/11] cpufreq/schedutil: take into account interrupt

2018-06-12 Thread Quentin Perret
On Tuesday 12 Jun 2018 at 11:16:56 (+0200), Vincent Guittot wrote:
> The time spent under interrupt can be significant but it is not reflected
> in the utilization of CPU when deciding to choose an OPP. Now that we have
> access to this metric, schedutil can take it into account when selecting
> the OPP for a CPU.
> rqs utilization don't see the time spend under interrupt context and report
> their value in the normal context time window. We need to compensate this when
> adding interrupt utilization
> 
> The CPU utilization is :
>   irq util_avg + (1 - irq util_avg / max capacity ) * /Sum rq util_avg
> 
> A test with iperf on hikey (octo arm64) gives:
> iperf -c server_address -r -t 5
> 
> w/o patch w/ patch
> Tx 276 Mbits/sec304 Mbits/sec +10%
> Rx 299 Mbits/sec328 Mbits/sec +09%
> 
> 8 iterations
> stdev is lower than 1%
> Only WFI idle state is enable (shallowest diel state)

nit: s/diel/idle

And, out of curiosity, what happens if you leave the idle states
untouched ? Do you still see an improvement ? Or is it lost in the
noise ?

Thanks,
Quentin


Re: [PATCH v6 08/11] cpufreq/schedutil: take into account interrupt

2018-06-12 Thread Vincent Guittot
On 12 June 2018 at 10:54, Dietmar Eggemann  wrote:
> On 06/08/2018 02:09 PM, Vincent Guittot wrote:
>
> [...]
>
>> @@ -182,21 +183,30 @@ static void sugov_get_util(struct sugov_cpu *sg_cpu)
>> sg_cpu->util_dl  = cpu_util_dl(rq);
>> sg_cpu->bw_dl= cpu_bw_dl(rq);
>> sg_cpu->util_rt  = cpu_util_rt(rq);
>> +   sg_cpu->util_irq = cpu_util_irq(rq);
>>   }
>> static unsigned long sugov_aggregate_util(struct sugov_cpu *sg_cpu)
>>   {
>> struct rq *rq = cpu_rq(sg_cpu->cpu);
>> -   unsigned long util;
>> +   unsigned long util, max = sg_cpu->max;
>> if (rq->rt.rt_nr_running)
>> return sg_cpu->max;
>>   + if (unlikely(sg_cpu->util_irq >= max))
>> +   return max;
>> +
>> +   /* Sum rq utilization */
>> util = sg_cpu->util_cfs;
>> util += sg_cpu->util_rt;
>>   - if ((util + sg_cpu->util_dl) >= sg_cpu->max)
>> -   return sg_cpu->max;
>> :confirm b9
>
>
> This didn't let me apply the patch ;-) After removing this line it worked.

Argh ... i have done something wrong
I'm going to resend it
>
> [...]


Re: [PATCH v6 08/11] cpufreq/schedutil: take into account interrupt

2018-06-12 Thread Dietmar Eggemann

On 06/08/2018 02:09 PM, Vincent Guittot wrote:

[...]


@@ -182,21 +183,30 @@ static void sugov_get_util(struct sugov_cpu *sg_cpu)
sg_cpu->util_dl  = cpu_util_dl(rq);
sg_cpu->bw_dl= cpu_bw_dl(rq);
sg_cpu->util_rt  = cpu_util_rt(rq);
+   sg_cpu->util_irq = cpu_util_irq(rq);
  }
  
  static unsigned long sugov_aggregate_util(struct sugov_cpu *sg_cpu)

  {
struct rq *rq = cpu_rq(sg_cpu->cpu);
-   unsigned long util;
+   unsigned long util, max = sg_cpu->max;
  
  	if (rq->rt.rt_nr_running)

return sg_cpu->max;
  
+	if (unlikely(sg_cpu->util_irq >= max))

+   return max;
+
+   /* Sum rq utilization */
util = sg_cpu->util_cfs;
util += sg_cpu->util_rt;
  
-	if ((util + sg_cpu->util_dl) >= sg_cpu->max)

-   return sg_cpu->max;
:confirm b9


This didn't let me apply the patch ;-) After removing this line it worked.

[...]