A CFS (SCHED_OTHER, SCHED_BATCH or SCHED_IDLE policy) task's
se->runnable_weight must always be in sync with its se->load.weight.
se->runnable_weight is set to se->load.weight when the task is
forked (init_entity_runnable_average()) or reniced (reweight_entity()).
There are two cases in set_load_weight() which since they currently only
set se->load.weight could lead to a situation in which se->load.weight
is different to se->runnable_weight for a CFS task:
(1) A task switches to SCHED_IDLE.
(2) A SCHED_FIFO, SCHED_RR or SCHED_DEADLINE task which has been reniced
(during which only its static priority gets set) switches to
SCHED_OTHER or SCHED_BATCH.
Set se->runnable_weight to se->load.weight in these two cases to prevent
this. This eliminates the need to explicitly set it to se->load.weight
during PELT updates in the CFS scheduler fastpath.
Signed-off-by: Dietmar Eggemann <[email protected]>
---
kernel/sched/core.c | 2 ++
kernel/sched/fair.c | 6 ------
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index fe365c9a08e9..4eaf6166a293 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -724,6 +724,7 @@ static void set_load_weight(struct task_struct *p, bool update_load)
if (idle_policy(p->policy)) {
load->weight = scale_load(WEIGHT_IDLEPRIO);
load->inv_weight = WMULT_IDLEPRIO;
+ p->se.runnable_weight = load->weight;
return;
}
@@ -736,6 +737,7 @@ static void set_load_weight(struct task_struct *p, bool update_load)
} else {
load->weight = scale_load(sched_prio_to_weight[prio]);
load->inv_weight = sched_prio_to_wmult[prio];
+ p->se.runnable_weight = load->weight;
}
}
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 321cd5dcf2e8..8c09c4974edf 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3327,9 +3327,6 @@ static inline void cfs_se_util_change(struct sched_avg *avg)
static int
__update_load_avg_blocked_se(u64 now, int cpu, struct sched_entity *se)
{
- if (entity_is_task(se))
- se->runnable_weight = se->load.weight;
-
if (___update_load_sum(now, cpu, &se->avg, 0, 0, 0)) {
___update_load_avg(&se->avg, se_weight(se), se_runnable(se));
return 1;
@@ -3341,9 +3338,6 @@ __update_load_avg_blocked_se(u64 now, int cpu, struct sched_entity *se)
static int
__update_load_avg_se(u64 now, int cpu, struct cfs_rq *cfs_rq, struct sched_entity *se)
{
- if (entity_is_task(se))
- se->runnable_weight = se->load.weight;
-
if (___update_load_sum(now, cpu, &se->avg, !!se->on_rq, !!se->on_rq,
cfs_rq->curr == se)) {
--
2.11.0
On Mon, Jul 09, 2018 at 05:47:53PM +0100, Dietmar Eggemann wrote:
> A CFS (SCHED_OTHER, SCHED_BATCH or SCHED_IDLE policy) task's
> se->runnable_weight must always be in sync with its se->load.weight.
>
> se->runnable_weight is set to se->load.weight when the task is
> forked (init_entity_runnable_average()) or reniced (reweight_entity()).
>
> There are two cases in set_load_weight() which since they currently only
> set se->load.weight could lead to a situation in which se->load.weight
> is different to se->runnable_weight for a CFS task:
>
> (1) A task switches to SCHED_IDLE.
>
> (2) A SCHED_FIFO, SCHED_RR or SCHED_DEADLINE task which has been reniced
> (during which only its static priority gets set) switches to
> SCHED_OTHER or SCHED_BATCH.
>
> Set se->runnable_weight to se->load.weight in these two cases to prevent
> this. This eliminates the need to explicitly set it to se->load.weight
> during PELT updates in the CFS scheduler fastpath.
Looks good to me. By the way just asking, is there a chance where
se_weight(se) and se_runnable(se) can ever be different? Seems not, then in
that case we pointlessly do this division in ___update_load_avg if the sa
belongs to the task:
sa->load_avg = div_u64(load * sa->load_sum, divider);
sa->runnable_load_avg = div_u64(runnable * sa->runnable_load_sum, divider);
and we could probably just do this if se is a task?
sa->load_avg = div_u64(load * sa->load_sum, divider);
sa->runnable_load_avg = sa->load_avg;
thanks,
-Joel
On 07/11/2018 01:09 AM, Joel Fernandes wrote:
> On Mon, Jul 09, 2018 at 05:47:53PM +0100, Dietmar Eggemann wrote:
>> A CFS (SCHED_OTHER, SCHED_BATCH or SCHED_IDLE policy) task's
>> se->runnable_weight must always be in sync with its se->load.weight.
>>
>> se->runnable_weight is set to se->load.weight when the task is
>> forked (init_entity_runnable_average()) or reniced (reweight_entity()).
>>
>> There are two cases in set_load_weight() which since they currently only
>> set se->load.weight could lead to a situation in which se->load.weight
>> is different to se->runnable_weight for a CFS task:
>>
>> (1) A task switches to SCHED_IDLE.
>>
>> (2) A SCHED_FIFO, SCHED_RR or SCHED_DEADLINE task which has been reniced
>> (during which only its static priority gets set) switches to
>> SCHED_OTHER or SCHED_BATCH.
>>
>> Set se->runnable_weight to se->load.weight in these two cases to prevent
>> this. This eliminates the need to explicitly set it to se->load.weight
>> during PELT updates in the CFS scheduler fastpath.
>
> Looks good to me. By the way just asking, is there a chance where
> se_weight(se) and se_runnable(se) can ever be different?
Yes they can be different, not for a task though but for se's
representing task groups. It got introduced to be able to propagate load
and runnable load independently through the task groups hierarchies.
On Wed, Jul 11, 2018 at 10:43:28AM +0200, Dietmar Eggemann wrote:
> On 07/11/2018 01:09 AM, Joel Fernandes wrote:
> > On Mon, Jul 09, 2018 at 05:47:53PM +0100, Dietmar Eggemann wrote:
> > > A CFS (SCHED_OTHER, SCHED_BATCH or SCHED_IDLE policy) task's
> > > se->runnable_weight must always be in sync with its se->load.weight.
> > >
> > > se->runnable_weight is set to se->load.weight when the task is
> > > forked (init_entity_runnable_average()) or reniced (reweight_entity()).
> > >
> > > There are two cases in set_load_weight() which since they currently only
> > > set se->load.weight could lead to a situation in which se->load.weight
> > > is different to se->runnable_weight for a CFS task:
> > >
> > > (1) A task switches to SCHED_IDLE.
> > >
> > > (2) A SCHED_FIFO, SCHED_RR or SCHED_DEADLINE task which has been reniced
> > > (during which only its static priority gets set) switches to
> > > SCHED_OTHER or SCHED_BATCH.
> > >
> > > Set se->runnable_weight to se->load.weight in these two cases to prevent
> > > this. This eliminates the need to explicitly set it to se->load.weight
> > > during PELT updates in the CFS scheduler fastpath.
> >
> > Looks good to me. By the way just asking, is there a chance where
> > se_weight(se) and se_runnable(se) can ever be different?
>
> Yes they can be different, not for a task though but for se's representing
> task groups. It got introduced to be able to propagate load and runnable
> load independently through the task groups hierarchies.
I know that task-group se has different values. I was saying for task ses,
the extra division can be skipped possibly improving performance (if at all).
thanks,
- Joel
On 07/12/2018 10:17 AM, Joel Fernandes wrote:
> On Wed, Jul 11, 2018 at 10:43:28AM +0200, Dietmar Eggemann wrote:
>> On 07/11/2018 01:09 AM, Joel Fernandes wrote:
>>> On Mon, Jul 09, 2018 at 05:47:53PM +0100, Dietmar Eggemann wrote:
>>>> A CFS (SCHED_OTHER, SCHED_BATCH or SCHED_IDLE policy) task's
>>>> se->runnable_weight must always be in sync with its se->load.weight.
>>>>
>>>> se->runnable_weight is set to se->load.weight when the task is
>>>> forked (init_entity_runnable_average()) or reniced (reweight_entity()).
>>>>
>>>> There are two cases in set_load_weight() which since they currently only
>>>> set se->load.weight could lead to a situation in which se->load.weight
>>>> is different to se->runnable_weight for a CFS task:
>>>>
>>>> (1) A task switches to SCHED_IDLE.
>>>>
>>>> (2) A SCHED_FIFO, SCHED_RR or SCHED_DEADLINE task which has been reniced
>>>> (during which only its static priority gets set) switches to
>>>> SCHED_OTHER or SCHED_BATCH.
>>>>
>>>> Set se->runnable_weight to se->load.weight in these two cases to prevent
>>>> this. This eliminates the need to explicitly set it to se->load.weight
>>>> during PELT updates in the CFS scheduler fastpath.
>>>
>>> Looks good to me. By the way just asking, is there a chance where
>>> se_weight(se) and se_runnable(se) can ever be different?
>>
>> Yes they can be different, not for a task though but for se's representing
>> task groups. It got introduced to be able to propagate load and runnable
>> load independently through the task groups hierarchies.
>
> I know that task-group se has different values. I was saying for task ses,
> the extra division can be skipped possibly improving performance (if at all).
Ah, OK, I didn't pay attention to the '... if the sa belongs to the
task:' part.
Since ___update_load_sum() is either called with !!se->on_rq or 0 for
both ('load' and 'runnable'), sa->load_sum and sa->runnable_load_sum
should be equal for a task.
And since se_weight(se) and se_runnable(se) for 'load' and 'runnable' of
___update_load_avg() are the same for a task, sa->load_avg and
sa->runnable_load_avg should be also equal for a task.
You would have to pass the information that sa belongs to a se and not
to a cfs_rq into ___update_load_avg() though to be able to use
entity_is_task().
[...]