From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiao Guangrong Subject: Re: [PATCH v2] KVM: MMU: Don't use RCU for lockless shadow walking Date: Fri, 27 Apr 2012 14:07:57 +0800 Message-ID: <4F9A37BD.8030700@linux.vnet.ibm.com> References: <1335260845-16271-1-git-send-email-avi@redhat.com> <20120426220000.GA30343@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Avi Kivity , kvm@vger.kernel.org To: Marcelo Tosatti Return-path: Received: from e28smtp01.in.ibm.com ([122.248.162.1]:53139 "EHLO e28smtp01.in.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751503Ab2D0GIE (ORCPT ); Fri, 27 Apr 2012 02:08:04 -0400 Received: from /spool/local by e28smtp01.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 27 Apr 2012 11:38:01 +0530 Received: from d28av03.in.ibm.com (d28av03.in.ibm.com [9.184.220.65]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q3R67x8h8781852 for ; Fri, 27 Apr 2012 11:37:59 +0530 Received: from d28av03.in.ibm.com (loopback [127.0.0.1]) by d28av03.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q3RBbEbN002218 for ; Fri, 27 Apr 2012 21:37:14 +1000 In-Reply-To: <20120426220000.GA30343@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On 04/27/2012 06:00 AM, Marcelo Tosatti wrote: >> static void walk_shadow_page_lockless_end(struct kvm_vcpu *vcpu) >> { >> - /* Decrease the counter after walking shadow page table finished */ >> - smp_mb__before_atomic_dec(); >> - atomic_dec(&vcpu->kvm->arch.reader_counter); >> - rcu_read_unlock(); >> + /* >> + * Make our reads and writes to shadow page tables globally visible >> + * before leaving READING_SHADOW_PAGE_TABLES mode. >> + */ > > This comment is misleading. Writes to shadow page tables must be > performed with locked instructions outside the mmu_lock. > You mean that the write should guarantee a correct memory order by itself? >> + smp_mb(); >> + vcpu->mode = OUTSIDE_GUEST_MODE; > > Don't you want > > vcpu->mode = OUTSIDE_GUEST_MODE; > smp_mb(); > It is unsafe i think, it is a problem if spte read / spte update is ordered to the behind of vcpu->mode = OUTSIDE_GUEST_MODE, like below: VCPU 0 VCPU 1 commit_zapped_page: /* * setting vcpu->mode is reordered * to the head of read spte. */ vcpu->mode = OUTSIDE_GUEST_MODE; see VCPU 0 is out-of-guest-mode, IPI is not sent, and the sp is free immediately. read spte; OOPS!!! (It is invalid since spte is freed.) smp_mb