From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [patch 1/3] KVM: MMU: collapse remote TLB flushes on root sync Date: Sun, 26 Oct 2008 13:17:14 +0200 Message-ID: <490451BA.8040800@redhat.com> References: <20081025223111.498934405@localhost.localdomain> <20081025223243.782692567@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org To: Marcelo Tosatti Return-path: Received: from mx2.redhat.com ([66.187.237.31]:35530 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751703AbYJZLRV (ORCPT ); Sun, 26 Oct 2008 07:17:21 -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 m9QBHK63001850 for ; Sun, 26 Oct 2008 07:17:20 -0400 In-Reply-To: <20081025223243.782692567@localhost.localdomain> Sender: kvm-owner@vger.kernel.org List-ID: Marcelo Tosatti wrote: > Instead of flushing remote TLB's at every page resync, do an initial > pass to write protect the sptes, collapsing the flushes on a single > remote TLB invalidation. > > kernbench is 2.3% faster on 4-way guest. Improvements have been seen > with other loads such as AIM7. > > Avi: feel free to change this if you dislike the style (I do, but can't > think of anything nicer). > > static void mmu_sync_children(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp) > { > struct sync_walker walker = { > - .walker = { .entry = mmu_sync_fn, }, > + .walker = { .entry = mmu_wprotect_fn, > + .clear_unsync = false, }, > .vcpu = vcpu, > + .write_protected = false > }; > > + /* collapse the TLB flushes as an optimization */ > + mmu_unsync_walk(sp, &walker.walker); > + if (walker.write_protected) > + kvm_flush_remote_tlbs(vcpu->kvm); > + > + walker.walker.entry = mmu_sync_fn; > + walker.walker.clear_unsync = true; > + > while (mmu_unsync_walk(sp, &walker.walker)) > cond_resched_lock(&vcpu->kvm->mmu_lock); > We're always doing two passes here, which is a bit sad. How about having a single pass which: - collects unsync pages into an array - exits on no more unsync pages or max array size reached Then, iterate over the array: - write protect all pages - flush tlb - sync pages Loop until the root is synced. If the number of pages to sync is typically small, and the array is sized to be larger than this, then we only walk the pagetables once. btw, our walkers are a bit awkward (though still better than what we had before). If we rewrite them into for_each style iterators, the code could become cleaner and shorter. -- error compiling committee.c: too many arguments to function