linux-kernel - Re: [patch 02/15] sched: validate CFS quota hierarchies

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20110323103906.GC5507@siel.b>
Date:	Wed, 23 Mar 2011 11:39:06 +0100
From:	torbenh <torbenh@....de>
To:	Paul Turner <pjt@...gle.com>
Cc:	linux-kernel@...r.kernel.org,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Bharata B Rao <bharata@...ux.vnet.ibm.com>,
	Dhaval Giani <dhaval.giani@...il.com>,
	Balbir Singh <balbir@...ux.vnet.ibm.com>,
	Vaidyanathan Srinivasan <svaidy@...ux.vnet.ibm.com>,
	Srivatsa Vaddagiri <vatsa@...ibm.com>,
	Kamalesh Babulal <kamalesh@...ux.vnet.ibm.com>,
	Ingo Molnar <mingo@...e.hu>, Pavel Emelyanov <xemul@...nvz.org>
Subject: Re: [patch 02/15] sched: validate CFS quota hierarchies

On Tue, Mar 22, 2011 at 08:03:28PM -0700, Paul Turner wrote:
> Add constraints validation for CFS bandwidth hierachies.
> 
> It is checked that:
>    sum(child bandwidth) <= parent_bandwidth
> 
> In a quota limited hierarchy, an unconstrainted entity
> (e.g. bandwidth==RUNTIME_INF) inherits the bandwidth of its parent.
> 
> Since bandwidth periods may be non-uniform we normalize to the maximum allowed
> period, 5 seconds.
> 
> This behavior may be disabled (allowing child bandwidth to exceed parent) via
> kernel.sched_cfs_bandwidth_consistent=0
> 
> Signed-off-by: Paul Turner <pjt@...gle.com>
> 
> ---
>  include/linux/sched.h |    8 +++
>  kernel/sched.c        |  127 +++++++++++++++++++++++++++++++++++++++++++++++---
>  kernel/sched_fair.c   |    8 +++
>  kernel/sysctl.c       |   11 ++++
>  4 files changed, 147 insertions(+), 7 deletions(-)
> 
> Index: tip/kernel/sched.c
> ===================================================================
> --- tip.orig/kernel/sched.c
> +++ tip/kernel/sched.c
> +static int tg_cfs_schedulable_down(struct task_group *tg, void *data)
> +{
> +	struct cfs_schedulable_data *d = data;
> +	struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(tg);
> +	s64 quota = 0, parent_quota = -1;
> +
> +	quota = normalize_cfs_quota(tg, d);
> +	if (!tg->parent) {
> +		quota = RUNTIME_INF;
> +	} else {
> +		struct cfs_bandwidth *parent_b = tg_cfs_bandwidth(tg->parent);
> +
> +		parent_quota = parent_b->hierarchal_quota;
> +		if (parent_quota != RUNTIME_INF) {
> +			parent_quota -= quota;
> +			/* invalid hierarchy, child bandwidth exceeds parent */
> +			if (parent_quota < 0)
> +				return -EINVAL;
> +		}
> +
> +		/* if no inherent limit then inherit parent quota */
> +		if (quota == RUNTIME_INF)
> +			quota = parent_quota;
> +		parent_b->hierarchal_quota = parent_quota;
> +	}
> +	cfs_b->hierarchal_quota = quota;
> +
> +	return 0;
> +}

I find this logic pretty weird.
As long as quota == INF i can overcommit, but as soon as there is some
quota, i can not ?

Its clear, that one needs to be able to overcommit runtime, or the
default runtime for a new cgroup would need to be 0.
The root problem imo is that runtime and shares should not be in the
same cgroup subsystem. The semantics are too different.


> +
> +static int __cfs_schedulable(struct task_group *tg, u64 period, u64 quota)
> +{
> +	int ret;
> +	struct cfs_schedulable_data data = {
> +		.tg = tg,
> +		.period = period / NSEC_PER_USEC,
> +		.quota = quota / NSEC_PER_USEC,
> +	};
> +
> +	if (!sysctl_sched_cfs_bandwidth_consistent)
> +		return 0;
> +
> +	rcu_read_lock();
> +	ret = walk_tg_tree(tg_cfs_schedulable_down, tg_nop,
> +			    &data);
> +	rcu_read_unlock();

walk_tg_tree does the rcu_read_lock itself.
not necessary to do that here.
look at __rt_schedulable there is no rcu.


> +
> +	return ret;
> +}

-- 
torben Hohn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/