From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755219AbcFGKsF (ORCPT ); Tue, 7 Jun 2016 06:48:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39303 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755167AbcFGKsC (ORCPT ); Tue, 7 Jun 2016 06:48:02 -0400 Subject: Re: [PATCH v4 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting 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: <56bdc61d-fed4-76bd-4e5f-5172ad0e20fa@redhat.com> Date: Tue, 7 Jun 2016 12:47:53 +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.32]); Tue, 07 Jun 2016 10:48:01 +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 > > 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. I still have no idea how to parse this. What are "ring boundaries"? Rik, can you suggest a better commit message? > Suggested-and-Reviewed-by: Rik van Riel Please split Suggested-by and Reviewed-by. > diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c > index 75f98c5..9ff036b 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; > } > > /* > @@ -691,9 +691,13 @@ static cputime_t get_vtime_delta(struct task_struct *tsk) > > static void __vtime_account_system(struct task_struct *tsk) > { > - cputime_t delta_cpu = get_vtime_delta(tsk); > + cputime_t delta_time = get_vtime_delta(tsk); > + cputime_t steal_time = jiffies_to_cputime(steal_account_process_tick()); > > - account_system_time(tsk, irq_count(), delta_cpu, cputime_to_scaled(delta_cpu)); > + if (steal_time < delta_time) { > + delta_time -= steal_time; > + account_system_time(tsk, irq_count(), delta_time, cputime_to_scaled(delta_time)); > + } > } > > void vtime_account_system(struct task_struct *tsk) > @@ -718,13 +722,18 @@ void vtime_gen_account_irq_exit(struct task_struct *tsk) > > void vtime_account_user(struct task_struct *tsk) > { > - cputime_t delta_cpu; > + cputime_t delta_time, steal_time; > > write_seqcount_begin(&tsk->vtime_seqcount); > tsk->vtime_snap_whence = VTIME_SYS; > if (vtime_delta(tsk)) { > - delta_cpu = get_vtime_delta(tsk); > - account_user_time(tsk, delta_cpu, cputime_to_scaled(delta_cpu)); > + delta_time = get_vtime_delta(tsk); > + steal_time = jiffies_to_cputime(steal_account_process_tick()); > + > + if (steal_time < delta_time) { > + delta_time -= steal_time; > + account_user_time(tsk, delta_time, cputime_to_scaled(delta_time)); > + } > } > write_seqcount_end(&tsk->vtime_seqcount); > } > You're adding almost the same code to two callers of get_vtime_delta out of three. I don't know the vtime accounting code very well, but why doesn't the same apply to account_idle_time? If it does, you should instead change get_vtime_delta to process steal time and subtract it from the result. Secondarily, when can it happen that steal_time > delta_time? Thanks, Paolo