From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [RFC PATCH 1/2] timekeeper: change interface of clocksource reding functions Date: Mon, 10 Jul 2017 15:00:53 +0200 Message-ID: <20170710130050.GD30880@potion> References: <1498647301-130851-1-git-send-email-dplotnikov@virtuozzo.com> <1498647301-130851-2-git-send-email-dplotnikov@virtuozzo.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, pbonzini@redhat.com, den@virtuozzo.com, rkagan@virtuozzo.com To: Denis Plotnikov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:41900 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753864AbdGJNBQ (ORCPT ); Mon, 10 Jul 2017 09:01:16 -0400 Content-Disposition: inline In-Reply-To: <1498647301-130851-2-git-send-email-dplotnikov@virtuozzo.com> Sender: kvm-owner@vger.kernel.org List-ID: 2017-06-28 13:55+0300, Denis Plotnikov: > When using timekeepeing API in some cases it is useful to return > timestamp value used along with the time calculated to use that > timestamp value for other purpuses (e.g. in KVM master clock timestamp) Makes sense. What I don't like about this interface is the TSC centric approach in a code that isn't even x86 specific -- other architectures might have a similar counter they'd like to use. Could we get it a bit more generic? At least returning the type of the clock and its counter value. (kvmclock is a bit problematic for the generic solution, because it is TSC based, but should pass through the kvmclock value if we were going to make it clean ...) --- Actually, we might be overengineering it. With the master clock we have now, I'm thinking that the gtod is not crucial and we could as well be using something like: static bool kvm_get_time_and_clockread(s64 *kernel_ns, u64 *cycle_now) { local_irq_save(flags); *cycle_now = rdtsc(); *kernel_ns = ktime_get_boot_ns(); local_irq_restore(flags); return todo_tsc_hardware_is_good_enough(); } because kvmclock is shifted when master clock is recomputed or disabled, so it should never happen. (And if it happens, then this bit of imprecision doesn't matter much.) The host doesn't need to use TSC either, because master clock uses TSC frequency that differs from boot_ns -- we only need to be sure that the TSC hardware doesn't change its frequency. The small delta introduced with this (rdtsc <-> ktime_get_boot_ns) is the same that the guest would see if we didn't enable master clock. kvmclock updates are then normally using the same cycle_now/kernel_ns pair to remain stable.