From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751947AbdHBXhC (ORCPT ); Wed, 2 Aug 2017 19:37:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48872 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751136AbdHBXhA (ORCPT ); Wed, 2 Aug 2017 19:37:00 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 8ED0E356C0 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=mtosatti@redhat.com Date: Wed, 2 Aug 2017 20:36:20 -0300 From: Marcelo Tosatti To: Denis Plotnikov Cc: pbonzini@redhat.com, rkrcmar@redhat.com, kvm@vger.kernel.org, john.stultz@linaro.org, tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com, linux-kernel@vger.kernel.org, x86@kernel.org, rkagan@virtuozzo.com, den@virtuozzo.com Subject: Re: [PATCH v4 09/10] pvclock: add clocksource change notification on changing of tsc stable bit Message-ID: <20170802233617.GB20388@amt.cnet> References: <1501684690-211093-1-git-send-email-dplotnikov@virtuozzo.com> <1501684690-211093-10-git-send-email-dplotnikov@virtuozzo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1501684690-211093-10-git-send-email-dplotnikov@virtuozzo.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Wed, 02 Aug 2017 23:37:00 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Aug 02, 2017 at 05:38:09PM +0300, Denis Plotnikov wrote: > It's needed to notify the KVM guest about critical changes in pvclock > and make it to update its masterclock. > > This is a part of the work aiming to make kvmclock be a clocksource > providing valid cycles value for KVM masterclock, another words > make possible to use KVM masterclock over kvmclock clocksource. > > Signed-off-by: Denis Plotnikov > --- > arch/x86/kernel/pvclock.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) Please do an analysis similar to the comment which starts at "* Assuming a stable TSC across physical CPUS, and a stable TSC * across virtual CPUs, the following condition is possible. * Each numbered line represents an event visible to both * CPUs at the next numbered event." Describing why its safe to use kvmclock as source for the masterclock (honestly i haven't gone through the details, but someone should before this patch is merged). For one thing, its only safe to use kvmclock masterclock if the TSCs are synchronized in the host (so that you can read offset on vcpu-0 using a TSC that has been initialized on vcpu-1). So masterclock in the L1 guest is necessary. Do you enforce that? Also L2 guest TSCs must be synchronized. Where is that enforced? Also why its safe to use non-TSC-clocksource (clock_read, tsc_read) + tsc offsets from that point. > > diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c > index bece384..5898f20 100644 > --- a/arch/x86/kernel/pvclock.c > +++ b/arch/x86/kernel/pvclock.c > @@ -22,6 +22,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -73,6 +74,8 @@ u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src) > return flags & valid_flags; > } > > +static atomic_t clocksource_stable = ATOMIC_INIT(0); > + > u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src, > u64 *cycles_stamp, u8 *flags_stamp) > { > @@ -102,10 +105,20 @@ u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src, > pvclock_touch_watchdogs(); > } > > - if ((valid_flags & PVCLOCK_TSC_STABLE_BIT) && > - (flags & PVCLOCK_TSC_STABLE_BIT)) > - return ret; > + if (likely(valid_flags & PVCLOCK_TSC_STABLE_BIT)) { > + bool stable_now = !!(flags & PVCLOCK_TSC_STABLE_BIT); > + bool stable_last = (bool) atomic_read(&clocksource_stable); > + > + if (unlikely(stable_now != stable_last)) { > + /* send notification once */ > + if (stable_last == atomic_cmpxchg( > + &clocksource_stable, stable_last, stable_now)) > + clocksource_changes_notify(); > + } > > + if (stable_now) > + return ret; > + } > /* > * Assumption here is that last_value, a global accumulator, always goes > * forward. If we are less than that, we should not be much smaller. > -- > 2.7.4