Re: KVM: x86: update masterclock on TSC writes

[Marcelo Tosatti <mtosatti@redhat.com>]

On Tue, Nov 04, 2014 at 06:10:34PM +0100, Paolo Bonzini wrote:
> Extending the context we have:
> >         if (vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC)
> >  		if (!ka->use_master_clock)
> >  			do_request = 1;
> >  
> > -	if (!vcpus_matched && ka->use_master_clock)
> > +	if (ka->use_master_clock)
> >  			do_request = 1;
> >  
> >         if (do_request)
> >                 kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
> 
> The patch also makes the previous "if (!ka->use_master_clock)" redundant.
> If you enter the first "if", do_request will be 1 independent of
> ka->use_master_clock.  So you should also drop that one, and possibly
> rewrite it simply like this:
> 
> 	if ((vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) ||
> 	    ka->use_master_clock)
> 		kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
> 
> But this brings the question: what is vclock_mode in your case?  If it
> is VCLOCK_TSC, are you sure that the bug is fixed because you modified
> the second "if", or could it be fixed also by removing instead the
> "if (!ka->use_master_clock)"?  This would leave the optimization in the
> case "!vcpus_matched && ka->use_master_clock".  Or is the optimization
> always invalid?

The bug is fixed by always updating masterclock values, when it is
enabled.

> A different way to state the same question: can you explain the
> resulting condition
> 
> 	((vcpus_matched && gtod->clock.vclock_mode == VCLOCK_TSC) ||
> 	 ka->use_master_clock)
> 
> ?  Please add a comment to kvm_track_tsc_matching that clarifies this
> logic.
> 
> Paolo

Sure, sending v2.