From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751570AbcFFJy7 (ORCPT ); Mon, 6 Jun 2016 05:54:59 -0400 Received: from merlin.infradead.org ([205.233.59.134]:54270 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750927AbcFFJy6 (ORCPT ); Mon, 6 Jun 2016 05:54:58 -0400 Date: Mon, 6 Jun 2016 11:54:50 +0200 From: Peter Zijlstra To: Yuyang Du Cc: mingo@kernel.org, linux-kernel@vger.kernel.org, bsegall@google.com, pjt@google.com, morten.rasmussen@arm.com, vincent.guittot@linaro.org, dietmar.eggemann@arm.com Subject: Re: [PATCH v4 2/5] sched/fair: Fix attaching task sched avgs twice when switching to fair or changing task group Message-ID: <20160606095450.GA30909@twins.programming.kicks-ass.net> References: <1465172441-27727-1-git-send-email-yuyang.du@intel.com> <1465172441-27727-3-git-send-email-yuyang.du@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1465172441-27727-3-git-send-email-yuyang.du@intel.com> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jun 06, 2016 at 08:20:38AM +0800, Yuyang Du 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 worse. The sched avgs can be sometimes attached twice > not only when we change task groups but also when we switch to fair class, > these two scenarios will descripbe in the following respectively. > > 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 sched avgs to the cfs_rq, if so, we shouldn't attach > it again in switched_to(), otherwise, we attach it twice. > > To address both the on_rq and !on_rq cases, as well as both the task > was switched from fair and otherwise, the simplest solution is to reset > the task's last_update_time to 0, when the task is switched from fair. > Then let task enqueue do the sched avgs attachment only once. > > 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 case, 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(), but not to > attach sched avgs in task_move_group(), and then let enqueue_task() do > the sched avgs attachment. So this patch completely removes the detach->attach aging you moved around in the previous patch -- leading me to wonder what the purpose of the previous patch was. Also, this Changelog completely fails to mention this fact, nor does it explain why this is 'right'. > +/* Virtually synchronize task with its cfs_rq */ I don't feel this comment actually enlightens the function much. > @@ -8372,9 +8363,6 @@ static void attach_task_cfs_rq(struct task_struct *p) > se->depth = se->parent ? se->parent->depth + 1 : 0; > #endif > > - /* Synchronize task with its cfs_rq */ > - attach_entity_load_avg(cfs_rq, se); > - > if (!vruntime_normalized(p)) > se->vruntime += cfs_rq->min_vruntime; > } You leave attach/detach asymmetric and not a comment in sight explaining why. > @@ -8382,16 +8370,18 @@ static void attach_task_cfs_rq(struct task_struct *p) > static void switched_from_fair(struct rq *rq, struct task_struct *p) > { > detach_task_cfs_rq(p); > + reset_task_last_update_time(p); > + /* > + * If we change back to fair class, we will attach the sched > + * avgs when we are enqueued, which will be done only once. We > + * won't have the chance to consistently age the avgs before > + * attaching them, so we have to continue with the last updated > + * sched avgs when we were detached. > + */ This comment needs improvement; it confuses. > @@ -8444,6 +8434,11 @@ static void task_move_group_fair(struct task_struct *p) > detach_task_cfs_rq(p); > set_task_rq(p, task_cpu(p)); > attach_task_cfs_rq(p); > + /* > + * This assures we will attach the sched avgs when we are enqueued, "ensures" ? Also, more confusion. > + * which will be done only once. > + */ > + reset_task_last_update_time(p); > }