From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933490AbcFMDiO (ORCPT ); Sun, 12 Jun 2016 23:38:14 -0400 Received: from mail-oi0-f65.google.com ([209.85.218.65]:33771 "EHLO mail-oi0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933177AbcFMDiM convert rfc822-to-8bit (ORCPT ); Sun, 12 Jun 2016 23:38:12 -0400 MIME-Version: 1.0 In-Reply-To: References: <1465355110-21714-1-git-send-email-wanpeng.li@hotmail.com> <1465355110-21714-3-git-send-email-wanpeng.li@hotmail.com> From: Wanpeng Li Date: Mon, 13 Jun 2016 11:38:11 +0800 Message-ID: Subject: Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting To: Paolo Bonzini Cc: "linux-kernel@vger.kernel.org" , kvm , Wanpeng Li , Ingo Molnar , "Peter Zijlstra (Intel)" , Rik van Riel , Thomas Gleixner , Frederic Weisbecker , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= 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 2016-06-08 18:14 GMT+08:00 Paolo Bonzini : > > > On 08/06/2016 05:05, Wanpeng Li wrote: >> From: Wanpeng Li >> >> This patch adds guest steal-time support to full dynticks CPU >> time accounting. After the following commit: >> >> ff9a9b4c4334 ("sched, time: Switch VIRT_CPU_ACCOUNTING_GEN to jiffy granularity") >> >> ... time sampling became jiffy based, even if it's still listened >> to ring boundaries, so steal_account_process_tick() is reused >> to account how many 'ticks' are stolen-time, after the last accumulation. >> >> Suggested-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 >> --- >> v4 -> v5: >> * apply same logic to account_idle_time, so change get_vtime_delta instead >> v3 -> v4: >> * fix grammar errors, thanks Ingo >> * cleanup fragile codes, thanks Ingo >> v2 -> v3: >> * convert steal time jiffies to cputime >> v1 -> v2: >> * fix divide zero bug, thanks Rik >> >> kernel/sched/cputime.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c >> index 75f98c5..b62f9f8 100644 >> --- a/kernel/sched/cputime.c >> +++ b/kernel/sched/cputime.c >> @@ -257,7 +257,7 @@ void account_idle_time(cputime_t cputime) >> cpustat[CPUTIME_IDLE] += (__force u64) cputime; >> } >> >> -static __always_inline bool steal_account_process_tick(void) >> +static __always_inline unsigned long steal_account_process_tick(void) >> { >> #ifdef CONFIG_PARAVIRT >> if (static_key_false(¶virt_steal_enabled)) { >> @@ -279,7 +279,7 @@ static __always_inline bool steal_account_process_tick(void) >> return steal_jiffies; >> } >> #endif >> - return false; >> + return 0; >> } >> >> /* >> @@ -681,12 +681,17 @@ static cputime_t vtime_delta(struct task_struct *tsk) >> static cputime_t get_vtime_delta(struct task_struct *tsk) >> { >> unsigned long now = READ_ONCE(jiffies); >> - unsigned long delta = now - tsk->vtime_snap; >> + cputime_t delta_time, steal_time; >> >> + steal_time = jiffies_to_cputime(steal_account_process_tick()); >> + delta_time = jiffies_to_cputime(now - tsk->vtime_snap); >> WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); >> tsk->vtime_snap = now; >> >> - return jiffies_to_cputime(delta); >> + if (steal_time < delta_time) >> + delta_time -= steal_time; >> + >> + return delta_time; > > I think this is wrong. If you get more steal time than delta time > (which as Rik noticed can happen due to partial jiffies), you will end > up accounting things twice, once in steal_account_process_tick and once > here. In other words you'll get the exact bug you're trying to fix. > > The right thing is to add a max_jiffies argument to > steal_account_process_tick. steal_account_process_tick will not attempt > to remove more than max_jiffies. Here you pass delta_jiffies (i.e. now > - tsk->vtime_snap) to steal_account_process_tick, existing callers can > pass ULONG_MAX. You can then > > return jiffies_to_cputime(delta_jiffies - steal_jiffies); > > in get_vtime_delta and not worry about underflow. Do you mean something like below, actually I see delta_jiffies < steal_jiffies sometimes. diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c index 75f98c5..a7606a9 100644 --- a/kernel/sched/cputime.c +++ b/kernel/sched/cputime.c @@ -257,7 +257,7 @@ void account_idle_time(cputime_t cputime) cpustat[CPUTIME_IDLE] += (__force u64) cputime; } -static __always_inline bool steal_account_process_tick(void) +static __always_inline unsigned long steal_account_process_tick(unsigned long max_jiffies) { #ifdef CONFIG_PARAVIRT if (static_key_false(¶virt_steal_enabled)) { @@ -272,14 +272,14 @@ static __always_inline bool steal_account_process_tick(void) * time in jiffies. Lets cast the result to jiffies * granularity and account the rest on the next rounds. */ - steal_jiffies = nsecs_to_jiffies(steal); + steal_jiffies = min(nsecs_to_jiffies(steal), max_jiffies); this_rq()->prev_steal_time += jiffies_to_nsecs(steal_jiffies); account_steal_time(jiffies_to_cputime(steal_jiffies)); return steal_jiffies; } #endif - return false; + return 0; } /* @@ -346,7 +346,7 @@ static void irqtime_account_process_tick(struct task_struct *p, int user_tick, u64 cputime = (__force u64) cputime_one_jiffy; u64 *cpustat = kcpustat_this_cpu->cpustat; - if (steal_account_process_tick()) + if (steal_account_process_tick(ULONG_MAX)) return; cputime *= ticks; @@ -477,7 +477,7 @@ void account_process_tick(struct task_struct *p, int user_tick) return; } - if (steal_account_process_tick()) + if (steal_account_process_tick(ULONG_MAX)) return; if (user_tick) @@ -681,12 +681,14 @@ static cputime_t vtime_delta(struct task_struct *tsk) static cputime_t get_vtime_delta(struct task_struct *tsk) { unsigned long now = READ_ONCE(jiffies); - unsigned long delta = now - tsk->vtime_snap; + unsigned long delta_jiffies, steal_jiffies; + delta_jiffies = jiffies_to_cputime(now - tsk->vtime_snap); + steal_jiffies = jiffies_to_cputime(steal_account_process_tick(delta_jiffies)); WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); tsk->vtime_snap = now; - return jiffies_to_cputime(delta); + return jiffies_to_cputime(delta_jiffies - steal_jiffies); } static void __vtime_account_system(struct task_struct *tsk)