| Message ID | 1571776465-29763-5-git-send-email-thara.gopinath@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce Thermal Pressure | expand |
On 10/22/19 16:34, 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. > > 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 4f9c2cb..be3e802 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -7727,6 +7727,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); Maybe a naive question - but can we add util_avg with load_avg without a conversion? I thought the 2 signals have different properties. Cheers -- Qais Yousef > > if (unlikely(used >= max)) > return 1; > -- > 2.1.4 >
On Wed, Oct 23, 2019 at 01:28:40PM +0100, Qais Yousef wrote: > On 10/22/19 16:34, 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. > > > > 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 4f9c2cb..be3e802 100644 > > --- a/kernel/sched/fair.c > > +++ b/kernel/sched/fair.c > > @@ -7727,6 +7727,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); > > Maybe a naive question - but can we add util_avg with load_avg without > a conversion? I thought the 2 signals have different properties. Changelog of patch #1 explains, it's in that dense blob of text. But yes, you're quite right that that wants a comment here.
On 10/28/19 16:30, Peter Zijlstra wrote: > On Wed, Oct 23, 2019 at 01:28:40PM +0100, Qais Yousef wrote: > > On 10/22/19 16:34, 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. > > > > > > 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 4f9c2cb..be3e802 100644 > > > --- a/kernel/sched/fair.c > > > +++ b/kernel/sched/fair.c > > > @@ -7727,6 +7727,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); > > > > Maybe a naive question - but can we add util_avg with load_avg without > > a conversion? I thought the 2 signals have different properties. > > Changelog of patch #1 explains, it's in that dense blob of text. > > But yes, you're quite right that that wants a comment here. Thanks for the pointer! A comment would be nice indeed. To make sure I got this correctly - it's because avg_thermal.load_avg represents delta_capacity which is already a 'converted' form of load. So this makes avg_thermal.load_avg a util_avg really. Correct? If I managed to get it right somehow. It'd be nice if we can do inverse conversion on delta_capacity so that avg_thermal.{load_avg, util_avg} meaning is consistent across the board. But I don't feel strongly about it if this gets documented properly. Thanks -- Qais Yousef
On 31.10.19 11:53, Qais Yousef wrote: > On 10/28/19 16:30, Peter Zijlstra wrote: >> On Wed, Oct 23, 2019 at 01:28:40PM +0100, Qais Yousef wrote: >>> On 10/22/19 16:34, 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. >>>> >>>> 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 4f9c2cb..be3e802 100644 >>>> --- a/kernel/sched/fair.c >>>> +++ b/kernel/sched/fair.c >>>> @@ -7727,6 +7727,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); >>> >>> Maybe a naive question - but can we add util_avg with load_avg without >>> a conversion? I thought the 2 signals have different properties. >> >> Changelog of patch #1 explains, it's in that dense blob of text. >> >> But yes, you're quite right that that wants a comment here. > > Thanks for the pointer! A comment would be nice indeed. > > To make sure I got this correctly - it's because avg_thermal.load_avg > represents delta_capacity which is already a 'converted' form of load. So this > makes avg_thermal.load_avg a util_avg really. Correct? > > If I managed to get it right somehow. It'd be nice if we can do inverse > conversion on delta_capacity so that avg_thermal.{load_avg, util_avg} meaning > is consistent across the board. But I don't feel strongly about it if this gets > documented properly. So why can't we use rq->avg_thermal.util_avg here? Since capacity is closer to util than to load? Is it because you want to use the influence of ___update_load_sum(..., unsigned long load eq. per-cpu delta_capacity in your signal? Why not call it this way then? diff --git a/kernel/sched/pelt.c b/kernel/sched/pelt.c index 38210691c615..d3035457483f 100644 --- a/kernel/sched/pelt.c +++ b/kernel/sched/pelt.c @@ -357,9 +357,9 @@ int update_thermal_load_avg(u64 now, struct rq *rq, u64 capacity) { if (___update_load_sum(now, &rq->avg_thermal, capacity, - capacity, - capacity)) { - ___update_load_avg(&rq->avg_thermal, 1, 1); + 0, + 0)) { + ___update_load_avg(&rq->avg_thermal, 1, 0); return 1; }
On Thu, 31 Oct 2019 at 16:38, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: > > On 31.10.19 11:53, Qais Yousef wrote: > > On 10/28/19 16:30, Peter Zijlstra wrote: > >> On Wed, Oct 23, 2019 at 01:28:40PM +0100, Qais Yousef wrote: > >>> On 10/22/19 16:34, 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. > >>>> > >>>> 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 4f9c2cb..be3e802 100644 > >>>> --- a/kernel/sched/fair.c > >>>> +++ b/kernel/sched/fair.c > >>>> @@ -7727,6 +7727,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); > >>> > >>> Maybe a naive question - but can we add util_avg with load_avg without > >>> a conversion? I thought the 2 signals have different properties. > >> > >> Changelog of patch #1 explains, it's in that dense blob of text. > >> > >> But yes, you're quite right that that wants a comment here. > > > > Thanks for the pointer! A comment would be nice indeed. > > > > To make sure I got this correctly - it's because avg_thermal.load_avg > > represents delta_capacity which is already a 'converted' form of load. So this > > makes avg_thermal.load_avg a util_avg really. Correct? > > > > If I managed to get it right somehow. It'd be nice if we can do inverse > > conversion on delta_capacity so that avg_thermal.{load_avg, util_avg} meaning > > is consistent across the board. But I don't feel strongly about it if this gets > > documented properly. > > So why can't we use rq->avg_thermal.util_avg here? Since capacity is > closer to util than to load? > > Is it because you want to use the influence of ___update_load_sum(..., > unsigned long load eq. per-cpu delta_capacity in your signal? > > Why not call it this way then? util_avg tracks a binary state with 2 fixed weights: running(1024) vs not running (0) In the case of thermal pressure, we want to track how much pressure is put on the CPU: capping to half the max frequency is not the same as capping only 10% load_avg is not boolean but you set the weight you want to apply and this weight reflects the amount of pressure. > > diff --git a/kernel/sched/pelt.c b/kernel/sched/pelt.c > index 38210691c615..d3035457483f 100644 > --- a/kernel/sched/pelt.c > +++ b/kernel/sched/pelt.c > @@ -357,9 +357,9 @@ int update_thermal_load_avg(u64 now, struct rq *rq, > u64 capacity) > { > if (___update_load_sum(now, &rq->avg_thermal, > capacity, > - capacity, > - capacity)) { > - ___update_load_avg(&rq->avg_thermal, 1, 1); > + 0, > + 0)) { > + ___update_load_avg(&rq->avg_thermal, 1, 0); > return 1; > }
On 10/31/2019 06:53 AM, Qais Yousef wrote: > On 10/28/19 16:30, Peter Zijlstra wrote: >> On Wed, Oct 23, 2019 at 01:28:40PM +0100, Qais Yousef wrote: >>> On 10/22/19 16:34, 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. >>>> >>>> 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 4f9c2cb..be3e802 100644 >>>> --- a/kernel/sched/fair.c >>>> +++ b/kernel/sched/fair.c >>>> @@ -7727,6 +7727,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); >>> >>> Maybe a naive question - but can we add util_avg with load_avg without >>> a conversion? I thought the 2 signals have different properties. >> >> Changelog of patch #1 explains, it's in that dense blob of text. >> >> But yes, you're quite right that that wants a comment here. > > Thanks for the pointer! A comment would be nice indeed. > > To make sure I got this correctly - it's because avg_thermal.load_avg > represents delta_capacity which is already a 'converted' form of load. So this > makes avg_thermal.load_avg a util_avg really. Correct? Hello Quais, Sorry for not replying to your earlier email. Thanks for the review. So if you look at the code, util_sum in calculated as a binary signal converted into capacity. Check out the the below snippet from accumulate_sum if (load) sa->load_sum += load * contrib; if (runnable) sa->runnable_load_sum += runnable * contrib; if (running) sa->util_sum += contrib << SCHED_CAPACITY_SHIFT; So the actual delta for the thermal pressure will never be considered if util_avg is used. I will update this patch with relevant comment. -- Warm Regards Thara
On 31.10.19 16:48, Vincent Guittot wrote: > On Thu, 31 Oct 2019 at 16:38, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: >> >> On 31.10.19 11:53, Qais Yousef wrote: >>> On 10/28/19 16:30, Peter Zijlstra wrote: >>>> On Wed, Oct 23, 2019 at 01:28:40PM +0100, Qais Yousef wrote: >>>>> On 10/22/19 16:34, Thara Gopinath wrote: [...] >>> To make sure I got this correctly - it's because avg_thermal.load_avg >>> represents delta_capacity which is already a 'converted' form of load. So this >>> makes avg_thermal.load_avg a util_avg really. Correct? >>> >>> If I managed to get it right somehow. It'd be nice if we can do inverse >>> conversion on delta_capacity so that avg_thermal.{load_avg, util_avg} meaning >>> is consistent across the board. But I don't feel strongly about it if this gets >>> documented properly. >> >> So why can't we use rq->avg_thermal.util_avg here? Since capacity is >> closer to util than to load? >> >> Is it because you want to use the influence of ___update_load_sum(..., >> unsigned long load eq. per-cpu delta_capacity in your signal? >> >> Why not call it this way then? > > util_avg tracks a binary state with 2 fixed weights: running(1024) vs > not running (0) > In the case of thermal pressure, we want to track how much pressure is > put on the CPU: capping to half the max frequency is not the same as > capping only 10% > load_avg is not boolean but you set the weight you want to apply and > this weight reflects the amount of pressure. I see. This is what I meant by 'load (weight) eq. per-cpu delta_capacity (pressure)'. >> diff --git a/kernel/sched/pelt.c b/kernel/sched/pelt.c >> index 38210691c615..d3035457483f 100644 >> --- a/kernel/sched/pelt.c >> +++ b/kernel/sched/pelt.c >> @@ -357,9 +357,9 @@ int update_thermal_load_avg(u64 now, struct rq *rq, >> u64 capacity) >> { >> if (___update_load_sum(now, &rq->avg_thermal, >> capacity, >> - capacity, >> - capacity)) { >> - ___update_load_avg(&rq->avg_thermal, 1, 1); >> + 0, >> + 0)) { >> + ___update_load_avg(&rq->avg_thermal, 1, 0); >> return 1; >> } So we could call it this way since we don't care about runnable_load or util?
On Thu, 31 Oct 2019 at 17:17, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: > > On 31.10.19 16:48, Vincent Guittot wrote: > > On Thu, 31 Oct 2019 at 16:38, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: > >> > >> On 31.10.19 11:53, Qais Yousef wrote: > >>> On 10/28/19 16:30, Peter Zijlstra wrote: > >>>> On Wed, Oct 23, 2019 at 01:28:40PM +0100, Qais Yousef wrote: > >>>>> On 10/22/19 16:34, Thara Gopinath wrote: > > [...] > > >>> To make sure I got this correctly - it's because avg_thermal.load_avg > >>> represents delta_capacity which is already a 'converted' form of load. So this > >>> makes avg_thermal.load_avg a util_avg really. Correct? > >>> > >>> If I managed to get it right somehow. It'd be nice if we can do inverse > >>> conversion on delta_capacity so that avg_thermal.{load_avg, util_avg} meaning > >>> is consistent across the board. But I don't feel strongly about it if this gets > >>> documented properly. > >> > >> So why can't we use rq->avg_thermal.util_avg here? Since capacity is > >> closer to util than to load? > >> > >> Is it because you want to use the influence of ___update_load_sum(..., > >> unsigned long load eq. per-cpu delta_capacity in your signal? > >> > >> Why not call it this way then? > > > > util_avg tracks a binary state with 2 fixed weights: running(1024) vs > > not running (0) > > In the case of thermal pressure, we want to track how much pressure is > > put on the CPU: capping to half the max frequency is not the same as > > capping only 10% > > load_avg is not boolean but you set the weight you want to apply and > > this weight reflects the amount of pressure. > > I see. This is what I meant by 'load (weight) eq. per-cpu delta_capacity > (pressure)'. > > > >> diff --git a/kernel/sched/pelt.c b/kernel/sched/pelt.c > >> index 38210691c615..d3035457483f 100644 > >> --- a/kernel/sched/pelt.c > >> +++ b/kernel/sched/pelt.c > >> @@ -357,9 +357,9 @@ int update_thermal_load_avg(u64 now, struct rq *rq, > >> u64 capacity) > >> { > >> if (___update_load_sum(now, &rq->avg_thermal, > >> capacity, > >> - capacity, > >> - capacity)) { > >> - ___update_load_avg(&rq->avg_thermal, 1, 1); > >> + 0, > >> + 0)) { > >> + ___update_load_avg(&rq->avg_thermal, 1, 0); > >> return 1; > >> } > > So we could call it this way since we don't care about runnable_load or > util? one way or the other is quite similar but the current solution is aligned with other irq, rt, dl signals which duplicates the same state in each fields
On 31.10.19 17:31, Vincent Guittot wrote: > On Thu, 31 Oct 2019 at 17:17, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: >> >> On 31.10.19 16:48, Vincent Guittot wrote: >>> On Thu, 31 Oct 2019 at 16:38, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: >>>> >>>> On 31.10.19 11:53, Qais Yousef wrote: >>>>> On 10/28/19 16:30, Peter Zijlstra wrote: >>>>>> On Wed, Oct 23, 2019 at 01:28:40PM +0100, Qais Yousef wrote: >>>>>>> On 10/22/19 16:34, Thara Gopinath wrote: [...] >>>> diff --git a/kernel/sched/pelt.c b/kernel/sched/pelt.c >>>> index 38210691c615..d3035457483f 100644 >>>> --- a/kernel/sched/pelt.c >>>> +++ b/kernel/sched/pelt.c >>>> @@ -357,9 +357,9 @@ int update_thermal_load_avg(u64 now, struct rq *rq, >>>> u64 capacity) >>>> { >>>> if (___update_load_sum(now, &rq->avg_thermal, >>>> capacity, >>>> - capacity, >>>> - capacity)) { >>>> - ___update_load_avg(&rq->avg_thermal, 1, 1); >>>> + 0, >>>> + 0)) { >>>> + ___update_load_avg(&rq->avg_thermal, 1, 0); >>>> return 1; >>>> } >> >> So we could call it this way since we don't care about runnable_load or >> util? > > one way or the other is quite similar but the current solution is > aligned with other irq, rt, dl signals which duplicates the same state > in each fields I see. But there is a subtle difference. For irq, rt, dl, we have to also set load (even we only use util) because of: ___update_load_sum() { ... if (!load) runnable = running = 0; ... } which is there for se's only. I like self-explanatory code but I agree in this case it's not obvious.
On 10/31/19 12:03, Thara Gopinath wrote: > On 10/31/2019 06:53 AM, Qais Yousef wrote: > > On 10/28/19 16:30, Peter Zijlstra wrote: > >> On Wed, Oct 23, 2019 at 01:28:40PM +0100, Qais Yousef wrote: > >>> On 10/22/19 16:34, 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. > >>>> > >>>> 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 4f9c2cb..be3e802 100644 > >>>> --- a/kernel/sched/fair.c > >>>> +++ b/kernel/sched/fair.c > >>>> @@ -7727,6 +7727,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); > >>> > >>> Maybe a naive question - but can we add util_avg with load_avg without > >>> a conversion? I thought the 2 signals have different properties. > >> > >> Changelog of patch #1 explains, it's in that dense blob of text. > >> > >> But yes, you're quite right that that wants a comment here. > > > > Thanks for the pointer! A comment would be nice indeed. > > > > To make sure I got this correctly - it's because avg_thermal.load_avg > > represents delta_capacity which is already a 'converted' form of load. So this > > makes avg_thermal.load_avg a util_avg really. Correct? > Hello Quais, > > Sorry for not replying to your earlier email. Thanks for the review. > So if you look at the code, util_sum in calculated as a binary signal > converted into capacity. Check out the the below snippet from accumulate_sum > > if (load) > sa->load_sum += load * contrib; > if (runnable) > sa->runnable_load_sum += runnable * contrib; > if (running) > sa->util_sum += contrib << SCHED_CAPACITY_SHIFT; > > So the actual delta for the thermal pressure will never be considered > if util_avg is used. > > I will update this patch with relevant comment. Okay thanks for the explanation. -- Qais Yousef
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 4f9c2cb..be3e802 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -7727,6 +7727,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;
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. Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> --- kernel/sched/fair.c | 1 + 1 file changed, 1 insertion(+) -- 2.1.4