From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755103AbcFGKek (ORCPT ); Tue, 7 Jun 2016 06:34:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38495 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755081AbcFGKej (ORCPT ); Tue, 7 Jun 2016 06:34:39 -0400 Subject: Re: [PATCH v4 2/3] sched/cputime: Fix prev steal time accouting during cpu hotplug To: Wanpeng Li , "linux-kernel@vger.kernel.org" , "kvm@vger.kernel.org" References: <1465661590-4732-1-git-send-email-wanpeng.li@hotmail.com> Cc: Ingo Molnar , "Peter Zijlstra (Intel)" , Rik van Riel , Thomas Gleixner , Frederic Weisbecker , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= From: Paolo Bonzini Message-ID: <15351b9b-563a-8945-5fb6-2df15107f0d6@redhat.com> Date: Tue, 7 Jun 2016 12:34:30 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Tue, 07 Jun 2016 10:34:38 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/06/2016 10:00, Wanpeng Li wrote: > From: Wanpeng Li > > 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 > Cc: Ingo Molnar > Cc: Peter Zijlstra (Intel) > Cc: Rik van Riel > Cc: Thomas Gleixner > Cc: Frederic Weisbecker > Cc: Paolo Bonzini > Cc: Radim Krčmář > Signed-off-by: Wanpeng Li > --- > 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 > } >