Re: [PATCH v4 2/3] sched/cputime: Fix prev steal time accouting during cpu hotplug

[Paolo Bonzini <pbonzini@redhat.com>]

On 07/06/2016 10:00, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@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 <riel@redhat.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@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
>  }
>