[PATCH] (latest tip) make dequeue_task less confusing

[PATCH] (latest tip) make dequeue_task less confusing

[David Newall]

[-- Attachment #1: Type: text/plain, Size: 319 bytes --]

The dequeue_patch function in kernel/sched.c is complicated by 
including a sleep parameter.  This parameter is always zero 
except in one instance.  This patch clarifies the task of 
dequeue_patch by removing the sleep parameter and moving the 
code that handles non-zero sleep to that one place where it is 
needed.


[-- Attachment #2: sched.patch --]
[-- Type: text/x-patch, Size: 1782 bytes --]

--- kernel/sched.c	2009-03-12 18:41:41.000000000 +1030
+++ kernel/sched.c.dn	2009-03-12 18:45:18.000000000 +1030
@@ -1791,21 +1791,10 @@
 	p->se.on_rq = 1;
 }
 
-static void dequeue_task(struct rq *rq, struct task_struct *p, int sleep)
+static void dequeue_task(struct rq *rq, struct task_struct *p)
 {
-	if (sleep) {
-		if (p->se.last_wakeup) {
-			update_avg(&p->se.avg_overlap,
-				p->se.sum_exec_runtime - p->se.last_wakeup);
-			p->se.last_wakeup = 0;
-		} else {
-			update_avg(&p->se.avg_wakeup,
-				sysctl_sched_wakeup_granularity);
-		}
-	}
-
 	sched_info_dequeued(p);
-	p->sched_class->dequeue_task(rq, p, sleep);
+	p->sched_class->dequeue_task(rq, p, 0);
 	p->se.on_rq = 0;
 }
 
@@ -1875,7 +1864,22 @@
 	if (task_contributes_to_load(p))
 		rq->nr_uninterruptible++;
 
-	dequeue_task(rq, p, sleep);
+	if (sleep) {
+		if (p->se.last_wakeup) {
+			update_avg(&p->se.avg_overlap,
+				p->se.sum_exec_runtime - p->se.last_wakeup);
+			p->se.last_wakeup = 0;
+		} else {
+			update_avg(&p->se.avg_wakeup,
+				sysctl_sched_wakeup_granularity);
+		}
+	}
+
+	/*dequeue_task(rq, p, sleep);*/
+	sched_info_dequeued(p);
+	p->sched_class->dequeue_task(rq, p, sleep);
+	p->se.on_rq = 0;
+
 	dec_nr_running(rq);
 }
 
@@ -5323,7 +5327,7 @@
 	on_rq = p->se.on_rq;
 	running = task_current(rq, p);
 	if (on_rq)
-		dequeue_task(rq, p, 0);
+		dequeue_task(rq, p);
 	if (running)
 		p->sched_class->put_prev_task(rq, p);
 
@@ -5372,7 +5376,7 @@
 	}
 	on_rq = p->se.on_rq;
 	if (on_rq)
-		dequeue_task(rq, p, 0);
+		dequeue_task(rq, p);
 
 	p->static_prio = NICE_TO_PRIO(nice);
 	set_load_weight(p);
@@ -9189,7 +9193,7 @@
 	on_rq = tsk->se.on_rq;
 
 	if (on_rq)
-		dequeue_task(rq, tsk, 0);
+		dequeue_task(rq, tsk);
 	if (unlikely(running))
 		tsk->sched_class->put_prev_task(rq, tsk);
 

Re: [PATCH] (latest tip) make dequeue_task less confusing

[Ingo Molnar]

* David Newall <davidn@davidnewall.com> wrote:

> The dequeue_patch function in kernel/sched.c is complicated by 
> including a sleep parameter.  This parameter is always zero 
> except in one instance.  This patch clarifies the task of 
> dequeue_patch by removing the sleep parameter and moving the 
> code that handles non-zero sleep to that one place where it is 
> needed.
> 

