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

[Denis Plotnikov <dplotnikov@virtuozzo.com>]

On 31.07.2017 17:20, Paolo Bonzini wrote:
> On 29/07/2017 14:35, Denis Plotnikov wrote:
>>   arch/x86/kernel/kvmclock.c  | 14 ++++++++++++--
>>   arch/x86/kernel/tsc.c       |  6 ++++++
>>   arch/x86/kvm/x86.c          | 26 ++++++++++++++++++--------
>>   include/linux/clocksource.h |  3 +++
>>   include/linux/timekeeping.h |  2 ++
>>   kernel/time/timekeeping.c   | 21 +++++++++++++++++++--
>>   6 files changed, 60 insertions(+), 12 deletions(-)
> 
> This is pretty clean, thanks.  Only it should be split in several patches:
> 
> - introducing read_with_cycles and using it in ktime_get_snapshot.
> Looking at the code, the meaning of "cycles" is overloaded, so perhaps
> rename it to read_clock_and_systime or something similar?
>
agree
> - introducing boot time in ktime_get_snapshot
agree
> 
> - 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
> 
> - adding the cs_stable field to struct system_time_snapshot (see below,
> maybe this can be merged with read_with_cycles)
agree
> 
> - 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.
> 
> so that the timekeeping maintainer can comment on each new feature you
> add to their code.
> 
> cs_stable is the part that I'm still a bit wary of; here are the doubts
> I have:
> 
> - if you want stability, you can use the CLOCK_SOURCE_UNSTABLE flag; a
> new callback shouldn't be needed (it's certainly not needed for TSC).
> 
> - the meaning of "stable" for kvmclock is not exactly the same as
> clocksource_mark_unstable.
> 
> 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". As you mentioned 
it's irrelevant for tsc but makes sense to kvmclock (and possibly some 
other clocksources). I think it's reasonable to ask a clocksource 
directly whether it's stable or not. That's why I made the callback.
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