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

[Ingo Molnar <mingo@elte.hu>]

* 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