Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1415973 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 2/3] sched/cputime: Fix prev steal time accouting  during cpu hotplug Paolo Bonzini <pbonzini@redhat.com> - 2016-06-07 12:40 +0200
    Re: Fw: [PATCH v4 2/3] sched/cputime: Fix prev steal time accouting  during cpu hotplug Wanpeng Li <kernellwp@gmail.com> - 2016-06-07 14:00 +0200
      Re: Fw: [PATCH v4 2/3] sched/cputime: Fix prev steal time accouting  during cpu hotplug Paolo Bonzini <pbonzini@redhat.com> - 2016-06-07 14:00 +0200

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

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

[toc] | [next] | [standalone]

#1416079

2016-06-07 19:41 GMT+08:00 Wanpeng Li <wanpeng.li@hotmail.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.

The reason why I'm not just simple revert it is that commit mentioned
"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." Though I didn't meet such
scenario. So I just do what that commit really want to do.

Regards,
Wanpeng Li

[toc] | [prev] | [next] | [standalone]

#1416091

On 07/06/2016 13:50, Wanpeng Li wrote:
>> > 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.
> The reason why I'm not just simple revert it is that commit mentioned
> "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."

With this patch, you go back to having underflow if steal is smaller
than rq->prev_steal_time.  The point is that it should never be smaller;
it was only smaller because of the bug that you are fixing in patch 1.

Thanks,

Paolo

 Though I didn't meet such
> scenario. So I just do what that commit really want to do.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web