Message-ID: <7b886c9f-70a7-951b-ff6f-f77f263754e7@redhat.com>
Date:   Tue, 26 Sep 2017 18:51:50 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Denis Plotnikov <dplotnikov@...tuozzo.com>
Cc:     rkrcmar@...hat.com, kvm@...r.kernel.org, john.stultz@...aro.org,
        mingo@...hat.com, hpa@...or.com, linux-kernel@...r.kernel.org,
        x86@...nel.org, rkagan@...tuozzo.com, den@...tuozzo.com
Subject: Re: [PATCH v5 1/6] timekeeper: introduce extended clocksource reading
 callback

On 26/09/2017 00:00, Thomas Gleixner wrote:
>> + * @read_with_stamp:	saves cycles value (got from timekeeper) and cycles
>> + *			stamp (got from hardware counter value and used by
>> + *			timekeeper to calculate the cycles value) to
>> + *			corresponding input pointers return true if cycles
>> + *			stamp holds real cycles and false if it holds some
>> + *			time derivative value
>
> Neither the changelog nor this comment make any sense. Not to talk about
> the actual TSC side implementation which does the same as tsc_read() - it
> actually uses tsc_read() - but stores the same value twice and
> unconditionally returns true.
> 
> Unless I'm missing something important all of this can be achieved with a
> minimal and actually understandable patch. See below.

If that is acceptable, that's certainly okay for us too as a start, in
order to clean up the KVM code.

*However*, I must once more ask for help understanding
ktime_get_snapshot, otherwise there is no way that I can start using it
in kvmclock.

In particular I'd like to understand the contract of ktime_get_snapshot,
namely the meaning of the "cycles" field in struct system_time_snapshot.

Does it have to be system clock cycles, like TSC, or can it be just the
value of the clock source?  And if the latter, are the users broken,
because they can receive any random clocksource output in ->cycles?  It
doesn't help that, for all of the users of ->cycles (which are all
reached from get_device_system_crosststamp), the code that actually uses
the member is never called in the current git master.

I asked these questions to John in the previous review, but got no
answer.  My understanding is that get_device_system_crosststamp is
broken for !tsc clocksource.  Because struct system_counterval_t must
have a TSC value, history_begin needs to contain a cross-timestamp of
the TSC (in ->cycles) and the clock (in ->real and ->raw).  And if this
cross-timestamp is not available, get_device_system_crosststamp needs to
know, so that it doesn't use history_begin at all.

Now, such cross-timestamp functionality is exactly what
"read_with_stamp" provides.  Patch 3 also introduces "->cycles_valid" in
struct system_time_snapshot, to report whether the cross-timestamp is
available.  In the case of the TSC clocksource, of course, the two
values of course are the same, and always available (so the function
always returns true).

However, if get_device_system_crosststamp ran with kvmclock or Hyper-V
clocksource, the two values stored by read_with_stamp would be
different, basically a (TSC, nanoseconds) pair.  ktime_get_snapshot
would then return the TSC in ->cycles, and use the nanoseconds value to
compute ->real and ->raw.  Because the cross timestamp is available,
->cycles_valid would be true.

So, if history_begin _is_ broken, after fixing it (with
"read_with_stamp" or otherwise) ktime_get_snapshot would provide KVM
with everything it needs.  If not, I'd be completely off base, and I'd
have to apologize to Denis for screwing up.

Paolo

Powered by blists - more mailing lists