> --- kernel/sched.c	2009-03-12 18:41:41.000000000 +1030
> +++ kernel/sched.c.dn	2009-03-12 18:45:18.000000000 +1030
> @@ -1791,21 +1791,10 @@
>  	p->se.on_rq = 1;
>  }
>  
> -static void dequeue_task(struct rq *rq, struct task_struct *p, int sleep)
> +static void dequeue_task(struct rq *rq, struct task_struct *p)
>  {
> -	if (sleep) {
> -		if (p->se.last_wakeup) {
> -			update_avg(&p->se.avg_overlap,
> -				p->se.sum_exec_runtime - p->se.last_wakeup);
> -			p->se.last_wakeup = 0;
> -		} else {
> -			update_avg(&p->se.avg_wakeup,
> -				sysctl_sched_wakeup_granularity);
> -		}
> -	}
> -
>  	sched_info_dequeued(p);
> -	p->sched_class->dequeue_task(rq, p, sleep);
> +	p->sched_class->dequeue_task(rq, p, 0);
>  	p->se.on_rq = 0;
>  }
>  
> @@ -1875,7 +1864,22 @@
>  	if (task_contributes_to_load(p))
>  		rq->nr_uninterruptible++;
>  
> -	dequeue_task(rq, p, sleep);
> +	if (sleep) {
> +		if (p->se.last_wakeup) {
> +			update_avg(&p->se.avg_overlap,
> +				p->se.sum_exec_runtime - p->se.last_wakeup);
> +			p->se.last_wakeup = 0;
> +		} else {
> +			update_avg(&p->se.avg_wakeup,
> +				sysctl_sched_wakeup_granularity);
> +		}
> +	}
> +
> +	/*dequeue_task(rq, p, sleep);*/
> +	sched_info_dequeued(p);
> +	p->sched_class->dequeue_task(rq, p, sleep);
> +	p->se.on_rq = 0;
> +
>  	dec_nr_running(rq);
>  }
>  
> @@ -5323,7 +5327,7 @@
>  	on_rq = p->se.on_rq;
>  	running = task_current(rq, p);
>  	if (on_rq)
> -		dequeue_task(rq, p, 0);
> +		dequeue_task(rq, p);
>  	if (running)
>  		p->sched_class->put_prev_task(rq, p);
>  
> @@ -5372,7 +5376,7 @@
>  	}
>  	on_rq = p->se.on_rq;
>  	if (on_rq)
> -		dequeue_task(rq, p, 0);
> +		dequeue_task(rq, p);
>  
>  	p->static_prio = NICE_TO_PRIO(nice);
>  	set_load_weight(p);
> @@ -9189,7 +9193,7 @@
>  	on_rq = tsk->se.on_rq;
>  
>  	if (on_rq)
> -		dequeue_task(rq, tsk, 0);
> +		dequeue_task(rq, tsk);
>  	if (unlikely(running))
>  		tsk->sched_class->put_prev_task(rq, tsk);

It would be cleaner to achieve this by introducing a 
__dequeue_task() inline function that does not have the sleep 
parameter and is a thin wrapper over 
p->sched_class->dequeue_task, and keep dequeue_task() with the 
sleep parameter - but update it to use __dequeue_task() and mark 
it an inline function.

[ Btw., could you please submit future patches in the regular
  -p1 format? See Documentation/SubmittingPatches. And please
  Cc: me to future scheduler patches. Thanks! ]

	Ingo

Re: [PATCH] (latest tip) make dequeue_task less confusing

[Mike Galbraith]

On Thu, 2009-03-12 at 10:55 +0100, Ingo Molnar wrote:
> * David Newall <davidn@davidnewall.com> wrote:
> 
> > The dequeue_patch function in kernel/sched.c is complicated by
        ^^^^^^^^^^^^^
