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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
