From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 1/2] KVM: mmu_notifier: Flush TLBs before releasing mmu_lock Date: Wed, 15 Feb 2012 16:07:49 +0200 Message-ID: <4F3BBC35.6010502@redhat.com> References: <20120210152831.6ac3ac87.yoshikawa.takuya@oss.ntt.co.jp> <20120214171044.GK9440@redhat.com> <20120214172947.GA22362@amt.cnet> <20120214185356.GQ9440@redhat.com> <20120214194342.GA24117@amt.cnet> <4F3B7867.7000807@redhat.com> <4F3B7F1F.40402@redhat.com> <4F3B9902.5000306@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , Andrea Arcangeli , Takuya Yoshikawa , kvm@vger.kernel.org To: Xiao Guangrong Return-path: Received: from mx1.redhat.com ([209.132.183.28]:2150 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754406Ab2BOOH6 (ORCPT ); Wed, 15 Feb 2012 09:07:58 -0500 In-Reply-To: <4F3B9902.5000306@linux.vnet.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: On 02/15/2012 01:37 PM, Xiao Guangrong wrote: > >> > >> I would really like to move the IPI back out of the lock. > >> > >> How about something like a sequence lock: > >> > >> > >> spin_lock(mmu_lock) > >> need_flush = write_protect_stuff(); > >> atomic_add(kvm->want_flush_counter, need_flush); > >> spin_unlock(mmu_lock); > >> > >> while ((done = atomic_read(kvm->done_flush_counter)) < (want = > >> atomic_read(kvm->want_flush_counter)) { > >> kvm_make_request(flush) > >> atomic_cmpxchg(kvm->done_flush_counter, done, want) > >> } > >> > >> This (or maybe a corrected and optimized version) ensures that any > >> need_flush cannot pass the while () barrier, no matter which thread > >> encounters it first. However it violates the "do not invent new locking > >> techniques" commandment. Can we map it to some existing method? > > > > There is no need to advance 'want' in the loop. So we could do > > > > /* must call with mmu_lock held */ > > void kvm_mmu_defer_remote_flush(kvm, need_flush) > > { > > if (need_flush) > > ++kvm->flush_counter.want; > > } > > > > /* may call without mmu_lock */ > > void kvm_mmu_commit_remote_flush(kvm) > > { > > want = ACCESS_ONCE(kvm->flush_counter.want) > > while ((done = atomic_read(kvm->flush_counter.done) < want) { > > kvm_make_request(flush) > > atomic_cmpxchg(kvm->flush_counter.done, done, want) > > } > > } > > > > > Hmm, we already have kvm->tlbs_dirty, so, we can do it like this: > > #define SPTE_INVALID_UNCLEAN (1 << 63 ) > > in invalid page path: > lock mmu_lock > if (spte is invalid) > kvm->tlbs_dirty |= SPTE_INVALID_UNCLEAN; > need_tlb_flush = kvm->tlbs_dirty; > unlock mmu_lock > if (need_tlb_flush) > kvm_flush_remote_tlbs() > > And in page write-protected path: > lock mmu_lock > if (it has spte change to readonly | > kvm->tlbs_dirty & SPTE_INVALID_UNCLEAN) > kvm_flush_remote_tlbs() > unlock mmu_lock > > How about this? Well, it still has flushes inside the lock. And it seems to be more complicated, but maybe that's because I thought of my idea and didn't fully grok yours yet. -- error compiling committee.c: too many arguments to function