Re: [PATCH v3 2/5] sched/fair: Skip detach and attach new group task

[Vincent Guittot]

On 1 June 2016 at 21:21, Yuyang Du <yuyang.du@xxxxxxxxx> wrote:
> On Wed, Jun 01, 2016 at 02:20:09PM +0200, Vincent Guittot wrote:
>> On 1 June 2016 at 05:41, Yuyang Du <yuyang.du@xxxxxxxxx> wrote:
>> > Vincent reported that the first task to a new task group's cfs_rq will
>> > be attached in attach_task_cfs_rq() and once more when it is enqueued
>> > (see https://lkml.org/lkml/2016/5/25/388).
>> >
>> > Actually, it is much worse. The load is currently attached mostly twice
>> > every time when we switch to fair class or change task groups. These two
>> > scenarios are concerned, which we will descripbe in the following
>> > respectively
>>
>> AFAICT and according to tests that i have done around these 2 use
>> cases, the task is attached only once during a switched to fair and a
>> sched_move_task. Have you face such situation during tests ? What is
>> the sequence that generates this issue ?
>>
>> >
>> > 1) Switch to fair class:
>> >
>> > The sched class change is done like this:
>> >
>> >         if (queued)
>> >           enqueue_task();
>> >         check_class_changed()
>> >           switched_from()
>> >           switched_to()
>> >
>> > If the task is on_rq, it should have already been enqueued, which
>> > MAY have attached the load to the cfs_rq, if so, we shouldn't attach
>>
>> No, it can't. The only way to attach task during enqueue is if
>> last_update_time has been reset which is not the case during a
>> switched_to_fair
>
> My response to your above two comments:
>
> As I said, there can be four possibilities going through the above sequences:
>
> (1) on_rq, (2) !on_rq, (a) was fair class (representing last_update_time != 0),
> (b) never was fair class (representing last_update_time == 0, but may not be
> limited to this)
>
> Crossing them, we have (1)(a), (1)(b), (2)(a), and (2)(b).
>
> Some will attach twice, which are (1)(b) and (2)(b), the other will attach
> once, which are (1)(a) and (2)(a). The difficult part is they can be attached
> at different places.

ok for (1)(b) but not for (2)(b) and it's far from "attached mostly
twice every time"

The root cause is that the last_update_time is initialize to 0 which
have a special meaning for the load_avg. We should better initialize
it to something different like for cfs_rq

>
> So, the simplest sulution is to reset the task's last_update_time to 0, when
> the task is switched from fair. Then let task enqueue do the load attachment,
> only once at this place under all circumstances.

IMHO, the better solution is to not initialize last_update_time to
something different from 0 which has a special meaning

>
>> > 2) Change between fair task groups:
>> >
>> > The task groups are changed like this:
>> >
>> >         if (queued)
>> >           dequeue_task()
>> >         task_move_group()
>> >         if (queued)
>> >           enqueue_task()
>> >
>> > Unlike the switch to fair class, if the task is on_rq, it will be enqueued
>> > after we move task groups, so the simplest solution is to reset the
>> > task's last_update_time when we do task_move_group(), and then let
>> > enqueue_task() do the load attachment.
>>
>> Same for this sequence, the task is explicitly attached only once
>> during the task_move_group but never during the enqueue.
>
> Your patch said there can be twice, :)

My patch says the 1st task that is attached on a cfs rq wil be
attached twice not "The load is currently attached mostly twice
every time when we switch to fair class or change task groups. "  You
say that it's happen mostly every time and i disagree on that.

For Change between fair task group, i still don't see how it can
attached mostly twice every time

>
>> So you want to delay the attach during the enqueue ?
>
> Yes, despite of delay or not delay, the key is to only attach at enqueue(),
> this is the simplest solution.
>
>> But what happen
>> if the task was not enqueue when it has been moved between groups ?
>> The load_avg of the task stays frozen during the period because its
>> last_update_time is reset
>
> That is the !on_rq case. By "frozen", you mean it won't be decayed, right?
> Yes, this is the downside. But what if the task will never be enqueued,

That's a big downside IMO

> that legacy load does not mean anything in this case.