Re: [PATCH 3/3] KVM: MMU: Make drop_large_spte() more generic

[Avi Kivity <avi@redhat.com>]

On 11/29/2011 07:04 AM, Takuya Yoshikawa wrote:
> There are many places where we drop large spte and we are always doing
> much the same as drop_large_spte().
>
> To avoid these duplications, this patch makes drop_large_spte() more
> generically usable: it now takes an argument to know if it must flush
> the remote tlbs and returns true or false depending on is_large_pte()
> check result.

> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 5e761ff..2db12b3 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -1023,6 +1023,19 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
>  		rmap_remove(kvm, sptep);
>  }
>  
> +static bool drop_large_spte(struct kvm *kvm, u64 *sptep, bool flush_now)
> +{
> +	if (!is_large_pte(*sptep))
> +		return false;
> +
> +	drop_spte(kvm, sptep);
> +	--kvm->stat.lpages;
> +
> +	if (flush_now)
> +		kvm_flush_remote_tlbs(kvm);
> +	return true;
> +}

I don't really like the pattern of adding more bools and conditionals
all over the place.  It makes the code harder to read and to execute.

I prefer separate functions like drop_large_spte() and
drop_large_spte_flush().

But it may be better to just drop kvm->stat, and open-code everything,
something like

   if (is_large_pte(*sptep))
       drop_spte(sptep)

is just as clear as drop_large_spte().

> @@ -1839,9 +1842,8 @@ static bool mmu_page_zap_pte(struct kvm *kvm, struct kvm_mmu_page *sp,
>  	pte = *spte;
>  	if (is_shadow_present_pte(pte)) {
>  		if (is_last_spte(pte, sp->role.level)) {
> -			drop_spte(kvm, spte);
> -			if (is_large_pte(pte))
> -				--kvm->stat.lpages;
> +			if (!drop_large_spte(kvm, spte, false))
> +				drop_spte(kvm, spte);

Now this is just confusing.


-- 
error compiling committee.c: too many arguments to function