From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH v3 2/6] KVM: x86: switch to masterclock update using timekeeper functionality Date: Tue, 1 Aug 2017 12:03:01 +0200 Message-ID: <82c81e37-9c56-26b9-d8c5-cf87eeb1d470@redhat.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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: rkagan@virtuozzo.com, den@virtuozzo.com, svt-core@lists.sw.ru To: Denis Plotnikov , rkrcmar@redhat.com, kvm@vger.kernel.org Return-path: Received: from mail-wr0-f196.google.com ([209.85.128.196]:37096 "EHLO mail-wr0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751012AbdHAKDE (ORCPT ); Tue, 1 Aug 2017 06:03:04 -0400 Received: by mail-wr0-f196.google.com with SMTP id 12so953387wrb.4 for ; Tue, 01 Aug 2017 03:03:03 -0700 (PDT) In-Reply-To: <80e6cc1b-bccb-6e5b-1d3a-28a54e564d6c@virtuozzo.com> Content-Language: en-US Sender: kvm-owner@vger.kernel.org List-ID: 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 > 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); >>> >> >