From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH 1/4] KVM: Add APIs for unlocked TLB flush Date: Wed, 9 May 2012 18:03:14 -0300 Message-ID: <20120509210314.GB26778@amt.cnet> References: <1336044182-12023-1-git-send-email-avi@redhat.com> <1336044182-12023-2-git-send-email-avi@redhat.com> <4FA286BD.2020009@linux.vnet.ibm.com> <4FA291F5.4090408@redhat.com> <20120508022513.GA26114@amt.cnet> <4FA9140F.6080601@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Xiao Guangrong , kvm@vger.kernel.org, takuya.yoshikawa@gmail.com To: Avi Kivity Return-path: Received: from mx1.redhat.com ([209.132.183.28]:4628 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758682Ab2EIVHw (ORCPT ); Wed, 9 May 2012 17:07:52 -0400 Content-Disposition: inline In-Reply-To: <4FA9140F.6080601@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, May 08, 2012 at 03:39:43PM +0300, Avi Kivity wrote: > On 05/08/2012 05:25 AM, Marcelo Tosatti wrote: > > > > > > > > We need call kvm_cond_flush_remote_tlbs in rmap_write-protect > > > > unconditionally? > > > > > > Yes, that's the point. We change > > > > > > spin_lock(mmu_lock) > > > conditionally flush the tlb > > > spin_unlock(mmu_lock) > > > > > > to > > > > > > spin_lock(mmu_lock) > > > conditionally mark the tlb as dirty > > > spin_unlock(mmu_lock) > > > kvm_cond_flush_remote_tlbs() > > > > > > but that means the entire codebase has to be converted. > > > > Is there any other site which expects sptes and TLBs to be in sync, > > other than rmap_write_protect? > > I wouldn't say rmap_write_protect() expects sptes and TLBs to be in > sync. Rather its callers. > > > Please convert the flush_remote_tlbs at the end of set_spte (RW->RO) to > > mark_dirty. > > I'd like to take an incremental approach, since there are many paths. I > don't have a concrete plan though. > > > Looks good in general (patchset is incomplete though). One thing that > > is annoying is that there is no guarantee of progress for flushed_count > > increment (it can, in theory, always race with a mark_dirty). But since > > that would be only a performance, and not correctness, aspect, it is > > fine. > > We don't have a while () look in cond_flush(), so it won't be slowed > down by the race; whoever caused the race will have to flush on their > own, but they do that anyway now. > > We can regress if vcpu 0 marks the tlb as dirty, and then vcpu 0 and 1 > simultaneously flush, even though vcpu 1 did nothing to deserve it. I > don't see a way around it except to hope its a rare event. > > > It has the advantage that it requires code to explicitly document where > > the TLB must be flushed and the sites which expect sptes to be in sync > > with TLBs. > > I'm looking for an idea of how to make the flush in those cases not hold > mmu_lock. You can't easily for rmap_write_protect, must check that the state is unchanged (write protect operation is still relevant). Current patchset (with corrections to comments by Xiao) is good enough already IMO.