From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: KVM: guest: only batch user pte updates Date: Tue, 10 Feb 2009 20:41:41 -0200 Message-ID: <20090210224141.GA4471@amt.cnet> References: <20090210214532.GA4082@amt.cnet> <4991FD0D.1070108@goop.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Avi Kivity , kvm-devel , Rusty Russell , Zachary Amsden To: Jeremy Fitzhardinge Return-path: Received: from mx2.redhat.com ([66.187.237.31]:44906 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757290AbZBJWmX (ORCPT ); Tue, 10 Feb 2009 17:42:23 -0500 Content-Disposition: inline In-Reply-To: <4991FD0D.1070108@goop.org> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Feb 10, 2009 at 02:17:49PM -0800, Jeremy Fitzhardinge wrote: > Marcelo Tosatti wrote: >> KVM's paravirt mmu pte batching has issues with, at least, kernel >> updates from DEBUG_PAGEALLOC. >> >> This has been experienced with slab allocation from irq context from >> within lazy mmu sections: >> >> https://bugzilla.redhat.com/show_bug.cgi?id=480822 >> >> DEBUG_PAGEALLOC will map/unmap the kernel pagetables to catch bad >> accesses, with code such as: >> >> __change_page_attr(): >> >> /* >> * Do we really change anything ? >> */ >> if (pte_val(old_pte) != pte_val(new_pte)) { >> set_pte_atomic(kpte, new_pte); >> cpa->flags |= CPA_FLUSHTLB; >> } >> >> A present->nonpresent update can be queued, but not yet committed to >> memory. So the set_pte_atomic will be skipped but the update flushed >> afterwards. set_pte_ATOMIC. >> > > Are you saying that there's a queued update which means that old_pte is > a stale value which happens to equal new_pte, so new_pte is never set? > OK, sounds like a generic problem, of the same sort we've had with > kmap_atomic being used in interrupt routines in lazy mode. Yes. It seems however that only set_pte_at/pte_update/_defer are used under significatly long lazy mmu sections (long as in number of updates). Is it worthwhile to bother (and risk) batching kernel pte updates ? Until someone forgets about arch_flush_lazy_mmu_mode again... > In this case, I think the proper fix is to call > arch_flush_lazy_mmu_mode() before reading old_pte to make sure its up to > date, and calling it again when processing CPA_FLUSHTLB. > Could you try the patch below instead? It should work yes. > (BTW, set_pte_atomic doesn't mean synchronous; it just means its safe to > use on live ptes on 32-bit PAE machines which can't otherwise atomically > update a pte.) Doh, of course.