Date:   Wed, 24 Aug 2022 17:22:57 +0200
From:   Pierre Gondois <pierre.gondois@....com>
To:     Vincent Guittot <vincent.guittot@...aro.org>
Cc:     linux-kernel@...r.kernel.org, qperret@...gle.com,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Valentin Schneider <vschneid@...hat.com>
Subject: Re: [PATCH 2/2] sched/fair: Use IRQ scaling for all sched classes



On 8/23/22 18:26, Vincent Guittot wrote:
> On Fri, 19 Aug 2022 at 17:33, Pierre Gondois <pierre.gondois@....com> wrote:
>>
>> The time spent executing IRQ handlers is not reflected in the
>> utilization of CPU. IRQ scaling reduces rq CFS, RT and DL
>> util by reflecting the CPU capacity reduction due to IRQs.
>>
>> commit 9033ea11889f ("cpufreq/schedutil: Take time spent in interrupts
>> into account")
>> introduced the notion of IRQ scaling for the now called
>> effective_cpu_util() function with the following expression (for the
>> CPU util):
>>    IRQ util_avg + (max_cap - IRQ util_avg / max_cap ) * /Sum rq util_avg
>>
>> commit 523e979d3164 ("sched/core: Use PELT for scale_rt_capacity()")
>> introduced IRQ scaling for scale_rt_capacity(), but without scaling
>> RT and DL rq util.
>>
>> scale_rt_capacity() excludes RT and DL rq signals from IRQ scaling.
>> Only the available capacity is scaled. However RT and DL rq util
>> should also be scaled.
> 
> RT and DL are not excluded, they are removed before scaling the
> available time. We compute the available cpu capacity in the clock
> task domain before scaling it in the full clock domain
> 
> Let imagine that we have the following timeshare:
> 
> |       100%      |
> |-IRQ-|-RT--|-----|
> | 33% | 33% | 33% |
>        |   100%    |
>        | 50% | 50% |
> 
> irq uses a third of the time
> 
> RT uses a third of the time but 50% of the clock task as the time in
> interrupt context is not accounted for. This means that the RT
> utilization is 50%
> 
> In order to know what is available for others we do:
> 
> 100% - 50% RT = 50% available in the clock task context
> 
> Then, we scale the 50% of available time to take the time stolen by IRQ
> 
> (100% - 33% IRQ) * 50% / 100% = 33%
> 
> So the available capacity for others is 33% of the original capacity
> which is correct
> 
> What you're proposing:
> 
> free = (100% - 33% IRQ) * 100% / 100% = 67%
> 
> used = 50% RT + 33% IRQ == 83% whereas it should be 33% RT + 33% IRQ == 66%
> 
> Then free < used which means that there is no capacity available for
> others which is not true

Ok yes indeed, the scaling is correct as it is.

Thanks for the detailed explanation,
Pierre

> 
>>
>> Applying IRQ scaling allows to extract the IRQ util avg. So IRQ util
>> avg should also be subtracted from the available capacity.
>> Thermal pressure is not execution time but reduces the maximum
>> possible capacity of a CPU. So IRQ scaling should not be applied.
> 
> thermal pressure uses clock_task so it has the same constraint as RT,
> DL and CFS signal i.e. irq time is not accounted
> 
>>
>> Thus, in this order:
>>   - subtract thermal pressure
>>   - apply IRQ scaling on the remaining capacity (RT + DL + CFS + free)
>>   - subtract IRQ util
>>
>> Also, sort variables in reverse tree order.
>>
>> Signed-off-by: Pierre Gondois <pierre.gondois@....com>
>> ---
>>   kernel/sched/fair.c | 19 ++++++++++++-------
>>   1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index bcae7bdd5582..546e490d6753 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -8468,16 +8468,23 @@ static inline void init_sd_lb_stats(struct sd_lb_stats *sds)
>>
>>   static unsigned long scale_rt_capacity(int cpu)
>>   {
>> -       struct rq *rq = cpu_rq(cpu);
>>          unsigned long max = arch_scale_cpu_capacity(cpu);
>> +       struct rq *rq = cpu_rq(cpu);
>> +       unsigned long irq, thermal;
>>          unsigned long used, free;
>> -       unsigned long irq;
>>
>>          irq = cpu_util_irq(rq);
>>
>>          if (unlikely(irq >= max))
>>                  return 1;
>>
>> +       thermal = thermal_load_avg(rq);
>> +       if (unlikely(thermal >= max))
>> +               return 1;
>> +
>> +       free = max - thermal;
>> +       free = scale_irq_capacity(free, irq, max);
>> +
>>          /*
>>           * avg_rt.util_avg and avg_dl.util_avg track binary signals
>>           * (running and not running) with weights 0 and 1024 respectively.
>> @@ -8486,14 +8493,12 @@ static unsigned long scale_rt_capacity(int cpu)
>>           */
>>          used = READ_ONCE(rq->avg_rt.util_avg);
>>          used += READ_ONCE(rq->avg_dl.util_avg);
>> -       used += thermal_load_avg(rq);
>> +       used += irq;
> 
> rq->avg_rt.util_avg and irq==cpu_util_irq(rq)) are not in the same
> time scale so you can't add them
> 
> 
>>
>> -       if (unlikely(used >= max))
>> +       if (unlikely(used >= free))
>>                  return 1;
>>
>> -       free = max - used;
>> -
>> -       return scale_irq_capacity(free, irq, max);
>> +       return free - used;
>>   }
>>
>>   static void update_cpu_capacity(struct sched_domain *sd, int cpu)
>> --
>> 2.25.1
>>

Powered by blists - more mailing lists