Re: [PATCH 2/2] KVM: Convert read-only users of vm_list to RCU

[Avi Kivity <avi@redhat.com>]

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).

> >
> >  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.

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?

(not that synchronize_rcu() is a good thing there, better do it with 
call_rcu()).

-- 
error compiling committee.c: too many arguments to function