From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932710AbdJKHaS (ORCPT ); Wed, 11 Oct 2017 03:30:18 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:50953 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932655AbdJKHaP (ORCPT ); Wed, 11 Oct 2017 03:30:15 -0400 Subject: Re: [PATCH 1/1] sched/cputime: do not decrease steal time after live migration on xen To: Stanislaw Gruszka , Ingo Molnar References: <1507626848-24148-1-git-send-email-dongli.zhang@oracle.com> <20171010105925.mla7tpdh6stlxie3@gmail.com> <20171010124201.GD8263@redhat.com> Cc: 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 From: Dongli Zhang Message-ID: <073e34c7-3320-c21a-a1a3-467728b87f3f@oracle.com> Date: Wed, 11 Oct 2017 15:29:08 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <20171010124201.GD8263@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Stanislaw and Peter, On 10/10/2017 08:42 PM, Stanislaw Gruszka wrote: > 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. If we do not set prev_steal_time to smaller steal (obtained from paravirt_steal_clock()), it will take a while for kernel to wait for new steal to catch up with this_rq()->prev_steal_time, and cpustat[CPUTIME_STEAL] will stay unchanged until steal is more than this_rq()->prev_steal_time again. Do you think it is fine? If it is fine, I will try to limit the fix to xen specific code in driver/xen/time.c so that we would not taint kernel/sched/cputime.c, as Peter has asked why not just fix up paravirt_steal_time() on migration. Thank you very much! Dongli Zhang > > 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 >