From mboxrd@z Thu Jan  1 00:00:00 1970
From: Izik Eidus <ieidus@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
Message-ID: <4A30D1E9.3090106@redhat.com>
References: <1244651005-18322-1-git-send-email-ieidus@redhat.com> <1244651005-18322-2-git-send-email-ieidus@redhat.com> <1244651005-18322-3-git-send-email-ieidus@redhat.com> <4A30BF4F.70406@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
	format=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE
Cc: kvm@vger.kernel.org, avi@redhat.com
To: Ulrich Drepper <drepper@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx2.redhat.com ([66.187.237.31]:56501 "EHLO mx2.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753949AbZFKJoF (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 11 Jun 2009 05:44:05 -0400
Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26])
	by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n5B9i8mm025793
	for <kvm@vger.kernel.org>; Thu, 11 Jun 2009 05:44:08 -0400
In-Reply-To: <4A30BF4F.70406@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Ulrich Drepper wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Izik Eidus wrote:
>  =20
>> +		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);
>>    =20
>
>  =20

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 cal=
l.
>  But the implementation is always a simple yes/no (it seems).  Indire=
ct
> calls are rather expensive (most of the time they cannot be predicted
> right).
>  =20

This function pointer will be called once every ioctl to get the dirty=20
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 dat=
a
> access.
>  =20

May be relevent, but i dont sure if it is needed optimization for this=20
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 th=
e
> implementations of this function.  Either use 0/1 or false/true.
>  =20

I will fix it, thanks.

> - --
> =E2=9E=A7 Ulrich Drepper =E2=9E=A7 Red Hat, Inc. =E2=9E=A7 444 Castro=
 St =E2=9E=A7 Mountain View, CA =E2=9D=96
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
>
> iEYEARECAAYFAkowv08ACgkQ2ijCOnn/RHR71ACdH3xr3XPnCLgsMMwdTawfehEN
> vs4An2DlErhU6SeanSYVIyP3eLB4sjsz
> =3DUZ32
> -----END PGP SIGNATURE-----
>  =20