From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: KVM: x86: update masterclock on TSC writes
Date: Tue, 4 Nov 2014 21:27:27 -0200
Message-ID: <20141104232727.GA29191@amt.cnet>
References: <20141103211627.GA19724@amt.cnet>
 <5459088A.50801@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm-devel <kvm@vger.kernel.org>
To: Paolo Bonzini <pbonzini@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:40617 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751447AbaKDXa7 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 4 Nov 2014 18:30:59 -0500
Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26])
	by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sA4NUvSF031712
	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL)
	for <kvm@vger.kernel.org>; Tue, 4 Nov 2014 18:30:58 -0500
Content-Disposition: inline
In-Reply-To: <5459088A.50801@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.