>  
> > including a sleep parameter.  This parameter is always zero 
> > except in one instance.  This patch clarifies the task of 
> > dequeue_patch by removing the sleep parameter and moving the 
> > code that handles non-zero sleep to that one place where it is 
> > needed.
> > 
> 
> > --- kernel/sched.c	2009-03-12 18:41:41.000000000 +1030
> > +++ kernel/sched.c.dn	2009-03-12 18:45:18.000000000 +1030
> > @@ -1791,21 +1791,10 @@
> >  	p->se.on_rq = 1;
> >  }
> >  
> > -static void dequeue_task(struct rq *rq, struct task_struct *p, int sleep)
> > +static void dequeue_task(struct rq *rq, struct task_struct *p)
> >  {
> > -	if (sleep) {
> > -		if (p->se.last_wakeup) {
> > -			update_avg(&p->se.avg_overlap,
> > -				p->se.sum_exec_runtime - p->se.last_wakeup);
> > -			p->se.last_wakeup = 0;
> > -		} else {
> > -			update_avg(&p->se.avg_wakeup,
> > -				sysctl_sched_wakeup_granularity);
> > -		}
> > -	}
> > -
> >  	sched_info_dequeued(p);
> > -	p->sched_class->dequeue_task(rq, p, sleep);
> > +	p->sched_class->dequeue_task(rq, p, 0);
> >  	p->se.on_rq = 0;
> >  }
> >  
> > @@ -1875,7 +1864,22 @@
> >  	if (task_contributes_to_load(p))
> >  		rq->nr_uninterruptible++;
> >  
> > -	dequeue_task(rq, p, sleep);
> > +	if (sleep) {
> > +		if (p->se.last_wakeup) {
> > +			update_avg(&p->se.avg_overlap,
> > +				p->se.sum_exec_runtime - p->se.last_wakeup);
> > +			p->se.last_wakeup = 0;
> > +		} else {
> > +			update_avg(&p->se.avg_wakeup,
> > +				sysctl_sched_wakeup_granularity);
> > +		}
> > +	}
> > +
> > +	/*dequeue_task(rq, p, sleep);*/
> > +	sched_info_dequeued(p);
> > +	p->sched_class->dequeue_task(rq, p, sleep);
> > +	p->se.on_rq = 0;
> > +
> >  	dec_nr_running(rq);
> >  }
> >  
> > @@ -5323,7 +5327,7 @@
> >  	on_rq = p->se.on_rq;
> >  	running = task_current(rq, p);
> >  	if (on_rq)
> > -		dequeue_task(rq, p, 0);
> > +		dequeue_task(rq, p);
> >  	if (running)
> >  		p->sched_class->put_prev_task(rq, p);
> >  
> > @@ -5372,7 +5376,7 @@
> >  	}
> >  	on_rq = p->se.on_rq;
> >  	if (on_rq)
> > -		dequeue_task(rq, p, 0);
> > +		dequeue_task(rq, p);
> >  
> >  	p->static_prio = NICE_TO_PRIO(nice);
> >  	set_load_weight(p);
> > @@ -9189,7 +9193,7 @@
> >  	on_rq = tsk->se.on_rq;
> >  
> >  	if (on_rq)
> > -		dequeue_task(rq, tsk, 0);
> > +		dequeue_task(rq, tsk);
> >  	if (unlikely(running))
> >  		tsk->sched_class->put_prev_task(rq, tsk);
> 
> It would be cleaner to achieve this by introducing a 
> __dequeue_task() inline function that does not have the sleep 
> parameter and is a thin wrapper over 
> p->sched_class->dequeue_task, and keep dequeue_task() with the 
> sleep parameter - but update it to use __dequeue_task() and mark 
> it an inline function.

Is there any real gain from doing this?  It messes up the symmetry of
enqueue/dequeue.

	-Mike

Re: [PATCH] (latest tip) make dequeue_task less confusing

[David Newall]

Mike Galbraith wrote:
> Is there any real gain from doing this?  It messes up the symmetry of
> enqueue/dequeue.


No.  On further consideration, I don't like my patch.

Re: [PATCH] (latest tip) make dequeue_task less confusing

[Ingo Molnar]

* David Newall <davidn@davidnewall.com> wrote:

> Mike Galbraith wrote:
> > Is there any real gain from doing this?  It messes up the symmetry of
> > enqueue/dequeue.
> 
> 
> No.  On further consideration, I don't like my patch.

With the __dequeue_task()+inline suggestion i made i think it 
would be a micro-optimization and would not break symmetry in a 
significant way.

	Ingo

Re: [PATCH] (latest tip) make dequeue_task less confusing

[David Newall]

Ingo Molnar wrote:
> * David Newall <davidn@davidnewall.com> wrote:
>   
>> No.  On further consideration, I don't like my patch.
>>     
>
> With the __dequeue_task()+inline suggestion i made i think it 
> would be a micro-optimization and would not break symmetry in a 
> significant way.

What I realised, upon closer examination, is that enqueue_task has the
same issue (with wakeup); and that the one places where sleep and wakeup
are set are in a single call for each, to (de|en)activate_task. If it
made sense to do what I suggested, then it should be done all the way,
that is, move the sleep and wakeup code to those two places, and
replicate the (admittedly smaller) *queue_task and *activate_task at
those points. This smacks of premature optimisation.