sched: fix cpu utilization account error
Subsystems (2)
PATCH 1/1 sched: fix cpu utilization account error
We setting clock_skip_update = 1 based on the assumption that the next call to update_rq_clock() will come nearly immediately after being set. However, it is not always true especially on non-preempt mode. In this case we may miss some clock update, which would cause an error curr->sum_exec_runtime account.
The test result show that test_kthread's exec_runtime has been added to watchdog.
PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ P COMMAND
28 root RT 0 0 0 0 S 100 0.0 0:05.39 5 watchdog/5
7 root RT 0 0 0 0 S 95 0.0 0:05.83 0 watchdog/0
12 root RT 0 0 0 0 S 94 0.0 0:05.79 1 watchdog/1
16 root RT 0 0 0 0 S 92 0.0 0:05.74 2 watchdog/2
20 root RT 0 0 0 0 S 91 0.0 0:05.71 3 watchdog/3
24 root RT 0 0 0 0 S 82 0.0 0:05.42 4 watchdog/4
32 root RT 0 0 0 0 S 79 0.0 0:05.35 6 watchdog/6
5200 root 20 0 0 0 0 R 21 0.0 0:08.88 6 test_kthread/6
5194 root 20 0 0 0 0 R 20 0.0 0:08.41 0 test_kthread/0
5195 root 20 0 0 0 0 R 20 0.0 0:08.44 1 test_kthread/1
5196 root 20 0 0 0 0 R 20 0.0 0:08.49 2 test_kthread/2
5197 root 20 0 0 0 0 R 20 0.0 0:08.53 3 test_kthread/3
5198 root 20 0 0 0 0 R 19 0.0 0:08.81 4 test_kthread/4
5199 root 20 0 0 0 0 R 2 0.0 0:08.66 5 test_kthread/5"test_kthread/i" is a kernel thread which has a infinity loop and it calls schedule() every 1s. It's main process as below:
static int main_loop (void *unused)
{
unsigned long flags;
unsigned long last = jiffies;
int i;
while (!kthread_should_stop()) {
/* call schedule every 1 sec */
if (HZ <= jiffies - last) {
last = jiffies;
schedule();
}
/* do some thing */
for (i = 0; i < 1000; i++)
;
if (kthread_should_stop())
break;
}
}In this patch, we do not skip clock update when current task is kernel thread in non-preempt mode.
--- kernel/sched/core.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)
kernel/sched/core.c +11 -0
@@ -970,8 +970,19 @@ void check_preempt_curr(struct rq *rq, struct task_struct *p, int flags)* A queue event has occurred, and we're going to schedule. In * this case, we can save a useless back to back clock update. */ +#ifdef CONFIG_PREEMPT if (rq->curr->on_rq &&test_tsk_need_resched (rq->curr)) rq->skip_clock_update =1 ; +#else +/* +* In non-preempt mode, a kernel thread may run for a long time +* until been scheduled out by itself. In this cace, we need update +* rq clock when calling schedule function, otherwise, we might +* miss rq clock update for a long time. +*/ +if (rq->curr->on_rq &&test_tsk_need_resched (rq->curr) && rq->curr->mm) + rq->skip_clock_update =1 ; +#endif }static ATOMIC_NOTIFIER_HEAD (task_migration_notifier);
Signature · 1 line
1.7.6.1
Quoted 25 lines · Xie XiuQi
We setting
clock_skip_update= 1 based on the assumption that the next call toupdate_rq_clock()will come nearly immediately after being set. However, it is not always true especially on non-preempt mode. In this case we may miss some clock update, which would cause an errorcurr->sum_exec_runtimeaccount.The test result show that test_kthread's
exec_runtimehas been added to watchdog.PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ P COMMAND 28 root RT 0 0 0 0 S 100 0.0 0:05.39 5 watchdog/5 7 root RT 0 0 0 0 S 95 0.0 0:05.83 0 watchdog/0 12 root RT 0 0 0 0 S 94 0.0 0:05.79 1 watchdog/1 16 root RT 0 0 0 0 S 92 0.0 0:05.74 2 watchdog/2 20 root RT 0 0 0 0 S 91 0.0 0:05.71 3 watchdog/3 24 root RT 0 0 0 0 S 82 0.0 0:05.42 4 watchdog/4 32 root RT 0 0 0 0 S 79 0.0 0:05.35 6 watchdog/6 5200 root 20 0 0 0 0 R 21 0.0 0:08.88 6 test_kthread/6 5194 root 20 0 0 0 0 R 20 0.0 0:08.41 0 test_kthread/0 5195 root 20 0 0 0 0 R 20 0.0 0:08.44 1 test_kthread/1 5196 root 20 0 0 0 0 R 20 0.0 0:08.49 2 test_kthread/2 5197 root 20 0 0 0 0 R 20 0.0 0:08.53 3 test_kthread/3 5198 root 20 0 0 0 0 R 19 0.0 0:08.81 4 test_kthread/4 5199 root 20 0 0 0 0 R 2 0.0 0:08.66 5 test_kthread/5
Quoted 2 lines
"test_kthread/i" is a kernel thread which has a infinity loop and it calls
schedule()every 1s. It's main process as below:
It'd be a shame to lose the cycle savings (we could use more) due to such horrible behavior. Where are you seeing this in real life?
That said, accounting funnies induced by skipped update are possible, which could trump the cycle savings I suppose, so maybe savings (sniff) should just go away?
Quoted 55 lines
static int
main_loop(void *unused){ unsigned long flags; unsigned long last = jiffies; int i; while (!kthread_should_stop()) { /* call schedule every 1 sec */ if (HZ <= jiffies - last) { last = jiffies; schedule(); } /* do some thing */ for (i = 0; i < 1000; i++) ; if (kthread_should_stop()) break; } }In this patch, we do not skip clock update when current task is kernel thread in non-preempt mode.
Reported-by Zhang Hang <[email protected]>Signed-off-by Xie XiuQi <[email protected]>--- kernel/sched/core.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)kernel/sched/core.c +11
-0@@ -970,8 +970,19 @@ void check_preempt_curr(struct rq *rq, struct task_struct *p, int flags)* A queue event has occurred, and we're going to schedule. In * this case, we can save a useless back to back clock update. */ +#ifdef CONFIG_PREEMPT if (rq->curr->on_rq &&test_tsk_need_resched (rq->curr)) rq->skip_clock_update =1 ; +#else +/* +* In non-preempt mode, a kernel thread may run for a long time +* until been scheduled out by itself. In this cace, we need update +* rq clock when calling schedule function, otherwise, we might +* miss rq clock update for a long time. +*/ +if (rq->curr->on_rq &&test_tsk_need_resched (rq->curr) && rq->curr->mm) + rq->skip_clock_update =1 ; +#endif }static ATOMIC_NOTIFIER_HEAD (task_migration_notifier);
Quoted 26 lines · Mike Galbraith
Quoted 25 lines · Xie XiuQi
We setting
clock_skip_update= 1 based on the assumption that the next call toupdate_rq_clock()will come nearly immediately after being set. However, it is not always true especially on non-preempt mode. In this case we may miss some clock update, which would cause an errorcurr->sum_exec_runtimeaccount.The test result show that test_kthread's
exec_runtimehas been added to watchdog.PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ P COMMAND 28 root RT 0 0 0 0 S 100 0.0 0:05.39 5 watchdog/5 7 root RT 0 0 0 0 S 95 0.0 0:05.83 0 watchdog/0 12 root RT 0 0 0 0 S 94 0.0 0:05.79 1 watchdog/1 16 root RT 0 0 0 0 S 92 0.0 0:05.74 2 watchdog/2 20 root RT 0 0 0 0 S 91 0.0 0:05.71 3 watchdog/3 24 root RT 0 0 0 0 S 82 0.0 0:05.42 4 watchdog/4 32 root RT 0 0 0 0 S 79 0.0 0:05.35 6 watchdog/6 5200 root 20 0 0 0 0 R 21 0.0 0:08.88 6 test_kthread/6 5194 root 20 0 0 0 0 R 20 0.0 0:08.41 0 test_kthread/0 5195 root 20 0 0 0 0 R 20 0.0 0:08.44 1 test_kthread/1 5196 root 20 0 0 0 0 R 20 0.0 0:08.49 2 test_kthread/2 5197 root 20 0 0 0 0 R 20 0.0 0:08.53 3 test_kthread/3 5198 root 20 0 0 0 0 R 19 0.0 0:08.81 4 test_kthread/4 5199 root 20 0 0 0 0 R 2 0.0 0:08.66 5 test_kthread/5
Quoted 6 lines
Quoted 2 lines
"test_kthread/i" is a kernel thread which has a infinity loop and it calls
schedule()every 1s. It's main process as below:It'd be a shame to lose the cycle savings (we could use more) due to such horrible behavior. Where are you seeing this in real life?
Thank you for your comments, Mike.
This issue was reported by a driver related pcie in which a kthread send huge amounts of data. In non-preempt mode, it would take a cpu for a long time. But, in preempt mode, I haven't found this issue yet.
Here is the kthread main logic. Although it's not a good idea, but it does exist:
while (!kthread_should_stop()) {
/* call schedule every 1 sec */
if (HZ <= jiffies - last) {
last = jiffies;
schedule();
}
/* get data and sent it */
get_msg();
send_msg();
if (kthread_should_stop())
break;
}Quoted 3 lines
That said, accounting funnies induced by skipped update are possible, which could trump the cycle savings I suppose, so maybe savings (sniff) should just go away?
Indeed, removing the skip_clock_update could resolve the issue, but I found there is no this issue in preempt mode. However, if remove skip_clock_update we'll get more precise time account.
So, what's your opinion, Mike.
Quoted 61 lines · Xie XiuQi
Quoted 26 lines · Mike Galbraith
Quoted 25 lines · Xie XiuQi
We setting
clock_skip_update= 1 based on the assumption that the next call toupdate_rq_clock()will come nearly immediately after being set. However, it is not always true especially on non-preempt mode. In this case we may miss some clock update, which would cause an errorcurr->sum_exec_runtimeaccount.The test result show that test_kthread's
exec_runtimehas been added to watchdog.PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ P COMMAND 28 root RT 0 0 0 0 S 100 0.0 0:05.39 5 watchdog/5 7 root RT 0 0 0 0 S 95 0.0 0:05.83 0 watchdog/0 12 root RT 0 0 0 0 S 94 0.0 0:05.79 1 watchdog/1 16 root RT 0 0 0 0 S 92 0.0 0:05.74 2 watchdog/2 20 root RT 0 0 0 0 S 91 0.0 0:05.71 3 watchdog/3 24 root RT 0 0 0 0 S 82 0.0 0:05.42 4 watchdog/4 32 root RT 0 0 0 0 S 79 0.0 0:05.35 6 watchdog/6 5200 root 20 0 0 0 0 R 21 0.0 0:08.88 6 test_kthread/6 5194 root 20 0 0 0 0 R 20 0.0 0:08.41 0 test_kthread/0 5195 root 20 0 0 0 0 R 20 0.0 0:08.44 1 test_kthread/1 5196 root 20 0 0 0 0 R 20 0.0 0:08.49 2 test_kthread/2 5197 root 20 0 0 0 0 R 20 0.0 0:08.53 3 test_kthread/3 5198 root 20 0 0 0 0 R 19 0.0 0:08.81 4 test_kthread/4 5199 root 20 0 0 0 0 R 2 0.0 0:08.66 5 test_kthread/5Quoted 6 lines
Quoted 2 lines
"test_kthread/i" is a kernel thread which has a infinity loop and it calls
schedule()every 1s. It's main process as below:It'd be a shame to lose the cycle savings (we could use more) due to such horrible behavior. Where are you seeing this in real life?
Thank you for your comments, Mike.
This issue was reported by a driver related pcie in which a kthread send huge amounts of data. In non-preempt mode, it would take a cpu for a long time. But, in preempt mode, I haven't found this issue yet.
Here is the kthread main logic. Although it's not a good idea, but it does exist:
while (!kthread_should_stop()) { /* call schedule every 1 sec */ if (HZ <= jiffies - last) { last = jiffies; schedule(); } /* get data and sent it */ get_msg(); send_msg(); if (kthread_should_stop()) break; }Quoted 3 lines
That said, accounting funnies induced by skipped update are possible, which could trump the cycle savings I suppose, so maybe savings (sniff) should just go away?
Quoted 5 lines
Indeed, removing the
skip_clock_updatecould resolve the issue, but I found there is no this issue in preempt mode. However, if removeskip_clock_updatewe'll get more precise time account.So, what's your opinion, Mike.
Other than take that thing out and shoot it? ;-)
It's definitely bad to hand off cycles expended to some other innocent bystander, especially an rt task that can then trip over the throttle, so in the safety first light, I'd say go with your fix I suppose, and hope that other things like contention won't show up doing the same thing. The cycle savings are nice to have, so I'd rather not just kill that entirely, but...
Maybe Peter has a better idea.
-Mike
Quoted 2 lines · Xie XiuQi
Here is the kthread main logic. Although it's not a good idea, but it does exist:
Why not fix this instead?
Quoted 6 lines
while (!kthread_should_stop()) {
/* call schedule every 1 sec */
if (HZ <= jiffies - last) {
last = jiffies;
schedule();
}Hanging out in the kernel for ages is not cool. That doesn't mean something else might not pop up that forces the issue, but to date it has not, and sacrificing precious fastpath cycles is not attractive.
-Mike
Quoted 14 lines · Mike Galbraith
Quoted 2 lines · Xie XiuQi
Here is the kthread main logic. Although it's not a good idea, but it does exist:
Why not fix this instead?
Quoted 6 lines
while (!kthread_should_stop()) { /* call schedule every 1 sec */ if (HZ <= jiffies - last) { last = jiffies; schedule(); }
Quoted 4 lines
Hanging out in the kernel for ages is not cool. That doesn't mean something else might not pop up that forces the issue, but to date it has not, and sacrificing precious fastpath cycles is not attractive.
That is to say, the driver's code needs improvement. Thank you Mike.
Quoted 2 lines
-Mike
Hi, Xie
On 07/01/2013 07:26 PM, Xie XiuQi wrote:
[snip]
Quoted 12 lines
Here is the kthread main logic. Although it's not a good idea, but it does exist:
while (!kthread_should_stop()) { /* call schedule every 1 sec */ if (HZ <= jiffies - last) { last = jiffies; schedule(); } /* get data and sent it */ get_msg(); send_msg();
What about use cond_resched() here? Isn't that more gentle?
Regards,
Michael Wang
Quoted 23 lines
if (kthread_should_stop()) break; }Quoted 3 lines
That said, accounting funnies induced by skipped update are possible, which could trump the cycle savings I suppose, so maybe savings (sniff) should just go away?
Indeed, removing the
skip_clock_updatecould resolve the issue, but I found there is no this issue in preempt mode. However, if removeskip_clock_updatewe'll get more precise time account.So, what's your opinion, Mike.
--
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/
Quoted 13 lines · Michael Wang
Hi, Xie
On 07/01/2013 07:26 PM, Xie XiuQi wrote:
[snip]Quoted 9 lines
Here is the kthread main logic. Although it's not a good idea, but it does exist:
while (!kthread_should_stop()) { /* call schedule every 1 sec */ if (HZ <= jiffies - last) { last = jiffies; schedule(); }
Quoted 6 lines
Quoted 3 lines
/* get data and sent it */ get_msg(); send_msg();What about use
cond_resched()here? Isn't that more gentle?
That's a good idea for driver implementation.
Thank you Michael.
Quoted 3 lines
Regards,
Michael Wang
Greg KH
maintainerQuoted 50 lines · Xie XiuQi
We setting
clock_skip_update= 1 based on the assumption that the next call toupdate_rq_clock()will come nearly immediately after being set. However, it is not always true especially on non-preempt mode. In this case we may miss some clock update, which would cause an errorcurr->sum_exec_runtimeaccount.The test result show that test_kthread's
exec_runtimehas been added to watchdog.PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ P COMMAND 28 root RT 0 0 0 0 S 100 0.0 0:05.39 5 watchdog/5 7 root RT 0 0 0 0 S 95 0.0 0:05.83 0 watchdog/0 12 root RT 0 0 0 0 S 94 0.0 0:05.79 1 watchdog/1 16 root RT 0 0 0 0 S 92 0.0 0:05.74 2 watchdog/2 20 root RT 0 0 0 0 S 91 0.0 0:05.71 3 watchdog/3 24 root RT 0 0 0 0 S 82 0.0 0:05.42 4 watchdog/4 32 root RT 0 0 0 0 S 79 0.0 0:05.35 6 watchdog/6 5200 root 20 0 0 0 0 R 21 0.0 0:08.88 6 test_kthread/6 5194 root 20 0 0 0 0 R 20 0.0 0:08.41 0 test_kthread/0 5195 root 20 0 0 0 0 R 20 0.0 0:08.44 1 test_kthread/1 5196 root 20 0 0 0 0 R 20 0.0 0:08.49 2 test_kthread/2 5197 root 20 0 0 0 0 R 20 0.0 0:08.53 3 test_kthread/3 5198 root 20 0 0 0 0 R 19 0.0 0:08.81 4 test_kthread/4 5199 root 20 0 0 0 0 R 2 0.0 0:08.66 5 test_kthread/5"test_kthread/i" is a kernel thread which has a infinity loop and it calls
schedule()every 1s. It's main process as below:static int
main_loop(void *unused){ unsigned long flags; unsigned long last = jiffies; int i; while (!kthread_should_stop()) { /* call schedule every 1 sec */ if (HZ <= jiffies - last) { last = jiffies; schedule(); } /* do some thing */ for (i = 0; i < 1000; i++) ; if (kthread_should_stop()) break; } }
Quoted 8 lines
In this patch, we do not skip clock update when current task is kernel thread in non-preempt mode.
Reported-by Zhang Hang <[email protected]>Signed-off-by Xie XiuQi <[email protected]>--- kernel/sched/core.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)
<formletter>
This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read Documentation/stable_kernel_rules.txt for how to do this properly.
</formletter>
Quoted 7 lines · Greg KH
Quoted 6 lines · Xie XiuQi
We setting
clock_skip_update= 1 based on the assumption that the next call toupdate_rq_clock()will come nearly immediately after being set. However, it is not always true especially on non-preempt mode. In this case we may miss some clock update, which would cause an errorcurr->sum_exec_runtimeaccount.
...
Quoted 6 lines
<formletter>
This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read
Documentation/stable_kernel_rules.txtfor how to do this properly.
Thanks Greg,
I've removed [email protected] from recipient list now.
Quoted 3 lines
</formletter>