[v5,4/6] sched/fair: update cpu_capcity to reflect thermal pressure

Message ID 1572979786-20361-5-git-send-email-thara.gopinath@linaro.org
State New
Headers show
Series
  • Introduce Thermal Pressure
Related show

Commit Message

Thara Gopinath Nov. 5, 2019, 6:49 p.m.
cpu_capacity relflects the maximum available capacity of a cpu. Thermal
pressure on a cpu means this maximum available capacity is reduced. This
patch reduces the average thermal pressure for a cpu from its maximum
available capacity so that cpu_capacity reflects the actual
available capacity.

Other signals that are deducted from cpu_capacity to reflect the actual
capacity available are rt and dl util_avg. util_avg tracks a binary signal
and uses the weights 1024 and 0. Whereas thermal pressure is tracked
using load_avg. load_avg uses the actual "delta" capacity as the weight.

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>

---
 kernel/sched/fair.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.1.4

Comments

Qais Yousef Nov. 6, 2019, 4:56 p.m. | #1
On 11/05/19 13:49, Thara Gopinath wrote:
> cpu_capacity relflects the maximum available capacity of a cpu. Thermal

> pressure on a cpu means this maximum available capacity is reduced. This

> patch reduces the average thermal pressure for a cpu from its maximum

> available capacity so that cpu_capacity reflects the actual

> available capacity.

> 

> Other signals that are deducted from cpu_capacity to reflect the actual

> capacity available are rt and dl util_avg. util_avg tracks a binary signal

> and uses the weights 1024 and 0. Whereas thermal pressure is tracked

> using load_avg. load_avg uses the actual "delta" capacity as the weight.


I think you intended to put this as comment...

> 

> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>

> ---

>  kernel/sched/fair.c | 1 +

>  1 file changed, 1 insertion(+)

> 

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c

> index 9fb0494..5f6c371 100644

> --- a/kernel/sched/fair.c

> +++ b/kernel/sched/fair.c

> @@ -7738,6 +7738,7 @@ static unsigned long scale_rt_capacity(struct sched_domain *sd, int cpu)

>  

>  	used = READ_ONCE(rq->avg_rt.util_avg);

>  	used += READ_ONCE(rq->avg_dl.util_avg);

> +	used += READ_ONCE(rq->avg_thermal.load_avg);


... here?

I find the explanation hard to parse too. Do you think you can rephrase it?
Something based on what you wrote here would be more understandable IMHO:
https://lore.kernel.org/lkml/5DBB05BC.40502@linaro.org/


Thanks!

--
Qais Yousef

>  

>  	if (unlikely(used >= max))

>  		return 1;

> -- 

> 2.1.4

>
Thara Gopinath Nov. 6, 2019, 5:31 p.m. | #2
On 11/06/2019 11:56 AM, Qais Yousef wrote:
> On 11/05/19 13:49, Thara Gopinath wrote:

>> cpu_capacity relflects the maximum available capacity of a cpu. Thermal

>> pressure on a cpu means this maximum available capacity is reduced. This

>> patch reduces the average thermal pressure for a cpu from its maximum

>> available capacity so that cpu_capacity reflects the actual

>> available capacity.

>>

>> Other signals that are deducted from cpu_capacity to reflect the actual

>> capacity available are rt and dl util_avg. util_avg tracks a binary signal

>> and uses the weights 1024 and 0. Whereas thermal pressure is tracked

>> using load_avg. load_avg uses the actual "delta" capacity as the weight.

> 

> I think you intended to put this as comment...

> 

>>

>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>

>> ---

>>  kernel/sched/fair.c | 1 +

>>  1 file changed, 1 insertion(+)

>>

>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c

>> index 9fb0494..5f6c371 100644

>> --- a/kernel/sched/fair.c

>> +++ b/kernel/sched/fair.c

>> @@ -7738,6 +7738,7 @@ static unsigned long scale_rt_capacity(struct sched_domain *sd, int cpu)

>>  

>>  	used = READ_ONCE(rq->avg_rt.util_avg);

>>  	used += READ_ONCE(rq->avg_dl.util_avg);

>> +	used += READ_ONCE(rq->avg_thermal.load_avg);

> 

> ... here?


I did not!  But I can.
> 

> I find the explanation hard to parse too. Do you think you can rephrase it?

> Something based on what you wrote here would be more understandable IMHO:

> https://lore.kernel.org/lkml/5DBB05BC.40502@linaro.org/

I will try to rephrase it! I am sorry that you found it hard to parse.
Honestly, I cannot copy paste the code snippet I pointed out to you here
in comment.(And I think that is the reason you found it easier to
understand) But I will try my best to put it in words.

> 

> 

> Thanks!

> 

> --

> Qais Yousef

> 

>>  

