LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Dhaval Giani <dhaval@linux.vnet.ibm.com>
To: Peter Zijlstra <a.p.zijlstra@chello.nl>
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: Fri, 29 Feb 2008 11:27:30 +0530	[thread overview]
Message-ID: <20080229055730.GA5478@linux.vnet.ibm.com> (raw)
In-Reply-To: <1204227766.6243.41.camel@lappy>

On Thu, Feb 28, 2008 at 08:42:46PM +0100, Peter Zijlstra wrote:
> 
> 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
> 

thanks!

> > 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 ?
> 

Will do. (need to start remembering to doing this one!)

> > +		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.
> 

eeeks! Yeah, you are right. Funny though how I did not hit it while
testing.
 

-- 
regards,
Dhaval

      reply	other threads:[~2008-02-29  5:57 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
2008-02-29  5:57     ` Dhaval Giani [this message]

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=20080229055730.GA5478@linux.vnet.ibm.com \
    --to=dhaval@linux.vnet.ibm.com \
    --cc=a.p.zijlstra@chello.nl \
    --cc=aneesh.kumar@linux.vnet.ibm.com \
    --cc=balbir@in.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).