From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [patch 09/10] KVM: MMU: out of sync shadow core v2 Date: Tue, 23 Sep 2008 13:46:23 +0300 Message-ID: <48D8C8FF.1040805@redhat.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, "David S. Ahern" To: Marcelo Tosatti Return-path: Received: from mx2.redhat.com ([66.187.237.31]:41234 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750899AbYIWKq1 (ORCPT ); Tue, 23 Sep 2008 06:46:27 -0400 In-Reply-To: <20080922215503.GA27744@dmt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: 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. 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. >>> 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) 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? -- error compiling committee.c: too many arguments to function