LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Peter Zijlstra <a.p.zijlstra@chello.nl>
To: Dhaval Giani <dhaval@linux.vnet.ibm.com>
Cc: Srivatsa Vaddagiri <vatsa@linux.vnet.ibm.com>,
	Ingo Molnar <mingo@elte.hu>,
	Sudhir Kumar <skumar@linux.vnet.ibm.com>,
	Balbir Singh <balbir@in.ibm.com>,
	Aneesh Kumar KV <aneesh.kumar@linux.vnet.ibm.com>,
	lkml <linux-kernel@vger.kernel.org>,
	vgoyal@redhat.com, serue@us.ibm.com, menage@google.com
Subject: Re: [RFC, PATCH 1/2] sched: allow the CFS group scheduler to have multiple levels
Date: Thu, 28 Feb 2008 20:42:46 +0100	[thread overview]
Message-ID: <1204227766.6243.41.camel@lappy> (raw)
In-Reply-To: <20080225141703.GB29213@linux.vnet.ibm.com>


On Mon, 2008-02-25 at 19:47 +0530, Dhaval Giani wrote:
> This patch makes the group scheduler multi hierarchy aware.

Ok, good thing to do in principle

> Signed-off-by: Dhaval Giani <dhaval@linux.vnet.ibm.com>
> 
> ---
>  include/linux/sched.h |    2 +-
>  kernel/sched.c        |   41 ++++++++++++++++++++++++-----------------
>  2 files changed, 25 insertions(+), 18 deletions(-)
> 
> Index: linux-2.6.25-rc2/include/linux/sched.h
> ===================================================================
> --- linux-2.6.25-rc2.orig/include/linux/sched.h
> +++ linux-2.6.25-rc2/include/linux/sched.h
> @@ -2031,7 +2031,7 @@ extern void normalize_rt_tasks(void);
>  
>  extern struct task_group init_task_group;
>  
> -extern struct task_group *sched_create_group(void);
> +extern struct task_group *sched_create_group(struct task_group *parent);
>  extern void sched_destroy_group(struct task_group *tg);
>  extern void sched_move_task(struct task_struct *tsk);
>  #ifdef CONFIG_FAIR_GROUP_SCHED
> Index: linux-2.6.25-rc2/kernel/sched.c
> ===================================================================
> --- linux-2.6.25-rc2.orig/kernel/sched.c
> +++ linux-2.6.25-rc2/kernel/sched.c
> @@ -7155,10 +7155,11 @@ static void init_rt_rq(struct rt_rq *rt_
>  }
>  
>  #ifdef CONFIG_FAIR_GROUP_SCHED
> -static void init_tg_cfs_entry(struct rq *rq, struct task_group *tg,
> -		struct cfs_rq *cfs_rq, struct sched_entity *se,
> -		int cpu, int add)
> +static void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq,
> +				struct sched_entity *se, int cpu, int add,
> +				struct sched_entity *parent)
>  {
> +	struct rq *rq = cpu_rq(cpu);
>  	tg->cfs_rq[cpu] = cfs_rq;
>  	init_cfs_rq(cfs_rq, rq);
>  	cfs_rq->tg = tg;
> @@ -7170,7 +7171,11 @@ static void init_tg_cfs_entry(struct rq 
>  	if (!se)
>  		return;
>  
> -	se->cfs_rq = &rq->cfs;
> +	if (parent == NULL)

!parent ?

> +		se->cfs_rq = &rq->cfs;
> +	else
> +		se->cfs_rq = parent->my_q;
> +
>  	se->my_q = cfs_rq;
>  	se->load.weight = tg->shares;
>  	se->load.inv_weight = div64_64(1ULL<<32, se->load.weight);
> @@ -7244,7 +7249,8 @@ void __init sched_init(void)
>  		 * We achieve this by letting init_task_group's tasks sit
>  		 * directly in rq->cfs (i.e init_task_group->se[] = NULL).
>  		 */
> -		init_tg_cfs_entry(rq, &init_task_group, &rq->cfs, NULL, i, 1);
> +		init_tg_cfs_entry(&init_task_group, &rq->cfs,
> +							NULL, i, 1, NULL);
>  		init_tg_rt_entry(rq, &init_task_group, &rq->rt, NULL, i, 1);
>  #elif defined CONFIG_USER_SCHED
>  		/*
> @@ -7260,7 +7266,7 @@ void __init sched_init(void)
>  		 */
>  		init_tg_cfs_entry(rq, &init_task_group,
>  				&per_cpu(init_cfs_rq, i),
> -				&per_cpu(init_sched_entity, i), i, 1);
> +				&per_cpu(init_sched_entity, i), i, 1, NULL);
>  
>  #endif
>  #endif /* CONFIG_FAIR_GROUP_SCHED */
> @@ -7630,7 +7636,8 @@ static void free_fair_sched_group(struct
>  	kfree(tg->se);
>  }
>  
> -static int alloc_fair_sched_group(struct task_group *tg)
> +static int alloc_fair_sched_group(struct task_group *tg,
> +					struct task_group *parent)
>  {
>  	struct cfs_rq *cfs_rq;
>  	struct sched_entity *se;
> @@ -7658,8 +7665,11 @@ static int alloc_fair_sched_group(struct
>  				GFP_KERNEL|__GFP_ZERO, cpu_to_node(i));
>  		if (!se)
>  			goto err;
> -
> -		init_tg_cfs_entry(rq, tg, cfs_rq, se, i, 0);
> +		if (!parent) {
> +			init_tg_cfs_entry(tg, cfs_rq, se, i, 0,	parent->se[i]);
> +		} else {
> +			init_tg_cfs_entry(tg, cfs_rq, se, i, 0, NULL);
> +		}

Looks like you got the cases switched, this looks like an instant NULL
deref.

>  	}
>  
>  	return 1;
> @@ -7788,7 +7798,7 @@ static void free_sched_group(struct task
>  }
>  
>  /* allocate runqueue etc for a new task group */
> -struct task_group *sched_create_group(void)
> +struct task_group *sched_create_group(struct task_group *parent)
>  {
>  	struct task_group *tg;
>  	unsigned long flags;
> @@ -7798,7 +7808,7 @@ struct task_group *sched_create_group(vo
>  	if (!tg)
>  		return ERR_PTR(-ENOMEM);
>  
> -	if (!alloc_fair_sched_group(tg))
> +	if (!alloc_fair_sched_group(tg, parent))
>  		goto err;
>  
>  	if (!alloc_rt_sched_group(tg))
> @@ -8049,7 +8059,7 @@ static inline struct task_group *cgroup_
>  static struct cgroup_subsys_state *
>  cpu_cgroup_create(struct cgroup_subsys *ss, struct cgroup *cgrp)
>  {
> -	struct task_group *tg;
> +	struct task_group *tg, *parent;
>  
>  	if (!cgrp->parent) {
>  		/* This is early initialization for the top cgroup */
> @@ -8057,11 +8067,8 @@ cpu_cgroup_create(struct cgroup_subsys *
>  		return &init_task_group.css;
>  	}
>  
> -	/* we support only 1-level deep hierarchical scheduler atm */
> -	if (cgrp->parent->parent)
> -		return ERR_PTR(-EINVAL);
> -
> -	tg = sched_create_group();
> +	parent = cgroup_tg(cgrp->parent);
> +	tg = sched_create_group(parent);
>  	if (IS_ERR(tg))
>  		return ERR_PTR(-ENOMEM);
>  


  parent reply	other threads:[~2008-02-28 19:53 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-25 14:15 [RFC, PATCH 0/2] sched: add multiple hierarchy support to the CFS group scheduler Dhaval Giani
2008-02-25 14:16 ` [RFC, PATCH 1/2] sched: change the fairness model of " Dhaval Giani
2008-02-27 20:23   ` Serge E. Hallyn
2008-02-28 19:42   ` Peter Zijlstra
2008-02-29  9:04     ` Dhaval Giani
2008-02-29 10:37       ` Peter Zijlstra
2008-03-04  9:49         ` Dhaval Giani
2008-02-25 14:17 ` [RFC, PATCH 1/2] sched: allow the CFS group scheduler to have multiple levels Dhaval Giani
2008-02-25 14:17   ` Dhaval Giani
2008-02-28 19:42   ` Peter Zijlstra [this message]
2008-02-29  5:57     ` Dhaval Giani

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1204227766.6243.41.camel@lappy \
    --to=a.p.zijlstra@chello.nl \
    --cc=aneesh.kumar@linux.vnet.ibm.com \
    --cc=balbir@in.ibm.com \
    --cc=dhaval@linux.vnet.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=menage@google.com \
    --cc=mingo@elte.hu \
    --cc=serue@us.ibm.com \
    --cc=skumar@linux.vnet.ibm.com \
    --cc=vatsa@linux.vnet.ibm.com \
    --cc=vgoyal@redhat.com \
    --subject='Re: [RFC, PATCH 1/2] sched: allow the CFS group scheduler to have multiple levels' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).