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
