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

[Nick Piggin]

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@xxxxxxxxx

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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/