From patchwork Thu Dec 29 13:54:49 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chen Yu X-Patchwork-Id: 9491231 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 10F2260453 for ; Thu, 29 Dec 2016 13:46:38 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 049701FF1D for ; Thu, 29 Dec 2016 13:46:38 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id EAD532018E; Thu, 29 Dec 2016 13:46:37 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4F6311FF1D for ; Thu, 29 Dec 2016 13:46:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752693AbcL2NqQ (ORCPT ); Thu, 29 Dec 2016 08:46:16 -0500 Received: from mga04.intel.com ([192.55.52.120]:47293 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752129AbcL2NqP (ORCPT ); Thu, 29 Dec 2016 08:46:15 -0500 Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga104.fm.intel.com with ESMTP; 29 Dec 2016 05:45:20 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,427,1477983600"; d="scan'208";a="917161875" Received: from yu-desktop-1.sh.intel.com ([10.239.160.134]) by orsmga003.jf.intel.com with ESMTP; 29 Dec 2016 05:45:17 -0800 Date: Thu, 29 Dec 2016 21:54:49 +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: <20161229135448.GA3416@yu-desktop-1.sh.intel.com> References: <1482482513-24151-1-git-send-email-yu.c.chen@intel.com> <20161226194422.GA30796@amt.cnet> <20161227080644.GA5370@yu-desktop-1.sh.intel.com> <20161227153244.GA14267@amt.cnet> <20161229085902.GA2474@yu-desktop-1.sh.intel.com> <20161229095733.GA21107@amt.cnet> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20161229095733.GA21107@amt.cnet> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Dec 29, 2016 at 07:57:33AM -0200, Marcelo Tosatti wrote: > On Thu, Dec 29, 2016 at 04:59:02PM +0800, Chen Yu wrote: > > On Tue, Dec 27, 2016 at 01:32:47PM -0200, Marcelo Tosatti wrote: > > > On Tue, Dec 27, 2016 at 04:06:44PM +0800, Chen Yu wrote: > > > > 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, > > > > > > Yes, it is updated during the normal tick, and mult/freq values change. > > > > > > However, if none of them change, its not necessary to call the callback. > > > Perhaps you can check if any of the values changed and only > > > invoke the callback in that case? > > > > > Yes, this should be an optimization, but most of the callers(workload) come > > from update_wall_time(), and in this code path the clock source's cycle > > should already be updated in most cases, so this optimization should not take > > much effect to reduce the burden I guess? > > > > Thanks, > > Yu > > I don't understand your reasoning. > > "If the clock source parameters are already updated then optimization > does not make much effect". > > If the clock source parameters are updated (that is there has been no > change in any of the values in pvclock_gtod_data), then you can skip > the callback. This case reduces the burden. > > Right? > > Yes, in general case we can improve the code logic. Previously I was thinking of the case I encountered: 1. There are quite some invokes of pvclock_gtod_notify() caught by ftrace, and most of them should be triggered by update_wall_time() 2. If we optimize the code not to invoke pvclock_gtod_notify() if there is no modification of pvclock_gtod_data, it will reduce the burden for general use cases, but not for update_wall_time(), as it has already been modified in update_wall_time. But yes, you are right, above is just my scenario, the optimization you mentioned is a generic solution for most cases. Do you mean the following solution? --- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Index: linux/kernel/time/timekeeping.c =================================================================== --- linux.orig/kernel/time/timekeeping.c +++ linux/kernel/time/timekeeping.c @@ -527,7 +527,13 @@ static RAW_NOTIFIER_HEAD(pvclock_gtod_ch static void update_pvclock_gtod(struct timekeeper *tk, bool was_set) { - raw_notifier_call_chain(&pvclock_gtod_chain, was_set, tk); + static struct timekeeper prev_timekeeper; + + /* Only notify if the clocksource has changed.*/ + if (memcmp(tk, &prev_timekeeper, sizeof(struct timekeeper))) { + raw_notifier_call_chain(&pvclock_gtod_chain, was_set, tk); + memcpy(&prev_timekeeper, tk, sizeof(struct timekeeper)); + } } /**