linux-kernel - Re: [PATCH v6 1/4] sched/fair: Fix attaching task sched avgs twice when switching to fair or changing task group

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160617094811.GN30154@twins.programming.kicks-ass.net>
Date:	Fri, 17 Jun 2016 11:48:11 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Vincent Guittot <vincent.guittot@...aro.org>
Cc:	Yuyang Du <yuyang.du@...el.com>, Ingo Molnar <mingo@...nel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Mike Galbraith <umgwanakikbuti@...il.com>,
	Benjamin Segall <bsegall@...gle.com>,
	Paul Turner <pjt@...gle.com>,
	Morten Rasmussen <morten.rasmussen@....com>,
	Dietmar Eggemann <dietmar.eggemann@....com>,
	Matt Fleming <matt@...eblueprint.co.uk>
Subject: Re: [PATCH v6 1/4] sched/fair: Fix attaching task sched avgs twice
 when switching to fair or changing task group

On Thu, Jun 16, 2016 at 11:21:55PM +0200, Vincent Guittot wrote:
> On 16 June 2016 at 22:07, Peter Zijlstra <peterz@...radead.org> wrote:

> > So I don't see how it can end up being attached again.
> 
> In fact it has already been attached during the sched_move_task. The
> sequence for the 1st task that is attached to a cfs_rq is :
> 
> sched_move_task()
>   task_move_group_fair()
>     detach_task_cfs_rq()
>     set_task_rq()
>     attach_task_cfs_rq()
>       attach_entity_load_avg()
>         se->avg.last_load_update = cfs_rq->avg.last_load_update == 0
> 
> Then we enqueue the task

> > enqueue_entity()
> >   enqueue_entity_load_avg()
> >     update_cfs_rq_load_avg()
> >       now = clock()
> >       __update_load_avg(&cfs_rq->avg)
> >         cfs_rq->avg.last_load_update = now
> >         // ages 0 load/util for: now - 0
> >     if (migrated)
> >       attach_entity_load_avg()
> >         se->avg.last_load_update = cfs_rq->avg.last_load_update; // now != 0

> but se->avg.last_load_update is still 0 so
> migrated is set and we attach the task one more time

Ok, we indeed attach twice here, however I don't actually see it be a
problem (much). Because the aging during the enqueue will age the entire
first attach right back down to 0 again; now-0 is a _huge_ delta and
will flatten everything right down to 0.

Of course, that also ages the new entity right back down to 0, making it
appear like it has no load what so ever.

But yes, /me puts in changelog.