Re: [PATCH 2/2] kvm: change the dirty page tracking to work with dirty bity

From: Ulrich Drepper <drepper@redhat.com>
To: Izik Eidus <ieidus@redhat.com>
Cc: kvm@vger.kernel.org, avi@redhat.com
Subject: Re: [PATCH 2/2] kvm: change the dirty page tracking to work with dirty bity
Date: Thu, 11 Jun 2009 01:24:47 -0700	[thread overview]
Message-ID: <4A30BF4F.70406@redhat.com> (raw)
In-Reply-To: <1244651005-18322-3-git-send-email-ieidus@redhat.com>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Izik Eidus wrote:
> +		if (!kvm_x86_ops->dirty_bit_support()) {
> +			spin_lock(&kvm->mmu_lock);
> +			/*  remove_write_access() flush the tlb */
> +			kvm_mmu_slot_remove_write_access(kvm, log->slot);
> +			spin_unlock(&kvm->mmu_lock);
> +		} else {
> +			kvm_flush_remote_tlbs(kvm);

It might not correspond to the common style, but I think a callback
function ->dirty_bit_support is overkill.  This is a function pointer
the compiler cannot see through.  Hence it's an indirect function call.
 But the implementation is always a simple yes/no (it seems).  Indirect
calls are rather expensive (most of the time they cannot be predicted
right).

Why not instead have a read-only data constants and have an inline
function test that value?  It means no function call and only one data
access.

Also, you're inconsistent in the use of integers and true/false in the
implementations of this function.  Either use 0/1 or false/true.

- --
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iEYEARECAAYFAkowv08ACgkQ2ijCOnn/RHR71ACdH3xr3XPnCLgsMMwdTawfehEN
vs4An2DlErhU6SeanSYVIyP3eLB4sjsz
=UZ32
-----END PGP SIGNATURE-----