Re: [PATCH 13/15] hotplug: introduce qdev_unplug_complete() to remove device from views
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 threadI/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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/15] hotplug: introduce qdev_unplug_complete() to remove device from views
On Fri, Aug 10, 2012 at 02:42:58PM +0800, liu ping fan wrote:
On Thu, Aug 9, 2012 at 4:00 PM, Paolo Bonzini pbonz...@redhat.com wrote:
Il 09/08/2012 09:28, liu ping fan ha scritto:
VCPU threadI/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.
But the RCU critical section should not include the whole processing of
MMIO, only the walk of the memory map.
Yes, you are right. And I think cur_map_get() can be broken into the
style lock, ref++, phys_page_find(); unlock. easily.
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.
Actually, there are pairs -- when dev enter mem view, inc ref; and
when it leave, dec ref.
But as Avi has pointed out, the mr-refcnt introduce complicate and no
gain. So I will discard this design
The plan that you refer that has been relatively well thought out, IIRC.
Instead of designing something, i would try to understand/improve on
that.
Thanks and regards,
pingfan
Paolo
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/15] hotplug: introduce qdev_unplug_complete() to remove device from views
On Thu, Aug 9, 2012 at 4:00 PM, Paolo Bonzini pbonz...@redhat.com wrote:
Il 09/08/2012 09:28, liu ping fan ha scritto:
VCPU threadI/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.
But the RCU critical section should not include the whole processing of
MMIO, only the walk of the memory map.
Yes, you are right. And I think cur_map_get() can be broken into the
style lock, ref++, phys_page_find(); unlock. easily.
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.
Actually, there are pairs -- when dev enter mem view, inc ref; and
when it leave, dec ref.
But as Avi has pointed out, the mr-refcnt introduce complicate and no
gain. So I will discard this design
Thanks and regards,
pingfan
Paolo
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/15] hotplug: introduce qdev_unplug_complete() to remove device from views
On Wed, Aug 8, 2012 at 5:52 PM, Paolo Bonzini pbonz...@redhat.com wrote:
Il 08/08/2012 08:25, Liu Ping Fan ha scritto:
+void qdev_unplug_complete(DeviceState *dev, Error **errp)
+{
+/* isolate from mem view */
+qdev_unmap(dev);
+qemu_lock_devtree();
+/* isolate from device tree */
+qdev_unset_parent(dev);
+qemu_unlock_devtree();
+object_unref(OBJECT(dev));
Rather than deferring the free, you should defer the unref. Otherwise
the following can happen when you have real RCU access to the memory
map on the read-side:
VCPU threadI/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.
rcu_read_unlock()
free()
dangling pointer!
If you defer the unref, you have instead
VCPU threadI/O thread
=
get MMIO request
rcu_read_lock()
walk memory map
qdev_unmap()
lock_devtree()
...
unlock_devtree
unref is enqueued
ref() - refcnt = 2
rcu_read_unlock()
unref() - refcnt=1
unref() - refcnt = 1
So this also makes patch 14 unnecessary.
Paolo
+}
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/15] hotplug: introduce qdev_unplug_complete() to remove device from views
Il 09/08/2012 09:28, liu ping fan ha scritto:
VCPU threadI/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.
But the RCU critical section should not include the whole processing of
MMIO, only the walk of the memory map.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/15] hotplug: introduce qdev_unplug_complete() to remove device from views
Il 08/08/2012 08:25, Liu Ping Fan ha scritto:
+void qdev_unplug_complete(DeviceState *dev, Error **errp)
+{
+/* isolate from mem view */
+qdev_unmap(dev);
+qemu_lock_devtree();
+/* isolate from device tree */
+qdev_unset_parent(dev);
+qemu_unlock_devtree();
+object_unref(OBJECT(dev));
Rather than deferring the free, you should defer the unref. Otherwise
the following can happen when you have real RCU access to the memory
map on the read-side:
VCPU threadI/O thread
=
get MMIO request
rcu_read_lock()
walk memory map
qdev_unmap()
lock_devtree()
...
unlock_devtree
unref dev - refcnt=0, free enqueued
ref()
rcu_read_unlock()
free()
dangling pointer!
If you defer the unref, you have instead
VCPU threadI/O thread
=
get MMIO request
rcu_read_lock()
walk memory map
qdev_unmap()
lock_devtree()
...
unlock_devtree
unref is enqueued
ref() - refcnt = 2
rcu_read_unlock()
unref() - refcnt=1
unref() - refcnt = 1
So this also makes patch 14 unnecessary.
Paolo
+}
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/15] hotplug: introduce qdev_unplug_complete() to remove device from views
On 08/08/2012 12:52 PM, Paolo Bonzini wrote:
Il 08/08/2012 08:25, Liu Ping Fan ha scritto:
+void qdev_unplug_complete(DeviceState *dev, Error **errp)
+{
+/* isolate from mem view */
+qdev_unmap(dev);
+qemu_lock_devtree();
+/* isolate from device tree */
+qdev_unset_parent(dev);
+qemu_unlock_devtree();
+object_unref(OBJECT(dev));
Rather than deferring the free, you should defer the unref. Otherwise
the following can happen when you have real RCU access to the memory
map on the read-side:
VCPU threadI/O thread
=
get MMIO request
rcu_read_lock()
walk memory map
qdev_unmap()
lock_devtree()
...
unlock_devtree
unref dev - refcnt=0, free enqueued
ref()
rcu_read_unlock()
free()
dangling pointer!
unref should follow either synchronize_rcu(), or be in a call_rcu()
callback (deferring the unref). IMO synchronize_rcu() is sufficient
here, unplug is a truly slow path, esp. on real hardware.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
