Re: [PATCH RESEND] KVM: MMU: Fix mmu_shrink() so that it can free mmu pages as intended

[Takuya Yoshikawa <takuya.yoshikawa@gmail.com>]

On Tue, 14 Aug 2012 12:17:12 -0300
Marcelo Tosatti <mtosatti@redhat.com> wrote:

> -               if (kvm->arch.n_used_mmu_pages > 0) {
> -                       if (!nr_to_scan--)
> -                               break;

-- (*1)

> +               if (!kvm->arch.n_used_mmu_pages)
>                         continue;

-- (*2)

> -               }
> 
>                 idx = srcu_read_lock(&kvm->srcu);
>                 spin_lock(&kvm->mmu_lock);
> 
> This patch removes the maximum (successful) loops, which is nr_scan ==
> sc->nr_to_scan.

IIUC, there was no successful loop from the beginning:

  if (kvm->arch.n_used_mmu_pages > 0) {
    if (!nr_to_scan--)
      break;  -- (*1)
    continue; -- (*2)
  }

Before the patch even when we find a VM with kvm->arch.n_used_mmu_pages
greater than 0, we just do either:
  skip it (*2) or
  break   (*1) if nr_to_scan becomes 0.

We only reach to
  kvm_mmu_remove_some_alloc_mmu_pages(kvm, &invalid_list);
  kvm_mmu_commit_zap_page(kvm, &invalid_list);
when (kvm->arch.n_used_mmu_pages == 0) that is probably why 

  commit 85b7059169e128c57a3a8a3e588fb89cb2031da1
  KVM: MMU: fix shrinking page from the empty mmu

could hit the very unlikely condition so easily.

So we are just looping for trying to free from empty MMUs.

> The description above where you say 'possibility that we see
> "n_used_mmu_pages == 0" 128 times' does not match the patch above.

Sorry about that.

> If the patch is correct, then please explain it clearly in the
> changelog.

Yes, I will do so.

> What is the reasoning to remove nr_to_scan? What tests did you perform?

I just confirmed:
 - mmu_shrink() did not free any pages:
   just checked all VMs and did "continue"
 - with my patch, it could free from the first VM with (n_used_mmu_pages > 0)

About nr_to_scan:
If my explanation above is right, this is not functioning at all.
But since it will not hurt anyone and may help us when we change
our batch size, I won't remove it in the next version.

Thanks,
	Takuya