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

[Paolo Bonzini <pbonzini@redhat.com>]

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?

- introducing boot time in ktime_get_snapshot

- implementing kvm_clock_read_with_cycles (can be merged with patch 6)

- adding the cs_stable field to struct system_time_snapshot (see below,
maybe this can be merged with read_with_cycles)

- using ktime_get_snapshot in KVM (can be merged with patch 4?)

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).

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);
>  
>