Re: CFS and suspend2: hang in atomic copy (was: [Announce] [patch] Modular Scheduler Core and Completely Fair Scheduler [CFS])
* Ingo Molnar <[EMAIL PROTECTED]> wrote:
> > 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.
>
> yeah. I had that implemented at a stage but was trying to be too
> clever for my own good ;-)
i have fixed it via the patch below. (I'm using rb_last() because that
way the normal scheduling codepaths are not burdened with the
maintainance of a rightmost entry.)
Ingo
---
kernel/sched.c |3 ++-
kernel/sched_fair.c | 24 +---
2 files changed, 15 insertions(+), 12 deletions(-)
Index: linux/kernel/sched.c
===
--- linux.orig/kernel/sched.c
+++ linux/kernel/sched.c
@@ -3806,7 +3806,8 @@ asmlinkage long sys_sched_yield(void)
schedstat_inc(rq, yld_cnt);
if (rq->nr_running == 1)
schedstat_inc(rq, yld_act_empty);
- current->sched_class->yield_task(rq, current);
+ else
+ current->sched_class->yield_task(rq, current);
/*
* Since we are going to call schedule() anyway, there's
Index: linux/kernel/sched_fair.c
===
--- linux.orig/kernel/sched_fair.c
+++ linux/kernel/sched_fair.c
@@ -275,21 +275,23 @@ static void dequeue_task_fair(struct rq
*/
static void yield_task_fair(struct rq *rq, struct task_struct *p)
{
+ struct rb_node *entry;
+ struct task_struct *last;
+
dequeue_task_fair(rq, p);
p->on_rq = 0;
+
/*
-* Temporarily insert at the last position of the tree:
+* Temporarily insert at the last position of the tree.
+* The key will be updated back to (near) its old value
+* when the task gets scheduled.
*/
- p->fair_key = LLONG_MAX;
+ entry = rb_last(>tasks_timeline);
+ last = rb_entry(entry, struct task_struct, run_node);
+
+ p->fair_key = last->fair_key + 1;
__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;
}
/*
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: CFS and suspend2: hang in atomic copy (was: [Announce] [patch] Modular Scheduler Core and Completely Fair Scheduler [CFS])
* Esben Nielsen <[EMAIL PROTECTED]> wrote: > >+/* > >+ * 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". yeah, indeed. I hoped that once this rightmost entry is removed (as soon as it gets scheduled next time) the tree goes back to a correct shape, but that's not the case - the left sub-tree and the right sub-tree is merged by the rbtree code with the assumption that the entry had a correct key. > 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. yeah. I had that implemented at a stage but was trying to be too clever for my own good ;-) Ingo - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: CFS and suspend2: hang in atomic copy (was: [Announce] [patch] Modular Scheduler Core and Completely Fair Scheduler [CFS])
On Wed, 18 Apr 2007, Ingo Molnar wrote:
* Christian Hesse <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: CFS and suspend2: hang in atomic copy (was: [Announce] [patch] Modular Scheduler Core and Completely Fair Scheduler [CFS])
On Wed, 18 Apr 2007, Ingo Molnar wrote:
* Christian Hesse [EMAIL PROTECTED] 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: CFS and suspend2: hang in atomic copy (was: [Announce] [patch] Modular Scheduler Core and Completely Fair Scheduler [CFS])
* Esben Nielsen [EMAIL PROTECTED] wrote: +/* + * 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. yeah, indeed. I hoped that once this rightmost entry is removed (as soon as it gets scheduled next time) the tree goes back to a correct shape, but that's not the case - the left sub-tree and the right sub-tree is merged by the rbtree code with the assumption that the entry had a correct key. 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. yeah. I had that implemented at a stage but was trying to be too clever for my own good ;-) Ingo - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: CFS and suspend2: hang in atomic copy (was: [Announce] [patch] Modular Scheduler Core and Completely Fair Scheduler [CFS])
* Ingo Molnar [EMAIL PROTECTED] wrote:
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.
yeah. I had that implemented at a stage but was trying to be too
clever for my own good ;-)
i have fixed it via the patch below. (I'm using rb_last() because that
way the normal scheduling codepaths are not burdened with the
maintainance of a rightmost entry.)
Ingo
---
kernel/sched.c |3 ++-
kernel/sched_fair.c | 24 +---
2 files changed, 15 insertions(+), 12 deletions(-)
Index: linux/kernel/sched.c
===
--- linux.orig/kernel/sched.c
+++ linux/kernel/sched.c
@@ -3806,7 +3806,8 @@ asmlinkage long sys_sched_yield(void)
schedstat_inc(rq, yld_cnt);
if (rq-nr_running == 1)
schedstat_inc(rq, yld_act_empty);
- current-sched_class-yield_task(rq, current);
+ else
+ current-sched_class-yield_task(rq, current);
/*
* Since we are going to call schedule() anyway, there's
Index: linux/kernel/sched_fair.c
===
--- linux.orig/kernel/sched_fair.c
+++ linux/kernel/sched_fair.c
@@ -275,21 +275,23 @@ static void dequeue_task_fair(struct rq
*/
static void yield_task_fair(struct rq *rq, struct task_struct *p)
{
+ struct rb_node *entry;
+ struct task_struct *last;
+
dequeue_task_fair(rq, p);
p-on_rq = 0;
+
/*
-* Temporarily insert at the last position of the tree:
+* Temporarily insert at the last position of the tree.
+* The key will be updated back to (near) its old value
+* when the task gets scheduled.
*/
- p-fair_key = LLONG_MAX;
+ entry = rb_last(rq-tasks_timeline);
+ last = rb_entry(entry, struct task_struct, run_node);
+
+ p-fair_key = last-fair_key + 1;
__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;
}
/*
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: CFS and suspend2: hang in atomic copy (was: [Announce] [patch] Modular Scheduler Core and Completely Fair Scheduler [CFS])
* Christian Hesse <[EMAIL PROTECTED]> 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;
}
/*
@@ -380,7 +391,10 @@ static void task_tick_fair(struct rq *rq
* Dequeue and enqueue the task to update its
* position within the tree:
*/
- requeue_task_fair(rq, curr);
+ dequeue_task_fair(rq, curr);
+ curr->on_rq = 0;
+ enqueue_task_fair(rq, curr);
+ curr->on_rq = 1;
/*
* Reschedule if another task tops the current one.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
CFS and suspend2: hang in atomic copy (was: [Announce] [patch] Modular Scheduler Core and Completely Fair Scheduler [CFS])
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. 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 cced suspend2 devel list, perhaps Nigel is interested as well. -- Regards, Chris signature.asc Description: This is a digitally signed message part.
CFS and suspend2: hang in atomic copy (was: [Announce] [patch] Modular Scheduler Core and Completely Fair Scheduler [CFS])
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. 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 cced suspend2 devel list, perhaps Nigel is interested as well. -- Regards, Chris signature.asc Description: This is a digitally signed message part.
Re: CFS and suspend2: hang in atomic copy (was: [Announce] [patch] Modular Scheduler Core and Completely Fair Scheduler [CFS])
* Christian Hesse [EMAIL PROTECTED] 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;
}
/*
@@ -380,7 +391,10 @@ static void task_tick_fair(struct rq *rq
* Dequeue and enqueue the task to update its
* position within the tree:
*/
- requeue_task_fair(rq, curr);
+ dequeue_task_fair(rq, curr);
+ curr-on_rq = 0;
+ enqueue_task_fair(rq, curr);
+ curr-on_rq = 1;
/*
* Reschedule if another task tops the current one.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
