Re: [PATCH v3 2/6] KVM: x86: switch to masterclock update using timekeeper functionality

[Denis Plotnikov <dplotnikov@virtuozzo.com>]

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