From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH 2/2] KVM: Convert read-only users of vm_list to RCU Date: Thu, 10 Feb 2011 13:45:07 +0100 Message-ID: <4D53DDD3.5020704@siemens.com> References: <4D512EF7.8040409@siemens.com> <4D512F3B.1080107@siemens.com> <4D53BB02.20206@redhat.com> <4D53CCAB.8040204@siemens.com> <4D53DB54.90605@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , kvm , Zachary Amsden To: Avi Kivity Return-path: Received: from thoth.sbs.de ([192.35.17.2]:32396 "EHLO thoth.sbs.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751188Ab1BJMpV (ORCPT ); Thu, 10 Feb 2011 07:45:21 -0500 In-Reply-To: <4D53DB54.90605@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 2011-02-10 13:34, Avi Kivity wrote: > On 02/10/2011 01:31 PM, Jan Kiszka wrote: >>> >>>> >>>> @@ -3607,10 +3607,14 @@ static int mmu_shrink(struct shrinker *shrink, int nr_to_scan, gfp_t gfp_mask) >>>> spin_unlock(&kvm->mmu_lock); >>>> srcu_read_unlock(&kvm->srcu, idx); >>>> } >>>> - if (kvm_freed) >>>> - list_move_tail(&kvm_freed->vm_list,&vm_list); >>>> + if (kvm_freed) { >>>> + raw_spin_lock(&kvm_lock); >>>> + if (!kvm->deleted) >>>> + list_move_tail(&kvm_freed->vm_list,&vm_list); >>> >>> There is no list_move_tail_rcu(). >> >> ...specifically not for this one. > > Well, we can add one if needed (and if possible). I can have a look, at least at the lower hanging fruits. > >>> >>> Why check kvm->deleted? it's in the process of being torn down anyway, >>> it doesn't matter if mmu_shrink or kvm_destroy_vm pulls the trigger. >> >> kvm_destroy_vm removes a vm from the list while mmu_shrink is running. >> Then mmu_shrink's list_move_tail will re-add that vm to the list tail >> again (unless already the removal in move_tail produces a crash). > > It's too subtle. Communication across threads with a variable needs > memory barriers (even though they're nops on x86) and documentation. The barriers are provided by this spin lock we acquire for testing are modifying deleted. > > btw, not even sure if it's legal: you have a mutating call within an rcu > read critical section for the same object. If synchronize_rcu() were > called there, would it ever terminate? Why not? kvm_destroy_vm is not preventing blocking mmu_shrink to acquire the kvm_lock where we then find the vm deleted and release both kvm_lock and the rcu read "lock" afterwards. > > (not that synchronize_rcu() is a good thing there, better do it with > call_rcu()). What's the benefit? The downside is a bit more complexity as you need an additional callback handler. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux