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

From: Izik Eidus <ieidus@redhat.com>
To: Ulrich Drepper <drepper@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 12:44:09 +0300	[thread overview]
Message-ID: <4A30D1E9.3090106@redhat.com> (raw)
In-Reply-To: <4A30BF4F.70406@redhat.com>

Ulrich Drepper wrote:
> -----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);
>>     
>
>   

Hi.

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

This function pointer will be called once every ioctl to get the dirty 
bit tracking, so i dont think it is a big issue (normal 30 times a sec)

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

May be relevent, but i dont sure if it is needed optimization for this 
patch consider the amount of time ->dirty_bit_support() will be called

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

I will fix it, thanks.

> - --
> ➧ 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-----
>   