>>  	if (unlikely(used >= max))

>>  		return 1;

>> -- 

>> 2.1.4

>>



-- 
Warm Regards
Thara
Qais Yousef Nov. 6, 2019, 5:41 p.m. | #3
On 11/06/19 12:31, Thara Gopinath wrote:
> On 11/06/2019 11:56 AM, Qais Yousef wrote:

> > On 11/05/19 13:49, Thara Gopinath wrote:

> >> cpu_capacity relflects the maximum available capacity of a cpu. Thermal

> >> pressure on a cpu means this maximum available capacity is reduced. This

> >> patch reduces the average thermal pressure for a cpu from its maximum

> >> available capacity so that cpu_capacity reflects the actual

> >> available capacity.

> >>

> >> Other signals that are deducted from cpu_capacity to reflect the actual

> >> capacity available are rt and dl util_avg. util_avg tracks a binary signal

> >> and uses the weights 1024 and 0. Whereas thermal pressure is tracked

> >> using load_avg. load_avg uses the actual "delta" capacity as the weight.

> > 

> > I think you intended to put this as comment...

> > 

> >>

> >> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>

> >> ---

> >>  kernel/sched/fair.c | 1 +

> >>  1 file changed, 1 insertion(+)

> >>

> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c

> >> index 9fb0494..5f6c371 100644

> >> --- a/kernel/sched/fair.c

> >> +++ b/kernel/sched/fair.c

> >> @@ -7738,6 +7738,7 @@ static unsigned long scale_rt_capacity(struct sched_domain *sd, int cpu)

> >>  

> >>  	used = READ_ONCE(rq->avg_rt.util_avg);

> >>  	used += READ_ONCE(rq->avg_dl.util_avg);

> >> +	used += READ_ONCE(rq->avg_thermal.load_avg);

> > 

> > ... here?

> 

> I did not!  But I can.

> > 

> > I find the explanation hard to parse too. Do you think you can rephrase it?

> > Something based on what you wrote here would be more understandable IMHO:

> > https://lore.kernel.org/lkml/5DBB05BC.40502@linaro.org/

> I will try to rephrase it! I am sorry that you found it hard to parse.

> Honestly, I cannot copy paste the code snippet I pointed out to you here

> in comment.(And I think that is the reason you found it easier to

> understand) But I will try my best to put it in words.


No worries. The problem could be me :-)

But a comment in the code is very important as util_avg + load_avg is confusing
without a comment. I wouldn't expect both signal to be compatible but the
thermal one is special. A comment explaining why it's special is all we need.


Thanks

--
Qais Yousef
Amit Kucheria Nov. 19, 2019, 10:51 a.m. | #4
On Wed, Nov 6, 2019 at 12:20 AM Thara Gopinath
<thara.gopinath@linaro.org> wrote:
>

> cpu_capacity relflects the maximum available capacity of a cpu. Thermal


s/relflects/reflects

> pressure on a cpu means this maximum available capacity is reduced. This

> patch reduces the average thermal pressure for a cpu from its maximum

> available capacity so that cpu_capacity reflects the actual

> available capacity.

>

> Other signals that are deducted from cpu_capacity to reflect the actual

> capacity available are rt and dl util_avg. util_avg tracks a binary signal

> and uses the weights 1024 and 0. Whereas thermal pressure is tracked

> using load_avg. load_avg uses the actual "delta" capacity as the weight.


Consider rephrasing as,

Currently, effective cpu capacity is calculated by deducting RT and DL
average utilization (util_avg) from cpu_capacity. For thermal
pressure, we use load_avg instead of util_avg which is a binary signal
(0 or 1024) because <put why here>


> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>

> ---

>  kernel/sched/fair.c | 1 +

>  1 file changed, 1 insertion(+)

>

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c

> index 9fb0494..5f6c371 100644

> --- a/kernel/sched/fair.c

> +++ b/kernel/sched/fair.c

> @@ -7738,6 +7738,7 @@ static unsigned long scale_rt_capacity(struct sched_domain *sd, int cpu)

>

>         used = READ_ONCE(rq->avg_rt.util_avg);

>         used += READ_ONCE(rq->avg_dl.util_avg);

> +       used += READ_ONCE(rq->avg_thermal.load_avg);

>

>         if (unlikely(used >= max))

>                 return 1;

> --

> 2.1.4

>

Patch

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 9fb0494..5f6c371 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7738,6 +7738,7 @@  static unsigned long scale_rt_capacity(struct sched_domain *sd, int cpu)
 
 	used = READ_ONCE(rq->avg_rt.util_avg);
 	used += READ_ONCE(rq->avg_dl.util_avg);
+	used += READ_ONCE(rq->avg_thermal.load_avg);
 
 	if (unlikely(used >= max))
 		return 1;