From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 1/4] KVM: Add APIs for unlocked TLB flush Date: Tue, 08 May 2012 15:39:43 +0300 Message-ID: <4FA9140F.6080601@redhat.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Xiao Guangrong , kvm@vger.kernel.org, takuya.yoshikawa@gmail.com To: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:39217 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754006Ab2EHMjs (ORCPT ); Tue, 8 May 2012 08:39:48 -0400 In-Reply-To: <20120508022513.GA26114@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: 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. -- error compiling committee.c: too many arguments to function