On 07/06/2016 10:00, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng...@hotmail.com>
>
> Commit e9532e69b8d1 ("sched/cputime: Fix steal time accounting vs. CPU
> hotplug")
> set rq->prev_* to 0 after a cpu hotplug comes back in order to fix the
> scenario:
>
> | steal is smaller than rq->prev_steal_time we end up with an insane large
> | value which then gets added to rq->prev_steal_time, resulting in a permanent
> | wreckage of the accounting.
>
> However, it is still buggy.
>
> rq->prev_steal_time = 0:
>
> As Rik pointed out:
>
> | setting rq->prev_irq_time to 0 in the guest, and then getting a giant value
> from
> | the host, could result in a very large of steal_jiffies.
>
> rq->prev_steal_time_rq = 0:
>
> | steal = paravirt_steal_clock(cpu_of(rq));
> | steal -= rq->prev_steal_time_rq;
> |
> | if (unlikely(steal > delta))
> | steal = delta;
> |
> | rq->prev_steal_time_rq += steal;
> | delta -= steal;
> |
> | rq->clock_task += delta;
>
> steal is a giant value and rq->prev_steal_time_rq is 0,
> rq->prev_steal_time_rq
> grows in delta granularity, rq->clock_task can't ramp up until
> rq->prev_steal_time_rq
> catches up steal clock since delta value will be 0 after reducing steal time
> from
> normal execution time. That's why I obersved that cpuhg/1-12 continue running
> until rq->prev_steal_time_rq catches up steal clock timestamp.
>
> I believe rq->prev_irq_time has similar issue. So this patch fix it by
> setting
> rq->prev_* to current irq time and steal clock timestamp after a cpu hotplug
> comes back.
I'm not sure this patch is necessary. Instead you could just revert
commit e9532e69b8d1. The previous patch obviously makes it unnecessary
to reset rq->prev_steal_time and rq->prev_steal_time_rq, and the reset
of rq->prev_irq_time looks like a no-op to me.
Paolo
> Fixes: 'commit e9532e69b8d1 ("sched/cputime: Fix steal time accounting vs.
> CPU hotplug")'
> Acked-by: Rik van Riel <r...@redhat.com>
> Cc: Ingo Molnar <mi...@kernel.org>
> Cc: Peter Zijlstra (Intel) <pet...@infradead.org>
> Cc: Rik van Riel <r...@redhat.com>
> Cc: Thomas Gleixner <t...@linutronix.de>
> Cc: Frederic Weisbecker <fweis...@gmail.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: Radim Krčmář <rkrc...@redhat.com>
> Signed-off-by: Wanpeng Li <wanpeng...@hotmail.com>
> ---
> kernel/sched/sched.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 72f1f30..e6758af 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -1813,12 +1813,12 @@ static inline void cpufreq_trigger_update(u64 time) {}
> static inline void account_reset_rq(struct rq *rq)
> {
> #ifdef CONFIG_IRQ_TIME_ACCOUNTING
> - rq->prev_irq_time = 0;
> + rq->prev_irq_time = irq_time_read(cpu_of(rq));
> #endif
> #ifdef CONFIG_PARAVIRT
> - rq->prev_steal_time = 0;
> + rq->prev_steal_time = paravirt_steal_clock(cpu_of(rq));
> #endif
> #ifdef CONFIG_PARAVIRT_TIME_ACCOUNTING
> - rq->prev_steal_time_rq = 0;
> + rq->prev_steal_time_rq = paravirt_steal_clock(cpu_of(rq));
> #endif
> }
>
