Re: [PATCH] kernel <linux-2.6.11.10> kernel/sched.c

[Chen Shang <shangcs@gmail.com>]

/*===== ISSUE ====*/
My second version of patch has a defect.

+  if (unlikely(old_prio != next->prio))             {
+      dequeue_task(next, array);  --> ### dequeue should against
old_prio, NOT next->prio ###
+      enqueue_task(next, array);
+  }

unforunately, dequeue_task does not accept the third parameter to make
adjustment. Personally, I feel it's good to add extra function as my
first version of patch to combine dequeue and enqueue together.
Reasons as following:
1) adding the third parameter to dequeue_task() would cause other
places' code change;
2) for schedule functions, performance is the first consideration.
Notice both dequeue_task() and enqueue_task() are NOT inline.
Combining those two in one saves one function call overhead;


/* ===== NEW PATCH ===== */
The new patch, see below, adds new function change_queue_task() to
dequeue from "old_prio queue" and enqueue the "next task" to
"next->prio queue".

The patch also inlines requeue_task().

The patch has been tested with 2.6.11.10, looks good. -For somehow,
2.6.12-rc4 is still not stable on my machine (Fedora 3).


/* ===== [PATCH 2.6.11.?] kernel/sched.c =====*/
--- linux-2.6.12-rc4.orig/kernel/sched.c	2005-05-06 22:20:31.000000000 -0700
+++ linux12/kernel/sched.c	2005-05-21 16:19:11.000000000 -0700
@@ -556,11 +556,23 @@
 	p->array = array;
 }
 
+static void change_queue_task(struct task_struct *p, prio_array_t
*array, int old_prio)
+{
+	list_del(&p->run_list);
+	if (list_empty(array->queue + old_prio))
+		__clear_bit(old_prio, array->bitmap);
+
+	sched_info_queued(p);
+	list_add_tail(&p->run_list, array->queue + p->prio);
+	__set_bit(p->prio, array->bitmap);
+	p->array = array;
+}
+
 /*
  * Put task to the end of the run list without the overhead of dequeue
  * followed by enqueue.
  */
-static void requeue_task(struct task_struct *p, prio_array_t *array)
+static inline void requeue_task(struct task_struct *p, prio_array_t *array)
 {
 	list_move_tail(&p->run_list, array->queue + p->prio);
 }
@@ -2613,7 +2625,7 @@
 	struct list_head *queue;
 	unsigned long long now;
 	unsigned long run_time;
-	int cpu, idx;
+	int cpu, idx, old_prio;
 
 	/*
 	 * Test if we are atomic.  Since do_exit() needs to call into
@@ -2735,9 +2747,14 @@
 			delta = delta * (ON_RUNQUEUE_WEIGHT * 128 / 100) / 128;
 
 		array = next->array;
-		dequeue_task(next, array);
+		old_prio = next->prio;
+
 		recalc_task_prio(next, next->timestamp + delta);
-		enqueue_task(next, array);
+		
+		if (unlikely(old_prio != next->prio)) 
+			change_queue_task(next, array, old_prio);
+		else
+			requeue_task(next, array);
 	}
 	next->activated = 0;
 switch_tasks:

/* ===== PATCH END ===== */

Thanks,
-chen

On 5/20/05, Ingo Molnar <mingo@elte.hu> wrote:
> 
> * Con Kolivas <kernel@kolivas.org> wrote:
> 
> > On Fri, 20 May 2005 19:49, Ingo Molnar wrote:
> > > * chen Shang <shangcs@gmail.com> wrote:
> > > > I minimized my patch and against to 2.6.12-rc4 this time, see below.
> > >
> > > looks good - i've done some small style/whitespace cleanups and renamed
> > > prio to old_prio, patch against -rc4 below.
> >
> > We should inline requeue_task as well.
> 
> yeah.
> 
> Acked-by: Ingo Molnar <mingo@elte.hu>
> 
>         Ingo
>