From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [patch 09/10] KVM: MMU: out of sync shadow core v2 Date: Tue, 23 Sep 2008 10:17:19 -0300 Message-ID: <20080923131719.GA5038@dmt.cnet> References: <20080918212749.800177179@localhost.localdomain> <20080918213337.148804603@localhost.localdomain> <48D4506C.5070804@redhat.com> <20080921004515.GC10120@dmt.cnet> <48D802EA.9070807@redhat.com> <20080922215503.GA27744@dmt.cnet> <48D8C8FF.1040805@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, "David S. Ahern" To: Avi Kivity Return-path: Received: from mx2.redhat.com ([66.187.237.31]:43790 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751354AbYIWNTY (ORCPT ); Tue, 23 Sep 2008 09:19:24 -0400 Content-Disposition: inline In-Reply-To: <48D8C8FF.1040805@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Sep 23, 2008 at 01:46:23PM +0300, Avi Kivity wrote: > Marcelo Tosatti wrote: >>>> >>> I don't understand how the variables sp, child, and parent interact. >>> You either need recursion or an explicit stack? >>> >> >> It restarts at parent level whenever finishing any children: >> >> + if (i == PT64_ENT_PER_PAGE) { >> + sp->unsync_children = 0; >> + sp = parent; >> + } >> >> No efficiency. >> >> > > Oh okay. 'parent' is never assigned to. Lack of concentration. > >>>> Yes. The next element for_each_entry_safe saved could have been zapped. >>>> >>>> >>> Ouch. Ouch. >>> >>> I hate doing this. Can see no alternative though. >>> >> >> Me neither. >> >> > > Well. But I don't see kvm_mmu_zap_page()'s return value used anywhere. It is. List walk becomes unsafe otherwise. > Actually, I think I see an alternative: set the invalid flag on these > pages and queue them in a list, like we do for roots in use. Flush the > list on some cleanup path. Yes, it is an alternative. But then you would have to test for the invalid flag on all those paths that currently test for kvm_mmu_zap_page return value. I'm not sure if thats any better? >>>> Windows 2008 64-bit has all sorts of sharing a pagetable at multiple >>>> levels too. >>>> >>>> >>> We still want to allow oos for the two quadrants of a nonpae shadow page. >>> >> >> Sure, can be an optimization step later? >> > > I'd like to reexamine this from another angle: what if we allow oos of > any level? > > This will simplify the can_unsync path (always true) The can_unsync flag is there to avoid the resync path (mmu_unsync_walk->kvm_sync_page) from unsyncing pages of the root being synced. Say, if at every resync you end up unsyncing two pages (unlikely but possible). However, we can probably get rid of it the bitmap walk (which won't restart the walk from the beginning). > and remove a special case. The cost is implementing invlpg and resync > for non-leaf pages (invlpg has to resync the pte for every level). Are > there other problems with this? There is no gfn cache for non-leaf pages, so you either need to introduce it or go for gfn_to_page_atomic-like functionality (expensive). I was hoping to look into non-leaf unsync to be another "for later" optimization step, if found to be worthwhile.