Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754010Ab1BOABG (ORCPT ); Mon, 14 Feb 2011 19:01:06 -0500 Received: from e33.co.us.ibm.com ([32.97.110.151]:46619 "EHLO e33.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753461Ab1BOABC (ORCPT ); Mon, 14 Feb 2011 19:01:02 -0500 Date: Mon, 14 Feb 2011 16:00:55 -0800 From: Matt Helsley To: "Kirill A. Shutemov" Cc: Matt Helsley , Paul Menage , Li Zefan , containers@lists.linux-foundation.org, jacob.jun.pan@linux.intel.com, Arjan van de Ven , linux-kernel@vger.kernel.org, Andrew Morton , linux-api@vger.kernel.org Subject: Re: [PATCH, v6 3/3] cgroups: introduce timer slack controller Message-ID: <20110215000055.GR16432@count0.beaverton.ibm.com> References: <1297688787-3592-1-git-send-email-kirill@shutemov.name> <1297688787-3592-4-git-send-email-kirill@shutemov.name> <20110214135926.GO16432@count0.beaverton.ibm.com> <20110214225940.GB6230@shutemov.name> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20110214225940.GB6230@shutemov.name> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2731 Lines: 74 On Tue, Feb 15, 2011 at 12:59:40AM +0200, Kirill A. Shutemov wrote: > On Mon, Feb 14, 2011 at 05:59:26AM -0800, Matt Helsley wrote: > > On Mon, Feb 14, 2011 at 03:06:27PM +0200, Kirill A. Shutsemov wrote: > > > From: Kirill A. Shutemov > > > + list_for_each_entry(cur, &cgroup->children, sibling) { > > > + child = cgroup_to_tslack_cgroup(cur); > > > + if (type == TIMER_SLACK_MIN && val > child->min_slack_ns) > > > + return -EBUSY; > > > + if (type == TIMER_SLACK_MAX && val < child->max_slack_ns) > > > + return -EBUSY; > > > + } > > > > This doesn't look right. Child cgroups should not constrain their > > parents. Instead you should allow the change and propagate the > > constraint to the children. > > See discussion with Thomas. > > > +static struct cftype files[] = { > > > + { > > > + .name = "set_slack_ns", > > > + .write_u64 = tslack_write_set_slack_ns, > > > + }, > > > + { > > > + .name = "min_slack_ns", > > > + .private = TIMER_SLACK_MIN, > > > + .read_u64 = tslack_read_range, > > > + .write_u64 = tslack_write_range, > > > + }, > > > + { > > > + .name = "max_slack_ns", > > > + .private = TIMER_SLACK_MAX, > > > + .read_u64 = tslack_read_range, > > > + .write_u64 = tslack_write_range, > > > + }, > > > > I didn't get a reply on how a max_slack_ns is useful. It seems > > prudent to add as little interface as possible and only when > > we clearly see the utility of it. > > For example, you can create two groups (excluding root cgroup): > > default - timer slack range 50000-50000 > relaxed - timer slack range 500000-unlimited. > > Now you can drag tasks between these group without need to reset value on > relaxed -> default transition. Perhaps you misunderstood my point. Yes, I can see that a maximum allows you to do counter-productive/pointless little tricks like "setting" the timer slack when you move the task. I just don't get the point of it. Why is setting a maximum timer slack useful? If anything it seems like it would be quite counterproductive or pointless *at best* because limiting the amount of timer slack would not improve the wakeup situation -- it could easily make it worse. Are there *any* negative consequences to allowing timer slacks as large as userspace requests -- perhaps even up to ULLONG_MAX? If there are none then why should we bother providing userspace a knob to set and enforce such a limit? Cheers, -Matt Helsley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/