Re: [PATCH] sched/fair: vruntime should normalize when switching from fair

[Joel Fernandes]

On Wed, Sep 26, 2018 at 6:19 PM, Wanpeng Li <kernellwp@xxxxxxxxx> wrote:
> On Thu, 27 Sep 2018 at 06:38, Dietmar Eggemann <dietmar.eggemann@xxxxxxx> wrote:
>>
>> Hi,
>>
>> On 09/26/2018 11:50 AM, Wanpeng Li wrote:
>> > Hi Dietmar,
>> > On Tue, 28 Aug 2018 at 22:55, Dietmar Eggemann <dietmar.eggemann@xxxxxxx> wrote:
>> >>
>> >> On 08/27/2018 12:14 PM, Peter Zijlstra wrote:
>> >>> On Fri, Aug 24, 2018 at 02:24:48PM -0700, Steve Muckle wrote:
>> >>>> On 08/24/2018 02:47 AM, Peter Zijlstra wrote:
>> >>>>>>> On 08/17/2018 11:27 AM, Steve Muckle wrote:
>>
>> [...]
>>
>> >>>>>>>> - later, when the prio is deboosted and the task is moved back
>> >>>>>>>>       to the fair class, the fair rq's min_vruntime is added to
>> >>>>>>>>       the task's vruntime, even though it wasn't subtracted earlier.
>> >
>> > Could you point out when the fair rq's min_vruntime is added to the
>> > task's vruntime in your *later* scenario? attach_task_cfs_rq will not
>> > do that the same reason as detach_task_cfs_rq. fair task's
>> > sched_remote_wakeup is false which results in vruntime will not be
>> > renormalized in enqueue_entity.
>>
>> The cfs_rq->min_vruntime is still added to the se->vruntime in
>> enqueue_task_fair().
>
> I understand what your patch done,
>
> On your CPU4:
> scheduler_ipi()
>  -> sched_ttwu_pending()
>       -> ttwu_do_activate()    => p->sched_remote_wakeup should be
> false, so ENQUEUE_WAKEUP is set, ENQUEUE_MIGRATED is not
>            -> ttwu_activate()
>                 -> activate_task()
>                      -> enqueue_task()
>                           -> enqueue_task_fair()
>                                -> enqueue_entity()
>                                     bool renorm = !(flags &
> ENQUEUE_WAKEUP) || (flags & ENQUEUE_MIGRATE)
> so renorm is false in enqueue_entity(), why you mentioned that the
> cfs_rq->min_vruntime is still added to the se->vruntime in
> enqueue_task_fair()?

If I understand John's original patch correctly, the additional
vruntime is added when the class of the waking task is changed during
priority deboost so that path is different from the one you listed
above? Perhaps you should be looking at rt_mutex_setprio?

 - Joel