Re: CFS and suspend2: hang in atomic copy (was: [Announce] [patch]Modular Scheduler Core and Completely Fair Scheduler [CFS])

[Esben Nielsen]

On Wed, 18 Apr 2007, Ingo Molnar wrote:

> * Christian Hesse <mail@xxxxxxxxxxxx> wrote:
>
> > Hi Ingo and all,
> >
> > On Friday 13 April 2007, Ingo Molnar wrote:
> > > as usual, any sort of feedback, bugreports, fixes and suggestions are
> > > more than welcome,
> >
> > I just gave CFS a try on my system. From a user's point of view it
> > looks good so far. Thanks for your work.
>
> you are welcome!
>
> > However I found a problem: When trying to suspend a system patched
> > with suspend2 2.2.9.11 it hangs with "doing atomic copy". Pressing the
> > ESC key results in a message that it tries to abort suspend, but then
> > still hangs.
>
> i took a quick look at suspend2 and it makes some use of yield().
> There's a bug in CFS's yield code, i've attached a patch that should fix
> it, does it make any difference to the hang?
>
> 	Ingo
>
> Index: linux/kernel/sched_fair.c
> ===================================================================
> --- linux.orig/kernel/sched_fair.c
> +++ linux/kernel/sched_fair.c
> @@ -264,15 +264,26 @@ static void dequeue_task_fair(struct rq
>
> /*
>  * sched_yield() support is very simple via the rbtree, we just
> - * dequeue and enqueue the task, which causes the task to
> - * roundrobin to the end of the tree:
> + * dequeue the task and move it to the rightmost position, which
> + * causes the task to roundrobin to the end of the tree.
>  */
> static void requeue_task_fair(struct rq *rq, struct task_struct *p)
> {
> 	dequeue_task_fair(rq, p);
> 	p->on_rq = 0;
> -	enqueue_task_fair(rq, p);
> +	/*
> +	 * Temporarily insert at the last position of the tree:
> +	 */
> +	p->fair_key = LLONG_MAX;
> +	__enqueue_task_fair(rq, p);
> 	p->on_rq = 1;
> +
> +	/*
> +	 * Update the key to the real value, so that when all other
> +	 * tasks from before the rightmost position have executed,
> +	 * this task is picked up again:
> +	 */
> +	p->fair_key = rq->fair_clock - p->wait_runtime + p->nice_offset;

I don't think it safe to change the key after inserting the element in the 
tree. You end up with an unsorted tree giving where new entries end up in 
wrong places "randomly".
I think a better approach would be to keep track of the rightmost entry, 
set the key to the rightmost's key +1 and then simply insert it there.

Esben

>
-
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/