update_cfs_rq_load_avg() calls cfs_rq_util_change() everytime pelt decays,
which might be inefficient when cpufreq driver has rate limitation.
When a task is attached on a CPU, we have call path:
update_blocked_averages()
update_cfs_rq_load_avg()
cfs_rq_util_change -- > trig frequency update
attach_entity_load_avg()
cfs_rq_util_change -- > trig frequency update
The 1st frequency update will not take into account the utilization of the
newly attached task and the 2nd one might be discard because of rate
limitation of the cpufreq driver.
update_cfs_rq_load_avg() is only called by update_blocked_averages()
and update_load_avg() so we can move the call to
{cfs_rq,cpufreq}_util_change() into these 2 functions. It's also
interesting to notice that update_load_avg() already calls directly
cfs_rq_util_change() for !SMP case.
This changes will also ensure that cpufreq_update_util() is called even
when there is no more CFS rq in the leaf_cfs_rq_list to update but only
irq, rt or dl pelt signals.
Reported-by: Doug Smythies <[email protected]>
Fixes: 039ae8bcf7a5 ("sched/fair: Fix O(nr_cgroups) in the load balancing path")
Signed-off-by: Vincent Guittot <[email protected]>
---
I have just rebased the patch on latest tip/sched/core and made it a proper
patchset after Doug reported that the problem has diseappeared according to
his 1st results but tests results are not all based on the same v5.4-rcX
and with menu instead of teo governor.
kernel/sched/fair.c | 33 +++++++++++++++++++++------------
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index e458f52..c93d534 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3508,9 +3508,6 @@ update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq)
cfs_rq->load_last_update_time_copy = sa->last_update_time;
#endif
- if (decayed)
- cfs_rq_util_change(cfs_rq, 0);
-
return decayed;
}
@@ -3620,8 +3617,12 @@ static inline void update_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s
attach_entity_load_avg(cfs_rq, se, SCHED_CPUFREQ_MIGRATION);
update_tg_load_avg(cfs_rq, 0);
- } else if (decayed && (flags & UPDATE_TG))
- update_tg_load_avg(cfs_rq, 0);
+ } else if (decayed) {
+ cfs_rq_util_change(cfs_rq, 0);
+
+ if (flags & UPDATE_TG)
+ update_tg_load_avg(cfs_rq, 0);
+ }
}
#ifndef CONFIG_64BIT
@@ -7484,6 +7485,7 @@ static void update_blocked_averages(int cpu)
const struct sched_class *curr_class;
struct rq_flags rf;
bool done = true;
+ int decayed = 0;
rq_lock_irqsave(rq, &rf);
update_rq_clock(rq);
@@ -7493,9 +7495,9 @@ static void update_blocked_averages(int cpu)
* that RT, DL and IRQ signals have been updated before updating CFS.
*/
curr_class = rq->curr->sched_class;
- update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
- update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
- update_irq_load_avg(rq, 0);
+ decayed |= update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
+ decayed |= update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
+ decayed |= update_irq_load_avg(rq, 0);
/* Don't need periodic decay once load/util_avg are null */
if (others_have_blocked(rq))
@@ -7529,6 +7531,9 @@ static void update_blocked_averages(int cpu)
}
update_blocked_load_status(rq, !done);
+
+ if (decayed)
+ cpufreq_update_util(rq, 0);
rq_unlock_irqrestore(rq, &rf);
}
@@ -7585,6 +7590,7 @@ static inline void update_blocked_averages(int cpu)
struct cfs_rq *cfs_rq = &rq->cfs;
const struct sched_class *curr_class;
struct rq_flags rf;
+ int decayed = 0;
rq_lock_irqsave(rq, &rf);
update_rq_clock(rq);
@@ -7594,13 +7600,16 @@ static inline void update_blocked_averages(int cpu)
* that RT, DL and IRQ signals have been updated before updating CFS.
*/
curr_class = rq->curr->sched_class;
- update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
- update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
- update_irq_load_avg(rq, 0);
+ decayed |= update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
+ decayed |= update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
+ decayed |= update_irq_load_avg(rq, 0);
- update_cfs_rq_load_avg(cfs_rq_clock_pelt(cfs_rq), cfs_rq);
+ decayed |= update_cfs_rq_load_avg(cfs_rq_clock_pelt(cfs_rq), cfs_rq);
update_blocked_load_status(rq, cfs_rq_has_blocked(cfs_rq) || others_have_blocked(rq));
+
+ if (decayed)
+ cpufreq_update_util(rq, 0);
rq_unlock_irqrestore(rq, &rf);
}
--
2.7.4
Le Tuesday 12 Nov 2019 ? 15:48:13 (+0100), Vincent Guittot a ?crit :
> update_cfs_rq_load_avg() calls cfs_rq_util_change() everytime pelt decays,
> which might be inefficient when cpufreq driver has rate limitation.
>
> When a task is attached on a CPU, we have call path:
>
> update_blocked_averages()
> update_cfs_rq_load_avg()
> cfs_rq_util_change -- > trig frequency update
> attach_entity_load_avg()
> cfs_rq_util_change -- > trig frequency update
>
> The 1st frequency update will not take into account the utilization of the
> newly attached task and the 2nd one might be discard because of rate
> limitation of the cpufreq driver.
>
> update_cfs_rq_load_avg() is only called by update_blocked_averages()
> and update_load_avg() so we can move the call to
> {cfs_rq,cpufreq}_util_change() into these 2 functions. It's also
> interesting to notice that update_load_avg() already calls directly
> cfs_rq_util_change() for !SMP case.
>
> This changes will also ensure that cpufreq_update_util() is called even
> when there is no more CFS rq in the leaf_cfs_rq_list to update but only
> irq, rt or dl pelt signals.
>
> Reported-by: Doug Smythies <[email protected]>
> Fixes: 039ae8bcf7a5 ("sched/fair: Fix O(nr_cgroups) in the load balancing path")
> Signed-off-by: Vincent Guittot <[email protected]>
>
> ---
>
> I have just rebased the patch on latest tip/sched/core and made it a proper
> patchset after Doug reported that the problem has diseappeared according to
> his 1st results but tests results are not all based on the same v5.4-rcX
> and with menu instead of teo governor.
>
> kernel/sched/fair.c | 33 +++++++++++++++++++++------------
> 1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index e458f52..c93d534 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -3508,9 +3508,6 @@ update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq)
> cfs_rq->load_last_update_time_copy = sa->last_update_time;
> #endif
>
> - if (decayed)
> - cfs_rq_util_change(cfs_rq, 0);
> -
> return decayed;
> }
>
> @@ -3620,8 +3617,12 @@ static inline void update_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s
> attach_entity_load_avg(cfs_rq, se, SCHED_CPUFREQ_MIGRATION);
> update_tg_load_avg(cfs_rq, 0);
>
> - } else if (decayed && (flags & UPDATE_TG))
> - update_tg_load_avg(cfs_rq, 0);
> + } else if (decayed) {
> + cfs_rq_util_change(cfs_rq, 0);
> +
> + if (flags & UPDATE_TG)
> + update_tg_load_avg(cfs_rq, 0);
> + }
> }
>
> #ifndef CONFIG_64BIT
> @@ -7484,6 +7485,7 @@ static void update_blocked_averages(int cpu)
> const struct sched_class *curr_class;
> struct rq_flags rf;
> bool done = true;
> + int decayed = 0;
>
> rq_lock_irqsave(rq, &rf);
> update_rq_clock(rq);
> @@ -7493,9 +7495,9 @@ static void update_blocked_averages(int cpu)
> * that RT, DL and IRQ signals have been updated before updating CFS.
> */
> curr_class = rq->curr->sched_class;
> - update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
> - update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
> - update_irq_load_avg(rq, 0);
> + decayed |= update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
> + decayed |= update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
> + decayed |= update_irq_load_avg(rq, 0);
>
> /* Don't need periodic decay once load/util_avg are null */
> if (others_have_blocked(rq))
> @@ -7529,6 +7531,9 @@ static void update_blocked_averages(int cpu)
> }
>
> update_blocked_load_status(rq, !done);
> +
> + if (decayed)
> + cpufreq_update_util(rq, 0);
> rq_unlock_irqrestore(rq, &rf);
> }
>
> @@ -7585,6 +7590,7 @@ static inline void update_blocked_averages(int cpu)
> struct cfs_rq *cfs_rq = &rq->cfs;
> const struct sched_class *curr_class;
> struct rq_flags rf;
> + int decayed = 0;
>
> rq_lock_irqsave(rq, &rf);
> update_rq_clock(rq);
> @@ -7594,13 +7600,16 @@ static inline void update_blocked_averages(int cpu)
> * that RT, DL and IRQ signals have been updated before updating CFS.
> */
> curr_class = rq->curr->sched_class;
> - update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
> - update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
> - update_irq_load_avg(rq, 0);
> + decayed |= update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
> + decayed |= update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
> + decayed |= update_irq_load_avg(rq, 0);
>
> - update_cfs_rq_load_avg(cfs_rq_clock_pelt(cfs_rq), cfs_rq);
> + decayed |= update_cfs_rq_load_avg(cfs_rq_clock_pelt(cfs_rq), cfs_rq);
>
> update_blocked_load_status(rq, cfs_rq_has_blocked(cfs_rq) || others_have_blocked(rq));
> +
> + if (decayed)
> + cpufreq_update_util(rq, 0);
> rq_unlock_irqrestore(rq, &rf);
> }
>
> --
> 2.7.4
>
Hi Vincent,
I didn't see anything in that reply, was that just a spurious email?
On 12/11/2019 15:05, Vincent Guittot wrote:
> Le Tuesday 12 Nov 2019 à 15:48:13 (+0100), Vincent Guittot a écrit :
>> update_cfs_rq_load_avg() calls cfs_rq_util_change() everytime pelt decays,
>> which might be inefficient when cpufreq driver has rate limitation.
>>
>> When a task is attached on a CPU, we have call path:
>>
>> update_blocked_averages()
>> update_cfs_rq_load_avg()
>> cfs_rq_util_change -- > trig frequency update
>> attach_entity_load_avg()
>> cfs_rq_util_change -- > trig frequency update
>>
>> The 1st frequency update will not take into account the utilization of the
>> newly attached task and the 2nd one might be discard because of rate
>> limitation of the cpufreq driver.
>>
>> update_cfs_rq_load_avg() is only called by update_blocked_averages()
>> and update_load_avg() so we can move the call to
>> {cfs_rq,cpufreq}_util_change() into these 2 functions. It's also
>> interesting to notice that update_load_avg() already calls directly
>> cfs_rq_util_change() for !SMP case.
>>
>> This changes will also ensure that cpufreq_update_util() is called even
>> when there is no more CFS rq in the leaf_cfs_rq_list to update but only
>> irq, rt or dl pelt signals.
>>
>> Reported-by: Doug Smythies <[email protected]>
>> Fixes: 039ae8bcf7a5 ("sched/fair: Fix O(nr_cgroups) in the load balancing path")
>> Signed-off-by: Vincent Guittot <[email protected]>
>>
>> ---
>>
>> I have just rebased the patch on latest tip/sched/core and made it a proper
>> patchset after Doug reported that the problem has diseappeared according to
>> his 1st results but tests results are not all based on the same v5.4-rcX
>> and with menu instead of teo governor.
>>
>> kernel/sched/fair.c | 33 +++++++++++++++++++++------------
>> 1 file changed, 21 insertions(+), 12 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index e458f52..c93d534 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -3508,9 +3508,6 @@ update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq)
>> cfs_rq->load_last_update_time_copy = sa->last_update_time;
>> #endif
>>
>> - if (decayed)
>> - cfs_rq_util_change(cfs_rq, 0);
>> -
>> return decayed;
>> }
>>
>> @@ -3620,8 +3617,12 @@ static inline void update_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s
>> attach_entity_load_avg(cfs_rq, se, SCHED_CPUFREQ_MIGRATION);
>> update_tg_load_avg(cfs_rq, 0);
>>
>> - } else if (decayed && (flags & UPDATE_TG))
>> - update_tg_load_avg(cfs_rq, 0);
>> + } else if (decayed) {
>> + cfs_rq_util_change(cfs_rq, 0);
>> +
>> + if (flags & UPDATE_TG)
>> + update_tg_load_avg(cfs_rq, 0);
>> + }
>> }
>>
>> #ifndef CONFIG_64BIT
>> @@ -7484,6 +7485,7 @@ static void update_blocked_averages(int cpu)
>> const struct sched_class *curr_class;
>> struct rq_flags rf;
>> bool done = true;
>> + int decayed = 0;
>>
>> rq_lock_irqsave(rq, &rf);
>> update_rq_clock(rq);
>> @@ -7493,9 +7495,9 @@ static void update_blocked_averages(int cpu)
>> * that RT, DL and IRQ signals have been updated before updating CFS.
>> */
>> curr_class = rq->curr->sched_class;
>> - update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
>> - update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
>> - update_irq_load_avg(rq, 0);
>> + decayed |= update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
>> + decayed |= update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
>> + decayed |= update_irq_load_avg(rq, 0);
>>
>> /* Don't need periodic decay once load/util_avg are null */
>> if (others_have_blocked(rq))
>> @@ -7529,6 +7531,9 @@ static void update_blocked_averages(int cpu)
>> }
>>
>> update_blocked_load_status(rq, !done);
>> +
>> + if (decayed)
>> + cpufreq_update_util(rq, 0);
>> rq_unlock_irqrestore(rq, &rf);
>> }
>>
>> @@ -7585,6 +7590,7 @@ static inline void update_blocked_averages(int cpu)
>> struct cfs_rq *cfs_rq = &rq->cfs;
>> const struct sched_class *curr_class;
>> struct rq_flags rf;
>> + int decayed = 0;
>>
>> rq_lock_irqsave(rq, &rf);
>> update_rq_clock(rq);
>> @@ -7594,13 +7600,16 @@ static inline void update_blocked_averages(int cpu)
>> * that RT, DL and IRQ signals have been updated before updating CFS.
>> */
>> curr_class = rq->curr->sched_class;
>> - update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
>> - update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
>> - update_irq_load_avg(rq, 0);
>> + decayed |= update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
>> + decayed |= update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
>> + decayed |= update_irq_load_avg(rq, 0);
>>
>> - update_cfs_rq_load_avg(cfs_rq_clock_pelt(cfs_rq), cfs_rq);
>> + decayed |= update_cfs_rq_load_avg(cfs_rq_clock_pelt(cfs_rq), cfs_rq);
>>
>> update_blocked_load_status(rq, cfs_rq_has_blocked(cfs_rq) || others_have_blocked(rq));
>> +
>> + if (decayed)
>> + cpufreq_update_util(rq, 0);
>> rq_unlock_irqrestore(rq, &rf);
>> }
>>
>> --
>> 2.7.4
>>
On Tue, 12 Nov 2019 at 18:11, Valentin Schneider
<[email protected]> wrote:
>
> Hi Vincent,
>
> I didn't see anything in that reply, was that just a spurious email?
there were a typo in one email address in the 1st message
>
> On 12/11/2019 15:05, Vincent Guittot wrote:
> > Le Tuesday 12 Nov 2019 à 15:48:13 (+0100), Vincent Guittot a écrit :
> >> update_cfs_rq_load_avg() calls cfs_rq_util_change() everytime pelt decays,
> >> which might be inefficient when cpufreq driver has rate limitation.
> >>
> >> When a task is attached on a CPU, we have call path:
> >>
> >> update_blocked_averages()
> >> update_cfs_rq_load_avg()
> >> cfs_rq_util_change -- > trig frequency update
> >> attach_entity_load_avg()
> >> cfs_rq_util_change -- > trig frequency update
> >>
> >> The 1st frequency update will not take into account the utilization of the
> >> newly attached task and the 2nd one might be discard because of rate
> >> limitation of the cpufreq driver.
> >>
> >> update_cfs_rq_load_avg() is only called by update_blocked_averages()
> >> and update_load_avg() so we can move the call to
> >> {cfs_rq,cpufreq}_util_change() into these 2 functions. It's also
> >> interesting to notice that update_load_avg() already calls directly
> >> cfs_rq_util_change() for !SMP case.
> >>
> >> This changes will also ensure that cpufreq_update_util() is called even
> >> when there is no more CFS rq in the leaf_cfs_rq_list to update but only
> >> irq, rt or dl pelt signals.
> >>
> >> Reported-by: Doug Smythies <[email protected]>
> >> Fixes: 039ae8bcf7a5 ("sched/fair: Fix O(nr_cgroups) in the load balancing path")
> >> Signed-off-by: Vincent Guittot <[email protected]>
> >>
> >> ---
> >>
> >> I have just rebased the patch on latest tip/sched/core and made it a proper
> >> patchset after Doug reported that the problem has diseappeared according to
> >> his 1st results but tests results are not all based on the same v5.4-rcX
> >> and with menu instead of teo governor.
> >>
> >> kernel/sched/fair.c | 33 +++++++++++++++++++++------------
> >> 1 file changed, 21 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >> index e458f52..c93d534 100644
> >> --- a/kernel/sched/fair.c
> >> +++ b/kernel/sched/fair.c
> >> @@ -3508,9 +3508,6 @@ update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq)
> >> cfs_rq->load_last_update_time_copy = sa->last_update_time;
> >> #endif
> >>
> >> - if (decayed)
> >> - cfs_rq_util_change(cfs_rq, 0);
> >> -
> >> return decayed;
> >> }
> >>
> >> @@ -3620,8 +3617,12 @@ static inline void update_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s
> >> attach_entity_load_avg(cfs_rq, se, SCHED_CPUFREQ_MIGRATION);
> >> update_tg_load_avg(cfs_rq, 0);
> >>
> >> - } else if (decayed && (flags & UPDATE_TG))
> >> - update_tg_load_avg(cfs_rq, 0);
> >> + } else if (decayed) {
> >> + cfs_rq_util_change(cfs_rq, 0);
> >> +
> >> + if (flags & UPDATE_TG)
> >> + update_tg_load_avg(cfs_rq, 0);
> >> + }
> >> }
> >>
> >> #ifndef CONFIG_64BIT
> >> @@ -7484,6 +7485,7 @@ static void update_blocked_averages(int cpu)
> >> const struct sched_class *curr_class;
> >> struct rq_flags rf;
> >> bool done = true;
> >> + int decayed = 0;
> >>
> >> rq_lock_irqsave(rq, &rf);
> >> update_rq_clock(rq);
> >> @@ -7493,9 +7495,9 @@ static void update_blocked_averages(int cpu)
> >> * that RT, DL and IRQ signals have been updated before updating CFS.
> >> */
> >> curr_class = rq->curr->sched_class;
> >> - update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
> >> - update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
> >> - update_irq_load_avg(rq, 0);
> >> + decayed |= update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
> >> + decayed |= update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
> >> + decayed |= update_irq_load_avg(rq, 0);
> >>
> >> /* Don't need periodic decay once load/util_avg are null */
> >> if (others_have_blocked(rq))
> >> @@ -7529,6 +7531,9 @@ static void update_blocked_averages(int cpu)
> >> }
> >>
> >> update_blocked_load_status(rq, !done);
> >> +
> >> + if (decayed)
> >> + cpufreq_update_util(rq, 0);
> >> rq_unlock_irqrestore(rq, &rf);
> >> }
> >>
> >> @@ -7585,6 +7590,7 @@ static inline void update_blocked_averages(int cpu)
> >> struct cfs_rq *cfs_rq = &rq->cfs;
> >> const struct sched_class *curr_class;
> >> struct rq_flags rf;
> >> + int decayed = 0;
> >>
> >> rq_lock_irqsave(rq, &rf);
> >> update_rq_clock(rq);
> >> @@ -7594,13 +7600,16 @@ static inline void update_blocked_averages(int cpu)
> >> * that RT, DL and IRQ signals have been updated before updating CFS.
> >> */
> >> curr_class = rq->curr->sched_class;
> >> - update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
> >> - update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
> >> - update_irq_load_avg(rq, 0);
> >> + decayed |= update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
> >> + decayed |= update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
> >> + decayed |= update_irq_load_avg(rq, 0);
> >>
> >> - update_cfs_rq_load_avg(cfs_rq_clock_pelt(cfs_rq), cfs_rq);
> >> + decayed |= update_cfs_rq_load_avg(cfs_rq_clock_pelt(cfs_rq), cfs_rq);
> >>
> >> update_blocked_load_status(rq, cfs_rq_has_blocked(cfs_rq) || others_have_blocked(rq));
> >> +
> >> + if (decayed)
> >> + cpufreq_update_util(rq, 0);
> >> rq_unlock_irqrestore(rq, &rf);
> >> }
> >>
> >> --
> >> 2.7.4
> >>
On 12/11/2019 17:20, Vincent Guittot wrote:
>> I didn't see anything in that reply, was that just a spurious email?
>
> there were a typo in one email address in the 1st message
Oh right, got it, sorry.
On 12.11.19 16:05, Vincent Guittot wrote:
> Le Tuesday 12 Nov 2019 à 15:48:13 (+0100), Vincent Guittot a écrit :
>> update_cfs_rq_load_avg() calls cfs_rq_util_change() everytime pelt decays,
>> which might be inefficient when cpufreq driver has rate limitation.
>>
>> When a task is attached on a CPU, we have call path:
>>
>> update_blocked_averages()
>> update_cfs_rq_load_avg()
>> cfs_rq_util_change -- > trig frequency update
>> attach_entity_load_avg()
>> cfs_rq_util_change -- > trig frequency update
This looks like attach_entity_load_avg() is called from
update_blocked_averages(). Do you refer to the attach_entity_load_avg()
call from attach_entity_cfs_rq() or update_load_avg() here? I assume the
former.
>> The 1st frequency update will not take into account the utilization of the
>> newly attached task and the 2nd one might be discard because of rate
>> limitation of the cpufreq driver.
>>
>> update_cfs_rq_load_avg() is only called by update_blocked_averages()
>> and update_load_avg() so we can move the call to
>> {cfs_rq,cpufreq}_util_change() into these 2 functions. It's also
s/cpufreq_util_change()/cpufreq_update_util() ?
[...]
>> I have just rebased the patch on latest tip/sched/core and made it a proper
>> patchset after Doug reported that the problem has diseappeared according to
>> his 1st results but tests results are not all based on the same v5.4-rcX
>> and with menu instead of teo governor.
I had some minor tweaks to do putting this on a0e813f26ebc ("sched/core:
Further clarify sched_class::set_next_task()") ? I saw the '[tip:
sched/urgent] sched/pelt: Fix update of blocked PELT ordering' tip-bot
msg this morning though.
[...]
>> @@ -7493,9 +7495,9 @@ static void update_blocked_averages(int cpu)
>> * that RT, DL and IRQ signals have been updated before updating CFS.
>> */
>> curr_class = rq->curr->sched_class;
>> - update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
>> - update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
>> - update_irq_load_avg(rq, 0);
>> + decayed |= update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
>> + decayed |= update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
>> + decayed |= update_irq_load_avg(rq, 0);
Why not 'decayed = update_cfs_rq_load_avg()' like in the
!CONFIG_FAIR_GROUP_SCHED case?
[...]
On Wed, 13 Nov 2019 at 11:50, Dietmar Eggemann <[email protected]> wrote:
>
> On 12.11.19 16:05, Vincent Guittot wrote:
> > Le Tuesday 12 Nov 2019 à 15:48:13 (+0100), Vincent Guittot a écrit :
> >> update_cfs_rq_load_avg() calls cfs_rq_util_change() everytime pelt decays,
> >> which might be inefficient when cpufreq driver has rate limitation.
> >>
> >> When a task is attached on a CPU, we have call path:
> >>
> >> update_blocked_averages()
> >> update_cfs_rq_load_avg()
> >> cfs_rq_util_change -- > trig frequency update
> >> attach_entity_load_avg()
> >> cfs_rq_util_change -- > trig frequency update
>
> This looks like attach_entity_load_avg() is called from
> update_blocked_averages(). Do you refer to the attach_entity_load_avg()
> call from attach_entity_cfs_rq() or update_load_avg() here? I assume the
> former.
ah... typo mistake, i wanted to write update_load_avg
update_blocked_averages()
update_cfs_rq_load_avg()
cfs_rq_util_change -- > trig frequency update
attach_entity_load_avg()
cfs_rq_util_change -- > trig frequency update
>
> >> The 1st frequency update will not take into account the utilization of the
> >> newly attached task and the 2nd one might be discard because of rate
> >> limitation of the cpufreq driver.
> >>
> >> update_cfs_rq_load_avg() is only called by update_blocked_averages()
> >> and update_load_avg() so we can move the call to
> >> {cfs_rq,cpufreq}_util_change() into these 2 functions. It's also
>
> s/cpufreq_util_change()/cpufreq_update_util() ?
yes
>
> [...]
>
> >> I have just rebased the patch on latest tip/sched/core and made it a proper
> >> patchset after Doug reported that the problem has diseappeared according to
> >> his 1st results but tests results are not all based on the same v5.4-rcX
> >> and with menu instead of teo governor.
>
> I had some minor tweaks to do putting this on a0e813f26ebc ("sched/core:
> Further clarify sched_class::set_next_task()") ? I saw the '[tip:
> sched/urgent] sched/pelt: Fix update of blocked PELT ordering' tip-bot
> msg this morning though.
yes, a0e813f26ebc was item 1 and this patch is item 2 on top
>
> [...]
>
> >> @@ -7493,9 +7495,9 @@ static void update_blocked_averages(int cpu)
> >> * that RT, DL and IRQ signals have been updated before updating CFS.
> >> */
> >> curr_class = rq->curr->sched_class;
> >> - update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
> >> - update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
> >> - update_irq_load_avg(rq, 0);
> >> + decayed |= update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
> >> + decayed |= update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
> >> + decayed |= update_irq_load_avg(rq, 0);
>
> Why not 'decayed = update_cfs_rq_load_avg()' like in the
> !CONFIG_FAIR_GROUP_SCHED case?
Because it is handled by the update_load_avg() in
for_each_leaf_cfs_rq_safe() loop
This means that we can have 2 calls to cpufreq_update_util in
update_blocked_average() but at least the values will be up to date in
both calls unlike previously.
I'm going to prepare an additional patch to remove this useless call.
I have also seen some possible further optimization that i need to
study a bit more before preparing a patch
>
> [...]
On 13.11.19 14:30, Vincent Guittot wrote:
> On Wed, 13 Nov 2019 at 11:50, Dietmar Eggemann <[email protected]> wrote:
>>
>> On 12.11.19 16:05, Vincent Guittot wrote:
>>> Le Tuesday 12 Nov 2019 à 15:48:13 (+0100), Vincent Guittot a écrit :
[...]
>>>> @@ -7493,9 +7495,9 @@ static void update_blocked_averages(int cpu)
>>>> * that RT, DL and IRQ signals have been updated before updating CFS.
>>>> */
>>>> curr_class = rq->curr->sched_class;
>>>> - update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
>>>> - update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
>>>> - update_irq_load_avg(rq, 0);
>>>> + decayed |= update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
>>>> + decayed |= update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
>>>> + decayed |= update_irq_load_avg(rq, 0);
>>
>> Why not 'decayed = update_cfs_rq_load_avg()' like in the
>> !CONFIG_FAIR_GROUP_SCHED case?
>
> Because it is handled by the update_load_avg() in
> for_each_leaf_cfs_rq_safe() loop
>
> This means that we can have 2 calls to cpufreq_update_util in
> update_blocked_average() but at least the values will be up to date in
> both calls unlike previously.
>
> I'm going to prepare an additional patch to remove this useless call.
> I have also seen some possible further optimization that i need to
> study a bit more before preparing a patch
I see. The update_load_avg() call for the taskgroup skeleton se
(cfs_rq->tg->se[cpu]). But what happens to the cpu which only has the
root cfs_rq i the list? It doesn't have a skeleton se.
Le Wednesday 13 Nov 2019 ? 15:09:47 (+0100), Dietmar Eggemann a ?crit :
> On 13.11.19 14:30, Vincent Guittot wrote:
> > On Wed, 13 Nov 2019 at 11:50, Dietmar Eggemann <[email protected]> wrote:
> >>
> >> On 12.11.19 16:05, Vincent Guittot wrote:
> >>> Le Tuesday 12 Nov 2019 ? 15:48:13 (+0100), Vincent Guittot a ?crit :
>
> [...]
>
> >>>> @@ -7493,9 +7495,9 @@ static void update_blocked_averages(int cpu)
> >>>> * that RT, DL and IRQ signals have been updated before updating CFS.
> >>>> */
> >>>> curr_class = rq->curr->sched_class;
> >>>> - update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
> >>>> - update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
> >>>> - update_irq_load_avg(rq, 0);
> >>>> + decayed |= update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
> >>>> + decayed |= update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
> >>>> + decayed |= update_irq_load_avg(rq, 0);
> >>
> >> Why not 'decayed = update_cfs_rq_load_avg()' like in the
> >> !CONFIG_FAIR_GROUP_SCHED case?
> >
> > Because it is handled by the update_load_avg() in
> > for_each_leaf_cfs_rq_safe() loop
> >
> > This means that we can have 2 calls to cpufreq_update_util in
> > update_blocked_average() but at least the values will be up to date in
> > both calls unlike previously.
> >
> > I'm going to prepare an additional patch to remove this useless call.
> > I have also seen some possible further optimization that i need to
> > study a bit more before preparing a patch
>
> I see. The update_load_avg() call for the taskgroup skeleton se
> (cfs_rq->tg->se[cpu]). But what happens to the cpu which only has the
> root cfs_rq i the list? It doesn't have a skeleton se.
you're right. I have to add the following to make sure it will be called
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 2eb1aa8..9fc077c 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7604,9 +7604,13 @@ static void update_blocked_averages(int cpu)
cpu,
cfs_rq == &rq->cfs ? 0 : (long)cfs_rq->tg );
- if (update_cfs_rq_load_avg(cfs_rq_clock_pelt(cfs_rq), cfs_rq))
+ if (update_cfs_rq_load_avg(cfs_rq_clock_pelt(cfs_rq), cfs_rq)) {
update_tg_load_avg(cfs_rq, 0);
+ if (cfs_rq == &rq->cfs)
+ decayed = 1;
+ }
+
trace_sched_load_contrib_blocked(cpu,
&cfs_rq->avg,
cfs_rq == &rq->cfs ? 0 : (long)cfs_rq->tg );
Le Wednesday 13 Nov 2019 ? 18:50:35 (+0100), Vincent Guittot a ?crit :
> Le Wednesday 13 Nov 2019 ? 15:09:47 (+0100), Dietmar Eggemann a ?crit :
> > On 13.11.19 14:30, Vincent Guittot wrote:
> > > On Wed, 13 Nov 2019 at 11:50, Dietmar Eggemann <[email protected]> wrote:
> > >>
> > >> On 12.11.19 16:05, Vincent Guittot wrote:
> > >>> Le Tuesday 12 Nov 2019 ? 15:48:13 (+0100), Vincent Guittot a ?crit :
> >
> > [...]
> >
> > >>>> @@ -7493,9 +7495,9 @@ static void update_blocked_averages(int cpu)
> > >>>> * that RT, DL and IRQ signals have been updated before updating CFS.
> > >>>> */
> > >>>> curr_class = rq->curr->sched_class;
> > >>>> - update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
> > >>>> - update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
> > >>>> - update_irq_load_avg(rq, 0);
> > >>>> + decayed |= update_rt_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &rt_sched_class);
> > >>>> + decayed |= update_dl_rq_load_avg(rq_clock_pelt(rq), rq, curr_class == &dl_sched_class);
> > >>>> + decayed |= update_irq_load_avg(rq, 0);
> > >>
> > >> Why not 'decayed = update_cfs_rq_load_avg()' like in the
> > >> !CONFIG_FAIR_GROUP_SCHED case?
> > >
> > > Because it is handled by the update_load_avg() in
> > > for_each_leaf_cfs_rq_safe() loop
> > >
> > > This means that we can have 2 calls to cpufreq_update_util in
> > > update_blocked_average() but at least the values will be up to date in
> > > both calls unlike previously.
> > >
> > > I'm going to prepare an additional patch to remove this useless call.
> > > I have also seen some possible further optimization that i need to
> > > study a bit more before preparing a patch
> >
> > I see. The update_load_avg() call for the taskgroup skeleton se
> > (cfs_rq->tg->se[cpu]). But what happens to the cpu which only has the
> > root cfs_rq i the list? It doesn't have a skeleton se.
>
> you're right. I have to add the following to make sure it will be called
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 2eb1aa8..9fc077c 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7604,9 +7604,13 @@ static void update_blocked_averages(int cpu)
> cpu,
> cfs_rq == &rq->cfs ? 0 : (long)cfs_rq->tg );
>
> - if (update_cfs_rq_load_avg(cfs_rq_clock_pelt(cfs_rq), cfs_rq))
> + if (update_cfs_rq_load_avg(cfs_rq_clock_pelt(cfs_rq), cfs_rq)) {
> update_tg_load_avg(cfs_rq, 0);
>
> + if (cfs_rq == &rq->cfs)
> + decayed = 1;
> + }
> +
> trace_sched_load_contrib_blocked(cpu,
> &cfs_rq->avg,
> cfs_rq == &rq->cfs ? 0 : (long)cfs_rq->tg );
the proper fix without some debug trace events :-)
@@ -7567,9 +7569,13 @@ static void update_blocked_averages(int cpu)
for_each_leaf_cfs_rq_safe(rq, cfs_rq, pos) {
struct sched_entity *se;
- if (update_cfs_rq_load_avg(cfs_rq_clock_pelt(cfs_rq), cfs_rq))
+ if (update_cfs_rq_load_avg(cfs_rq_clock_pelt(cfs_rq), cfs_rq)) {
update_tg_load_avg(cfs_rq, 0);
+ if (cfs_rq == &rq->cfs)
+ decayed = 1;
+ }
+
/* Propagate pending load changes to the parent, if any: */
se = cfs_rq->tg->se[cpu];
if (se && !skip_blocked_update(se))
>
>