linux-kernel - Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks

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 for Android: free password hash cracker in your pocket

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

Message-ID: <576C01DA.2050406@arm.com>
Date:	Thu, 23 Jun 2016 16:35:54 +0100
From:	Dietmar Eggemann <dietmar.eggemann@....com>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Vincent Guittot <vincent.guittot@...aro.org>,
	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>,
	Matt Fleming <matt@...eblueprint.co.uk>
Subject: Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks

On 21/06/16 09:41, Peter Zijlstra wrote:
> On Mon, Jun 20, 2016 at 03:49:34PM +0100, Dietmar Eggemann wrote:
>> On 20/06/16 13:35, Vincent Guittot wrote:
> 
>>> It will go through wake_up_new_task and post_init_entity_util_avg
>>> during its fork which is enough to set last_update_time. Then, it will
>>> use the switched_to_fair if the task becomes a fair one
>>
>> Oh I see. We want to make sure that every task (even when forked as
>> !fair) has a last_update_time value != 0, when becoming fair one day.
> 
> Right, see 2 below. I need to write a bunch of comments explaining PELT
> proper, as well as document these things.
> 
> The things we ran into with these patches were that:
> 
>  1) You need to update the cfs_rq _before_ any entity attach/detach
>     (and might need to update_tg_load_avg when update_cfs_rq_load_avg()
>     returns true).
> 
>  2) (fair) entities are always attached, switched_from/to deal with !fair.
> 
>  3) cpu migration is the only exception and uses the last_update_time=0
>     thing -- because refusal to take second rq->lock.

2) is about changing sched classes, 3) is about changing cpus but what
about 4) changing task groups?

There is still this last_update_time = 0 between
detach_task_cfs_rq()/set_task_rq() and attach_task_cfs_rq() in
task_move_group_fair() preventing the call __update_load_avg(...
p->se->avg, ...) in attach_task_cfs_rq() -> attach_entity_load_avg().

Shouldn't be necessary any more since cfs_rq 'next' is up-to-date now.

Assuming here that the exception in 3) relates to the fact that the
rq->lock is not taken.

Or is 4) a second exception in the sense that the se has been aged in
remove_entity_load_avg() (3)) resp. detach_entity_load_avg() (4))?

> Which is why I dislike Yuyang's patches, they create more exceptions
> instead of applying existing rules (albeit undocumented).
> 
> Esp. 1 is important, because while for mathematically consistency you
> don't actually need to do this, you only need the entities to be
> up-to-date with the cfs rq when you attach/detach, but that forgets the
> temporal aspect of _when_ you do this.