From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH v2 3/4] KVM: cleanup: remove kvm_get_dirty_log() Date: Wed, 23 Jun 2010 21:58:59 -0300 Message-ID: <20100624005859.GD9388@amt.cnet> References: <20100623145807.07a07c4c.yoshikawa.takuya@oss.ntt.co.jp> <20100623150134.9fb5915b.yoshikawa.takuya@oss.ntt.co.jp> <4C21CA5D.4060800@redhat.com> <4C21CD59.9020000@oss.ntt.co.jp> <4C21CD3F.4030308@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Takuya Yoshikawa , agraf@suse.de, kvm@vger.kernel.org, kvm-ia64@vger.kernel.org, kvm-ppc@vger.kernel.org, takuya.yoshikawa@gmail.com To: Avi Kivity Return-path: Received: from mx1.redhat.com ([209.132.183.28]:23670 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754141Ab0FXBaj (ORCPT ); Wed, 23 Jun 2010 21:30:39 -0400 Content-Disposition: inline In-Reply-To: <4C21CD3F.4030308@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Jun 23, 2010 at 12:00:47PM +0300, Avi Kivity wrote: > On 06/23/2010 12:01 PM, Takuya Yoshikawa wrote: > >(2010/06/23 17:48), Avi Kivity wrote: > > > >>>diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c > >>>index 801d9f3..bea6f7c 100644 > >>>--- a/arch/powerpc/kvm/book3s.c > >>>+++ b/arch/powerpc/kvm/book3s.c > >>>@@ -1185,28 +1185,43 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, > >>>struct kvm_memory_slot *memslot; > >>>struct kvm_vcpu *vcpu; > >>>ulong ga, ga_end; > >>>- int is_dirty = 0; > >>>- int r; > >>>+ unsigned long is_dirty = 0; > >>>+ int r, i; > >>>unsigned long n; > >>> > >>>mutex_lock(&kvm->slots_lock); > >>> > >>>- r = kvm_get_dirty_log(kvm, log,&is_dirty); > >>>- if (r) > >>>+ r = -EINVAL; > >>>+ if (log->slot>= KVM_MEMORY_SLOTS) > >>>+ goto out; > >>>+ > >>>+ memslot =&kvm->memslots->memslots[log->slot]; > >> > >>Not introduced by this patch, but shouldn't this use rcu_dereference()? > >> > >> > > > >I was thinking like that, but sorry I don't know well about ppc. > > > >My final goal is to make everything except > > > > /* If nothing is dirty, don't bother messing with page tables. */ > > > >part arch independent, so that we can concentrate on the truely > >dirty logging things in the future. > > > >So, making ppc code same as x86, rcu_dereference() for example , really > >helps me. > > > > > >Do you like this approach? > > Well yes, I expect that not using rcu_dereference() (and > srcu_read_lock()) is a bug here. > > Marcelo? No because slots_lock is held which serializes with memslot updates. > On a related note, I'd like to consolidate rcu locking: > > - all vcpu ioctls take srcu_read_lock() when they start and drop it > at the end. > - KVM_VCPU_RUN drops the lock when sleeping and entering the guest > (also on context switch?) Yep, makes sense.