From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756178AbdJJMnt (ORCPT ); Tue, 10 Oct 2017 08:43:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43882 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751415AbdJJMnr (ORCPT ); Tue, 10 Oct 2017 08:43:47 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 755F2267C4 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=sgruszka@redhat.com Date: Tue, 10 Oct 2017 14:42:01 +0200 From: Stanislaw Gruszka To: Ingo Molnar Cc: Dongli Zhang , Wanpeng Li , Rik van Riel , Xiaolong Ye , Frederic Weisbecker , linux-kernel@vger.kernel.org, xen-devel@lists.xen.org, mingo@redhat.com, peterz@infradead.org, dario.faggioli@citrix.com, bevan@bi-co.net, xen.list@daevel.fr, joao.m.martins@oracle.com Subject: Re: [PATCH 1/1] sched/cputime: do not decrease steal time after live migration on xen Message-ID: <20171010124201.GD8263@redhat.com> References: <1507626848-24148-1-git-send-email-dongli.zhang@oracle.com> <20171010105925.mla7tpdh6stlxie3@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171010105925.mla7tpdh6stlxie3@gmail.com> User-Agent: Mutt/1.5.20 (2009-12-10) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Tue, 10 Oct 2017 12:43:47 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Oct 10, 2017 at 12:59:26PM +0200, Ingo Molnar wrote: > > (Cc:-ed more gents involved in kernel/sched/cputime.c work. Full patch quoted > below.) > > * Dongli Zhang wrote: > > > After guest live migration on xen, steal time in /proc/stat > > (cpustat[CPUTIME_STEAL]) might decrease because steal returned by > > paravirt_steal_clock() might be less than this_rq()->prev_steal_time. > > > > For instance, steal time of each vcpu is 335 before live migration. > > > > cpu 198 0 368 200064 1962 0 0 1340 0 0 > > cpu0 38 0 81 50063 492 0 0 335 0 0 > > cpu1 65 0 97 49763 634 0 0 335 0 0 > > cpu2 38 0 81 50098 462 0 0 335 0 0 > > cpu3 56 0 107 50138 374 0 0 335 0 0 > > > > After live migration, steal time is reduced to 312. > > > > cpu 200 0 370 200330 1971 0 0 1248 0 0 > > cpu0 38 0 82 50123 500 0 0 312 0 0 > > cpu1 65 0 97 49832 634 0 0 312 0 0 > > cpu2 39 0 82 50167 462 0 0 312 0 0 > > cpu3 56 0 107 50207 374 0 0 312 0 0 > > > > The code in this patch is borrowed from do_stolen_accounting() which has > > already been removed from linux source code since commit ecb23dc6 ("xen: > > add steal_clock support on x86"). > > > > Similar and more severe issue would impact prior linux 4.8-4.10 as > > discussed by Michael Las at > > https://0xstubs.org/debugging-a-flaky-cpu-steal-time-counter-on-a-paravirtualized-xen-guest. > > Unlike the issue discussed by Michael Las which would overflow steal time > > and lead to 100% st usage in top command for linux 4.8-4.10, the issue for > > linux 4.11+ would only decrease but not overflow steal time after live > > migration. > > > > References: https://0xstubs.org/debugging-a-flaky-cpu-steal-time-counter-on-a-paravirtualized-xen-guest > > Signed-off-by: Dongli Zhang > > --- > > kernel/sched/cputime.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c > > index 14d2dbf..57d09cab 100644 > > --- a/kernel/sched/cputime.c > > +++ b/kernel/sched/cputime.c > > @@ -238,10 +238,17 @@ static __always_inline u64 steal_account_process_time(u64 maxtime) > > { > > #ifdef CONFIG_PARAVIRT > > if (static_key_false(¶virt_steal_enabled)) { > > - u64 steal; > > + u64 steal, steal_time; > > + s64 steal_delta; > > + > > + steal_time = paravirt_steal_clock(smp_processor_id()); > > + steal = steal_delta = steal_time - this_rq()->prev_steal_time; > > + > > + if (unlikely(steal_delta < 0)) { > > + this_rq()->prev_steal_time = steal_time; I don't think setting prev_steal_time to smaller value is right thing to do. Beside, I don't think we need to check for overflow condition for cputime variables (it will happen after 279 years :-). So instead of introducing signed steal_delta variable I would just add below check, which should be sufficient to fix the problem: if (unlikely(steal <= this_rq()->prev_steal_time)) return 0; Thanks Stanislaw