From mboxrd@z Thu Jan 1 00:00:00 1970 From: Denis Plotnikov Subject: Re: [PATCH v3 2/6] KVM: x86: switch to masterclock update using timekeeper functionality Date: Tue, 1 Aug 2017 15:11:10 +0300 Message-ID: <616b4427-49a9-7815-56c3-9171248d9673@virtuozzo.com> References: <1501331711-12961-1-git-send-email-dplotnikov@virtuozzo.com> <1501331711-12961-3-git-send-email-dplotnikov@virtuozzo.com> <80e6cc1b-bccb-6e5b-1d3a-28a54e564d6c@virtuozzo.com> <82c81e37-9c56-26b9-d8c5-cf87eeb1d470@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: rkagan@virtuozzo.com, den@virtuozzo.com, svt-core@lists.sw.ru To: Paolo Bonzini , rkrcmar@redhat.com, kvm@vger.kernel.org Return-path: Received: from mail-eopbgr10119.outbound.protection.outlook.com ([40.107.1.119]:24818 "EHLO EUR02-HE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751910AbdHAMLU (ORCPT ); Tue, 1 Aug 2017 08:11:20 -0400 In-Reply-To: <82c81e37-9c56-26b9-d8c5-cf87eeb1d470@redhat.com> Content-Language: en-US Sender: kvm-owner@vger.kernel.org List-ID: On 01.08.2017 13:03, Paolo Bonzini wrote: > On 01/08/2017 11:30, Denis Plotnikov wrote: >>> - implementing kvm_clock_read_with_cycles (can be merged with patch 6) >> >> Having a stable clocksource for kvmklock means making kvmclock stable. >> The patch enables this functionality that's why I'd prefer to keep patch >> 6 separate > > Ok, though it depends on how you deal with cs_stable. > >>> - using ktime_get_snapshot in KVM (can be merged with patch 4?) >> >> agree, but want to keep 4 separate. Just to make the changes done >> logically consecutive in git tree. > > Fine by me. > >>> Maybe what we want is some kind of "bool cycles_valid", and then >>> read_clock_and_systime can return it: >>> >>> >>> if (clock->read_clock_and_systime) { >>> systime_snapshot->cycles_valid = >>> clock->read_clock_and_systime( >>> &now, &systime_snapshot->cycles); >>> } else { >>> now = tk_clock_read(&tk->tkr_mono); >>> systime_snapshot->cycles_valid = true; >>> systime_snapshot->cycles = now; >>> } >>> >>> ? (This is honestly just a suggestion, which may be wrong depedning >>> on the answer to the two questions above). >> >> cs_stable means "there is no unexpected time jumps". > > But even for kvmclock there are no unexpected time jumps, the global > accumulator in pvclock_clocksource_read ensures that. And the concept > is different from CLOCK_SOURCE_UNSTABLE which will basically blacklist > the clocksource for hrtimers. > > It seems a different concept to me, somewhat specific to > ktime_get_snapshot. More precisely, the question is "is there a 1:1 > mapping from cycles to nanoseconds?"---but if there is no such mapping > cycles is useless, hence my proposal of "cycles_valid". > > Thanks, > > Paolo In fact, this "cycles_valid" is going to be used for deciding whether to use KVM masterclock or not. And if it's not we still want to know cycles_stamp value to use it in KVM. So the cycles is valid, but clocksource is not reliable (why? let decide to a clocksource, by default assume they are all not stable), thus we must calculate time values for a guest each time its needed. So, my proposal is to name the variable sightly differently: cs_reliable and go like: if (clock->read_clock_with_stamp) { systime_snapshot->cs_reliable = clock->read_clock_with_stamp( &now, &systime_snapshot->cycles); } else { now = tk_clock_read(&tk->tkr_mono); systime_snapshot->cs_reliable = false; systime_snapshot->cycles = now; } What do you think? Thanks! Denis > >> I don't mind merging this "check stability" functionality with >> read_clock_and_systime. Actually, I thought about having it there but >> eventually split it to make the code more explicit >> (detailed and understandable). >> I'd like to use your approach but keep the variable name the same. >> >> Thanks for reviewing! >> >> Denis >>> >>> Paolo >>> >>>> systime_snapshot->cs_was_changed_seq = tk->cs_was_changed_seq; >>>> systime_snapshot->clock_was_set_seq = tk->clock_was_set_seq; >>>> base_real = ktime_add(tk->tkr_mono.base, >>>> tk_core.timekeeper.offs_real); >>>> base_raw = tk->tkr_raw.base; >>>> + base_boot = ktime_add(tk->tkr_mono.base, >>>> + tk_core.timekeeper.offs_boot); >>>> nsec_real = timekeeping_cycles_to_ns(&tk->tkr_mono, now); >>>> nsec_raw = timekeeping_cycles_to_ns(&tk->tkr_raw, now); >>>> } while (read_seqcount_retry(&tk_core.seq, seq)); >>>> - systime_snapshot->cycles = now; >>>> systime_snapshot->real = ktime_add_ns(base_real, nsec_real); >>>> systime_snapshot->raw = ktime_add_ns(base_raw, nsec_raw); >>>> + systime_snapshot->boot = ktime_add_ns(base_boot, nsec_real); >>>> } >>>> EXPORT_SYMBOL_GPL(ktime_get_snapshot); >>>> >>> >> > -- Best, Denis