Re: [PATCH v5 3/8] KVM: MMU: fast invalidate all pages

[Gleb Natapov <gleb@redhat.com>]

On Thu, May 16, 2013 at 03:14:35PM +0200, Paolo Bonzini wrote:
> Il 16/05/2013 14:43, Gleb Natapov ha scritto:
> >> > +restart:
> >> > +	list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
> >> > +		if (!is_obsolete_sp(kvm, sp))
> >> > +			continue;
> > What if we save kvm->arch.active_mmu_pages on the stack and init
> > kvm->arch.active_mmu_pages to be empty at the entrance to
> > zap_invalid_pages(). This loop will iterate over saved list. This will
> > allow us to drop the is_obsolete_sp() check and will save time since we
> > will not be iterating over newly created sps.
> > 
> 
> But when you add cond_resched_lock a thread may want to zap pages itself
> (e.g. from prepare_zap_oldest_mmu_page) and it won't find them.
> 
Yes, this will break mmu pages accounting. We can make
prepare_zap_oldest_mmu_page() wait while zap_invalid_pages()
frees needed amount of pages if one is in progress.

> Here is another proposal...  The idea is to avoid looking at new pages
> more than necessary after a "goto restart".
> 
> Basically, you alternate between two phases:
> 
> - look for pages to be zapped, group them together
> 
> - zap the pages
> 
> Something like:
> 
>       moved = 0;
> restart:
>       zapping = true;
>       for each page in active_mmu_pages [reverse and safe] {
>              if (!is_obsolete || invalid) {
>                  /*
>                   * Found a new page, stop zapping for now and
>                   * try to segregate the invalid ones at one end
>                   * of the list.
>                   */
>                  zapping = false;
>                  continue;
>              }
> 
>              if (batch > 10 && ...) {
>                  cond_resched_lock
>                  batch = 0;
>                  goto restart;
>              }
> 
>              if (!zapping) {
>                  /*
>                   * Segregate pages to one end of the list where
>                   * new pages don't get in the way.
>                   */
>                  list_move_tail(page, active_mmu_pages)
>                  batch++; /* or maybe not? */
>                  moved++;
>              } else {
>                  batch += prepare_zap_page
>                  goto restart;
>              }
>      }
> 
>      /* Need another pass to look at segregated pages?  */
>      if (moved) {
>          moved = 0;
>          goto restart;
>      }
Not sure what are you trying to achieve with "moved" tricks. Just
walking the list from the end and stopping on first valid sp should be
enough since active_mmu_pages list is a FIFO right now.

--
			Gleb.