Re: [PATCH 13/15] hotplug: introduce qdev_unplug_complete() to remove device from views

From: Marcelo Tosatti <mtosatti@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: kvm@vger.kernel.org, "Jan Kiszka" <jan.kiszka@siemens.com>,
	"liu ping fan" <qemulist@gmail.com>,
	qemu-devel@nongnu.org, "Blue Swirl" <blauwirbel@gmail.com>,
	"Avi Kivity" <avi@redhat.com>,
	"Anthony Liguori" <anthony@codemonkey.ws>,
	"Stefan Hajnoczi" <stefanha@gmail.com>,
	"Andreas Färber" <afaerber@suse.de>
Subject: Re: [PATCH 13/15] hotplug: introduce qdev_unplug_complete() to remove device from views
Date: Mon, 13 Aug 2012 15:51:58 -0300	[thread overview]
Message-ID: <20120813185158.GD25268@amt.cnet> (raw)
In-Reply-To: <50236E10.8030709@redhat.com>

On Thu, Aug 09, 2012 at 10:00:16AM +0200, Paolo Bonzini wrote:
> Il 09/08/2012 09:28, liu ping fan ha scritto:
> >> >     VCPU thread                    I/O thread
> >> > =====================================================================
> >> >     get MMIO request
> >> >     rcu_read_lock()
> >> >     walk memory map
> >> >                                    qdev_unmap()
> >> >                                    lock_devtree()
> >> >                                    ...
> >> >                                    unlock_devtree
> >> >                                    unref dev -> refcnt=0, free enqueued
> >> >     ref()
> > No ref() for dev here, while we have ref to flatview+radix in my patches.
> > I use rcu to protect radix+flatview+mr refered. As to dev, its ref has
> > inc when it is added into mem view -- that is
> > memory_region_add_subregion -> memory_region_get() {
> > if(atomic_add_and_return()) dev->ref++  }.
> > So not until reclaimer of mem view, the dev's ref is hold by mem view.
> > In a short word, rcu protect mem view, while device is protected by refcnt.

The idea, written on that plan, was:

- RCU protects memory maps.
- Object reference protects device in the window between 4. and 5.

The unplug/remove path should:

1) Lock memmap_lock for write (if not using RCU).
2) Remove any memmap entries (which is possible due to write lock on
memmap_lock. Alternatively wait for an RCU grace period). Device should
not be visible after that.
3) Lock dev->lock.
4) Wait until references are removed (no new references can be made
since device is not visible).
5) Remove device. 

So its a combination of both dev->lock and reference counter.

Note: a first step can be only parallel execution of MMIO lookups
(actually that is a very good first target). dev->lock above would be 
qemu_big_lock in that first stage, then _only devices which are 
performance sensitive need to be converted_.

> But the RCU critical section should not include the whole processing of
> MMIO, only the walk of the memory map.

Yes.

> And in general I think this is a bit too tricky... I understand not
> adding refcounting to all of bottom halves, timers, etc., but if you are
> using a device you should have explicit ref/unref pairs.
> 
> Paolo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html