From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752401AbcL0H5V (ORCPT ); Tue, 27 Dec 2016 02:57:21 -0500 Received: from mga06.intel.com ([134.134.136.31]:62529 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751884AbcL0H5S (ORCPT ); Tue, 27 Dec 2016 02:57:18 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,415,1477983600"; d="scan'208";a="207086839" Date: Tue, 27 Dec 2016 16:06:44 +0800 From: Chen Yu To: Marcelo Tosatti Cc: kvm@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org, "Rafael J. Wysocki" , Len Brown , Paolo Bonzini , Radim Krcmar , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Wanpeng Li Subject: Re: [PATCH][RFC] VM: x86: Return ealier if clocksource has not changed Message-ID: <20161227080644.GA5370@yu-desktop-1.sh.intel.com> References: <1482482513-24151-1-git-send-email-yu.c.chen@intel.com> <20161226194422.GA30796@amt.cnet> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161226194422.GA30796@amt.cnet> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Marcelo, On Mon, Dec 26, 2016 at 05:44:25PM -0200, Marcelo Tosatti wrote: > On Fri, Dec 23, 2016 at 04:41:53PM +0800, Chen Yu wrote: > > Currently the notifier of pvclock_gtod_notify() get invoked > > frequently due to the periodic update_wall_time(). This might > > slow down the system a little bit as there might be redundant > > execution code path and unnecessary lock contention > > in update_pvclock_gtod(), which was found when I was doing > > suspend/resume speed testings. As pvclock_gtod_notify() > > should be invoked only when clocksource has changed, according to > > Commit 16e8d74d2da9 ("KVM: x86: notifier for clocksource changes") > > , either we can add a new notifier for clocksource switch, > > or we can simply bypass the following code in pvclock_gtod_notify() > > earlier if there is no clocksource switch. > > > > Cc: Paolo Bonzini > > Cc: "Radim Krcmar" > > Cc: Marcelo Tosatti > > Cc: Thomas Gleixner > > Cc: Ingo Molnar > > Cc: "H. Peter Anvin" > > Cc: Wanpeng Li > > Signed-off-by: Chen Yu > > --- > > arch/x86/kvm/x86.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 445c51b..54aa32d 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -5961,13 +5961,14 @@ static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused, > > struct pvclock_gtod_data *gtod = &pvclock_gtod_data; > > struct timekeeper *tk = priv; > > > > + if (likely(gtod->clock.vclock_mode == VCLOCK_TSC)) > > + return 0; > > I think this is only safe if any of the values in "struct > pvclock_gtod_data" are unchanged. Otherwise the local (KVM) copy is > kept incorrect. I missread the code previously and I thought only under the condition the clocksource has been switched to another one will the KVM copy be touched. Apparently it is not the case because the copy should be updated on-time during normal tick, right? thanks for your reply, Yu > > > update_pvclock_gtod(tk); > > > > /* disable master clock if host does not trust, or does not > > * use, TSC clocksource > > */ > > - if (gtod->clock.vclock_mode != VCLOCK_TSC && > > - atomic_read(&kvm_guest_has_master_clock) != 0) > > + if (atomic_read(&kvm_guest_has_master_clock) != 0) > > queue_work(system_long_wq, &pvclock_gtod_work); > > > > return 0; > > -- > > 2.7.4