Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <26a71d5c-9a4a-4064-8155-a14126a654f8@gmail.com>
Date:   Fri, 3 Nov 2023 22:01:55 +0800
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/1] sched/fair: Track current se's EEVDF parameters
Content-Language: en-US
To:     Abel Wu <wuyun.abel@bytedance.com>, mingo@redhat.com,
        peterz@infradead.org
Cc:     vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        linux-kernel@vger.kernel.org
References: <20231102142022.19302-1-s921975628@gmail.com>
 <20231102142022.19302-2-s921975628@gmail.com>
 <1f90d44f-e20c-445a-8bc5-8440c870176e@bytedance.com>
From:   Yiwei Lin <s921975628@gmail.com>
In-Reply-To: <1f90d44f-e20c-445a-8bc5-8440c870176e@bytedance.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 11/3/23 14:45, Abel Wu wrote:
> On 11/2/23 10:20 PM, Yiwei Lin Wrote:
>> After dequeuing the current-picked scheduling entity with
>> `__dequeue_entity`, its contribution to the EEVDF parameters
>> cfs_rq->avg_vruntime and cfs_rq->avg_load are also removed.
>> Because these should in fact be considered for the EEVDF algorithm,
>> we took curr as the special case and inserted back the contributions
>> when requests for cfs_rq->avg_vruntime and cfs_rq->avg_load.
>>
>> Functions like `entity_eligible` which is called insied a loop may
>> therefore recalculate these statistics repeatly and require more effort.
>> Instead, we could just avoid to remove these statistics from
>> cfs_rq->avg_vruntime and cfs_rq->avg_load directly.
>>
>> Signed-off-by: Yiwei Lin <s921975628@gmail.com>
>> ---
>>   kernel/sched/fair.c | 39 +++++++++++----------------------------
>>   1 file changed, 11 insertions(+), 28 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 876798824..a10a73603 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -655,17 +655,9 @@ void avg_vruntime_update(struct cfs_rq *cfs_rq, 
>> s64 delta)
>>    */
>>   u64 avg_vruntime(struct cfs_rq *cfs_rq)
>>   {
>> -    struct sched_entity *curr = cfs_rq->curr;
>>       s64 avg = cfs_rq->avg_vruntime;
>>       long load = cfs_rq->avg_load;
>>   -    if (curr && curr->on_rq) {
>> -        unsigned long weight = scale_load_down(curr->load.weight);
>> -
>> -        avg += entity_key(cfs_rq, curr) * weight;
>> -        load += weight;
>> -    }
>> -
>>       if (load) {
>>           /* sign flips effective floor / ceil */
>>           if (avg < 0)
>> @@ -722,17 +714,9 @@ static void update_entity_lag(struct cfs_rq 
>> *cfs_rq, struct sched_entity *se)
>>    */
>>   int entity_eligible(struct cfs_rq *cfs_rq, struct sched_entity *se)
>>   {
>> -    struct sched_entity *curr = cfs_rq->curr;
>>       s64 avg = cfs_rq->avg_vruntime;
>>       long load = cfs_rq->avg_load;
>>   -    if (curr && curr->on_rq) {
>> -        unsigned long weight = scale_load_down(curr->load.weight);
>> -
>> -        avg += entity_key(cfs_rq, curr) * weight;
>> -        load += weight;
>> -    }
>> -
>>       return avg >= entity_key(cfs_rq, se) * load;
>>   }
>>   @@ -821,11 +805,12 @@ static void __enqueue_entity(struct cfs_rq 
>> *cfs_rq, struct sched_entity *se)
>>                   __entity_less, &min_deadline_cb);
>>   }
>>   -static void __dequeue_entity(struct cfs_rq *cfs_rq, struct 
>> sched_entity *se)
>> +static void __dequeue_entity(struct cfs_rq *cfs_rq, struct 
>> sched_entity *se, bool on_rq)
>
> Please don't add such complexity. Since avg_vruntime includes the whole
> cfs_rq after this change, you can simply avg_vruntime_add() when doing
> enqueue_entity() and avg_vruntime_sub() in dequeue_entity().
>
Right. I should consider that we'll also use __enqueue_entity to put 
back the 'curr', and
avg_vruntime_add() when doing enqueue_entity() and avg_vruntime_sub() in 
dequeue_entity()
as you mentioned could not only simplify the implementation but also 
address to this issue.
I'll fix it.

>>   {
>>       rb_erase_augmented_cached(&se->run_node, &cfs_rq->tasks_timeline,
>>                     &min_deadline_cb);
>> -    avg_vruntime_sub(cfs_rq, se);
>> +    if (!on_rq)
>> +        avg_vruntime_sub(cfs_rq, se);
>>   }
>>     struct sched_entity *__pick_first_entity(struct cfs_rq *cfs_rq)
>> @@ -1137,6 +1122,7 @@ static void update_curr(struct cfs_rq *cfs_rq)
>>       struct sched_entity *curr = cfs_rq->curr;
>>       u64 now = rq_clock_task(rq_of(cfs_rq));
>>       u64 delta_exec;
>> +    u64 delta_fair;
>>         if (unlikely(!curr))
>>           return;
>> @@ -1158,7 +1144,9 @@ static void update_curr(struct cfs_rq *cfs_rq)
>>       curr->sum_exec_runtime += delta_exec;
>>       schedstat_add(cfs_rq->exec_clock, delta_exec);
>>   -    curr->vruntime += calc_delta_fair(delta_exec, curr);
>> +    delta_fair = calc_delta_fair(delta_exec, curr);
>> +    curr->vruntime += delta_fair;
>> +    cfs_rq->avg_vruntime += delta_fair * 
>> scale_load_down(curr->load.weight);
>
> What if curr->load.weight changes in this time slice?

The change on reweight_entity now don't separate curr and other 
entities. If curr->load.weight is changed,
reweight_entity will do:
1. update_curr() to account the latest contribution of vruntime with old 
weight.
2. avg_vruntime_sub() to remove the curr vruntime with old weight
3. avg_vruntime_add() to insert the curr vruntime with new weight
Although step1 and step2 could be merged together when re-weighting the 
curr entity, but it
seems just find to general implement reweight_entity.