From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753052AbbJMMH3 (ORCPT ); Tue, 13 Oct 2015 08:07:29 -0400 Received: from casper.infradead.org ([85.118.1.10]:51518 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752443AbbJMMH2 (ORCPT ); Tue, 13 Oct 2015 08:07:28 -0400 Date: Tue, 13 Oct 2015 14:07:21 +0200 From: Peter Zijlstra To: Byungchul Park Cc: mingo@kernel.org, linux-kernel@vger.kernel.org, yuyang.du@intel.com, pjt@google.com, efault@gmx.de, tglx@linutronix.de Subject: Re: [PATCH 1/2] sched: make fair sched class can handle the cgroup change by other class Message-ID: <20151013120721.GP11639@twins.programming.kicks-ass.net> References: <1444036584-14079-1-git-send-email-byungchul.park@lge.com> <1444036584-14079-2-git-send-email-byungchul.park@lge.com> <20151013090654.GR3604@twins.programming.kicks-ass.net> <20151013112645.GV6455@byungchulpark-X58A-UD3R> <20151013120438.GQ3816@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151013120438.GQ3816@twins.programming.kicks-ass.net> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Oct 13, 2015 at 02:04:38PM +0200, Peter Zijlstra wrote: > On Tue, Oct 13, 2015 at 08:26:45PM +0900, Byungchul Park wrote: > > On Tue, Oct 13, 2015 at 11:06:54AM +0200, Peter Zijlstra wrote: > > > On Mon, Oct 05, 2015 at 06:16:23PM +0900, byungchul.park@lge.com wrote: > > > > From: Byungchul Park > > > > > > > > Original fair sched class can handle the cgroup change occured within its > > > > class with task_move_group_fair(), but there is no way to know it if the > > > > change happened outside. This patch makes the fair sched class can handle > > > > the change of cgroup which happened even at other sched class. > > > > > > > > Additionally, it makes sched_move_task() more flexable so that any other > > > > sched class can add task_move_group_xx() callback easily in future when > > > > it is needed. > > > > > > I don't get the problem... when !fair, set_task_rq() will do what needs > > > doing. > > > > set_task_rq() changes se's cfs_rq properly. > > > > > > > > The only reason we need task_move_group_fair() is the extra accounting > > > required when we actually _are_ of the fair class, it needs to > > > unaccount, move and reaccount. > > > > i agree with you mostly. but let's consider following sequence. > > > > 1. switch se's class from fair to rt > > 2. change se's group within the rt class > > 3. switch se's class back to fair > > > > now, se->avg.last_update_time has a wrong value which is not synced with > > the current cfs_rq yet before calling attach_entity_load_avg(). so > > ATTACH_AGE_LOAD won't work expectedly. to be honest with you, no problem > > if we disable ATTACH_AGE_LOAD. but i think ATTACH_AGE_LOAD is a valuable > > feature, so i hope this patch will be added so that the ATTACH_AGE_LOAD > > feature works properly. > > Ah, see details like that make or break a Changelog, since you've > clearly thought about it, you might as well write it down and safe me > the trouble of trying to puzzle it out on me own ;-) > > OK, now that I understand the problem, let me consider it a bit. So the problem I have with your approach is that I would prefer to isolate the classes as much as possible. If its not currently of a class, we should not call into it. Now, there's a few exceptions to that already, but I would really prefer not to make it worse.