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
