From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1424339AbcFHKBy (ORCPT ); Wed, 8 Jun 2016 06:01:54 -0400 Received: from mail-wm0-f45.google.com ([74.125.82.45]:36799 "EHLO mail-wm0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161924AbcFHKBv (ORCPT ); Wed, 8 Jun 2016 06:01:51 -0400 Subject: Re: [PATCH v5 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: <1465355110-21714-1-git-send-email-wanpeng.li@hotmail.com> <1465355110-21714-2-git-send-email-wanpeng.li@hotmail.com> Cc: Wanpeng Li , Ingo Molnar , "Peter Zijlstra (Intel)" , Rik van Riel , Thomas Gleixner , Frederic Weisbecker , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= From: Paolo Bonzini Message-ID: Date: Wed, 8 Jun 2016 12:01:44 +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: <1465355110-21714-2-git-send-email-wanpeng.li@hotmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/06/2016 05:05, 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 reverting > commit e9532e69b8d1. This is still very hard to read. I think the justification for this patch is much simpler: ------- 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 case where (after CPU hotplug) steal is smaller than rq->prev_steal_time. However, this should never happen. steal was only smaller because of the KVM-specific bug fixed by the previous patch. Worse, the previous patch triggers a bug on CPU hot-unplug/plug operation: because rq->prev_steal_time is cleared, all of the CPU's past steal time will be accounted again on hot-plug. Since the root cause has been fixed, we can just revert commit e9532e69b8d1. ------- Since things are currently pretty broken, I think it's fair to keep the two patches separate instead of squashing them. Paolo