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

[Nick Piggin <nickpiggin@yahoo.com.au>]

chen Shang wrote:

>Given the frequency of schedule() calling, there is a margin to
>improve it. In step of recalculate task priority, it first dequeues
>from one priority queue, calls recalc_task_prio(), then enqueue the
>task into another priority queue. However, statistics shows only
>around 0.5% of recalculation changed task priority (see below). While
>rest of 99.5% of recalculation do not change priority, it is
>reasonably to use requeue_task() to avoid overhead of dequeue and
>enqueue on the same priority queue.
>
>The patch is implemented with above idea. Note, a new help function,
>change_queue_task(), to combine dequeue() and enqueue() to reduce one
>function call overhead. Two statistics fields, sched_prio_changed and
>sched_prio_unchanged, are added to provide statistic data on priority
>recalculation.
>
>Thanks,
>
>chen
>shangcs@gmail.com
>

Hi Chen,
With the added branch and the extra icache footprint, it isn't clear
that this would be a win.

Also, you didn't say where your statistics came from (what workload).

So you really need to start by demonstrating some increase on some workload.

Also, minor comments on the patch: please work against mm kernels, 
please follow
kernel coding style, and don't change schedstat output format in the 
same patch
(makes it easier for those with schedstat parsing tools).

> 
>+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.
>@@ -2668,7 +2690,7 @@
> 	struct list_head *queue;
> 	unsigned long long now;
> 	unsigned long run_time;
>-	int cpu, idx;
>+	int cpu, idx, prio;
> 
> 	/*
> 	 * Test if we are atomic.  Since do_exit() needs to call into
>@@ -2787,9 +2809,19 @@
> 			delta = delta * (ON_RUNQUEUE_WEIGHT * 128 / 100) / 128;
> 
> 		array = next->array;
>-		dequeue_task(next, array);
>+		prio = next->prio;
> 		recalc_task_prio(next, next->timestamp + delta);
>-		enqueue_task(next, array);
>+		
>+		if (unlikely(prio != next->prio))
>+		{
>+			change_queue_task(next, array, prio);
>+			schedstat_inc(rq, sched_prio_changed);
>+		}
>+		else
>+		{
>+			requeue_task(next, array);
>+			schedstat_inc(rq, sched_prio_unchanged);
>+		}
> 	}
> 	next->activated = 0;
> switch_tasks:
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/
>
>