From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751390AbcFWPgA (ORCPT ); Thu, 23 Jun 2016 11:36:00 -0400 Received: from foss.arm.com ([217.140.101.70]:54278 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750960AbcFWPf7 (ORCPT ); Thu, 23 Jun 2016 11:35:59 -0400 Subject: Re: [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks To: Peter Zijlstra References: <20160617120136.064100812@infradead.org> <20160617120454.150630859@infradead.org> <20160617142814.GT30154@twins.programming.kicks-ass.net> <20160617160239.GL30927@twins.programming.kicks-ass.net> <20160617161831.GM30927@twins.programming.kicks-ass.net> <5767D51F.3080600@arm.com> <5768027E.1090408@arm.com> <20160621084119.GN30154@twins.programming.kicks-ass.net> Cc: Vincent Guittot , Yuyang Du , Ingo Molnar , linux-kernel , Mike Galbraith , Benjamin Segall , Paul Turner , Morten Rasmussen , Matt Fleming From: Dietmar Eggemann Message-ID: <576C01DA.2050406@arm.com> Date: Thu, 23 Jun 2016 16:35:54 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <20160621084119.GN30154@twins.programming.kicks-ass.net> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.