[Qemu-devel] [PATCH 0/10] rework on hot unplug

[Qemu-devel] [PATCH 0/10] rework on hot unplug

[Liu Ping Fan]

This patchset takes the mmio-dispatch out of big lock's protection into consideration.
In that scene, the finalize of parent can not mean the end of children immediately.

Also, this patchset elimate one major obstale to change the 3rd param of
memory_region_init_io from void* to Object *
refer to:
http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg02409.html

Background:
Inspired by the disscussion between Paolo Bonzini and Anthony Liguori
refer to
http://www.mail-archive.com/qemu-devel@nongnu.org/msg127344.html


Liu Ping Fan (10)
qom: add,remove of link property need to ref,unref its target
qdev: change iterator callback seq 
qom: export object_property_is_child,object_property_is_link
qdev: introduce new interface to remove composite sub-tree
qdev: finalize of qbus,qdev will not the right place to free children
qom: expose object_property_del_child
unplug: using new intf qdev_delete_subtree in acpi_piix_eject_slot
qdev: rename qdev_unplug to qdev_unplug_req
mon: release dev's ref hold by qdev_get_peripheral
qdev: fix create in place obj's life cycle problem

 hw/acpi_piix4.c       |    3 +
 hw/pci-hotplug.c      |    4 +-
 hw/qdev-monitor.c     |    8 +++-
 hw/qdev.c             |  113 ++++++++++++++++++++++++++++++++++++-------------
 hw/qdev.h             |    8 ++-
 hw/scsi-bus.c         |    4 +-
 hw/usb/dev-storage.c  |    2 +-
 hw/xen_platform.c     |    2 +-
 include/qemu/object.h |    6 +++
 qom/object.c          |   22 +++++++--
 10 files changed, 128 insertions(+), 44 deletions(-)

[Qemu-devel] [PATCH 01/10] qom: add, remove of link property need to ref, unref its target

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Currently, link property's target is only managed by
object_set_link_property(). This will raise such issue that when
the property is finalized, its target has no opportunity to release.

Fix this issue by introduce object_finalize_link_property()
In correspond, ref target when adding link prpoerty.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 qom/object.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index a552be2..00f98d7 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -957,6 +957,16 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
     }
 }
 
+static void object_finalize_link_property(Object *obj, const char *name,
+                                           void *opaque)
+{
+    Object **child = opaque;
+
+    if (*child != NULL) {
+        object_unref(*child);
+    }
+}
+
 void object_property_add_link(Object *obj, const char *name,
                               const char *type, Object **child,
                               Error **errp)
@@ -968,8 +978,10 @@ void object_property_add_link(Object *obj, const char *name,
     object_property_add(obj, name, full_type,
                         object_get_link_property,
                         object_set_link_property,
-                        NULL, child, errp);
-
+                        object_finalize_link_property, child, errp);
+    if (*child != NULL) {
+        object_ref(*child);
+    }
     g_free(full_type);
 }
 
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 01/10] qom: add, remove of link property need to ref, unref its target

[Paolo Bonzini]

Il 24/08/2012 11:49, Liu Ping Fan ha scritto:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> Currently, link property's target is only managed by
> object_set_link_property(). This will raise such issue that when
> the property is finalized, its target has no opportunity to release.
> 
> Fix this issue by introduce object_finalize_link_property()
> In correspond, ref target when adding link prpoerty.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---

This patch is fine, but it is at the wrong place in the series.  It will
cause an assertion failure due to the unbroken circular reference.

Paolo

[Qemu-devel] [PATCH 02/10] qdev: change iterator callback seq

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Some func should be called after iteration over object's children,
for example, func will put ref of object.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/qdev.c |   30 ++++++++++++++++++++++--------
 hw/qdev.h |    4 ++--
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index b5b74b9..ad618dd 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -230,13 +230,13 @@ static int qbus_reset_one(BusState *bus, void *opaque)
 
 void qdev_reset_all(DeviceState *dev)
 {
-    qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL);
+    qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL, 0);
 }
 
 void qbus_reset_all_fn(void *opaque)
 {
     BusState *bus = opaque;
-    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
+    qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL, 0);
 }
 
 /* can be used as ->unplug() callback for the simple cases */
@@ -342,12 +342,12 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
 }
 
 int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
-                       qbus_walkerfn *busfn, void *opaque)
+                       qbus_walkerfn *busfn, void *opaque, int after)
 {
     BusChild *kid;
     int err;
 
-    if (busfn) {
+    if ((after == 0) && busfn) {
         err = busfn(bus, opaque);
         if (err) {
             return err;
@@ -355,22 +355,29 @@ int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
     }
 
     QTAILQ_FOREACH(kid, &bus->children, sibling) {
-        err = qdev_walk_children(kid->child, devfn, busfn, opaque);
+        err = qdev_walk_children(kid->child, devfn, busfn, opaque, after);
         if (err < 0) {
             return err;
         }
     }
 
+    if ((after != 0) && busfn) {
+        err = busfn(bus, opaque);
+        if (err) {
+            return err;
+        }
+    }
+
     return 0;
 }
 
 int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
-                       qbus_walkerfn *busfn, void *opaque)
+                       qbus_walkerfn *busfn, void *opaque, int after)
 {
     BusState *bus;
     int err;
 
-    if (devfn) {
+    if ((after == 0) && devfn) {
         err = devfn(dev, opaque);
         if (err) {
             return err;
@@ -378,12 +385,19 @@ int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
     }
 
     QLIST_FOREACH(bus, &dev->child_bus, sibling) {
-        err = qbus_walk_children(bus, devfn, busfn, opaque);
+        err = qbus_walk_children(bus, devfn, busfn, opaque, after);
         if (err < 0) {
             return err;
         }
     }
 
+    if ((after != 0) && devfn) {
+        err = devfn(dev, opaque);
+        if (err) {
+            return err;
+        }
+    }
+
     return 0;
 }
 
diff --git a/hw/qdev.h b/hw/qdev.h
index d699194..9fc8b46 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -195,9 +195,9 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
  *         < 0 if either devfn or busfn terminate walk somewhere in cursion,
  *           0 otherwise. */
 int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn,
-                       qbus_walkerfn *busfn, void *opaque);
+                       qbus_walkerfn *busfn, void *opaque, int after);
 int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
-                       qbus_walkerfn *busfn, void *opaque);
+                       qbus_walkerfn *busfn, void *opaque, int after);
 void qdev_reset_all(DeviceState *dev);
 void qbus_reset_all_fn(void *opaque);
 
-- 
1.7.4.4

[Qemu-devel] [PATCH 03/10] qom: export object_property_is_child, object_property_is_link

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

qdev will use them to judge how to remove the bus and device's
reference. So export them in object.h

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 include/qemu/object.h |    3 +++
 qom/object.c          |    4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index cc75fee..7cc3ebb 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -431,6 +431,9 @@ struct InterfaceClass
 #define INTERFACE_CHECK(interface, obj, name) \
     ((interface *)object_dynamic_cast_assert(OBJECT((obj)), (name)))
 
+inline bool object_property_is_child(ObjectProperty *prop);
+inline bool object_property_is_link(ObjectProperty *prop);
+
 /**
  * object_new:
  * @typename: The name of the type of the object to instantiate.
diff --git a/qom/object.c b/qom/object.c
index 00f98d7..be460df 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -318,12 +318,12 @@ void object_initialize(void *data, const char *typename)
     object_initialize_with_type(data, type);
 }
 
-static inline bool object_property_is_child(ObjectProperty *prop)
+inline bool object_property_is_child(ObjectProperty *prop)
 {
     return strstart(prop->type, "child<", NULL);
 }
 
-static inline bool object_property_is_link(ObjectProperty *prop)
+inline bool object_property_is_link(ObjectProperty *prop)
 {
     return strstart(prop->type, "link<", NULL);
 }
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 03/10] qom: export object_property_is_child, object_property_is_link

[Paolo Bonzini]

Il 24/08/2012 11:49, Liu Ping Fan ha scritto:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> qdev will use them to judge how to remove the bus and device's
> reference. So export them in object.h

This series doesn't use them.

Paolo

> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  include/qemu/object.h |    3 +++
>  qom/object.c          |    4 ++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/object.h b/include/qemu/object.h
> index cc75fee..7cc3ebb 100644
> --- a/include/qemu/object.h
> +++ b/include/qemu/object.h
> @@ -431,6 +431,9 @@ struct InterfaceClass
>  #define INTERFACE_CHECK(interface, obj, name) \
>      ((interface *)object_dynamic_cast_assert(OBJECT((obj)), (name)))
>  
> +inline bool object_property_is_child(ObjectProperty *prop);
> +inline bool object_property_is_link(ObjectProperty *prop);
> +
>  /**
>   * object_new:
>   * @typename: The name of the type of the object to instantiate.
> diff --git a/qom/object.c b/qom/object.c
> index 00f98d7..be460df 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -318,12 +318,12 @@ void object_initialize(void *data, const char *typename)
>      object_initialize_with_type(data, type);
>  }
>  
> -static inline bool object_property_is_child(ObjectProperty *prop)
> +inline bool object_property_is_child(ObjectProperty *prop)
>  {
>      return strstart(prop->type, "child<", NULL);
>  }
>  
> -static inline bool object_property_is_link(ObjectProperty *prop)
> +inline bool object_property_is_link(ObjectProperty *prop)
>  {
>      return strstart(prop->type, "link<", NULL);
>  }
> 

Re: [Qemu-devel] [PATCH 03/10] qom: export object_property_is_child, object_property_is_link

[liu ping fan]

On Fri, Aug 24, 2012 at 10:51 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 24/08/2012 11:49, Liu Ping Fan ha scritto:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> qdev will use them to judge how to remove the bus and device's
>> reference. So export them in object.h
>
> This series doesn't use them.
>
Yeap,  will fix it in V2.

Thanks,
pingfan

> Paolo
>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  include/qemu/object.h |    3 +++
>>  qom/object.c          |    4 ++--
>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/qemu/object.h b/include/qemu/object.h
>> index cc75fee..7cc3ebb 100644
>> --- a/include/qemu/object.h
>> +++ b/include/qemu/object.h
>> @@ -431,6 +431,9 @@ struct InterfaceClass
>>  #define INTERFACE_CHECK(interface, obj, name) \
>>      ((interface *)object_dynamic_cast_assert(OBJECT((obj)), (name)))
>>
>> +inline bool object_property_is_child(ObjectProperty *prop);
>> +inline bool object_property_is_link(ObjectProperty *prop);
>> +
>>  /**
>>   * object_new:
>>   * @typename: The name of the type of the object to instantiate.
>> diff --git a/qom/object.c b/qom/object.c
>> index 00f98d7..be460df 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -318,12 +318,12 @@ void object_initialize(void *data, const char *typename)
>>      object_initialize_with_type(data, type);
>>  }
>>
>> -static inline bool object_property_is_child(ObjectProperty *prop)
>> +inline bool object_property_is_child(ObjectProperty *prop)
>>  {
>>      return strstart(prop->type, "child<", NULL);
>>  }
>>
>> -static inline bool object_property_is_link(ObjectProperty *prop)
>> +inline bool object_property_is_link(ObjectProperty *prop)
>>  {
>>      return strstart(prop->type, "link<", NULL);
>>  }
>>
>

Re: [Qemu-devel] [PATCH 03/10] qom: export object_property_is_child, object_property_is_link

[Blue Swirl]

On Fri, Aug 24, 2012 at 9:49 AM, Liu Ping Fan <qemulist@gmail.com> wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> qdev will use them to judge how to remove the bus and device's
> reference. So export them in object.h
>
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  include/qemu/object.h |    3 +++
>  qom/object.c          |    4 ++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu/object.h b/include/qemu/object.h
> index cc75fee..7cc3ebb 100644
> --- a/include/qemu/object.h
> +++ b/include/qemu/object.h
> @@ -431,6 +431,9 @@ struct InterfaceClass
>  #define INTERFACE_CHECK(interface, obj, name) \
>      ((interface *)object_dynamic_cast_assert(OBJECT((obj)), (name)))
>
> +inline bool object_property_is_child(ObjectProperty *prop);
> +inline bool object_property_is_link(ObjectProperty *prop);

This linkage does not make sense, please remove 'inline'.

> +
>  /**
>   * object_new:
>   * @typename: The name of the type of the object to instantiate.
> diff --git a/qom/object.c b/qom/object.c
> index 00f98d7..be460df 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -318,12 +318,12 @@ void object_initialize(void *data, const char *typename)
>      object_initialize_with_type(data, type);
>  }
>
> -static inline bool object_property_is_child(ObjectProperty *prop)
> +inline bool object_property_is_child(ObjectProperty *prop)
>  {
>      return strstart(prop->type, "child<", NULL);
>  }
>
> -static inline bool object_property_is_link(ObjectProperty *prop)
> +inline bool object_property_is_link(ObjectProperty *prop)
>  {
>      return strstart(prop->type, "link<", NULL);
>  }
> --
> 1.7.4.4
>
>

[Qemu-devel] [PATCH 04/10] qdev: introduce new interface to remove composite sub-tree

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

When a bridge device removed, all the children bebind it should be
removed. Using qdev_delete_subtree(dev) to emulate this event.
And it is achieved by iterated laughed by qdev_walk_children()

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/qdev.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h |    1 +
 2 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index ad618dd..570f0bf 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -239,6 +239,50 @@ void qbus_reset_all_fn(void *opaque)
     qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL, 0);
 }
 
+static int qdev_device_delete(DeviceState *dev, void *opaque)
+{
+    Object *obj = OBJECT(dev);
+    BusState *b = dev->parent_bus;
+
+    dev->parent_bus = NULL;
+    /* removed from list and bus->dev link */
+    bus_remove_child(b, dev);
+    /* remove bus<-dev link */
+    object_property_del(OBJECT(dev), "parent_bus", NULL);
+
+    /* when mmio-dispatch out of big lock, remove it!
+         * refcnt hold by
+         * object_property_add_child(qdev_get_peripheral(), ., OBJECT(qdev),..)
+         * should be released before here.
+         */
+    g_assert(obj->ref == 1);
+    object_unref(OBJECT(dev));
+
+    return 0;
+}
+
+static int qdev_bus_delete(BusState *bus, void *opaque)
+{
+    Object *obj = OBJECT(bus);
+    DeviceState *d = bus->parent;
+
+    bus->parent = NULL;
+    QLIST_REMOVE(bus, sibling);
+    d->num_child_bus--;
+    object_property_del_child(OBJECT(d), OBJECT(bus), NULL);
+    /* when mmio-dispatch out of big lock, remove it!*/
+    g_assert(obj->ref == 1);
+    object_unref(obj);
+
+    return 0;
+}
+
+/* Delete the composite tree which starts from @dev */
+void qdev_delete_subtree(DeviceState *dev)
+{
+    qdev_walk_children(dev, qdev_device_delete, qdev_bus_delete, NULL, 1);
+}
+
 /* can be used as ->unplug() callback for the simple cases */
 int qdev_simple_unplug_cb(DeviceState *dev)
 {
diff --git a/hw/qdev.h b/hw/qdev.h
index 9fc8b46..6f97990 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -162,6 +162,7 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
 void qdev_unplug(DeviceState *dev, Error **errp);
 void qdev_free(DeviceState *dev);
+void qdev_delete_subtree(DeviceState *dev);
 int qdev_simple_unplug_cb(DeviceState *dev);
 void qdev_machine_creation_done(void);
 bool qdev_machine_modified(void);
-- 
1.7.4.4

[Qemu-devel] [PATCH 05/10] qdev: finalize of qbus, qdev will not the right place to free children

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

When breaking big lock, the child object can be hold by mmio-dispatch,
and it is not right to free them when their parent gone.
We will isolate and release the children by qdev_delete_subtree(),
and let each object manage its own life cycle.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/qdev.c |   30 ++++++++++++------------------
 1 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 570f0bf..d6c8130 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -756,10 +756,8 @@ static void device_finalize(Object *obj)
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
 
     if (dev->state == DEV_STATE_INITIALIZED) {
-        while (dev->num_child_bus) {
-            bus = QLIST_FIRST(&dev->child_bus);
-            qbus_free(bus);
-        }
+
+        g_assert(QLIST_EMPTY(&dev->child_bus));
         if (qdev_get_vmsd(dev)) {
             vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
         }
@@ -770,9 +768,8 @@ static void device_finalize(Object *obj)
             qemu_opts_del(dev->opts);
         }
     }
-    if (dev->parent_bus) {
-        bus_remove_child(dev->parent_bus, dev);
-    }
+
+    g_assert((dev->parent_bus == NULL));
 }
 
 static void device_class_base_init(ObjectClass *class, void *data)
@@ -826,19 +823,16 @@ static void qbus_initfn(Object *obj)
 static void qbus_finalize(Object *obj)
 {
     BusState *bus = BUS(obj);
-    BusChild *kid;
 
-    while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
-        DeviceState *dev = kid->child;
-        qdev_free(dev);
-    }
-    if (bus->parent) {
-        QLIST_REMOVE(bus, sibling);
-        bus->parent->num_child_bus--;
-    } else {
-        assert(bus != sysbus_get_default()); /* main_system_bus is never freed */
-        qemu_unregister_reset(qbus_reset_all_fn, bus);
+    assert(bus != sysbus_get_default()); /* main_system_bus is never freed */
+    /* just blind check, since bus->parent has been set to NULL */
+    qemu_unregister_reset(qbus_reset_all_fn, bus);
+    if (bus->overlap != NULL) {
+        object_unref(OBJECT(bus->overlap));
     }
+    assert(QTAILQ_EMPTY(&bus->children));
+    /* have reset it to NULL, to prevent reader */
+    assert((bus->parent == NULL));
     g_free((char *)bus->name);
 }
 
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 05/10] qdev: finalize of qbus, qdev will not the right place to free children

[Paolo Bonzini]

Il 24/08/2012 11:49, Liu Ping Fan ha scritto:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> When breaking big lock, the child object can be hold by mmio-dispatch,
> and it is not right to free them when their parent gone.
> We will isolate and release the children by qdev_delete_subtree(),
> and let each object manage its own life cycle.

I think this makes sense, but is it already working at this point of the
series?  That is, before patch 7 starts using qdev_delete_subtree()?

Paolo

> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/qdev.c |   30 ++++++++++++------------------
>  1 files changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 570f0bf..d6c8130 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -756,10 +756,8 @@ static void device_finalize(Object *obj)
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>  
>      if (dev->state == DEV_STATE_INITIALIZED) {
> -        while (dev->num_child_bus) {
> -            bus = QLIST_FIRST(&dev->child_bus);
> -            qbus_free(bus);
> -        }
> +
> +        g_assert(QLIST_EMPTY(&dev->child_bus));
>          if (qdev_get_vmsd(dev)) {
>              vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
>          }
> @@ -770,9 +768,8 @@ static void device_finalize(Object *obj)
>              qemu_opts_del(dev->opts);
>          }
>      }
> -    if (dev->parent_bus) {
> -        bus_remove_child(dev->parent_bus, dev);
> -    }
> +
> +    g_assert((dev->parent_bus == NULL));
>  }
>  
>  static void device_class_base_init(ObjectClass *class, void *data)
> @@ -826,19 +823,16 @@ static void qbus_initfn(Object *obj)
>  static void qbus_finalize(Object *obj)
>  {
>      BusState *bus = BUS(obj);
> -    BusChild *kid;
>  
> -    while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
> -        DeviceState *dev = kid->child;
> -        qdev_free(dev);
> -    }
> -    if (bus->parent) {
> -        QLIST_REMOVE(bus, sibling);
> -        bus->parent->num_child_bus--;
> -    } else {
> -        assert(bus != sysbus_get_default()); /* main_system_bus is never freed */
> -        qemu_unregister_reset(qbus_reset_all_fn, bus);
> +    assert(bus != sysbus_get_default()); /* main_system_bus is never freed */
> +    /* just blind check, since bus->parent has been set to NULL */
> +    qemu_unregister_reset(qbus_reset_all_fn, bus);
> +    if (bus->overlap != NULL) {
> +        object_unref(OBJECT(bus->overlap));
>      }
> +    assert(QTAILQ_EMPTY(&bus->children));
> +    /* have reset it to NULL, to prevent reader */
> +    assert((bus->parent == NULL));
>      g_free((char *)bus->name);
>  }
>  
> 

[Qemu-devel] [PATCH 06/10] qom: expose object_property_del_child

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

qmp and qdev need it.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 include/qemu/object.h |    3 +++
 qom/object.c          |    2 +-
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index 7cc3ebb..0142da4 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -914,6 +914,9 @@ Object *object_resolve_path_component(Object *parent, gchar *part);
 void object_property_add_child(Object *obj, const char *name,
                                Object *child, struct Error **errp);
 
+void object_property_del_child(Object *obj, Object *child,
+                                struct Error **errp);
+
 /**
  * object_property_add_link:
  * @obj: the object to add a property to
diff --git a/qom/object.c b/qom/object.c
index be460df..6c90cfa 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -345,7 +345,7 @@ static void object_property_del_all(Object *obj)
     }
 }
 
-static void object_property_del_child(Object *obj, Object *child, Error **errp)
+void object_property_del_child(Object *obj, Object *child, Error **errp)
 {
     ObjectProperty *prop;
 
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 06/10] qom: expose object_property_del_child

[Paolo Bonzini]

Il 24/08/2012 11:49, Liu Ping Fan ha scritto:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> qmp and qdev need it.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  include/qemu/object.h |    3 +++
>  qom/object.c          |    2 +-
>  2 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/include/qemu/object.h b/include/qemu/object.h
> index 7cc3ebb..0142da4 100644
> --- a/include/qemu/object.h
> +++ b/include/qemu/object.h
> @@ -914,6 +914,9 @@ Object *object_resolve_path_component(Object *parent, gchar *part);
>  void object_property_add_child(Object *obj, const char *name,
>                                 Object *child, struct Error **errp);
>  
> +void object_property_del_child(Object *obj, Object *child,
> +                                struct Error **errp);
> +
>  /**
>   * object_property_add_link:
>   * @obj: the object to add a property to
> diff --git a/qom/object.c b/qom/object.c
> index be460df..6c90cfa 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -345,7 +345,7 @@ static void object_property_del_all(Object *obj)
>      }
>  }
>  
> -static void object_property_del_child(Object *obj, Object *child, Error **errp)
> +void object_property_del_child(Object *obj, Object *child, Error **errp)
>  {
>      ObjectProperty *prop;
>  
> 

Not needed, see my reply to patch 9.

Paolo

[Qemu-devel] [PATCH 07/10] unplug: using new intf qdev_delete_subtree in acpi_piix_eject_slot

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

We are not long to force to delete the obj at that place, just
let its refcnt handle this issue.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/acpi_piix4.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 0aace60..f00d4d8 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -305,8 +305,11 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
             if (pc->no_hotplug) {
                 slot_free = false;
             } else {
+#if 0
                 object_unparent(OBJECT(dev));
                 qdev_free(qdev);
+#endif
+                qdev_delete_subtree(qdev);
             }
         }
     }
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 07/10] unplug: using new intf qdev_delete_subtree in acpi_piix_eject_slot

[Paolo Bonzini]

Il 24/08/2012 11:49, Liu Ping Fan ha scritto:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> We are not long to force to delete the obj at that place, just
> let its refcnt handle this issue.

This seems wrong.  If anything, unplug requests should propagate down
the tree and the top device should only acknowledge it hot-unplug after
all its children.  You are effectively surprise-removing everything
below a bridge.


Paolo

Re: [Qemu-devel] [PATCH 07/10] unplug: using new intf qdev_delete_subtree in acpi_piix_eject_slot

[liu ping fan]

On Fri, Aug 24, 2012 at 6:24 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 24/08/2012 11:49, Liu Ping Fan ha scritto:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> We are not long to force to delete the obj at that place, just
>> let its refcnt handle this issue.
>
> This seems wrong.  If anything, unplug requests should propagate down
> the tree and the top device should only acknowledge it hot-unplug after
> all its children.  You are effectively surprise-removing everything
> below a bridge.
>
I had thought that the bridge's acknowledge will be the last one eject
by guest, can not assume that?  Another question is that if we got ack
for a bridge, but not for its child, then we will just leave the
bridge on the fly?

Anyway, I thought another method to work around this. Will send out it later.

Thanks and regards,
pingfan
>
> Paolo

[Qemu-devel] [PATCH 08/10] qdev: rename qdev_unplug to qdev_unplug_req

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Unplug divides into two steps: request and complete
The name "req" show more clearly about its meaning
and as to the complete, it is qdev_delete_subtree()

Also adding ret to indicate the request can be eject or not

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/pci-hotplug.c     |    4 ++--
 hw/qdev-monitor.c    |    2 +-
 hw/qdev.c            |    7 ++++---
 hw/qdev.h            |    2 +-
 hw/scsi-bus.c        |    4 ++--
 hw/usb/dev-storage.c |    2 +-
 hw/xen_platform.c    |    2 +-
 7 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index e7fb780..6faff33 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -192,7 +192,7 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
             dev = NULL;
         if (dev && dinfo) {
             if (scsi_hot_add(mon, &dev->qdev, dinfo, 0) != 0) {
-                qdev_unplug(&dev->qdev, NULL);
+                qdev_unplug_req(&dev->qdev, NULL);
                 dev = NULL;
             }
         }
@@ -271,7 +271,7 @@ static int pci_device_hot_remove(Monitor *mon, const char *pci_addr)
         return -1;
     }
 
-    qdev_unplug(&d->qdev, &local_err);
+    qdev_unplug_req(&d->qdev, &local_err);
     if (error_is_set(&local_err)) {
         monitor_printf(mon, "%s\n", error_get_pretty(local_err));
         error_free(local_err);
diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index 018b386..fdd9539 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -606,7 +606,7 @@ void qmp_device_del(const char *id, Error **errp)
         return;
     }
 
-    qdev_unplug(dev, errp);
+    qdev_unplug_req(dev, errp);
 }
 
 void qdev_machine_init(void)
diff --git a/hw/qdev.c b/hw/qdev.c
index d6c8130..e2339a1 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -194,13 +194,13 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
     dev->alias_required_for_version = required_for_version;
 }
 
-void qdev_unplug(DeviceState *dev, Error **errp)
+int qdev_unplug_req(DeviceState *dev, Error **errp)
 {
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
 
     if (!dev->parent_bus->allow_hotplug) {
         error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
-        return;
+        return -1;
     }
     assert(dc->unplug != NULL);
 
@@ -208,8 +208,9 @@ void qdev_unplug(DeviceState *dev, Error **errp)
 
     if (dc->unplug(dev) < 0) {
         error_set(errp, QERR_UNDEFINED_ERROR);
-        return;
+        return -1;
     }
+    return 0;
 }
 
 static int qdev_reset_one(DeviceState *dev, void *opaque)
diff --git a/hw/qdev.h b/hw/qdev.h
index 6f97990..182cfa5 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -160,7 +160,7 @@ int qdev_init(DeviceState *dev) QEMU_WARN_UNUSED_RESULT;
 void qdev_init_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
-void qdev_unplug(DeviceState *dev, Error **errp);
+int qdev_unplug_req(DeviceState *dev, Error **errp);
 void qdev_free(DeviceState *dev);
 void qdev_delete_subtree(DeviceState *dev);
 int qdev_simple_unplug_cb(DeviceState *dev);
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 4981a02..023211a 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -1802,7 +1802,7 @@ static int get_scsi_requests(QEMUFile *f, void *pv, size_t size)
     return 0;
 }
 
-static int scsi_qdev_unplug(DeviceState *qdev)
+static int scsi_qdev_unplug_req(DeviceState *qdev)
 {
     SCSIDevice *dev = SCSI_DEVICE(qdev);
     SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
@@ -1849,7 +1849,7 @@ static void scsi_device_class_init(ObjectClass *klass, void *data)
     DeviceClass *k = DEVICE_CLASS(klass);
     k->bus_type = TYPE_SCSI_BUS;
     k->init     = scsi_qdev_init;
-    k->unplug   = scsi_qdev_unplug;
+    k->unplug   = scsi_qdev_unplug_req;
     k->exit     = scsi_qdev_exit;
     k->props    = scsi_props;
 }
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index ff48d91..8871b2b 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -510,7 +510,7 @@ static void usb_msd_password_cb(void *opaque, int err)
         err = usb_device_attach(&s->dev);
 
     if (err)
-        qdev_unplug(&s->dev.qdev, NULL);
+        qdev_unplug_req(&s->dev.qdev, NULL);
 }
 
 static void *usb_msd_load_request(QEMUFile *f, SCSIRequest *req)
diff --git a/hw/xen_platform.c b/hw/xen_platform.c
index c1fe984..0231a40 100644
--- a/hw/xen_platform.c
+++ b/hw/xen_platform.c
@@ -103,7 +103,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *o)
 {
     if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
             PCI_CLASS_STORAGE_IDE) {
-        qdev_unplug(&(d->qdev), NULL);
+        qdev_unplug_req(&(d->qdev), NULL);
     }
 }
 
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 08/10] qdev: rename qdev_unplug to qdev_unplug_req

[Paolo Bonzini]

Il 24/08/2012 11:49, Liu Ping Fan ha scritto:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> Unplug divides into two steps: request and complete
> The name "req" show more clearly about its meaning

But not enough.  Let's make it "qdev_request_unplug", and change the
method name in the DeviceClass too.

> and as to the complete, it is qdev_delete_subtree()

Completing unplug is just qdev_free after commit da5a44e (qom:
object_delete should unparent the object first, 2012-08-22).

> Also adding ret to indicate the request can be eject or not

This is also given by the Error object.  Returning the same thing twice
is guaranteed to give bugs because the two get out of sync.

Paolo

> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

[Qemu-devel] [PATCH 09/10] mon: release dev's ref hold by qdev_get_peripheral

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

When add it at qdev_device_add(), so need to release it
at qmp_device_del().

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/qdev-monitor.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index fdd9539..29ace17 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -599,6 +599,7 @@ int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
 void qmp_device_del(const char *id, Error **errp)
 {
     DeviceState *dev;
+    int ret;
 
     dev = qdev_find_recursive(sysbus_get_default(), id);
     if (NULL == dev) {
@@ -606,7 +607,12 @@ void qmp_device_del(const char *id, Error **errp)
         return;
     }
 
-    qdev_unplug_req(dev, errp);
+    ret = qdev_unplug_req(dev, errp);
+    if (ret == 0) {
+        /* to here,  dev->id is not zero, so need to release */
+        object_property_del_child(qdev_get_peripheral(),
+                    OBJECT(dev), NULL);
+    }
 }
 
 void qdev_machine_init(void)
-- 
1.7.4.4

[Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Scene:
  obja lies in objA, when objA's ref->0, it will be freed,
but at that time obja can still be in use.

The real example is:
typedef struct PCIIDEState {
    PCIDevice dev;
    IDEBus bus[2]; --> create in place
    .....
}

When without big lock protection for mmio-dispatch, we will hold
obj's refcnt. So memory_region_init_io() will replace the third para
"void *opaque" with "Object *obj".
With this patch, we can protect PCIIDEState from disappearing during
mmio-dispatch hold the IDEBus->ref.

And the ref circle has been broken when calling qdev_delete_subtree().

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/qdev.c |    2 ++
 hw/qdev.h |    1 +
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index e2339a1..b09ebbf 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -510,6 +510,8 @@ void qbus_create_inplace(BusState *bus, const char *typename,
 {
     object_initialize(bus, typename);
 
+    bus->overlap = parent;
+    object_ref(OBJECT(bus->overlap));
     bus->parent = parent;
     bus->name = name ? g_strdup(name) : NULL;
     qbus_realize(bus);
diff --git a/hw/qdev.h b/hw/qdev.h
index 182cfa5..9bc5783 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -117,6 +117,7 @@ struct BusState {
     int allow_hotplug;
     bool qom_allocated;
     bool glib_allocated;
+    DeviceState *overlap;
     int max_index;
     QTAILQ_HEAD(ChildrenHead, BusChild) children;
     QLIST_ENTRY(BusState) sibling;
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Paolo Bonzini]

Il 24/08/2012 11:49, Liu Ping Fan ha scritto:
> With this patch, we can protect PCIIDEState from disappearing during
> mmio-dispatch hold the IDEBus->ref.

I don't see why MMIO dispatch should hold the IDEBus ref rather than the
PCIIDEState.

In the case of the PIIX, the BARs are set up by the PCIIDEState in
bmdma_setup_bar (called by bmdma_setup_bar).

Also, containment may happen just as well for devices, not buses.  Why
isn't it a problem in that case?  It looks like you're papering over a
different bug.

Paolo

> And the ref circle has been broken when calling qdev_delete_subtree().
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/qdev.c |    2 ++
>  hw/qdev.h |    1 +
>  2 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index e2339a1..b09ebbf 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -510,6 +510,8 @@ void qbus_create_inplace(BusState *bus, const char *typename,
>  {
>      object_initialize(bus, typename);
>  
> +    bus->overlap = parent;
> +    object_ref(OBJECT(bus->overlap));
>      bus->parent = parent;
>      bus->name = name ? g_strdup(name) : NULL;
>      qbus_realize(bus);
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 182cfa5..9bc5783 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -117,6 +117,7 @@ struct BusState {
>      int allow_hotplug;
>      bool qom_allocated;
>      bool glib_allocated;
> +    DeviceState *overlap;
>      int max_index;
>      QTAILQ_HEAD(ChildrenHead, BusChild) children;
>      QLIST_ENTRY(BusState) sibling;
> -- 1.7.4.4

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[liu ping fan]

On Fri, Aug 24, 2012 at 10:42 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 24/08/2012 11:49, Liu Ping Fan ha scritto:
>> With this patch, we can protect PCIIDEState from disappearing during
>> mmio-dispatch hold the IDEBus->ref.
>
> I don't see why MMIO dispatch should hold the IDEBus ref rather than the
> PCIIDEState.
>
When transfer memory_region_init_io()  3rd para from void* opaque to
Object* obj,  the obj : opaque is not neccessary 1:1 map. For such
situation, in order to let MemoryRegionOps tell between them, we
should pass PCIIDEState->bus[0], bus[1] separately.

> In the case of the PIIX, the BARs are set up by the PCIIDEState in
> bmdma_setup_bar (called by bmdma_setup_bar).
>
Supposing we have convert  PCIIDEState->bmdma[0]/[1] to Object. And in
mmio-dispatch, object_ref will impose on bmdma[0/[1], but this can not
prevent  PCIIDEState->refcnt=0, and then the whole object disappear!

Thanks and regards,
pingfan


> Also, containment may happen just as well for devices, not buses.  Why
> isn't it a problem in that case?  It looks like you're papering over a
> different bug.
>
> Paolo
>
>> And the ref circle has been broken when calling qdev_delete_subtree().
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  hw/qdev.c |    2 ++
>>  hw/qdev.h |    1 +
>>  2 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index e2339a1..b09ebbf 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -510,6 +510,8 @@ void qbus_create_inplace(BusState *bus, const char *typename,
>>  {
>>      object_initialize(bus, typename);
>>
>> +    bus->overlap = parent;
>> +    object_ref(OBJECT(bus->overlap));
>>      bus->parent = parent;
>>      bus->name = name ? g_strdup(name) : NULL;
>>      qbus_realize(bus);
>> diff --git a/hw/qdev.h b/hw/qdev.h
>> index 182cfa5..9bc5783 100644
>> --- a/hw/qdev.h
>> +++ b/hw/qdev.h
>> @@ -117,6 +117,7 @@ struct BusState {
>>      int allow_hotplug;
>>      bool qom_allocated;
>>      bool glib_allocated;
>> +    DeviceState *overlap;
>>      int max_index;
>>      QTAILQ_HEAD(ChildrenHead, BusChild) children;
>>      QLIST_ENTRY(BusState) sibling;
>> -- 1.7.4.4
>

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Paolo Bonzini]

Il 25/08/2012 09:42, liu ping fan ha scritto:
>> >
>> > I don't see why MMIO dispatch should hold the IDEBus ref rather than the
>> > PCIIDEState.
>> >
> When transfer memory_region_init_io()  3rd para from void* opaque to
> Object* obj,  the obj : opaque is not neccessary 1:1 map. For such
> situation, in order to let MemoryRegionOps tell between them, we
> should pass PCIIDEState->bus[0], bus[1] separately.

The rule should be that the obj is the object that you want referenced,
and that should be the PCIIDEState.

But this is anyway moot because it only applies to objects that are
converted to use unlocked dispatch.  This likely will not be the case
for IDE.

Paolo

>> > In the case of the PIIX, the BARs are set up by the PCIIDEState in
>> > bmdma_setup_bar (called by bmdma_setup_bar).
>> >
> Supposing we have convert  PCIIDEState->bmdma[0]/[1] to Object. And in
> mmio-dispatch, object_ref will impose on bmdma[0/[1], but this can not
> prevent  PCIIDEState->refcnt=0, and then the whole object disappear!

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]

On 2012-08-27 09:01, Paolo Bonzini wrote:
> Il 25/08/2012 09:42, liu ping fan ha scritto:
>>>>
>>>> I don't see why MMIO dispatch should hold the IDEBus ref rather than the
>>>> PCIIDEState.
>>>>
>> When transfer memory_region_init_io()  3rd para from void* opaque to
>> Object* obj,  the obj : opaque is not neccessary 1:1 map. For such
>> situation, in order to let MemoryRegionOps tell between them, we
>> should pass PCIIDEState->bus[0], bus[1] separately.
> 
> The rule should be that the obj is the object that you want referenced,
> and that should be the PCIIDEState.
> 
> But this is anyway moot because it only applies to objects that are
> converted to use unlocked dispatch.  This likely will not be the case
> for IDE.

BTW, I'm pretty sure - after implementing the basics for BQL-free PIO
dispatching - that device objects are the wrong target for reference
counting. We keep memory regions in our dispatching tables (PIO
dispatching needs some refactoring for this), and those regions need
protection for BQL-free use. Devices can't pass away as long as the have
referenced regions, memory region deregistration services will have to
take care of this.

I'm currently not using reference counting at all, I'm enforcing that
only BQL-protected regions can be deregistered.

Also note that there seems to be another misconception in the
discussions: deregistration is not only bound to device unplug. It also
happens on device reconfiguration, e.g. PCI BAR (re-)mapping. Another
strong indicator that we should worry about individual memory regions,
not devices.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[liu ping fan]

On Mon, Aug 27, 2012 at 3:47 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2012-08-27 09:01, Paolo Bonzini wrote:
>> Il 25/08/2012 09:42, liu ping fan ha scritto:
>>>>>
>>>>> I don't see why MMIO dispatch should hold the IDEBus ref rather than the
>>>>> PCIIDEState.
>>>>>
>>> When transfer memory_region_init_io()  3rd para from void* opaque to
>>> Object* obj,  the obj : opaque is not neccessary 1:1 map. For such
>>> situation, in order to let MemoryRegionOps tell between them, we
>>> should pass PCIIDEState->bus[0], bus[1] separately.
>>
>> The rule should be that the obj is the object that you want referenced,
>> and that should be the PCIIDEState.
>>
>> But this is anyway moot because it only applies to objects that are
>> converted to use unlocked dispatch.  This likely will not be the case
>> for IDE.
>
> BTW, I'm pretty sure - after implementing the basics for BQL-free PIO
> dispatching - that device objects are the wrong target for reference

Hi Jan, thanks for reminder, but could you explain it more detail?
mmio dispatch table holds 1 ref for device, before releasing this
ref,( When unplugging, we detach all the device's mr from memory, then
drop the ref. So I think that no leak will be exposed by mr  and it is
safe to use device as target for reference.

> counting. We keep memory regions in our dispatching tables (PIO
> dispatching needs some refactoring for this), and those regions need
> protection for BQL-free use. Devices can't pass away as long as the have
Yes, it is right. Device can pass away only after mr removed from
dispatching tables

Thanx pingfan
> referenced regions, memory region deregistration services will have to
> take care of this.
>

> I'm currently not using reference counting at all, I'm enforcing that
> only BQL-protected regions can be deregistered.
>
> Also note that there seems to be another misconception in the
> discussions: deregistration is not only bound to device unplug. It also
> happens on device reconfiguration, e.g. PCI BAR (re-)mapping. Another
> strong indicator that we should worry about individual memory regions,
> not devices.
>
> Jan
>
>

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2302 bytes --]

On 2012-08-27 10:17, liu ping fan wrote:
> On Mon, Aug 27, 2012 at 3:47 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2012-08-27 09:01, Paolo Bonzini wrote:
>>> Il 25/08/2012 09:42, liu ping fan ha scritto:
>>>>>>
>>>>>> I don't see why MMIO dispatch should hold the IDEBus ref rather than the
>>>>>> PCIIDEState.
>>>>>>
>>>> When transfer memory_region_init_io()  3rd para from void* opaque to
>>>> Object* obj,  the obj : opaque is not neccessary 1:1 map. For such
>>>> situation, in order to let MemoryRegionOps tell between them, we
>>>> should pass PCIIDEState->bus[0], bus[1] separately.
>>>
>>> The rule should be that the obj is the object that you want referenced,
>>> and that should be the PCIIDEState.
>>>
>>> But this is anyway moot because it only applies to objects that are
>>> converted to use unlocked dispatch.  This likely will not be the case
>>> for IDE.
>>
>> BTW, I'm pretty sure - after implementing the basics for BQL-free PIO
>> dispatching - that device objects are the wrong target for reference
> 
> Hi Jan, thanks for reminder, but could you explain it more detail?
> mmio dispatch table holds 1 ref for device, before releasing this
> ref,( When unplugging, we detach all the device's mr from memory, then
> drop the ref. So I think that no leak will be exposed by mr  and it is
> safe to use device as target for reference.

It would be a mistake to assume that memory regions can only be embedded
in device objects. Memory regions can be reconfigured or dynamically
added/removed (see e.g. portio lists) - there is no "device" in this
sentence. Regions are stored in the dispatching table, they will first
of all be touched without holding the BQL. So their content has to be
stable in that period, and it is the proper abstraction, IMHO, to focus
on their life cycle management and attach all the rest to them.

> 
>> counting. We keep memory regions in our dispatching tables (PIO
>> dispatching needs some refactoring for this), and those regions need
>> protection for BQL-free use. Devices can't pass away as long as the have
> Yes, it is right. Device can pass away only after mr removed from
> dispatching tables

Great, then you don't have to worry about device objects in the context
of dispatching.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 08/27/2012 12:47 AM, Jan Kiszka wrote:
> On 2012-08-27 09:01, Paolo Bonzini wrote:
> > Il 25/08/2012 09:42, liu ping fan ha scritto:
> >>>>
> >>>> I don't see why MMIO dispatch should hold the IDEBus ref rather than the
> >>>> PCIIDEState.
> >>>>
> >> When transfer memory_region_init_io()  3rd para from void* opaque to
> >> Object* obj,  the obj : opaque is not neccessary 1:1 map. For such
> >> situation, in order to let MemoryRegionOps tell between them, we
> >> should pass PCIIDEState->bus[0], bus[1] separately.
> > 
> > The rule should be that the obj is the object that you want referenced,
> > and that should be the PCIIDEState.
> > 
> > But this is anyway moot because it only applies to objects that are
> > converted to use unlocked dispatch.  This likely will not be the case
> > for IDE.
>
> BTW, I'm pretty sure - after implementing the basics for BQL-free PIO
> dispatching - that device objects are the wrong target for reference
> counting. We keep memory regions in our dispatching tables (PIO
> dispatching needs some refactoring for this), and those regions need
> protection for BQL-free use. Devices can't pass away as long as the have
> referenced regions, memory region deregistration services will have to
> take care of this.
>
> I'm currently not using reference counting at all, I'm enforcing that
> only BQL-protected regions can be deregistered.

That's a pretty harsh constraint.

> Also note that there seems to be another misconception in the
> discussions: deregistration is not only bound to device unplug. It also
> happens on device reconfiguration, e.g. PCI BAR (re-)mapping. Another
> strong indicator that we should worry about individual memory regions,
> not devices.
>
>

Deregistration is fine, the problem is destruction.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-08-27 19:09, Avi Kivity wrote:
> On 08/27/2012 12:47 AM, Jan Kiszka wrote:
>> On 2012-08-27 09:01, Paolo Bonzini wrote:
>>> Il 25/08/2012 09:42, liu ping fan ha scritto:
>>>>>>
>>>>>> I don't see why MMIO dispatch should hold the IDEBus ref rather than the
>>>>>> PCIIDEState.
>>>>>>
>>>> When transfer memory_region_init_io()  3rd para from void* opaque to
>>>> Object* obj,  the obj : opaque is not neccessary 1:1 map. For such
>>>> situation, in order to let MemoryRegionOps tell between them, we
>>>> should pass PCIIDEState->bus[0], bus[1] separately.
>>>
>>> The rule should be that the obj is the object that you want referenced,
>>> and that should be the PCIIDEState.
>>>
>>> But this is anyway moot because it only applies to objects that are
>>> converted to use unlocked dispatch.  This likely will not be the case
>>> for IDE.
>>
>> BTW, I'm pretty sure - after implementing the basics for BQL-free PIO
>> dispatching - that device objects are the wrong target for reference
>> counting. We keep memory regions in our dispatching tables (PIO
>> dispatching needs some refactoring for this), and those regions need
>> protection for BQL-free use. Devices can't pass away as long as the have
>> referenced regions, memory region deregistration services will have to
>> take care of this.
>>
>> I'm currently not using reference counting at all, I'm enforcing that
>> only BQL-protected regions can be deregistered.
> 
> That's a pretty harsh constraint.

Of course. It's a first step, not a long-term restriction. But we have
to do small steps.

> 
>> Also note that there seems to be another misconception in the
>> discussions: deregistration is not only bound to device unplug. It also
>> happens on device reconfiguration, e.g. PCI BAR (re-)mapping. Another
>> strong indicator that we should worry about individual memory regions,
>> not devices.
>>
>>
> 
> Deregistration is fine, the problem is destruction.
> 

It isn't as you access memory region states that can change after
deregistration. Devices can remove memory regions from the mapping,
alter and then reinsert them. The last to steps must not happen while
anyone is still using a reference to that region.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 08/27/2012 10:14 AM, Jan Kiszka wrote:
> > 
> > Deregistration is fine, the problem is destruction.
> > 
>
> It isn't as you access memory region states that can change after
> deregistration. Devices can remove memory regions from the mapping,
> alter and then reinsert them. The last to steps must not happen while
> anyone is still using a reference to that region.
>

Why not?  If the guest is accessing an mmio region while reconfiguring
it in a way that changes its meaning, either the previous or the next
meaning is valid.

It is true that memory_region_set_enabled(..., false) will become weaker
as a result.  Code will have to be prepared for that.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-08-27 20:09, Avi Kivity wrote:
> On 08/27/2012 10:14 AM, Jan Kiszka wrote:
>>>
>>> Deregistration is fine, the problem is destruction.
>>>
>>
>> It isn't as you access memory region states that can change after
>> deregistration. Devices can remove memory regions from the mapping,
>> alter and then reinsert them. The last to steps must not happen while
>> anyone is still using a reference to that region.
>>
> 
> Why not?  If the guest is accessing an mmio region while reconfiguring
> it in a way that changes its meaning, either the previous or the next
> meaning is valid.

If the memory region owner sets the content to zero or even releases it
(nothing states a memory region can only live inside a device
structure), we will crash. Restricting how a memory region can be
created and handled after it was once registered somewhere is an
unnatural interface, waiting to cause subtle bugs.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 08/27/2012 11:17 AM, Jan Kiszka wrote:
> On 2012-08-27 20:09, Avi Kivity wrote:
> > On 08/27/2012 10:14 AM, Jan Kiszka wrote:
> >>>
> >>> Deregistration is fine, the problem is destruction.
> >>>
> >>
> >> It isn't as you access memory region states that can change after
> >> deregistration. Devices can remove memory regions from the mapping,
> >> alter and then reinsert them. The last to steps must not happen while
> >> anyone is still using a reference to that region.
> >>
> > 
> > Why not?  If the guest is accessing an mmio region while reconfiguring
> > it in a way that changes its meaning, either the previous or the next
> > meaning is valid.
>
> If the memory region owner sets the content to zero or even releases it
> (nothing states a memory region can only live inside a device
> structure), we will crash. Restricting how a memory region can be
> created and handled after it was once registered somewhere is an
> unnatural interface, waiting to cause subtle bugs.

Using an Object * allows the simple case to be really simple (object ==
device) and the hard cases to be doable.

What would you suggest as a better interface?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-08-27 20:20, Avi Kivity wrote:
> On 08/27/2012 11:17 AM, Jan Kiszka wrote:
>> On 2012-08-27 20:09, Avi Kivity wrote:
>>> On 08/27/2012 10:14 AM, Jan Kiszka wrote:
>>>>>
>>>>> Deregistration is fine, the problem is destruction.
>>>>>
>>>>
>>>> It isn't as you access memory region states that can change after
>>>> deregistration. Devices can remove memory regions from the mapping,
>>>> alter and then reinsert them. The last to steps must not happen while
>>>> anyone is still using a reference to that region.
>>>>
>>>
>>> Why not?  If the guest is accessing an mmio region while reconfiguring
>>> it in a way that changes its meaning, either the previous or the next
>>> meaning is valid.
>>
>> If the memory region owner sets the content to zero or even releases it
>> (nothing states a memory region can only live inside a device
>> structure), we will crash. Restricting how a memory region can be
>> created and handled after it was once registered somewhere is an
>> unnatural interface, waiting to cause subtle bugs.
> 
> Using an Object * allows the simple case to be really simple (object ==
> device) and the hard cases to be doable.
> 
> What would you suggest as a better interface?

To protect the life cycle of the object we manage in the memory layer:
regions. We don't manage devices there. If there is any implementation
benefit in having a full QOM object, then make memory regions objects.

I simply don't like this indirection, having the memory layer pick up
the opaque value of the region and interpret it. Even worse, apply
restrictions on how the dispatched objects, the regions, have to be
treated because of this.

Also, using memory regions to control the locking behaviour allows for
more fine-grained control. A device may expose certain regions with
self-managed locking while others, less time critical ones, can still be
handled under BQL for simplicity reasons.

Example: regions that translate MMIO to PIO (alpha-pci.c, every user of
isa_mmio.c, ...). If PIO dispatching runs outside of the BQL, these
regions must not be protected by the BQL anymore. At the same time, we
may not want to convert the device exposing the region to its own
locking scheme (yet). And we surely don't want to take the per-device
lock for this state-less PIO dispatching.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 08/27/2012 11:39 AM, Jan Kiszka wrote:
> On 2012-08-27 20:20, Avi Kivity wrote:
> > On 08/27/2012 11:17 AM, Jan Kiszka wrote:
> >> On 2012-08-27 20:09, Avi Kivity wrote:
> >>> On 08/27/2012 10:14 AM, Jan Kiszka wrote:
> >>>>>
> >>>>> Deregistration is fine, the problem is destruction.
> >>>>>
> >>>>
> >>>> It isn't as you access memory region states that can change after
> >>>> deregistration. Devices can remove memory regions from the mapping,
> >>>> alter and then reinsert them. The last to steps must not happen while
> >>>> anyone is still using a reference to that region.
> >>>>
> >>>
> >>> Why not?  If the guest is accessing an mmio region while reconfiguring
> >>> it in a way that changes its meaning, either the previous or the next
> >>> meaning is valid.
> >>
> >> If the memory region owner sets the content to zero or even releases it
> >> (nothing states a memory region can only live inside a device
> >> structure), we will crash. Restricting how a memory region can be
> >> created and handled after it was once registered somewhere is an
> >> unnatural interface, waiting to cause subtle bugs.
> > 
> > Using an Object * allows the simple case to be really simple (object ==
> > device) and the hard cases to be doable.
> > 
> > What would you suggest as a better interface?
>
> To protect the life cycle of the object we manage in the memory layer:
> regions. We don't manage devices there. If there is any implementation
> benefit in having a full QOM object, then make memory regions objects.

I see and sympathise with this point of view.

The idea to use opaque and convert it to Object * is that often a device
has multiple regions but no interest in managing them separately.  So
managing regions will cause the device model authors to create
boilerplate code to push region management to device management.

I had this experience with the first iterations of the region interface,
where instead of opaque we had MemoryRegion *.  Device code would use
container_of() in the simple case, but the non-simple case caused lots
of pain.  I believe it is the natural interface, but in practice it
turned out to cause too much churn.

> I simply don't like this indirection, having the memory layer pick up
> the opaque value of the region and interpret it. 

That's just an intermediate step.  The idea is that eventually opaque
will be changed to Object *.

> Even worse, apply
> restrictions on how the dispatched objects, the regions, have to be
> treated because of this.

Please elaborate.

> Also, using memory regions to control the locking behaviour allows for
> more fine-grained control. A device may expose certain regions with
> self-managed locking while others, less time critical ones, can still be
> handled under BQL for simplicity reasons.

You can still aquire the bql in one callback and your local lock in
another if you choose (but that would be most confusing IMO).

> Example: regions that translate MMIO to PIO (alpha-pci.c, every user of
> isa_mmio.c, ...). If PIO dispatching runs outside of the BQL, these
> regions must not be protected by the BQL anymore. At the same time, we
> may not want to convert the device exposing the region to its own
> locking scheme (yet). And we surely don't want to take the per-device
> lock for this state-less PIO dispatching.

We're diverging, but eventually PIO dispatch will share the same code as
mmio dispatch.  PIO-to-MMIO bridges will simply be containers or
aliases, they will not call cpu_inb() and relatives.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-08-27 20:52, Avi Kivity wrote:
> On 08/27/2012 11:39 AM, Jan Kiszka wrote:
>> On 2012-08-27 20:20, Avi Kivity wrote:
>>> On 08/27/2012 11:17 AM, Jan Kiszka wrote:
>>>> On 2012-08-27 20:09, Avi Kivity wrote:
>>>>> On 08/27/2012 10:14 AM, Jan Kiszka wrote:
>>>>>>>
>>>>>>> Deregistration is fine, the problem is destruction.
>>>>>>>
>>>>>>
>>>>>> It isn't as you access memory region states that can change after
>>>>>> deregistration. Devices can remove memory regions from the mapping,
>>>>>> alter and then reinsert them. The last to steps must not happen while
>>>>>> anyone is still using a reference to that region.
>>>>>>
>>>>>
>>>>> Why not?  If the guest is accessing an mmio region while reconfiguring
>>>>> it in a way that changes its meaning, either the previous or the next
>>>>> meaning is valid.
>>>>
>>>> If the memory region owner sets the content to zero or even releases it
>>>> (nothing states a memory region can only live inside a device
>>>> structure), we will crash. Restricting how a memory region can be
>>>> created and handled after it was once registered somewhere is an
>>>> unnatural interface, waiting to cause subtle bugs.
>>>
>>> Using an Object * allows the simple case to be really simple (object ==
>>> device) and the hard cases to be doable.
>>>
>>> What would you suggest as a better interface?
>>
>> To protect the life cycle of the object we manage in the memory layer:
>> regions. We don't manage devices there. If there is any implementation
>> benefit in having a full QOM object, then make memory regions objects.
> 
> I see and sympathise with this point of view.
> 
> The idea to use opaque and convert it to Object * is that often a device
> has multiple regions but no interest in managing them separately.  So
> managing regions will cause the device model authors to create
> boilerplate code to push region management to device management.
> 
> I had this experience with the first iterations of the region interface,
> where instead of opaque we had MemoryRegion *.  Device code would use
> container_of() in the simple case, but the non-simple case caused lots
> of pain.  I believe it is the natural interface, but in practice it
> turned out to cause too much churn.
> 
>> I simply don't like this indirection, having the memory layer pick up
>> the opaque value of the region and interpret it. 
> 
> That's just an intermediate step.  The idea is that eventually opaque
> will be changed to Object *.
> 
>> Even worse, apply
>> restrictions on how the dispatched objects, the regions, have to be
>> treated because of this.
> 
> Please elaborate.

The fact that you can't manipulate a memory region object arbitrarily
after removing it from the mapping because you track the references to
the object that the region points to, not the region itself. The region
remains in use by the dispatching layer and potentially the called
device, even after deregistration.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 08/27/2012 12:38 PM, Jan Kiszka wrote:
> >> Even worse, apply
> >> restrictions on how the dispatched objects, the regions, have to be
> >> treated because of this.
> > 
> > Please elaborate.
>
> The fact that you can't manipulate a memory region object arbitrarily
> after removing it from the mapping because you track the references to
> the object that the region points to, not the region itself. The region
> remains in use by the dispatching layer and potentially the called
> device, even after deregistration.

That object will be a container_of() the region, usually literally but
sometimes only in spirit.  Reference counting the regions means they
cannot be embedded into other objects any more.

We can probably figure out a way to flush out accesses.  After switching
to rcu, for example, all we need is synchronize_rcu() in a
non-deadlocking place.  But my bet is that it will not be needed.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1379 bytes --]

On 2012-08-27 22:53, Avi Kivity wrote:
> On 08/27/2012 12:38 PM, Jan Kiszka wrote:
>>>> Even worse, apply
>>>> restrictions on how the dispatched objects, the regions, have to be
>>>> treated because of this.
>>>
>>> Please elaborate.
>>
>> The fact that you can't manipulate a memory region object arbitrarily
>> after removing it from the mapping because you track the references to
>> the object that the region points to, not the region itself. The region
>> remains in use by the dispatching layer and potentially the called
>> device, even after deregistration.
> 
> That object will be a container_of() the region, usually literally but
> sometimes only in spirit.  Reference counting the regions means they
> cannot be embedded into other objects any more.

I cannot follow the logic of the last sentence. Reference counting just
means that we track if there are users left, not necessarily that we
perform asynchronous releases. We can simply wait for those users to
complete.

> 
> We can probably figure out a way to flush out accesses.  After switching
> to rcu, for example, all we need is synchronize_rcu() in a
> non-deadlocking place.  But my bet is that it will not be needed.

If you properly flush out accesses, you don't need to track at device
level anymore - and mess with abstraction layers. That's my whole point.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 08/27/2012 06:01 PM, Jan Kiszka wrote:
> On 2012-08-27 22:53, Avi Kivity wrote:
> > On 08/27/2012 12:38 PM, Jan Kiszka wrote:
> >>>> Even worse, apply
> >>>> restrictions on how the dispatched objects, the regions, have to be
> >>>> treated because of this.
> >>>
> >>> Please elaborate.
> >>
> >> The fact that you can't manipulate a memory region object arbitrarily
> >> after removing it from the mapping because you track the references to
> >> the object that the region points to, not the region itself. The region
> >> remains in use by the dispatching layer and potentially the called
> >> device, even after deregistration.
> > 
> > That object will be a container_of() the region, usually literally but
> > sometimes only in spirit.  Reference counting the regions means they
> > cannot be embedded into other objects any more.
>
> I cannot follow the logic of the last sentence. Reference counting just
> means that we track if there are users left, not necessarily that we
> perform asynchronous releases. We can simply wait for those users to
> complete.

I don't see how.  Suppose you add a reference count to MemoryRegion. 
How do you delay its containing object's destructor from running?  Do
you iterate over all member MemoryRegion and examine their reference counts?

Usually a reference count controls the lifetime of the reference counted
object, what are you suggesting here?

> > 
> > We can probably figure out a way to flush out accesses.  After switching
> > to rcu, for example, all we need is synchronize_rcu() in a
> > non-deadlocking place.  But my bet is that it will not be needed.
>
> If you properly flush out accesses, you don't need to track at device
> level anymore - and mess with abstraction layers. That's my whole point.

To flush out an access you need either rwlock_write_lock() or
synchronize_rcu() (depending on the implementation).  But neither of
these can be run from an rcu read-side critical section or
rwlock_read_lock().

You could defer the change to a bottom half, but if the hardware demands
that the change be complete before returning, that doesn't work.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-08-29 19:13, Avi Kivity wrote:
> On 08/27/2012 06:01 PM, Jan Kiszka wrote:
>> On 2012-08-27 22:53, Avi Kivity wrote:
>>> On 08/27/2012 12:38 PM, Jan Kiszka wrote:
>>>>>> Even worse, apply
>>>>>> restrictions on how the dispatched objects, the regions, have to be
>>>>>> treated because of this.
>>>>>
>>>>> Please elaborate.
>>>>
>>>> The fact that you can't manipulate a memory region object arbitrarily
>>>> after removing it from the mapping because you track the references to
>>>> the object that the region points to, not the region itself. The region
>>>> remains in use by the dispatching layer and potentially the called
>>>> device, even after deregistration.
>>>
>>> That object will be a container_of() the region, usually literally but
>>> sometimes only in spirit.  Reference counting the regions means they
>>> cannot be embedded into other objects any more.
>>
>> I cannot follow the logic of the last sentence. Reference counting just
>> means that we track if there are users left, not necessarily that we
>> perform asynchronous releases. We can simply wait for those users to
>> complete.
> 
> I don't see how.  Suppose you add a reference count to MemoryRegion. 
> How do you delay its containing object's destructor from running?  Do
> you iterate over all member MemoryRegion and examine their reference counts?

memory_region_transaction_commit (or calls that trigger this) will have
to wait. That is required as the caller may start fiddling with the
region right afterward.

> 
> Usually a reference count controls the lifetime of the reference counted
> object, what are you suggesting here?

To synchronize on reference count going to zero. Or all readers leaving
the read-side critical sections.

> 
>>>
>>> We can probably figure out a way to flush out accesses.  After switching
>>> to rcu, for example, all we need is synchronize_rcu() in a
>>> non-deadlocking place.  But my bet is that it will not be needed.
>>
>> If you properly flush out accesses, you don't need to track at device
>> level anymore - and mess with abstraction layers. That's my whole point.
> 
> To flush out an access you need either rwlock_write_lock() or
> synchronize_rcu() (depending on the implementation).  But neither of
> these can be run from an rcu read-side critical section or
> rwlock_read_lock().
> 
> You could defer the change to a bottom half, but if the hardware demands
> that the change be complete before returning, that doesn't work.

Right, therefore synchronous transactions.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 08/29/2012 10:21 AM, Jan Kiszka wrote:
> On 2012-08-29 19:13, Avi Kivity wrote:
> > On 08/27/2012 06:01 PM, Jan Kiszka wrote:
> >> On 2012-08-27 22:53, Avi Kivity wrote:
> >>> On 08/27/2012 12:38 PM, Jan Kiszka wrote:
> >>>>>> Even worse, apply
> >>>>>> restrictions on how the dispatched objects, the regions, have to be
> >>>>>> treated because of this.
> >>>>>
> >>>>> Please elaborate.
> >>>>
> >>>> The fact that you can't manipulate a memory region object arbitrarily
> >>>> after removing it from the mapping because you track the references to
> >>>> the object that the region points to, not the region itself. The region
> >>>> remains in use by the dispatching layer and potentially the called
> >>>> device, even after deregistration.
> >>>
> >>> That object will be a container_of() the region, usually literally but
> >>> sometimes only in spirit.  Reference counting the regions means they
> >>> cannot be embedded into other objects any more.
> >>
> >> I cannot follow the logic of the last sentence. Reference counting just
> >> means that we track if there are users left, not necessarily that we
> >> perform asynchronous releases. We can simply wait for those users to
> >> complete.
> > 
> > I don't see how.  Suppose you add a reference count to MemoryRegion. 
> > How do you delay its containing object's destructor from running?  Do
> > you iterate over all member MemoryRegion and examine their reference counts?
>
> memory_region_transaction_commit (or calls that trigger this) will have
> to wait. That is required as the caller may start fiddling with the
> region right afterward.

However, it may be running within an mmio dispatch, so it may wait forever.

Ignoring this, how does it wait? sleep()? or wait for a completion?

> > 
> > Usually a reference count controls the lifetime of the reference counted
> > object, what are you suggesting here?
>
> To synchronize on reference count going to zero. 

Quite unorthodox.  Do you have real life examples?

> Or all readers leaving
> the read-side critical sections.

This is rcu.  But again, we may be in an rcu read-side critical section
while needing to wait.  In fact this was what I suggested in the first
place, until Marcelo pointed out the deadlock, so we came up with the
refcount.

>
> > 
> >>>
> >>> We can probably figure out a way to flush out accesses.  After switching
> >>> to rcu, for example, all we need is synchronize_rcu() in a
> >>> non-deadlocking place.  But my bet is that it will not be needed.
> >>
> >> If you properly flush out accesses, you don't need to track at device
> >> level anymore - and mess with abstraction layers. That's my whole point.
> > 
> > To flush out an access you need either rwlock_write_lock() or
> > synchronize_rcu() (depending on the implementation).  But neither of
> > these can be run from an rcu read-side critical section or
> > rwlock_read_lock().
> > 
> > You could defer the change to a bottom half, but if the hardware demands
> > that the change be complete before returning, that doesn't work.
>
> Right, therefore synchronous transactions.

I don't follow.  Synchronous transactions mean you can't
synchronize_rcu() or upgrade the lock or wait for the refcount.  That's
the source of the problem!

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-08-29 19:27, Avi Kivity wrote:
> On 08/29/2012 10:21 AM, Jan Kiszka wrote:
>> On 2012-08-29 19:13, Avi Kivity wrote:
>>> On 08/27/2012 06:01 PM, Jan Kiszka wrote:
>>>> On 2012-08-27 22:53, Avi Kivity wrote:
>>>>> On 08/27/2012 12:38 PM, Jan Kiszka wrote:
>>>>>>>> Even worse, apply
>>>>>>>> restrictions on how the dispatched objects, the regions, have to be
>>>>>>>> treated because of this.
>>>>>>>
>>>>>>> Please elaborate.
>>>>>>
>>>>>> The fact that you can't manipulate a memory region object arbitrarily
>>>>>> after removing it from the mapping because you track the references to
>>>>>> the object that the region points to, not the region itself. The region
>>>>>> remains in use by the dispatching layer and potentially the called
>>>>>> device, even after deregistration.
>>>>>
>>>>> That object will be a container_of() the region, usually literally but
>>>>> sometimes only in spirit.  Reference counting the regions means they
>>>>> cannot be embedded into other objects any more.
>>>>
>>>> I cannot follow the logic of the last sentence. Reference counting just
>>>> means that we track if there are users left, not necessarily that we
>>>> perform asynchronous releases. We can simply wait for those users to
>>>> complete.
>>>
>>> I don't see how.  Suppose you add a reference count to MemoryRegion. 
>>> How do you delay its containing object's destructor from running?  Do
>>> you iterate over all member MemoryRegion and examine their reference counts?
>>
>> memory_region_transaction_commit (or calls that trigger this) will have
>> to wait. That is required as the caller may start fiddling with the
>> region right afterward.
> 
> However, it may be running within an mmio dispatch, so it may wait forever.

We won't be able to wait for devices that can acquire the same lock we
hold while waiting, of course. That's why a lock ordering device-lock ->
BQL (the one we hold over memory region changes) won't be allowed. I was
wrong on this before: We must not take course-grained locks while
holding fine-grained ones, only the other way around. Pretty obvious,
specifically for realtime / low-latency.

> 
> Ignoring this, how does it wait? sleep()? or wait for a completion?

Ideally via completion.

> 
>>>
>>> Usually a reference count controls the lifetime of the reference counted
>>> object, what are you suggesting here?
>>
>> To synchronize on reference count going to zero. 
> 
> Quite unorthodox.  Do you have real life examples?

synchronize_rcu.

> 
>> Or all readers leaving
>> the read-side critical sections.
> 
> This is rcu.  But again, we may be in an rcu read-side critical section
> while needing to wait.  In fact this was what I suggested in the first
> place, until Marcelo pointed out the deadlock, so we came up with the
> refcount.

We must avoid that deadlock scenario. I bended my brain around it, and I
think that is the only answer. We can if we apply clear rules regarding
locking and BQL-protected services to those devices that will actually
use fine-grained locking, and there only for those paths that take
per-device locks. I'm pretty sure we won't get far with an
"all-or-nothing" model where every device uses a private lock.

> 
>>
>>>
>>>>>
>>>>> We can probably figure out a way to flush out accesses.  After switching
>>>>> to rcu, for example, all we need is synchronize_rcu() in a
>>>>> non-deadlocking place.  But my bet is that it will not be needed.
>>>>
>>>> If you properly flush out accesses, you don't need to track at device
>>>> level anymore - and mess with abstraction layers. That's my whole point.
>>>
>>> To flush out an access you need either rwlock_write_lock() or
>>> synchronize_rcu() (depending on the implementation).  But neither of
>>> these can be run from an rcu read-side critical section or
>>> rwlock_read_lock().
>>>
>>> You could defer the change to a bottom half, but if the hardware demands
>>> that the change be complete before returning, that doesn't work.
>>
>> Right, therefore synchronous transactions.
> 
> I don't follow.  Synchronous transactions mean you can't
> synchronize_rcu() or upgrade the lock or wait for the refcount.  That's
> the source of the problem!

Our current device models assume synchronous transactions on the memory
layer, actually on all layers. The proposals I saw so far try to change
this. But I'm very skeptical that this would succeed, due to the
conversion effort and due to the fact that it would give us a tricky  to
use API.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 08/29/2012 08:41 PM, Jan Kiszka wrote:
>>>
>>> memory_region_transaction_commit (or calls that trigger this) will have
>>> to wait. That is required as the caller may start fiddling with the
>>> region right afterward.
>> 
>> However, it may be running within an mmio dispatch, so it may wait forever.
> 
> We won't be able to wait for devices that can acquire the same lock we
> hold while waiting, of course. That's why a lock ordering device-lock ->
> BQL (the one we hold over memory region changes) won't be allowed. I was
> wrong on this before: We must not take course-grained locks while
> holding fine-grained ones, only the other way around. Pretty obvious,
> specifically for realtime / low-latency.

So how do we wait?

Detecting that we're in the same thread and allowing reentrancy in that
case in the option, but I can't say I like it much.

>> 
>> Ignoring this, how does it wait? sleep()? or wait for a completion?
> 
> Ideally via completion.

Then you have to manage the life cycle of the completion object.

> 
>> 
>>>>
>>>> Usually a reference count controls the lifetime of the reference counted
>>>> object, what are you suggesting here?
>>>
>>> To synchronize on reference count going to zero. 
>> 
>> Quite unorthodox.  Do you have real life examples?
> 
> synchronize_rcu.

This whole thing started because synchronize_rcu() will deadlock if
called from a read-side critical section.  The fix was taking the
reference, so that the read-side critical section is confined to the
lookup, not the call.  This way the lock ordering is always device->map.

We could schedule a bh or a thread and call synchronize_rcu() from
there, but then the commit is deferred to an unknown time, whereas we
need to guarantee that by the time the guest is reentered, the
transaction is committed.

> 
>> 
>>> Or all readers leaving
>>> the read-side critical sections.
>> 
>> This is rcu.  But again, we may be in an rcu read-side critical section
>> while needing to wait.  In fact this was what I suggested in the first
>> place, until Marcelo pointed out the deadlock, so we came up with the
>> refcount.
> 
> We must avoid that deadlock scenario. I bended my brain around it, and I
> think that is the only answer. We can if we apply clear rules regarding
> locking and BQL-protected services to those devices that will actually
> use fine-grained locking, and there only for those paths that take
> per-device locks. I'm pretty sure we won't get far with an
> "all-or-nothing" model where every device uses a private lock.

An all-or-nothing model is not proposed.  Unconverted devices will take
the BQL instead of the device lock (hopefully the BQL will be taken for
them during dispatch, so no code changes will be needed).

>> 
>> I don't follow.  Synchronous transactions mean you can't
>> synchronize_rcu() or upgrade the lock or wait for the refcount.  That's
>> the source of the problem!
> 
> Our current device models assume synchronous transactions on the memory
> layer, actually on all layers. The proposals I saw so far try to change
> this. But I'm very skeptical that this would succeed, due to the
> conversion effort and due to the fact that it would give us a tricky  to
> use API.

In what way do transactions become asynchronous?

It's true that io callbacks can be called concurrently, but as soon as
they take the device lock, they are serialized.  The only relaxation is
that a callback may be called after a region has been disabled or
removed from view.

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

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[liu ping fan]

On Tue, Aug 28, 2012 at 3:38 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2012-08-27 20:52, Avi Kivity wrote:
>> On 08/27/2012 11:39 AM, Jan Kiszka wrote:
>>> On 2012-08-27 20:20, Avi Kivity wrote:
>>>> On 08/27/2012 11:17 AM, Jan Kiszka wrote:
>>>>> On 2012-08-27 20:09, Avi Kivity wrote:
>>>>>> On 08/27/2012 10:14 AM, Jan Kiszka wrote:
>>>>>>>>
>>>>>>>> Deregistration is fine, the problem is destruction.
>>>>>>>>
>>>>>>>
>>>>>>> It isn't as you access memory region states that can change after
>>>>>>> deregistration. Devices can remove memory regions from the mapping,
>>>>>>> alter and then reinsert them. The last to steps must not happen while
>>>>>>> anyone is still using a reference to that region.
>>>>>>>
>>>>>>
>>>>>> Why not?  If the guest is accessing an mmio region while reconfiguring
>>>>>> it in a way that changes its meaning, either the previous or the next
>>>>>> meaning is valid.
>>>>>
>>>>> If the memory region owner sets the content to zero or even releases it
>>>>> (nothing states a memory region can only live inside a device
>>>>> structure), we will crash. Restricting how a memory region can be
>>>>> created and handled after it was once registered somewhere is an
>>>>> unnatural interface, waiting to cause subtle bugs.
>>>>
>>>> Using an Object * allows the simple case to be really simple (object ==
>>>> device) and the hard cases to be doable.
>>>>
>>>> What would you suggest as a better interface?
>>>
>>> To protect the life cycle of the object we manage in the memory layer:
>>> regions. We don't manage devices there. If there is any implementation
>>> benefit in having a full QOM object, then make memory regions objects.
>>
>> I see and sympathise with this point of view.
>>
>> The idea to use opaque and convert it to Object * is that often a device
>> has multiple regions but no interest in managing them separately.  So
>> managing regions will cause the device model authors to create
>> boilerplate code to push region management to device management.
>>
>> I had this experience with the first iterations of the region interface,
>> where instead of opaque we had MemoryRegion *.  Device code would use
>> container_of() in the simple case, but the non-simple case caused lots
>> of pain.  I believe it is the natural interface, but in practice it
>> turned out to cause too much churn.
>>
>>> I simply don't like this indirection, having the memory layer pick up
>>> the opaque value of the region and interpret it.
>>
>> That's just an intermediate step.  The idea is that eventually opaque
>> will be changed to Object *.
>>
>>> Even worse, apply
>>> restrictions on how the dispatched objects, the regions, have to be
>>> treated because of this.
>>
>> Please elaborate.
>
> The fact that you can't manipulate a memory region object arbitrarily
> after removing it from the mapping because you track the references to
> the object that the region points to, not the region itself. The region
> remains in use by the dispatching layer and potentially the called
> device, even after deregistration.

Why can it in use by dispatching layer? Memory region just servers as
interface for mem lookup, when dispatching in mr's MemoryRegionOps,
why do we need to access it?  Which means that read/write can cause
register/deregister mr?  Example?
Also I think MemoryRegionOps will eventually operate on Object (mr is
only interface). MRs is only represent of Object in the view of
memory(so their life cycle can be managed by Object). And there could
be some intermediate mr like those lie in subpage_t which are not
based on Object. But we can walk down to the terminal point and
extract the real Object,
so when dispatching, we only need to ensure that Object and its direct
mr are alive.

Regards,
pingfan
>
> Jan
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[liu ping fan]

On Tue, Aug 28, 2012 at 11:09 AM, liu ping fan <qemulist@gmail.com> wrote:
> On Tue, Aug 28, 2012 at 3:38 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2012-08-27 20:52, Avi Kivity wrote:
>>> On 08/27/2012 11:39 AM, Jan Kiszka wrote:
>>>> On 2012-08-27 20:20, Avi Kivity wrote:
>>>>> On 08/27/2012 11:17 AM, Jan Kiszka wrote:
>>>>>> On 2012-08-27 20:09, Avi Kivity wrote:
>>>>>>> On 08/27/2012 10:14 AM, Jan Kiszka wrote:
>>>>>>>>>
>>>>>>>>> Deregistration is fine, the problem is destruction.
>>>>>>>>>
>>>>>>>>
>>>>>>>> It isn't as you access memory region states that can change after
>>>>>>>> deregistration. Devices can remove memory regions from the mapping,
>>>>>>>> alter and then reinsert them. The last to steps must not happen while
>>>>>>>> anyone is still using a reference to that region.
>>>>>>>>
>>>>>>>
>>>>>>> Why not?  If the guest is accessing an mmio region while reconfiguring
>>>>>>> it in a way that changes its meaning, either the previous or the next
>>>>>>> meaning is valid.
>>>>>>
>>>>>> If the memory region owner sets the content to zero or even releases it
>>>>>> (nothing states a memory region can only live inside a device
>>>>>> structure), we will crash. Restricting how a memory region can be
>>>>>> created and handled after it was once registered somewhere is an
>>>>>> unnatural interface, waiting to cause subtle bugs.
>>>>>
>>>>> Using an Object * allows the simple case to be really simple (object ==
>>>>> device) and the hard cases to be doable.
>>>>>
>>>>> What would you suggest as a better interface?
>>>>
>>>> To protect the life cycle of the object we manage in the memory layer:
>>>> regions. We don't manage devices there. If there is any implementation
>>>> benefit in having a full QOM object, then make memory regions objects.
>>>
>>> I see and sympathise with this point of view.
>>>
>>> The idea to use opaque and convert it to Object * is that often a device
>>> has multiple regions but no interest in managing them separately.  So
>>> managing regions will cause the device model authors to create
>>> boilerplate code to push region management to device management.
>>>
>>> I had this experience with the first iterations of the region interface,
>>> where instead of opaque we had MemoryRegion *.  Device code would use
>>> container_of() in the simple case, but the non-simple case caused lots
>>> of pain.  I believe it is the natural interface, but in practice it
>>> turned out to cause too much churn.
>>>
>>>> I simply don't like this indirection, having the memory layer pick up
>>>> the opaque value of the region and interpret it.
>>>
>>> That's just an intermediate step.  The idea is that eventually opaque
>>> will be changed to Object *.
>>>
>>>> Even worse, apply
>>>> restrictions on how the dispatched objects, the regions, have to be
>>>> treated because of this.
>>>
>>> Please elaborate.
>>
>> The fact that you can't manipulate a memory region object arbitrarily
>> after removing it from the mapping because you track the references to
>> the object that the region points to, not the region itself. The region
>> remains in use by the dispatching layer and potentially the called
>> device, even after deregistration.
>
> Why can it in use by dispatching layer? Memory region just servers as
> interface for mem lookup, when dispatching in mr's MemoryRegionOps,
> why do we need to access it?  Which means that read/write can cause
> register/deregister mr?  Example?

I found example in pci_update_mappings(), but but my following point
still stand.
> Also I think MemoryRegionOps will eventually operate on Object (mr is
> only interface). MRs is only represent of Object in the view of
> memory(so their life cycle can be managed by Object). And there could
> be some intermediate mr like those lie in subpage_t which are not
> based on Object. But we can walk down to the terminal point and
> extract the real Object,
> so when dispatching, we only need to ensure that Object and its direct
> mr are alive.
>
> Regards,
> pingfan
>>
>> Jan
>>
>> --
>> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
>> Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-08-28 05:38, liu ping fan wrote:
> On Tue, Aug 28, 2012 at 11:09 AM, liu ping fan <qemulist@gmail.com> wrote:
>> On Tue, Aug 28, 2012 at 3:38 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2012-08-27 20:52, Avi Kivity wrote:
>>>> On 08/27/2012 11:39 AM, Jan Kiszka wrote:
>>>>> On 2012-08-27 20:20, Avi Kivity wrote:
>>>>>> On 08/27/2012 11:17 AM, Jan Kiszka wrote:
>>>>>>> On 2012-08-27 20:09, Avi Kivity wrote:
>>>>>>>> On 08/27/2012 10:14 AM, Jan Kiszka wrote:
>>>>>>>>>>
>>>>>>>>>> Deregistration is fine, the problem is destruction.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> It isn't as you access memory region states that can change after
>>>>>>>>> deregistration. Devices can remove memory regions from the mapping,
>>>>>>>>> alter and then reinsert them. The last to steps must not happen while
>>>>>>>>> anyone is still using a reference to that region.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Why not?  If the guest is accessing an mmio region while reconfiguring
>>>>>>>> it in a way that changes its meaning, either the previous or the next
>>>>>>>> meaning is valid.
>>>>>>>
>>>>>>> If the memory region owner sets the content to zero or even releases it
>>>>>>> (nothing states a memory region can only live inside a device
>>>>>>> structure), we will crash. Restricting how a memory region can be
>>>>>>> created and handled after it was once registered somewhere is an
>>>>>>> unnatural interface, waiting to cause subtle bugs.
>>>>>>
>>>>>> Using an Object * allows the simple case to be really simple (object ==
>>>>>> device) and the hard cases to be doable.
>>>>>>
>>>>>> What would you suggest as a better interface?
>>>>>
>>>>> To protect the life cycle of the object we manage in the memory layer:
>>>>> regions. We don't manage devices there. If there is any implementation
>>>>> benefit in having a full QOM object, then make memory regions objects.
>>>>
>>>> I see and sympathise with this point of view.
>>>>
>>>> The idea to use opaque and convert it to Object * is that often a device
>>>> has multiple regions but no interest in managing them separately.  So
>>>> managing regions will cause the device model authors to create
>>>> boilerplate code to push region management to device management.
>>>>
>>>> I had this experience with the first iterations of the region interface,
>>>> where instead of opaque we had MemoryRegion *.  Device code would use
>>>> container_of() in the simple case, but the non-simple case caused lots
>>>> of pain.  I believe it is the natural interface, but in practice it
>>>> turned out to cause too much churn.
>>>>
>>>>> I simply don't like this indirection, having the memory layer pick up
>>>>> the opaque value of the region and interpret it.
>>>>
>>>> That's just an intermediate step.  The idea is that eventually opaque
>>>> will be changed to Object *.
>>>>
>>>>> Even worse, apply
>>>>> restrictions on how the dispatched objects, the regions, have to be
>>>>> treated because of this.
>>>>
>>>> Please elaborate.
>>>
>>> The fact that you can't manipulate a memory region object arbitrarily
>>> after removing it from the mapping because you track the references to
>>> the object that the region points to, not the region itself. The region
>>> remains in use by the dispatching layer and potentially the called
>>> device, even after deregistration.
>>
>> Why can it in use by dispatching layer? Memory region just servers as
>> interface for mem lookup, when dispatching in mr's MemoryRegionOps,
>> why do we need to access it?  Which means that read/write can cause
>> register/deregister mr?  Example?
> 
> I found example in pci_update_mappings(), but but my following point
> still stand.
>> Also I think MemoryRegionOps will eventually operate on Object (mr is
>> only interface). MRs is only represent of Object in the view of
>> memory(so their life cycle can be managed by Object). And there could
>> be some intermediate mr like those lie in subpage_t which are not
>> based on Object. But we can walk down to the terminal point and
>> extract the real Object,
>> so when dispatching, we only need to ensure that Object and its direct
>> mr are alive.

Let's not talk about devices or MMIO dispatching. I think the problem is
way more generic, and we will face it multiple times in QEMU. Besides
MMIO and PIO dispatching, it will haunt us for file or event handlers,
any kind of callbacks etc.

Context A                               Context B
---------                               ---------
                                        object = lookup()
deregister(object)
modify(object) -> invalid state
...                                     use(object)
modify(object) -> valid state
register(object)

And with "object" I'm not talking about QOM but any data structure.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Paolo Bonzini]

Il 28/08/2012 11:42, Jan Kiszka ha scritto:
> Context A                               Context B
> ---------                               ---------
>                                         object = lookup()
> deregister(object)
> modify(object) -> invalid state
> ...                                     use(object)
> modify(object) -> valid state
> register(object)
> 
> And with "object" I'm not talking about QOM but any data structure.

If you want to avoid locks, the only way to do so is RCU.  Then you do
not modify object at all.

Paolo

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 08/28/2012 02:42 AM, Jan Kiszka wrote:
>
> Let's not talk about devices or MMIO dispatching. I think the problem is
> way more generic, and we will face it multiple times in QEMU. 

The problem exists outside qemu as well.  It is one of the reasons for
the popularity of garbage collection IMO, and the use of reference
counting when GC is not available.

This pattern is even documented in
Documentation/DocBook/kernel-locking.tmpl:

@@ -104,12 +114,11 @@
 struct object *cache_find(int id)
 {
         struct object *obj;
-        unsigned long flags;

-        spin_lock_irqsave(&cache_lock, flags);
+        rcu_read_lock();
         obj = __cache_find(id);
         if (obj)
                 object_get(obj);
-        spin_unlock_irqrestore(&cache_lock, flags);
+        rcu_read_unlock();
  

Of course that doesn't mean we should use it, but at least it indicates
that it is a standard pattern.  With MemoryRegion the pattern is
changed, since MemoryRegion is a thunk, not the object we're really
dispatching to.

> Besides
> MMIO and PIO dispatching, it will haunt us for file or event handlers,
> any kind of callbacks etc.
>
> Context A                               Context B
> ---------                               ---------
>                                         object = lookup()
> deregister(object)
> modify(object) -> invalid state
> ...                                     use(object)
> modify(object) -> valid state
> register(object)
>
> And with "object" I'm not talking about QOM but any data structure.
>


Context B
---------
rcu_read_lock()
object = lookup()
if (object) {
    ref(object)
}
rcu_read_unlock()

use(object)

unref(object)

(substitute locking scheme to conform to taste and/or dietary
restrictions)   

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-08-29 19:23, Avi Kivity wrote:
> On 08/28/2012 02:42 AM, Jan Kiszka wrote:
>>
>> Let's not talk about devices or MMIO dispatching. I think the problem is
>> way more generic, and we will face it multiple times in QEMU. 
> 
> The problem exists outside qemu as well.  It is one of the reasons for
> the popularity of garbage collection IMO, and the use of reference
> counting when GC is not available.
> 
> This pattern is even documented in
> Documentation/DocBook/kernel-locking.tmpl:
> 
> @@ -104,12 +114,11 @@
>  struct object *cache_find(int id)
>  {
>          struct object *obj;
> -        unsigned long flags;
> 
> -        spin_lock_irqsave(&cache_lock, flags);
> +        rcu_read_lock();
>          obj = __cache_find(id);
>          if (obj)
>                  object_get(obj);
> -        spin_unlock_irqrestore(&cache_lock, flags);
> +        rcu_read_unlock();
>   
> 
> Of course that doesn't mean we should use it, but at least it indicates
> that it is a standard pattern.  With MemoryRegion the pattern is
> changed, since MemoryRegion is a thunk, not the object we're really
> dispatching to.

We are dispatching according to the memory region (parameters, op
handlers, opaques). If we end up in device object is not decided at this
level. A memory region describes a dispatchable area - not to confuse
with a device that may only partially be able to receive such requests.

> 
>> Besides
>> MMIO and PIO dispatching, it will haunt us for file or event handlers,
>> any kind of callbacks etc.
>>
>> Context A                               Context B
>> ---------                               ---------
>>                                         object = lookup()
>> deregister(object)
>> modify(object) -> invalid state
>> ...                                     use(object)
>> modify(object) -> valid state
>> register(object)
>>
>> And with "object" I'm not talking about QOM but any data structure.
>>
> 
> 
> Context B
> ---------
> rcu_read_lock()
> object = lookup()
> if (object) {
>     ref(object)
> }
> rcu_read_unlock()
> 
> use(object)
> 
> unref(object)
> 
> (substitute locking scheme to conform to taste and/or dietary
> restrictions)   
> 

+ wait for refcount(object) == 0 in deregister(object). That's what I'm
proposing.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 08/29/2012 10:30 AM, Jan Kiszka wrote:
> On 2012-08-29 19:23, Avi Kivity wrote:
> > On 08/28/2012 02:42 AM, Jan Kiszka wrote:
> >>
> >> Let's not talk about devices or MMIO dispatching. I think the problem is
> >> way more generic, and we will face it multiple times in QEMU. 
> > 
> > The problem exists outside qemu as well.  It is one of the reasons for
> > the popularity of garbage collection IMO, and the use of reference
> > counting when GC is not available.
> > 
> > This pattern is even documented in
> > Documentation/DocBook/kernel-locking.tmpl:
> > 
> > @@ -104,12 +114,11 @@
> >  struct object *cache_find(int id)
> >  {
> >          struct object *obj;
> > -        unsigned long flags;
> > 
> > -        spin_lock_irqsave(&cache_lock, flags);
> > +        rcu_read_lock();
> >          obj = __cache_find(id);
> >          if (obj)
> >                  object_get(obj);
> > -        spin_unlock_irqrestore(&cache_lock, flags);
> > +        rcu_read_unlock();
> >   
> > 
> > Of course that doesn't mean we should use it, but at least it indicates
> > that it is a standard pattern.  With MemoryRegion the pattern is
> > changed, since MemoryRegion is a thunk, not the object we're really
> > dispatching to.
>
> We are dispatching according to the memory region (parameters, op
> handlers, opaques). If we end up in device object is not decided at this
> level. A memory region describes a dispatchable area - not to confuse
> with a device that may only partially be able to receive such requests.

Correct.

Let's experiment with refcounting MemoryRegion.  We can move the entire
contents of MemoryRegion to MemoryRegionImpl, add a reference count (to
MemoryRegionImpl), and change MemoryRegion to contain a pointer to the
refcounted MemoryRegionImpl:

struct MemoryRegion {
    struct MemoryRegionImpl *impl;
};

struct MemoryRegionImpl {
    atomic int refs;
    ...
};

The memory core can then store MemoryRegion copies (with elevated
refcounts) instead of pointers.  Devices can destroy MemoryRegions at
any time, the implementation will not go away.  However, what of the
opaque stored in MemoryRegionImpl?  It becomes a dangling pointer.

One way out is to add a lock to MemoryRegionImpl.  Dispatch takes the
lock, examines the ->enabled member, and bails out if it is cleared. 
The (MemoryRegion, not MemoryRegionImpl) destructor also takes the lock,
clears ->enabled,  releases the lock, and drops the reference.

The advantage to this scheme is that all changes are localized to the
memory core, no need for a full sweep.  It is a little complicated, but
we may be able to simplify it (or find another one).

>
> > 
> >> Besides
> >> MMIO and PIO dispatching, it will haunt us for file or event handlers,
> >> any kind of callbacks etc.
> >>
> >> Context A                               Context B
> >> ---------                               ---------
> >>                                         object = lookup()
> >> deregister(object)
> >> modify(object) -> invalid state
> >> ...                                     use(object)
> >> modify(object) -> valid state
> >> register(object)
> >>
> >> And with "object" I'm not talking about QOM but any data structure.
> >>
> > 
> > 
> > Context B
> > ---------
> > rcu_read_lock()
> > object = lookup()
> > if (object) {
> >     ref(object)
> > }
> > rcu_read_unlock()
> > 
> > use(object)
> > 
> > unref(object)
> > 
> > (substitute locking scheme to conform to taste and/or dietary
> > restrictions)   
> > 
>
> + wait for refcount(object) == 0 in deregister(object). That's what I'm
> proposing.

Consider timer_del() called from a timer callback.  It's often not doable.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-08-29 19:40, Avi Kivity wrote:
> On 08/29/2012 10:30 AM, Jan Kiszka wrote:
>> On 2012-08-29 19:23, Avi Kivity wrote:
>>> On 08/28/2012 02:42 AM, Jan Kiszka wrote:
>>>>
>>>> Let's not talk about devices or MMIO dispatching. I think the problem is
>>>> way more generic, and we will face it multiple times in QEMU. 
>>>
>>> The problem exists outside qemu as well.  It is one of the reasons for
>>> the popularity of garbage collection IMO, and the use of reference
>>> counting when GC is not available.
>>>
>>> This pattern is even documented in
>>> Documentation/DocBook/kernel-locking.tmpl:
>>>
>>> @@ -104,12 +114,11 @@
>>>  struct object *cache_find(int id)
>>>  {
>>>          struct object *obj;
>>> -        unsigned long flags;
>>>
>>> -        spin_lock_irqsave(&cache_lock, flags);
>>> +        rcu_read_lock();
>>>          obj = __cache_find(id);
>>>          if (obj)
>>>                  object_get(obj);
>>> -        spin_unlock_irqrestore(&cache_lock, flags);
>>> +        rcu_read_unlock();
>>>   
>>>
>>> Of course that doesn't mean we should use it, but at least it indicates
>>> that it is a standard pattern.  With MemoryRegion the pattern is
>>> changed, since MemoryRegion is a thunk, not the object we're really
>>> dispatching to.
>>
>> We are dispatching according to the memory region (parameters, op
>> handlers, opaques). If we end up in device object is not decided at this
>> level. A memory region describes a dispatchable area - not to confuse
>> with a device that may only partially be able to receive such requests.
> 
> Correct.
> 
> Let's experiment with refcounting MemoryRegion.  We can move the entire
> contents of MemoryRegion to MemoryRegionImpl, add a reference count (to
> MemoryRegionImpl), and change MemoryRegion to contain a pointer to the
> refcounted MemoryRegionImpl:
> 
> struct MemoryRegion {
>     struct MemoryRegionImpl *impl;
> };
> 
> struct MemoryRegionImpl {
>     atomic int refs;
>     ...
> };
> 
> The memory core can then store MemoryRegion copies (with elevated
> refcounts) instead of pointers.  Devices can destroy MemoryRegions at
> any time, the implementation will not go away.  However, what of the
> opaque stored in MemoryRegionImpl?  It becomes a dangling pointer.
> 
> One way out is to add a lock to MemoryRegionImpl.  Dispatch takes the
> lock, examines the ->enabled member, and bails out if it is cleared. 
> The (MemoryRegion, not MemoryRegionImpl) destructor also takes the lock,
> clears ->enabled,  releases the lock, and drops the reference.

That means holding the MemoryRegionImpl lock across the handler call?

> 
> The advantage to this scheme is that all changes are localized to the
> memory core, no need for a full sweep.  It is a little complicated, but
> we may be able to simplify it (or find another one).

May work. We just need to detect if memory region tries to delete itself
from its handler to prevent the deadlock.

> 
>>
>>>
>>>> Besides
>>>> MMIO and PIO dispatching, it will haunt us for file or event handlers,
>>>> any kind of callbacks etc.
>>>>
>>>> Context A                               Context B
>>>> ---------                               ---------
>>>>                                         object = lookup()
>>>> deregister(object)
>>>> modify(object) -> invalid state
>>>> ...                                     use(object)
>>>> modify(object) -> valid state
>>>> register(object)
>>>>
>>>> And with "object" I'm not talking about QOM but any data structure.
>>>>
>>>
>>>
>>> Context B
>>> ---------
>>> rcu_read_lock()
>>> object = lookup()
>>> if (object) {
>>>     ref(object)
>>> }
>>> rcu_read_unlock()
>>>
>>> use(object)
>>>
>>> unref(object)
>>>
>>> (substitute locking scheme to conform to taste and/or dietary
>>> restrictions)   
>>>
>>
>> + wait for refcount(object) == 0 in deregister(object). That's what I'm
>> proposing.
> 
> Consider timer_del() called from a timer callback.  It's often not doable.

This is inherently synchronous already (when working against the same
alarm timer backend). We can detect this in timer_del and skip waiting,
as in the above scenario.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 08/29/2012 10:49 AM, Jan Kiszka wrote:
> > 
> > Let's experiment with refcounting MemoryRegion.  We can move the entire
> > contents of MemoryRegion to MemoryRegionImpl, add a reference count (to
> > MemoryRegionImpl), and change MemoryRegion to contain a pointer to the
> > refcounted MemoryRegionImpl:
> > 
> > struct MemoryRegion {
> >     struct MemoryRegionImpl *impl;
> > };
> > 
> > struct MemoryRegionImpl {
> >     atomic int refs;
> >     ...
> > };
> > 
> > The memory core can then store MemoryRegion copies (with elevated
> > refcounts) instead of pointers.  Devices can destroy MemoryRegions at
> > any time, the implementation will not go away.  However, what of the
> > opaque stored in MemoryRegionImpl?  It becomes a dangling pointer.
> > 
> > One way out is to add a lock to MemoryRegionImpl.  Dispatch takes the
> > lock, examines the ->enabled member, and bails out if it is cleared. 
> > The (MemoryRegion, not MemoryRegionImpl) destructor also takes the lock,
> > clears ->enabled,  releases the lock, and drops the reference.
>
> That means holding the MemoryRegionImpl lock across the handler call?

Blech.  As you said on the other side of this thread, we must not take a
coarse grained lock within a fine grained one, and
MemoryRegionImpl::lock is as fine as they get.

> > 
> > The advantage to this scheme is that all changes are localized to the
> > memory core, no need for a full sweep.  It is a little complicated, but
> > we may be able to simplify it (or find another one).
>
> May work. We just need to detect if memory region tries to delete itself
> from its handler to prevent the deadlock.

Those types of hacks are fragile.  IMO it just demonstrates what I said
earlier (then tried to disprove with this): if we call an opaque's
method, we must refcount or otherwise lock that opaque.  No way around it.

>
> > 
> >>
> >>>
> >>>> Besides
> >>>> MMIO and PIO dispatching, it will haunt us for file or event handlers,
> >>>> any kind of callbacks etc.
> >>>>
> >>>> Context A                               Context B
> >>>> ---------                               ---------
> >>>>                                         object = lookup()
> >>>> deregister(object)
> >>>> modify(object) -> invalid state
> >>>> ...                                     use(object)
> >>>> modify(object) -> valid state
> >>>> register(object)
> >>>>
> >>>> And with "object" I'm not talking about QOM but any data structure.
> >>>>
> >>>
> >>>
> >>> Context B
> >>> ---------
> >>> rcu_read_lock()
> >>> object = lookup()
> >>> if (object) {
> >>>     ref(object)
> >>> }
> >>> rcu_read_unlock()
> >>>
> >>> use(object)
> >>>
> >>> unref(object)
> >>>
> >>> (substitute locking scheme to conform to taste and/or dietary
> >>> restrictions)   
> >>>
> >>
> >> + wait for refcount(object) == 0 in deregister(object). That's what I'm
> >> proposing.
> > 
> > Consider timer_del() called from a timer callback.  It's often not doable.
>
> This is inherently synchronous already (when working against the same
> alarm timer backend). We can detect this in timer_del and skip waiting,
> as in the above scenario.

It can always be broken.  The timer callback takes the device lock to
update the device.  The device mmio handler, holding the device lock,
takes the timer lock to timer_mod.  Deadlock.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4104 bytes --]

On 2012-09-01 10:31, Avi Kivity wrote:
> On 08/29/2012 10:49 AM, Jan Kiszka wrote:
>>>
>>> Let's experiment with refcounting MemoryRegion.  We can move the entire
>>> contents of MemoryRegion to MemoryRegionImpl, add a reference count (to
>>> MemoryRegionImpl), and change MemoryRegion to contain a pointer to the
>>> refcounted MemoryRegionImpl:
>>>
>>> struct MemoryRegion {
>>>     struct MemoryRegionImpl *impl;
>>> };
>>>
>>> struct MemoryRegionImpl {
>>>     atomic int refs;
>>>     ...
>>> };
>>>
>>> The memory core can then store MemoryRegion copies (with elevated
>>> refcounts) instead of pointers.  Devices can destroy MemoryRegions at
>>> any time, the implementation will not go away.  However, what of the
>>> opaque stored in MemoryRegionImpl?  It becomes a dangling pointer.
>>>
>>> One way out is to add a lock to MemoryRegionImpl.  Dispatch takes the
>>> lock, examines the ->enabled member, and bails out if it is cleared. 
>>> The (MemoryRegion, not MemoryRegionImpl) destructor also takes the lock,
>>> clears ->enabled,  releases the lock, and drops the reference.
>>
>> That means holding the MemoryRegionImpl lock across the handler call?
> 
> Blech.  As you said on the other side of this thread, we must not take a
> coarse grained lock within a fine grained one, and
> MemoryRegionImpl::lock is as fine as they get.

Not sure what you compare here. MemoryRegionImpl::lock would be per
memory region, so with finer scope than the BQL but with similar scope
like a per-device lock.

> 
>>>
>>> The advantage to this scheme is that all changes are localized to the
>>> memory core, no need for a full sweep.  It is a little complicated, but
>>> we may be able to simplify it (or find another one).
>>
>> May work. We just need to detect if memory region tries to delete itself
>> from its handler to prevent the deadlock.
> 
> Those types of hacks are fragile.  IMO it just demonstrates what I said
> earlier (then tried to disprove with this): if we call an opaque's
> method, we must refcount or otherwise lock that opaque.  No way around it.

But that still doesn't solve the problem that we need to lock down the
*state* of the opaque during dispatch /wrt to memory region changes.
Just ensuring its existence is not enough unless we declare memory
region transactions to be asynchronous - and adapt all users.

> 
>>
>>>
>>>>
>>>>>
>>>>>> Besides
>>>>>> MMIO and PIO dispatching, it will haunt us for file or event handlers,
>>>>>> any kind of callbacks etc.
>>>>>>
>>>>>> Context A                               Context B
>>>>>> ---------                               ---------
>>>>>>                                         object = lookup()
>>>>>> deregister(object)
>>>>>> modify(object) -> invalid state
>>>>>> ...                                     use(object)
>>>>>> modify(object) -> valid state
>>>>>> register(object)
>>>>>>
>>>>>> And with "object" I'm not talking about QOM but any data structure.
>>>>>>
>>>>>
>>>>>
>>>>> Context B
>>>>> ---------
>>>>> rcu_read_lock()
>>>>> object = lookup()
>>>>> if (object) {
>>>>>     ref(object)
>>>>> }
>>>>> rcu_read_unlock()
>>>>>
>>>>> use(object)
>>>>>
>>>>> unref(object)
>>>>>
>>>>> (substitute locking scheme to conform to taste and/or dietary
>>>>> restrictions)   
>>>>>
>>>>
>>>> + wait for refcount(object) == 0 in deregister(object). That's what I'm
>>>> proposing.
>>>
>>> Consider timer_del() called from a timer callback.  It's often not doable.
>>
>> This is inherently synchronous already (when working against the same
>> alarm timer backend). We can detect this in timer_del and skip waiting,
>> as in the above scenario.
> 
> It can always be broken.  The timer callback takes the device lock to
> update the device.  The device mmio handler, holding the device lock,
> takes the timer lock to timer_mod.  Deadlock.

Well, how is this solved in Linux? By waiting on the callback in
hrtimer_cancel. Not by wait_on_magic_opaque (well, there is even no
opaque in the hrtimer API).

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 09/01/2012 01:57 AM, Jan Kiszka wrote:
> On 2012-09-01 10:31, Avi Kivity wrote:
> > On 08/29/2012 10:49 AM, Jan Kiszka wrote:
> >>>
> >>> Let's experiment with refcounting MemoryRegion.  We can move the entire
> >>> contents of MemoryRegion to MemoryRegionImpl, add a reference count (to
> >>> MemoryRegionImpl), and change MemoryRegion to contain a pointer to the
> >>> refcounted MemoryRegionImpl:
> >>>
> >>> struct MemoryRegion {
> >>>     struct MemoryRegionImpl *impl;
> >>> };
> >>>
> >>> struct MemoryRegionImpl {
> >>>     atomic int refs;
> >>>     ...
> >>> };
> >>>
> >>> The memory core can then store MemoryRegion copies (with elevated
> >>> refcounts) instead of pointers.  Devices can destroy MemoryRegions at
> >>> any time, the implementation will not go away.  However, what of the
> >>> opaque stored in MemoryRegionImpl?  It becomes a dangling pointer.
> >>>
> >>> One way out is to add a lock to MemoryRegionImpl.  Dispatch takes the
> >>> lock, examines the ->enabled member, and bails out if it is cleared. 
> >>> The (MemoryRegion, not MemoryRegionImpl) destructor also takes the lock,
> >>> clears ->enabled,  releases the lock, and drops the reference.
> >>
> >> That means holding the MemoryRegionImpl lock across the handler call?
> > 
> > Blech.  As you said on the other side of this thread, we must not take a
> > coarse grained lock within a fine grained one, and
> > MemoryRegionImpl::lock is as fine as they get.
>
> Not sure what you compare here. MemoryRegionImpl::lock would be per
> memory region, so with finer scope than the BQL but with similar scope
> like a per-device lock.

The dispatch may acquire the bql (in fact most will, unless we convert
everything).  A design that doesn't allow this is broken.  Even the
device lock is more coarse grained (since it covers all regions) and is
taken in a deadlock scenario as region manipulation will be done under
the device lock.  You say we can detect this, but I dislike it.

> > 
> >>>
> >>> The advantage to this scheme is that all changes are localized to the
> >>> memory core, no need for a full sweep.  It is a little complicated, but
> >>> we may be able to simplify it (or find another one).
> >>
> >> May work. We just need to detect if memory region tries to delete itself
> >> from its handler to prevent the deadlock.
> > 
> > Those types of hacks are fragile.  IMO it just demonstrates what I said
> > earlier (then tried to disprove with this): if we call an opaque's
> > method, we must refcount or otherwise lock that opaque.  No way around it.
>
> But that still doesn't solve the problem that we need to lock down the
> *state* of the opaque during dispatch /wrt to memory region changes.
> Just ensuring its existence is not enough unless we declare memory
> region transactions to be asynchronous - and adapt all users.

I expect no users will need change.

Changing the region offset has no effect on dispatch (in fact the region
itself doesn't change).  Changing subregions is fine.  Disabling a
region concurrently with access is the only potential problem, but this
is rare, and I expect it to just work.  We will have to audit all users,
yes.

> >>>> + wait for refcount(object) == 0 in deregister(object). That's what I'm
> >>>> proposing.
> >>>
> >>> Consider timer_del() called from a timer callback.  It's often not doable.
> >>
> >> This is inherently synchronous already (when working against the same
> >> alarm timer backend). We can detect this in timer_del and skip waiting,
> >> as in the above scenario.
> > 
> > It can always be broken.  The timer callback takes the device lock to
> > update the device.  The device mmio handler, holding the device lock,
> > takes the timer lock to timer_mod.  Deadlock.
>
> Well, how is this solved in Linux? By waiting on the callback in
> hrtimer_cancel. Not by wait_on_magic_opaque (well, there is even no
> opaque in the hrtimer API).

I don't consider that busy loop an elegant solution.

The original proposal mirrors dcache.  You can perform operations on a
dentry even after it is unlinked.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[liu ping fan]

On Thu, Aug 30, 2012 at 1:40 AM, Avi Kivity <avi@redhat.com> wrote:
> On 08/29/2012 10:30 AM, Jan Kiszka wrote:
>> On 2012-08-29 19:23, Avi Kivity wrote:
>> > On 08/28/2012 02:42 AM, Jan Kiszka wrote:
>> >>
>> >> Let's not talk about devices or MMIO dispatching. I think the problem is
>> >> way more generic, and we will face it multiple times in QEMU.
>> >
>> > The problem exists outside qemu as well.  It is one of the reasons for
>> > the popularity of garbage collection IMO, and the use of reference
>> > counting when GC is not available.
>> >
>> > This pattern is even documented in
>> > Documentation/DocBook/kernel-locking.tmpl:
>> >
>> > @@ -104,12 +114,11 @@
>> >  struct object *cache_find(int id)
>> >  {
>> >          struct object *obj;
>> > -        unsigned long flags;
>> >
>> > -        spin_lock_irqsave(&amp;cache_lock, flags);
>> > +        rcu_read_lock();
>> >          obj = __cache_find(id);
>> >          if (obj)
>> >                  object_get(obj);
>> > -        spin_unlock_irqrestore(&amp;cache_lock, flags);
>> > +        rcu_read_unlock();
>> >
>> >
>> > Of course that doesn't mean we should use it, but at least it indicates
>> > that it is a standard pattern.  With MemoryRegion the pattern is
>> > changed, since MemoryRegion is a thunk, not the object we're really
>> > dispatching to.
>>
>> We are dispatching according to the memory region (parameters, op
>> handlers, opaques). If we end up in device object is not decided at this
>> level. A memory region describes a dispatchable area - not to confuse
>> with a device that may only partially be able to receive such requests.
>
But I think the meaning of the memory region is for dispatching. If no
dispatching associated with mr, why need it exist in the system?  And
could you elaborate that who will be the ref holder of mr?

> Correct.
>
> Let's experiment with refcounting MemoryRegion.  We can move the entire
> contents of MemoryRegion to MemoryRegionImpl, add a reference count (to
> MemoryRegionImpl), and change MemoryRegion to contain a pointer to the
> refcounted MemoryRegionImpl:
>
> struct MemoryRegion {
>     struct MemoryRegionImpl *impl;
> };
>
> struct MemoryRegionImpl {
>     atomic int refs;
>     ...
> };
>
> The memory core can then store MemoryRegion copies (with elevated
> refcounts) instead of pointers.  Devices can destroy MemoryRegions at
> any time, the implementation will not go away.  However, what of the
> opaque stored in MemoryRegionImpl?  It becomes a dangling pointer.
>
> One way out is to add a lock to MemoryRegionImpl.  Dispatch takes the
> lock, examines the ->enabled member, and bails out if it is cleared.
> The (MemoryRegion, not MemoryRegionImpl) destructor also takes the lock,
> clears ->enabled,  releases the lock, and drops the reference.
>
Assume we can get the host objectX of opaque, then we can just
object_ref(objectX). So dispatching will not face the hard nut to
handle opaque.

Regards,
pingfan
> The advantage to this scheme is that all changes are localized to the
> memory core, no need for a full sweep.  It is a little complicated, but
> we may be able to simplify it (or find another one).
>
>>
>> >
>> >> Besides
>> >> MMIO and PIO dispatching, it will haunt us for file or event handlers,
>> >> any kind of callbacks etc.
>> >>
>> >> Context A                               Context B
>> >> ---------                               ---------
>> >>                                         object = lookup()
>> >> deregister(object)
>> >> modify(object) -> invalid state
>> >> ...                                     use(object)
>> >> modify(object) -> valid state
>> >> register(object)
>> >>
>> >> And with "object" I'm not talking about QOM but any data structure.
>> >>
>> >
>> >
>> > Context B
>> > ---------
>> > rcu_read_lock()
>> > object = lookup()
>> > if (object) {
>> >     ref(object)
>> > }
>> > rcu_read_unlock()
>> >
>> > use(object)
>> >
>> > unref(object)
>> >
>> > (substitute locking scheme to conform to taste and/or dietary
>> > restrictions)
>> >
>>
>> + wait for refcount(object) == 0 in deregister(object). That's what I'm
>> proposing.
>
> Consider timer_del() called from a timer callback.  It's often not doable.
>
> --
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-08-30 07:54, liu ping fan wrote:
> On Thu, Aug 30, 2012 at 1:40 AM, Avi Kivity <avi@redhat.com> wrote:
>> On 08/29/2012 10:30 AM, Jan Kiszka wrote:
>>> On 2012-08-29 19:23, Avi Kivity wrote:
>>>> On 08/28/2012 02:42 AM, Jan Kiszka wrote:
>>>>>
>>>>> Let's not talk about devices or MMIO dispatching. I think the problem is
>>>>> way more generic, and we will face it multiple times in QEMU.
>>>>
>>>> The problem exists outside qemu as well.  It is one of the reasons for
>>>> the popularity of garbage collection IMO, and the use of reference
>>>> counting when GC is not available.
>>>>
>>>> This pattern is even documented in
>>>> Documentation/DocBook/kernel-locking.tmpl:
>>>>
>>>> @@ -104,12 +114,11 @@
>>>>  struct object *cache_find(int id)
>>>>  {
>>>>          struct object *obj;
>>>> -        unsigned long flags;
>>>>
>>>> -        spin_lock_irqsave(&amp;cache_lock, flags);
>>>> +        rcu_read_lock();
>>>>          obj = __cache_find(id);
>>>>          if (obj)
>>>>                  object_get(obj);
>>>> -        spin_unlock_irqrestore(&amp;cache_lock, flags);
>>>> +        rcu_read_unlock();
>>>>
>>>>
>>>> Of course that doesn't mean we should use it, but at least it indicates
>>>> that it is a standard pattern.  With MemoryRegion the pattern is
>>>> changed, since MemoryRegion is a thunk, not the object we're really
>>>> dispatching to.
>>>
>>> We are dispatching according to the memory region (parameters, op
>>> handlers, opaques). If we end up in device object is not decided at this
>>> level. A memory region describes a dispatchable area - not to confuse
>>> with a device that may only partially be able to receive such requests.
>>
> But I think the meaning of the memory region is for dispatching. If no
> dispatching associated with mr, why need it exist in the system?

Where did I say that memory regions should no longer be used for
dispatching? The point is to keep the clean layer separation between
memory regions and device objects instead of merging them together.

Given

       Object
       ^    ^
       |    |
Region 1    Region 2

you protect the object during dispatch, implicitly (and that is bad)
requiring that no region must change in that period. I say what rather
needs protection are the regions so that Region 2 can pass away and
maybe reappear independent of Region 1. And: I won't need to know the
type of that object the regions are referring to in this model. That's
the difference.

>  And
> could you elaborate that who will be the ref holder of mr?

The memory subsystem while running a memory region handler. If that will
be a reference counter or a per-region lock like Avi suggested, we still
need to find out.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[liu ping fan]

On Thu, Aug 30, 2012 at 3:08 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2012-08-30 07:54, liu ping fan wrote:
>> On Thu, Aug 30, 2012 at 1:40 AM, Avi Kivity <avi@redhat.com> wrote:
>>> On 08/29/2012 10:30 AM, Jan Kiszka wrote:
>>>> On 2012-08-29 19:23, Avi Kivity wrote:
>>>>> On 08/28/2012 02:42 AM, Jan Kiszka wrote:
>>>>>>
>>>>>> Let's not talk about devices or MMIO dispatching. I think the problem is
>>>>>> way more generic, and we will face it multiple times in QEMU.
>>>>>
>>>>> The problem exists outside qemu as well.  It is one of the reasons for
>>>>> the popularity of garbage collection IMO, and the use of reference
>>>>> counting when GC is not available.
>>>>>
>>>>> This pattern is even documented in
>>>>> Documentation/DocBook/kernel-locking.tmpl:
>>>>>
>>>>> @@ -104,12 +114,11 @@
>>>>>  struct object *cache_find(int id)
>>>>>  {
>>>>>          struct object *obj;
>>>>> -        unsigned long flags;
>>>>>
>>>>> -        spin_lock_irqsave(&amp;cache_lock, flags);
>>>>> +        rcu_read_lock();
>>>>>          obj = __cache_find(id);
>>>>>          if (obj)
>>>>>                  object_get(obj);
>>>>> -        spin_unlock_irqrestore(&amp;cache_lock, flags);
>>>>> +        rcu_read_unlock();
>>>>>
>>>>>
>>>>> Of course that doesn't mean we should use it, but at least it indicates
>>>>> that it is a standard pattern.  With MemoryRegion the pattern is
>>>>> changed, since MemoryRegion is a thunk, not the object we're really
>>>>> dispatching to.
>>>>
>>>> We are dispatching according to the memory region (parameters, op
>>>> handlers, opaques). If we end up in device object is not decided at this
>>>> level. A memory region describes a dispatchable area - not to confuse
>>>> with a device that may only partially be able to receive such requests.
>>>
>> But I think the meaning of the memory region is for dispatching. If no
>> dispatching associated with mr, why need it exist in the system?
>
> Where did I say that memory regions should no longer be used for
> dispatching? The point is to keep the clean layer separation between
> memory regions and device objects instead of merging them together.
>
> Given
>
>        Object
>        ^    ^
>        |    |
> Region 1    Region 2
>
> you protect the object during dispatch, implicitly (and that is bad)
> requiring that no region must change in that period. I say what rather
> needs protection are the regions so that Region 2 can pass away and
> maybe reappear independent of Region 1. And: I won't need to know the

OK, I see, this is a strong reason.

Regards,
pingfan

> type of that object the regions are referring to in this model. That's
> the difference.
>
>>  And
>> could you elaborate that who will be the ref holder of mr?
>
> The memory subsystem while running a memory region handler. If that will
> be a reference counter or a per-region lock like Avi suggested, we still
> need to find out.
>
> Jan
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 08/30/2012 12:08 AM, Jan Kiszka wrote:
> >>>
> >>> We are dispatching according to the memory region (parameters, op
> >>> handlers, opaques). If we end up in device object is not decided at this
> >>> level. A memory region describes a dispatchable area - not to confuse
> >>> with a device that may only partially be able to receive such requests.
> >>
> > But I think the meaning of the memory region is for dispatching. If no
> > dispatching associated with mr, why need it exist in the system?
>
> Where did I say that memory regions should no longer be used for
> dispatching? The point is to keep the clean layer separation between
> memory regions and device objects instead of merging them together.

Believe me, that's exactly how I feel.  I guess no author of a module
wants to see it swallowed by a larger framework and see its design
completely changed.  Modules should be decoupled as much as possible. 
But I don't see a better way yet.

>
> Given
>
>        Object
>        ^    ^
>        |    |
> Region 1    Region 2
>
> you protect the object during dispatch, implicitly (and that is bad)
> requiring that no region must change in that period.

We only protect the object against removal.  After object_ref() has run,
the region may still be disabled.

>  I say what rather
> needs protection are the regions so that Region 2 can pass away and
> maybe reappear independent of Region 1.

Region 2 can go away until the device-supplied dispatch code takes the
device lock.  If the device chooses to use finer-grained locking, it can
allow region 2 (or even region 1) to change during dispatch.  The only
requirement is that region 1 is not destroyed.

>  And: I won't need to know the
> type of that object the regions are referring to in this model. That's
> the difference.

Sorry, I lost the reference.  What model?  Are you referring to my
broken MemoryRegionImpl split?

> >  And
> > could you elaborate that who will be the ref holder of mr?
>
> The memory subsystem while running a memory region handler. If that will
> be a reference counter or a per-region lock like Avi suggested, we still
> need to find out.
>

If we make the refcount/lock internal to the region, we must remove the
opaque, since the region won't protect it.

Replacing the opaque with container_of(mr) doesn't help, since we can't
refcount mr, only mr->impl.

We could externalize the refcounting and push it into device code.  This
means:

   memory_region_init_io(&s->mem, dev)

   ...

   object_ref(dev)
   memory_region_add_subregion(..., &dev->mr)

   ...

   memory_region_del_subregion(..., &dev->mr)  // implied flush
   object_unref(dev)

er, this must be wrong, since it's so simple

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[liu ping fan]

On Sat, Sep 1, 2012 at 4:46 PM, Avi Kivity <avi@redhat.com> wrote:
> On 08/30/2012 12:08 AM, Jan Kiszka wrote:
>> >>>
>> >>> We are dispatching according to the memory region (parameters, op
>> >>> handlers, opaques). If we end up in device object is not decided at this
>> >>> level. A memory region describes a dispatchable area - not to confuse
>> >>> with a device that may only partially be able to receive such requests.
>> >>
>> > But I think the meaning of the memory region is for dispatching. If no
>> > dispatching associated with mr, why need it exist in the system?
>>
>> Where did I say that memory regions should no longer be used for
>> dispatching? The point is to keep the clean layer separation between
>> memory regions and device objects instead of merging them together.
>
> Believe me, that's exactly how I feel.  I guess no author of a module
> wants to see it swallowed by a larger framework and see its design
> completely changed.  Modules should be decoupled as much as possible.
> But I don't see a better way yet.
>
>>
>> Given
>>
>>        Object
>>        ^    ^
>>        |    |
>> Region 1    Region 2
>>
>> you protect the object during dispatch, implicitly (and that is bad)
>> requiring that no region must change in that period.
>
> We only protect the object against removal.  After object_ref() has run,
> the region may still be disabled.
>
>>  I say what rather
>> needs protection are the regions so that Region 2 can pass away and
>> maybe reappear independent of Region 1.
>
> Region 2 can go away until the device-supplied dispatch code takes the
> device lock.  If the device chooses to use finer-grained locking, it can
> allow region 2 (or even region 1) to change during dispatch.  The only
> requirement is that region 1 is not destroyed.
>
>>  And: I won't need to know the
>> type of that object the regions are referring to in this model. That's
>> the difference.
>
> Sorry, I lost the reference.  What model?  Are you referring to my
> broken MemoryRegionImpl split?
>
>> >  And
>> > could you elaborate that who will be the ref holder of mr?
>>
>> The memory subsystem while running a memory region handler. If that will
>> be a reference counter or a per-region lock like Avi suggested, we still
>> need to find out.
>>
>
> If we make the refcount/lock internal to the region, we must remove the
> opaque, since the region won't protect it.
>
> Replacing the opaque with container_of(mr) doesn't help, since we can't
> refcount mr, only mr->impl.
>
I think you mean if using MemoryRegionImpl, then at this level, we had
better not touch the refcnt of container_of(mr) and should face the
mr->impl->refcnt. Right?

> We could externalize the refcounting and push it into device code.  This
> means:
>
>    memory_region_init_io(&s->mem, dev)
>
>    ...
>
>    object_ref(dev)
>    memory_region_add_subregion(..., &dev->mr)
>
>    ...
>
>    memory_region_del_subregion(..., &dev->mr)  // implied flush
>    object_unref(dev)
>
I think "object_ref(dev)" just another style to push
MemoryRegionOps::object() to device level.  And I think we can bypass
it.   The caller (unplug, pci-reconfig ) of
memory_region_del_subregion() ensure the @dev is valid.
If the implied flush is implemented in synchronize,  _del_subregion()
will guarantee no reader for @dev->mr and @dev exist any longer. So I
think we can save both object_ref/unref(dev) for memory system.
The only problem is that whether we can implement it as synchronous or
not,  is it possible that we can launch a  _del_subregion(mr-X) in
mr-X's dispatcher?

Regards,
pingfan

> er, this must be wrong, since it's so simple
>
> --
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 09/03/2012 10:44 AM, liu ping fan wrote:
>>>
>>
>> If we make the refcount/lock internal to the region, we must remove the
>> opaque, since the region won't protect it.
>>
>> Replacing the opaque with container_of(mr) doesn't help, since we can't
>> refcount mr, only mr->impl.
>>
> I think you mean if using MemoryRegionImpl, then at this level, we had
> better not touch the refcnt of container_of(mr) and should face the
> mr->impl->refcnt. Right?

I did not understand the second part, sorry.

>> We could externalize the refcounting and push it into device code.  This
>> means:
>>
>>    memory_region_init_io(&s->mem, dev)
>>
>>    ...
>>
>>    object_ref(dev)
>>    memory_region_add_subregion(..., &dev->mr)
>>
>>    ...
>>
>>    memory_region_del_subregion(..., &dev->mr)  // implied flush
>>    object_unref(dev)
>>
> I think "object_ref(dev)" just another style to push
> MemoryRegionOps::object() to device level.  And I think we can bypass
> it.   The caller (unplug, pci-reconfig ) of
> memory_region_del_subregion() ensure the @dev is valid.
> If the implied flush is implemented in synchronize,  _del_subregion()
> will guarantee no reader for @dev->mr and @dev exist any longer. 

The above code has a deadlock.  memory_region_del_subregion() may be
called under the device lock (since it may be the result of mmio to the
device), and if the flush uses synchronized_rcu(), it will wait forever
for the read-side critical section to complete.

> So I
> think we can save both object_ref/unref(dev) for memory system.
> The only problem is that whether we can implement it as synchronous or
> not,  is it possible that we can launch a  _del_subregion(mr-X) in
> mr-X's dispatcher?

Yes.  Real cases exist.

What alternatives remain?

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

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[liu ping fan]

On Mon, Sep 3, 2012 at 4:52 PM, Avi Kivity <avi@redhat.com> wrote:
> On 09/03/2012 10:44 AM, liu ping fan wrote:
>>>>
>>>
>>> If we make the refcount/lock internal to the region, we must remove the
>>> opaque, since the region won't protect it.
>>>
>>> Replacing the opaque with container_of(mr) doesn't help, since we can't
>>> refcount mr, only mr->impl.
>>>
>> I think you mean if using MemoryRegionImpl, then at this level, we had
>> better not touch the refcnt of container_of(mr) and should face the
>> mr->impl->refcnt. Right?
>
> I did not understand the second part, sorry.
>
My understanding of "Replacing the opaque with container_of(mr)
doesn't help, since we can't  refcount mr, only
mr->impl." is that although Object_ref(container_of(mr)) can help us
to protect it from disappearing. But apparently it is not right place
to do it it in memory core.   Do I catch you meaning?

>>> We could externalize the refcounting and push it into device code.  This
>>> means:
>>>
>>>    memory_region_init_io(&s->mem, dev)
>>>
>>>    ...
>>>
>>>    object_ref(dev)
>>>    memory_region_add_subregion(..., &dev->mr)
>>>
>>>    ...
>>>
>>>    memory_region_del_subregion(..., &dev->mr)  // implied flush
>>>    object_unref(dev)
>>>
>> I think "object_ref(dev)" just another style to push
>> MemoryRegionOps::object() to device level.  And I think we can bypass
>> it.   The caller (unplug, pci-reconfig ) of
>> memory_region_del_subregion() ensure the @dev is valid.
>> If the implied flush is implemented in synchronize,  _del_subregion()
>> will guarantee no reader for @dev->mr and @dev exist any longer.
>
> The above code has a deadlock.  memory_region_del_subregion() may be
> called under the device lock (since it may be the result of mmio to the
> device), and if the flush uses synchronized_rcu(), it will wait forever
> for the read-side critical section to complete.
>
But if _del_subregion() just wait for mr-X quiescent period, while
calling in mr-Y's read side critical section, then we can avoid
deadlock.  I saw in pci-mapping, we delete mr-X in mr-Y read side.

>> So I
>> think we can save both object_ref/unref(dev) for memory system.
>> The only problem is that whether we can implement it as synchronous or
>> not,  is it possible that we can launch a  _del_subregion(mr-X) in
>> mr-X's dispatcher?
>
> Yes.  Real cases exist.

Oh, I find the sample code, then, the deadlock is unavoidable in this method.
>
> What alternatives remain?
>
I think a way out may be async+refcnt

Regards,
pingfan
> --
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 09/03/2012 01:06 PM, liu ping fan wrote:
> On Mon, Sep 3, 2012 at 4:52 PM, Avi Kivity <avi@redhat.com> wrote:
>> On 09/03/2012 10:44 AM, liu ping fan wrote:
>>>>>
>>>>
>>>> If we make the refcount/lock internal to the region, we must remove the
>>>> opaque, since the region won't protect it.
>>>>
>>>> Replacing the opaque with container_of(mr) doesn't help, since we can't
>>>> refcount mr, only mr->impl.
>>>>
>>> I think you mean if using MemoryRegionImpl, then at this level, we had
>>> better not touch the refcnt of container_of(mr) and should face the
>>> mr->impl->refcnt. Right?
>>
>> I did not understand the second part, sorry.
>>
> My understanding of "Replacing the opaque with container_of(mr)
> doesn't help, since we can't  refcount mr, only
> mr->impl." is that although Object_ref(container_of(mr)) can help us
> to protect it from disappearing. But apparently it is not right place
> to do it it in memory core.   Do I catch you meaning?

We cannot call container_of(mr) in the memory core, because the
parameters to container_of() are not known.  But that is not the main issue.

Something we can do is make MemoryRegionOps::object() take a mr
parameter instead of opaque.  MemoryRegionOps::object() then uses
container_of() to derive the object to ref.

(works with MemoryRegionOps::ref()/MemoryRegionOps::unref() too; Jan,
this decouples Object from MemoryRegion at the cost of extra boilerplate
in client code, but it may be worthwhile as a temporary measure while we
gain more experience with this)

> 
>>>> We could externalize the refcounting and push it into device code.  This
>>>> means:
>>>>
>>>>    memory_region_init_io(&s->mem, dev)
>>>>
>>>>    ...
>>>>
>>>>    object_ref(dev)
>>>>    memory_region_add_subregion(..., &dev->mr)
>>>>
>>>>    ...
>>>>
>>>>    memory_region_del_subregion(..., &dev->mr)  // implied flush
>>>>    object_unref(dev)
>>>>
>>> I think "object_ref(dev)" just another style to push
>>> MemoryRegionOps::object() to device level.  And I think we can bypass
>>> it.   The caller (unplug, pci-reconfig ) of
>>> memory_region_del_subregion() ensure the @dev is valid.
>>> If the implied flush is implemented in synchronize,  _del_subregion()
>>> will guarantee no reader for @dev->mr and @dev exist any longer.
>>
>> The above code has a deadlock.  memory_region_del_subregion() may be
>> called under the device lock (since it may be the result of mmio to the
>> device), and if the flush uses synchronized_rcu(), it will wait forever
>> for the read-side critical section to complete.
>>
> But if _del_subregion() just wait for mr-X quiescent period, while
> calling in mr-Y's read side critical section, then we can avoid
> deadlock.  I saw in pci-mapping, we delete mr-X in mr-Y read side.

Look at cirrus-vga.c, there are many calls to the memory API there.
While I doubt that we have one region disabling itself (or a subregion
of itself), that's all code that would be run under the same device
lock, and so would deadlock.


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

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[liu ping fan]

On Mon, Sep 3, 2012 at 6:16 PM, Avi Kivity <avi@redhat.com> wrote:
> On 09/03/2012 01:06 PM, liu ping fan wrote:
>> On Mon, Sep 3, 2012 at 4:52 PM, Avi Kivity <avi@redhat.com> wrote:
>>> On 09/03/2012 10:44 AM, liu ping fan wrote:
>>>>>>
>>>>>
>>>>> If we make the refcount/lock internal to the region, we must remove the
>>>>> opaque, since the region won't protect it.
>>>>>
>>>>> Replacing the opaque with container_of(mr) doesn't help, since we can't
>>>>> refcount mr, only mr->impl.
>>>>>
>>>> I think you mean if using MemoryRegionImpl, then at this level, we had
>>>> better not touch the refcnt of container_of(mr) and should face the
>>>> mr->impl->refcnt. Right?
>>>
>>> I did not understand the second part, sorry.
>>>
>> My understanding of "Replacing the opaque with container_of(mr)
>> doesn't help, since we can't  refcount mr, only
>> mr->impl." is that although Object_ref(container_of(mr)) can help us
>> to protect it from disappearing. But apparently it is not right place
>> to do it it in memory core.   Do I catch you meaning?
>
> We cannot call container_of(mr) in the memory core, because the
> parameters to container_of() are not known.  But that is not the main issue.
>
> Something we can do is make MemoryRegionOps::object() take a mr
> parameter instead of opaque.  MemoryRegionOps::object() then uses
> container_of() to derive the object to ref.
>
> (works with MemoryRegionOps::ref()/MemoryRegionOps::unref() too; Jan,
> this decouples Object from MemoryRegion at the cost of extra boilerplate
> in client code, but it may be worthwhile as a temporary measure while we
> gain more experience with this)
>
Exactly catch your meaning, thanks.
>>
>>>>> We could externalize the refcounting and push it into device code.  This
>>>>> means:
>>>>>
>>>>>    memory_region_init_io(&s->mem, dev)
>>>>>
>>>>>    ...
>>>>>
>>>>>    object_ref(dev)
>>>>>    memory_region_add_subregion(..., &dev->mr)
>>>>>
>>>>>    ...
>>>>>
>>>>>    memory_region_del_subregion(..., &dev->mr)  // implied flush
>>>>>    object_unref(dev)
>>>>>
>>>> I think "object_ref(dev)" just another style to push
>>>> MemoryRegionOps::object() to device level.  And I think we can bypass
>>>> it.   The caller (unplug, pci-reconfig ) of
>>>> memory_region_del_subregion() ensure the @dev is valid.
>>>> If the implied flush is implemented in synchronize,  _del_subregion()
>>>> will guarantee no reader for @dev->mr and @dev exist any longer.
>>>
>>> The above code has a deadlock.  memory_region_del_subregion() may be
>>> called under the device lock (since it may be the result of mmio to the
>>> device), and if the flush uses synchronized_rcu(), it will wait forever
>>> for the read-side critical section to complete.
>>>
>> But if _del_subregion() just wait for mr-X quiescent period, while
>> calling in mr-Y's read side critical section, then we can avoid
>> deadlock.  I saw in pci-mapping, we delete mr-X in mr-Y read side.
>
> Look at cirrus-vga.c, there are many calls to the memory API there.
> While I doubt that we have one region disabling itself (or a subregion
> of itself), that's all code that would be run under the same device
> lock, and so would deadlock.
>
Oh, yes, quiescent time will never come since reader wait for the
lock! Got it, thanks.

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

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[liu ping fan]

On Mon, Sep 3, 2012 at 6:06 PM, liu ping fan <qemulist@gmail.com> wrote:
> On Mon, Sep 3, 2012 at 4:52 PM, Avi Kivity <avi@redhat.com> wrote:
>> On 09/03/2012 10:44 AM, liu ping fan wrote:
>>>>>
>>>>
>>>> If we make the refcount/lock internal to the region, we must remove the
>>>> opaque, since the region won't protect it.
>>>>
>>>> Replacing the opaque with container_of(mr) doesn't help, since we can't
>>>> refcount mr, only mr->impl.
>>>>
>>> I think you mean if using MemoryRegionImpl, then at this level, we had
>>> better not touch the refcnt of container_of(mr) and should face the
>>> mr->impl->refcnt. Right?
>>
>> I did not understand the second part, sorry.
>>
> My understanding of "Replacing the opaque with container_of(mr)
> doesn't help, since we can't  refcount mr, only
> mr->impl." is that although Object_ref(container_of(mr)) can help us
> to protect it from disappearing. But apparently it is not right place
> to do it it in memory core.   Do I catch you meaning?
>
>>>> We could externalize the refcounting and push it into device code.  This
>>>> means:
>>>>
>>>>    memory_region_init_io(&s->mem, dev)
>>>>
>>>>    ...
>>>>
>>>>    object_ref(dev)
>>>>    memory_region_add_subregion(..., &dev->mr)
>>>>
>>>>    ...
>>>>
>>>>    memory_region_del_subregion(..., &dev->mr)  // implied flush
>>>>    object_unref(dev)
>>>>
>>> I think "object_ref(dev)" just another style to push
>>> MemoryRegionOps::object() to device level.  And I think we can bypass
>>> it.   The caller (unplug, pci-reconfig ) of
>>> memory_region_del_subregion() ensure the @dev is valid.
>>> If the implied flush is implemented in synchronize,  _del_subregion()
>>> will guarantee no reader for @dev->mr and @dev exist any longer.
>>
>> The above code has a deadlock.  memory_region_del_subregion() may be
>> called under the device lock (since it may be the result of mmio to the
>> device), and if the flush uses synchronized_rcu(), it will wait forever
>> for the read-side critical section to complete.
>>
> But if _del_subregion() just wait for mr-X quiescent period, while
> calling in mr-Y's read side critical section, then we can avoid
> deadlock.  I saw in pci-mapping, we delete mr-X in mr-Y read side.
>
>>> So I
>>> think we can save both object_ref/unref(dev) for memory system.
>>> The only problem is that whether we can implement it as synchronous or
>>> not,  is it possible that we can launch a  _del_subregion(mr-X) in
>>> mr-X's dispatcher?
>>
>> Yes.  Real cases exist.
>
> Oh, I find the sample code, then, the deadlock is unavoidable in this method.
>>
>> What alternatives remain?
>>
> I think a way out may be async+refcnt
>
If we consider the relationship of MemoryRegionImpl and device as the
one between file and file->private_data  in Linux. Then the creation
of impl will object_ref(dev) and when impl->ref=0, it will
object_unref(dev)
But this is an async model, for those client which need to know the
end of flush, we can adopt callback just like call_rcu().



> Regards,
> pingfan
>> --
>> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[liu ping fan]

[...]
>>
>>>>> We could externalize the refcounting and push it into device code.  This
>>>>> means:
>>>>>
>>>>>    memory_region_init_io(&s->mem, dev)
>>>>>
>>>>>    ...
>>>>>
>>>>>    object_ref(dev)
>>>>>    memory_region_add_subregion(..., &dev->mr)
>>>>>
>>>>>    ...
>>>>>
>>>>>    memory_region_del_subregion(..., &dev->mr)  // implied flush
>>>>>    object_unref(dev)
>>>>>
>>>> I think "object_ref(dev)" just another style to push
>>>> MemoryRegionOps::object() to device level.  And I think we can bypass
>>>> it.   The caller (unplug, pci-reconfig ) of
>>>> memory_region_del_subregion() ensure the @dev is valid.
>>>> If the implied flush is implemented in synchronize,  _del_subregion()
>>>> will guarantee no reader for @dev->mr and @dev exist any longer.
>>>
>>> The above code has a deadlock.  memory_region_del_subregion() may be
>>> called under the device lock (since it may be the result of mmio to the
>>> device), and if the flush uses synchronized_rcu(), it will wait forever
>>> for the read-side critical section to complete.
>>>
>> But if _del_subregion() just wait for mr-X quiescent period, while
>> calling in mr-Y's read side critical section, then we can avoid
>> deadlock.  I saw in pci-mapping, we delete mr-X in mr-Y read side.
>>
>>>> So I
>>>> think we can save both object_ref/unref(dev) for memory system.
>>>> The only problem is that whether we can implement it as synchronous or
>>>> not,  is it possible that we can launch a  _del_subregion(mr-X) in
>>>> mr-X's dispatcher?
>>>
>>> Yes.  Real cases exist.
>>
>> Oh, I find the sample code, then, the deadlock is unavoidable in this method.
>>>
>>> What alternatives remain?
>>>
>> I think a way out may be async+refcnt
>>
> If we consider the relationship of MemoryRegionImpl and device as the
> one between file and file->private_data  in Linux. Then the creation
> of impl will object_ref(dev) and when impl->ref=0, it will
> object_unref(dev)
> But this is an async model, for those client which need to know the
> end of flush, we can adopt callback just like call_rcu().
>
>
During this thread, it seems that no matter we adopt refcnt on
MemoryRegionImpl or not, protecting MemoryRegion::opaque during
dispatch is still required. It is challenging to substitute
memory_region_init_io() 's 3rd parameter from void* to Object*, owning
to the  conflict that life-cycle management need the host of opaque,
while Object and mr need 1:1 map. So I think, I will move forward on
the way based on MemoryRegionOps::object(). Right?

Regards,
pingfan

>
>> Regards,
>> pingfan
>>> --
>>> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 09/05/2012 11:19 AM, liu ping fan wrote:
> [...]
>>>
>>>>>> We could externalize the refcounting and push it into device code.  This
>>>>>> means:
>>>>>>
>>>>>>    memory_region_init_io(&s->mem, dev)
>>>>>>
>>>>>>    ...
>>>>>>
>>>>>>    object_ref(dev)
>>>>>>    memory_region_add_subregion(..., &dev->mr)
>>>>>>
>>>>>>    ...
>>>>>>
>>>>>>    memory_region_del_subregion(..., &dev->mr)  // implied flush
>>>>>>    object_unref(dev)
>>>>>>
>>>>> I think "object_ref(dev)" just another style to push
>>>>> MemoryRegionOps::object() to device level.  And I think we can bypass
>>>>> it.   The caller (unplug, pci-reconfig ) of
>>>>> memory_region_del_subregion() ensure the @dev is valid.
>>>>> If the implied flush is implemented in synchronize,  _del_subregion()
>>>>> will guarantee no reader for @dev->mr and @dev exist any longer.
>>>>
>>>> The above code has a deadlock.  memory_region_del_subregion() may be
>>>> called under the device lock (since it may be the result of mmio to the
>>>> device), and if the flush uses synchronized_rcu(), it will wait forever
>>>> for the read-side critical section to complete.
>>>>
>>> But if _del_subregion() just wait for mr-X quiescent period, while
>>> calling in mr-Y's read side critical section, then we can avoid
>>> deadlock.  I saw in pci-mapping, we delete mr-X in mr-Y read side.
>>>
>>>>> So I
>>>>> think we can save both object_ref/unref(dev) for memory system.
>>>>> The only problem is that whether we can implement it as synchronous or
>>>>> not,  is it possible that we can launch a  _del_subregion(mr-X) in
>>>>> mr-X's dispatcher?
>>>>
>>>> Yes.  Real cases exist.
>>>
>>> Oh, I find the sample code, then, the deadlock is unavoidable in this method.
>>>>
>>>> What alternatives remain?
>>>>
>>> I think a way out may be async+refcnt
>>>
>> If we consider the relationship of MemoryRegionImpl and device as the
>> one between file and file->private_data  in Linux. Then the creation
>> of impl will object_ref(dev) and when impl->ref=0, it will
>> object_unref(dev)
>> But this is an async model, for those client which need to know the
>> end of flush, we can adopt callback just like call_rcu().
>>
>>
> During this thread, it seems that no matter we adopt refcnt on
> MemoryRegionImpl or not, protecting MemoryRegion::opaque during
> dispatch is still required. 

Right.  So MemoryRegionImpl is pointless.

My current preference is MemoryRegionOps::ref(MemoryRegion *mr) (and a
corresponding unref), which has the following requirements:

- if the refcount is nonzero, MemoryRegion::opaque is safe to use
- if the refcount is nonzero, the MemoryRegion itself is stable.

Later we can change other callbacks to also use mr instead of opaque.
Usually the callback will be able to derive the device object from the
region, only special cases will require
memory_region_opaque(MemoryRegion *).  We can even drop the opaque from
memory_region_init_io() and replace it with memory_region_set_opaque(),
to be used in those special cases.

> It is challenging to substitute
> memory_region_init_io() 's 3rd parameter from void* to Object*, owning
> to the  conflict that life-cycle management need the host of opaque,
> while Object and mr need 1:1 map. So I think, I will move forward on
> the way based on MemoryRegionOps::object(). Right?

As outlined above, I now prefer MemoryRegionOps::ref/unref.

Advantages compared to MemoryRegionOps::object():
 - decoupled from the QOM framework

Disadvantages:
 - more boilerplate code in converted devices

Since we are likely to convert few devices to lockless dispatch, I
believe the tradeoff is justified.  But let's hear Jan and the others.

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

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-09-05 11:52, Avi Kivity wrote:
> On 09/05/2012 11:19 AM, liu ping fan wrote:
>> [...]
>>>>
>>>>>>> We could externalize the refcounting and push it into device code.  This
>>>>>>> means:
>>>>>>>
>>>>>>>    memory_region_init_io(&s->mem, dev)
>>>>>>>
>>>>>>>    ...
>>>>>>>
>>>>>>>    object_ref(dev)
>>>>>>>    memory_region_add_subregion(..., &dev->mr)
>>>>>>>
>>>>>>>    ...
>>>>>>>
>>>>>>>    memory_region_del_subregion(..., &dev->mr)  // implied flush
>>>>>>>    object_unref(dev)
>>>>>>>
>>>>>> I think "object_ref(dev)" just another style to push
>>>>>> MemoryRegionOps::object() to device level.  And I think we can bypass
>>>>>> it.   The caller (unplug, pci-reconfig ) of
>>>>>> memory_region_del_subregion() ensure the @dev is valid.
>>>>>> If the implied flush is implemented in synchronize,  _del_subregion()
>>>>>> will guarantee no reader for @dev->mr and @dev exist any longer.
>>>>>
>>>>> The above code has a deadlock.  memory_region_del_subregion() may be
>>>>> called under the device lock (since it may be the result of mmio to the
>>>>> device), and if the flush uses synchronized_rcu(), it will wait forever
>>>>> for the read-side critical section to complete.
>>>>>
>>>> But if _del_subregion() just wait for mr-X quiescent period, while
>>>> calling in mr-Y's read side critical section, then we can avoid
>>>> deadlock.  I saw in pci-mapping, we delete mr-X in mr-Y read side.
>>>>
>>>>>> So I
>>>>>> think we can save both object_ref/unref(dev) for memory system.
>>>>>> The only problem is that whether we can implement it as synchronous or
>>>>>> not,  is it possible that we can launch a  _del_subregion(mr-X) in
>>>>>> mr-X's dispatcher?
>>>>>
>>>>> Yes.  Real cases exist.
>>>>
>>>> Oh, I find the sample code, then, the deadlock is unavoidable in this method.
>>>>>
>>>>> What alternatives remain?
>>>>>
>>>> I think a way out may be async+refcnt
>>>>
>>> If we consider the relationship of MemoryRegionImpl and device as the
>>> one between file and file->private_data  in Linux. Then the creation
>>> of impl will object_ref(dev) and when impl->ref=0, it will
>>> object_unref(dev)
>>> But this is an async model, for those client which need to know the
>>> end of flush, we can adopt callback just like call_rcu().
>>>
>>>
>> During this thread, it seems that no matter we adopt refcnt on
>> MemoryRegionImpl or not, protecting MemoryRegion::opaque during
>> dispatch is still required. 
> 
> Right.  So MemoryRegionImpl is pointless.
> 
> My current preference is MemoryRegionOps::ref(MemoryRegion *mr) (and a
> corresponding unref), which has the following requirements:
> 
> - if the refcount is nonzero, MemoryRegion::opaque is safe to use
> - if the refcount is nonzero, the MemoryRegion itself is stable.

The second point means that the memory subsystem will cache the region
state and apply changes only after leaving a handler that performed them?

> 
> Later we can change other callbacks to also use mr instead of opaque.
> Usually the callback will be able to derive the device object from the
> region, only special cases will require
> memory_region_opaque(MemoryRegion *).  We can even drop the opaque from
> memory_region_init_io() and replace it with memory_region_set_opaque(),
> to be used in those special cases.
> 
>> It is challenging to substitute
>> memory_region_init_io() 's 3rd parameter from void* to Object*, owning
>> to the  conflict that life-cycle management need the host of opaque,
>> while Object and mr need 1:1 map. So I think, I will move forward on
>> the way based on MemoryRegionOps::object(). Right?
> 
> As outlined above, I now prefer MemoryRegionOps::ref/unref.
> 
> Advantages compared to MemoryRegionOps::object():
>  - decoupled from the QOM framework
> 
> Disadvantages:
>  - more boilerplate code in converted devices
> 
> Since we are likely to convert few devices to lockless dispatch, I
> believe the tradeoff is justified.  But let's hear Jan and the others.

I still need to dig into related posts of the past days, lost track, but
the above sounds much better.

Besides the question of what to protect and how, one important thing
IMHO is that we are able to switch locking behaviour on a per region
basis. And that should also be feasible this way.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 09/05/2012 01:36 PM, Jan Kiszka wrote:
>> 
>> My current preference is MemoryRegionOps::ref(MemoryRegion *mr) (and a
>> corresponding unref), which has the following requirements:
>> 
>> - if the refcount is nonzero, MemoryRegion::opaque is safe to use
>> - if the refcount is nonzero, the MemoryRegion itself is stable.
> 
> The second point means that the memory subsystem will cache the region
> state and apply changes only after leaving a handler that performed them?

No.  I/O callbacks may be called after a region has been disabled.

I guess we can restrict this to converted regions, so we don't introduce
regressions.

>> As outlined above, I now prefer MemoryRegionOps::ref/unref.
>> 
>> Advantages compared to MemoryRegionOps::object():
>>  - decoupled from the QOM framework
>> 
>> Disadvantages:
>>  - more boilerplate code in converted devices
>> 
>> Since we are likely to convert few devices to lockless dispatch, I
>> believe the tradeoff is justified.  But let's hear Jan and the others.
> 
> I still need to dig into related posts of the past days, lost track, but
> the above sounds much better.
> 
> Besides the question of what to protect and how, one important thing
> IMHO is that we are able to switch locking behaviour on a per region
> basis. And that should also be feasible this way.

It was also possible with MemoryRegionOps::object() - no one said all
regions for a device have to refer to the same object (and there is a
difference between locking and reference counting, each callback could
take a different lock).  But it is more natural with MemoryRegionOps::ref().

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

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-09-05 12:53, Avi Kivity wrote:
> On 09/05/2012 01:36 PM, Jan Kiszka wrote:
>>>
>>> My current preference is MemoryRegionOps::ref(MemoryRegion *mr) (and a
>>> corresponding unref), which has the following requirements:
>>>
>>> - if the refcount is nonzero, MemoryRegion::opaque is safe to use
>>> - if the refcount is nonzero, the MemoryRegion itself is stable.
>>
>> The second point means that the memory subsystem will cache the region
>> state and apply changes only after leaving a handler that performed them?
> 
> No.  I/O callbacks may be called after a region has been disabled.
> 
> I guess we can restrict this to converted regions, so we don't introduce
> regressions.

s/can/have to/. This property change will require some special care,
hopefully mostly at the memory layer. A simple scenario from recent patches:

    if (<enable>) {
        memory_region_set_address(&s->pm_io, pm_io_base);
        memory_region_set_enabled(&s->pm_io, true);
    } else {
        memory_region_set_enabled(&s->pm_io, false);
    }

We will have to ensure that set_enabled(..., true) will never cause a
dispatch using an outdated base address.

I think discussing semantics and usage patterns of the new memory API -
outside of the BQL - will be the next big topic. ;)

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 09/05/2012 02:11 PM, Jan Kiszka wrote:
> On 2012-09-05 12:53, Avi Kivity wrote:
>> On 09/05/2012 01:36 PM, Jan Kiszka wrote:
>>>>
>>>> My current preference is MemoryRegionOps::ref(MemoryRegion *mr) (and a
>>>> corresponding unref), which has the following requirements:
>>>>
>>>> - if the refcount is nonzero, MemoryRegion::opaque is safe to use
>>>> - if the refcount is nonzero, the MemoryRegion itself is stable.
>>>
>>> The second point means that the memory subsystem will cache the region
>>> state and apply changes only after leaving a handler that performed them?
>> 
>> No.  I/O callbacks may be called after a region has been disabled.
>> 
>> I guess we can restrict this to converted regions, so we don't introduce
>> regressions.
> 
> s/can/have to/. This property change will require some special care,
> hopefully mostly at the memory layer. A simple scenario from recent patches:
> 
>     if (<enable>) {
>         memory_region_set_address(&s->pm_io, pm_io_base);
>         memory_region_set_enabled(&s->pm_io, true);
>     } else {
>         memory_region_set_enabled(&s->pm_io, false);
>     }

I am unable to avoid pointing out that this can be collapsed to

  memory_region_set_address(&s->pm_io, pm_io_base);
  memory_region_set_enabled(&s->pm_io, <enable>);

as the address is meaningless when disabled. Sorry.

> 
> We will have to ensure that set_enabled(..., true) will never cause a
> dispatch using an outdated base address.

No, this is entirely safe.  If the guest changes a region offset
concurrently with issuing mmio on it, then it must expect either the old
or new offset to be used during dispatch.  In either case, the correct
intra-region offset will be provided to the I/O callback (no volatile
MemoryRegion fields except ->readable (IIRC) are used during dispatch -
the rest are all copied into data structures used during dispatch (this
is part of what makes the whole thing so rcu friendly).

> I think discussing semantics and usage patterns of the new memory API -
> outside of the BQL - will be the next big topic. ;)

I hope it won't prove to be that complicated.


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

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-09-05 13:25, Avi Kivity wrote:
> On 09/05/2012 02:11 PM, Jan Kiszka wrote:
>> On 2012-09-05 12:53, Avi Kivity wrote:
>>> On 09/05/2012 01:36 PM, Jan Kiszka wrote:
>>>>>
>>>>> My current preference is MemoryRegionOps::ref(MemoryRegion *mr) (and a
>>>>> corresponding unref), which has the following requirements:
>>>>>
>>>>> - if the refcount is nonzero, MemoryRegion::opaque is safe to use
>>>>> - if the refcount is nonzero, the MemoryRegion itself is stable.
>>>>
>>>> The second point means that the memory subsystem will cache the region
>>>> state and apply changes only after leaving a handler that performed them?
>>>
>>> No.  I/O callbacks may be called after a region has been disabled.
>>>
>>> I guess we can restrict this to converted regions, so we don't introduce
>>> regressions.
>>
>> s/can/have to/. This property change will require some special care,
>> hopefully mostly at the memory layer. A simple scenario from recent patches:
>>
>>     if (<enable>) {
>>         memory_region_set_address(&s->pm_io, pm_io_base);
>>         memory_region_set_enabled(&s->pm_io, true);
>>     } else {
>>         memory_region_set_enabled(&s->pm_io, false);
>>     }
> 
> I am unable to avoid pointing out that this can be collapsed to
> 
>   memory_region_set_address(&s->pm_io, pm_io_base);
>   memory_region_set_enabled(&s->pm_io, <enable>);
> 
> as the address is meaningless when disabled. Sorry.
> 
>>
>> We will have to ensure that set_enabled(..., true) will never cause a
>> dispatch using an outdated base address.
> 
> No, this is entirely safe.  If the guest changes a region offset
> concurrently with issuing mmio on it, then it must expect either the old
> or new offset to be used during dispatch.

You assume disable, reprogram, enable, ignoring that there could be
multiple, invalid states between disable and enable. Real hardware takes
care of the ordering.

>  In either case, the correct
> intra-region offset will be provided to the I/O callback (no volatile
> MemoryRegion fields except ->readable (IIRC) are used during dispatch -
> the rest are all copied into data structures used during dispatch (this
> is part of what makes the whole thing so rcu friendly).
> 
>> I think discussing semantics and usage patterns of the new memory API -
>> outside of the BQL - will be the next big topic. ;)
> 
> I hope it won't prove to be that complicated.
> 

Yeah, let's hope this...

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 09/05/2012 03:02 PM, Jan Kiszka wrote:
> On 2012-09-05 13:25, Avi Kivity wrote:
>> On 09/05/2012 02:11 PM, Jan Kiszka wrote:
>>> On 2012-09-05 12:53, Avi Kivity wrote:
>>>> On 09/05/2012 01:36 PM, Jan Kiszka wrote:
>>>>>>
>>>>>> My current preference is MemoryRegionOps::ref(MemoryRegion *mr) (and a
>>>>>> corresponding unref), which has the following requirements:
>>>>>>
>>>>>> - if the refcount is nonzero, MemoryRegion::opaque is safe to use
>>>>>> - if the refcount is nonzero, the MemoryRegion itself is stable.
>>>>>
>>>>> The second point means that the memory subsystem will cache the region
>>>>> state and apply changes only after leaving a handler that performed them?
>>>>
>>>> No.  I/O callbacks may be called after a region has been disabled.
>>>>
>>>> I guess we can restrict this to converted regions, so we don't introduce
>>>> regressions.
>>>
>>> s/can/have to/. This property change will require some special care,
>>> hopefully mostly at the memory layer. A simple scenario from recent patches:
>>>
>>>     if (<enable>) {
>>>         memory_region_set_address(&s->pm_io, pm_io_base);
>>>         memory_region_set_enabled(&s->pm_io, true);
>>>     } else {
>>>         memory_region_set_enabled(&s->pm_io, false);
>>>     }
>> 
>> I am unable to avoid pointing out that this can be collapsed to
>> 
>>   memory_region_set_address(&s->pm_io, pm_io_base);
>>   memory_region_set_enabled(&s->pm_io, <enable>);
>> 
>> as the address is meaningless when disabled. Sorry.
>> 
>>>
>>> We will have to ensure that set_enabled(..., true) will never cause a
>>> dispatch using an outdated base address.
>> 
>> No, this is entirely safe.  If the guest changes a region offset
>> concurrently with issuing mmio on it, then it must expect either the old
>> or new offset to be used during dispatch.
> 

I forgot to mention that my clever rewrite above actually breaks this -
it needs to be wrapped in a transaction, otherwise we have a
move-then-disable pattern.

> You assume disable, reprogram, enable, ignoring that there could be
> multiple, invalid states between disable and enable. Real hardware takes
> care of the ordering.

It's possible of course, but the snippet above isn't susceptible on its own.

I don't think it's solvable.  To serialize, you must hold the device
lock, but we don't want to take the device lock during dispatch.

Users can protect against them by checking for ->enabled:

void foo_io_read(...)
{
   FooState *s = container_of(mr, ...);

   qemu_mutex_lock(&s->lock);
   if (!memory_region_enabled(mr)) {
       *data = ~(uint64_t)0;
       goto out;
   }
   ...
out:
   qemu_mutex_unlock(&s->lock);
}

Non-converted users will be naturally protected since we will take the
bql on their behalf.

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

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Anthony Liguori]

Liu Ping Fan <qemulist@gmail.com> writes:

> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> Scene:
>   obja lies in objA, when objA's ref->0, it will be freed,
> but at that time obja can still be in use.
>
> The real example is:
> typedef struct PCIIDEState {
>     PCIDevice dev;
>     IDEBus bus[2]; --> create in place
>     .....
> }
>
> When without big lock protection for mmio-dispatch, we will hold
> obj's refcnt. So memory_region_init_io() will replace the third para
> "void *opaque" with "Object *obj".
> With this patch, we can protect PCIIDEState from disappearing during
> mmio-dispatch hold the IDEBus->ref.
>
> And the ref circle has been broken when calling qdev_delete_subtree().
>
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

I think this is solving the wrong problem.  There are many, many
dependencies a device may have on other devices.  Memory allocation
isn't the only one.

The problem is that we want to make sure that a device doesn't "go away"
while an MMIO dispatch is happening.  This is easy to solve without
touching referencing counting.

The device will hold a lock while the MMIO is being dispatched.  The
delete path simply needs to acquire that same lock.  This will ensure
that a delete operation cannot finish while MMIO is still in flight.

Regarding deleting a device, not all devices are capable of being
deleted and specifically, devices that are composed within the memory of
another device cannot be directly deleted (they can only be deleted
as part of their parent's destruction).

Regards,

Anthony Liguori

> ---
>  hw/qdev.c |    2 ++
>  hw/qdev.h |    1 +
>  2 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index e2339a1..b09ebbf 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -510,6 +510,8 @@ void qbus_create_inplace(BusState *bus, const char *typename,
>  {
>      object_initialize(bus, typename);
>  
> +    bus->overlap = parent;
> +    object_ref(OBJECT(bus->overlap));
>      bus->parent = parent;
>      bus->name = name ? g_strdup(name) : NULL;
>      qbus_realize(bus);
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 182cfa5..9bc5783 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -117,6 +117,7 @@ struct BusState {
>      int allow_hotplug;
>      bool qom_allocated;
>      bool glib_allocated;
> +    DeviceState *overlap;
>      int max_index;
>      QTAILQ_HEAD(ChildrenHead, BusChild) children;
>      QLIST_ENTRY(BusState) sibling;
> -- 
> 1.7.4.4

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-08-27 15:19, Anthony Liguori wrote:
> Liu Ping Fan <qemulist@gmail.com> writes:
> 
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> Scene:
>>   obja lies in objA, when objA's ref->0, it will be freed,
>> but at that time obja can still be in use.
>>
>> The real example is:
>> typedef struct PCIIDEState {
>>     PCIDevice dev;
>>     IDEBus bus[2]; --> create in place
>>     .....
>> }
>>
>> When without big lock protection for mmio-dispatch, we will hold
>> obj's refcnt. So memory_region_init_io() will replace the third para
>> "void *opaque" with "Object *obj".
>> With this patch, we can protect PCIIDEState from disappearing during
>> mmio-dispatch hold the IDEBus->ref.
>>
>> And the ref circle has been broken when calling qdev_delete_subtree().
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> I think this is solving the wrong problem.  There are many, many
> dependencies a device may have on other devices.  Memory allocation
> isn't the only one.
> 
> The problem is that we want to make sure that a device doesn't "go away"
> while an MMIO dispatch is happening.  This is easy to solve without
> touching referencing counting.
> 
> The device will hold a lock while the MMIO is being dispatched.  The
> delete path simply needs to acquire that same lock.  This will ensure
> that a delete operation cannot finish while MMIO is still in flight.

That's a bit too simple. Quite a few MMIO/PIO fast-paths will work
without any device-specific locking, e.g. just to read a simple register
value. So we will need reference counting (for devices using private
locks), but on the "front-line" object: the memory region. That region
will block its owner from disappearing by waiting on dispatch when
someone tries to unregister it.

Also note that "holding a lock" is easily said but will be more tricky
in practice. Quite a significant share of our code will continue to run
under BQL, even for devices with their own locks. Init/cleanup functions
will likely fall into this category, simply because the surrounding
logic is hard to convert into fine-grained locking and is also not
performance critical. At the same time, we can't take BQL -> device-lock
as we have to support device-lock -> BQL ordering for (slow-path) calls
into BQL-protected areas while holding a per-device lock (e.g. device
mapping changes).

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Anthony Liguori]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> On 2012-08-27 15:19, Anthony Liguori wrote:
>> Liu Ping Fan <qemulist@gmail.com> writes:
>> 
>>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>
>>> Scene:
>>>   obja lies in objA, when objA's ref->0, it will be freed,
>>> but at that time obja can still be in use.
>>>
>>> The real example is:
>>> typedef struct PCIIDEState {
>>>     PCIDevice dev;
>>>     IDEBus bus[2]; --> create in place
>>>     .....
>>> }
>>>
>>> When without big lock protection for mmio-dispatch, we will hold
>>> obj's refcnt. So memory_region_init_io() will replace the third para
>>> "void *opaque" with "Object *obj".
>>> With this patch, we can protect PCIIDEState from disappearing during
>>> mmio-dispatch hold the IDEBus->ref.
>>>
>>> And the ref circle has been broken when calling qdev_delete_subtree().
>>>
>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> 
>> I think this is solving the wrong problem.  There are many, many
>> dependencies a device may have on other devices.  Memory allocation
>> isn't the only one.
>> 
>> The problem is that we want to make sure that a device doesn't "go away"
>> while an MMIO dispatch is happening.  This is easy to solve without
>> touching referencing counting.
>> 
>> The device will hold a lock while the MMIO is being dispatched.  The
>> delete path simply needs to acquire that same lock.  This will ensure
>> that a delete operation cannot finish while MMIO is still in flight.
>
> That's a bit too simple. Quite a few MMIO/PIO fast-paths will work
> without any device-specific locking, e.g. just to read a simple register
> value. So we will need reference counting

But then you'll need to acquire a lock to take the reference/remove the
reference which sort of defeats the purpose of trying to fast path.

> (for devices using private
> locks), but on the "front-line" object: the memory region. That region
> will block its owner from disappearing by waiting on dispatch when
> someone tries to unregister it.
>
> Also note that "holding a lock" is easily said but will be more tricky
> in practice. Quite a significant share of our code will continue to run
> under BQL, even for devices with their own locks. Init/cleanup functions
> will likely fall into this category,

I'm not sure I'm convinced of this--but it's hard to tell until we
really start converting.

BTW, I'm pretty sure we have to tackle main loop functions first before
we try to convert any devices off the BQL.

Regards,

Anthony Liguori

> simply because the surrounding
> logic is hard to convert into fine-grained locking and is also not
> performance critical. At the same time, we can't take BQL -> device-lock
> as we have to support device-lock -> BQL ordering for (slow-path) calls
> into BQL-protected areas while holding a per-device lock (e.g. device
> mapping changes).


>
> Jan
>
> -- 
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-08-27 17:14, Anthony Liguori wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> On 2012-08-27 15:19, Anthony Liguori wrote:
>>> Liu Ping Fan <qemulist@gmail.com> writes:
>>>
>>>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>>
>>>> Scene:
>>>>   obja lies in objA, when objA's ref->0, it will be freed,
>>>> but at that time obja can still be in use.
>>>>
>>>> The real example is:
>>>> typedef struct PCIIDEState {
>>>>     PCIDevice dev;
>>>>     IDEBus bus[2]; --> create in place
>>>>     .....
>>>> }
>>>>
>>>> When without big lock protection for mmio-dispatch, we will hold
>>>> obj's refcnt. So memory_region_init_io() will replace the third para
>>>> "void *opaque" with "Object *obj".
>>>> With this patch, we can protect PCIIDEState from disappearing during
>>>> mmio-dispatch hold the IDEBus->ref.
>>>>
>>>> And the ref circle has been broken when calling qdev_delete_subtree().
>>>>
>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>
>>> I think this is solving the wrong problem.  There are many, many
>>> dependencies a device may have on other devices.  Memory allocation
>>> isn't the only one.
>>>
>>> The problem is that we want to make sure that a device doesn't "go away"
>>> while an MMIO dispatch is happening.  This is easy to solve without
>>> touching referencing counting.
>>>
>>> The device will hold a lock while the MMIO is being dispatched.  The
>>> delete path simply needs to acquire that same lock.  This will ensure
>>> that a delete operation cannot finish while MMIO is still in flight.
>>
>> That's a bit too simple. Quite a few MMIO/PIO fast-paths will work
>> without any device-specific locking, e.g. just to read a simple register
>> value. So we will need reference counting
> 
> But then you'll need to acquire a lock to take the reference/remove the
> reference which sort of defeats the purpose of trying to fast path.

Atomic ops? RCU? This problem won't be solved for the first time.

> 
>> (for devices using private
>> locks), but on the "front-line" object: the memory region. That region
>> will block its owner from disappearing by waiting on dispatch when
>> someone tries to unregister it.
>>
>> Also note that "holding a lock" is easily said but will be more tricky
>> in practice. Quite a significant share of our code will continue to run
>> under BQL, even for devices with their own locks. Init/cleanup functions
>> will likely fall into this category,
> 
> I'm not sure I'm convinced of this--but it's hard to tell until we
> really start converting.
> 
> BTW, I'm pretty sure we have to tackle main loop functions first before
> we try to convert any devices off the BQL.

I'm sure we should leave existing code alone wherever possible, focusing
on providing alternative versions for those paths that matter. Example:
Most timers are fine under BQL. But some sensitive devices (RTC or HPET
as clock source) will want their own timers. So the approach is to
instantiate a separate, also prioritizeable instance of the timer
subsystem for them and be done.

We won't convert QEMU in a day, but we surely want results before the
last corner is refactored (which would take years, at best).

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Anthony Liguori]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> On 2012-08-27 17:14, Anthony Liguori wrote:
>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>> 
>>> On 2012-08-27 15:19, Anthony Liguori wrote:
>>>> Liu Ping Fan <qemulist@gmail.com> writes:
>>>>
>>>>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>>>
>>>>> Scene:
>>>>>   obja lies in objA, when objA's ref->0, it will be freed,
>>>>> but at that time obja can still be in use.
>>>>>
>>>>> The real example is:
>>>>> typedef struct PCIIDEState {
>>>>>     PCIDevice dev;
>>>>>     IDEBus bus[2]; --> create in place
>>>>>     .....
>>>>> }
>>>>>
>>>>> When without big lock protection for mmio-dispatch, we will hold
>>>>> obj's refcnt. So memory_region_init_io() will replace the third para
>>>>> "void *opaque" with "Object *obj".
>>>>> With this patch, we can protect PCIIDEState from disappearing during
>>>>> mmio-dispatch hold the IDEBus->ref.
>>>>>
>>>>> And the ref circle has been broken when calling qdev_delete_subtree().
>>>>>
>>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>>
>>>> I think this is solving the wrong problem.  There are many, many
>>>> dependencies a device may have on other devices.  Memory allocation
>>>> isn't the only one.
>>>>
>>>> The problem is that we want to make sure that a device doesn't "go away"
>>>> while an MMIO dispatch is happening.  This is easy to solve without
>>>> touching referencing counting.
>>>>
>>>> The device will hold a lock while the MMIO is being dispatched.  The
>>>> delete path simply needs to acquire that same lock.  This will ensure
>>>> that a delete operation cannot finish while MMIO is still in flight.
>>>
>>> That's a bit too simple. Quite a few MMIO/PIO fast-paths will work
>>> without any device-specific locking, e.g. just to read a simple register
>>> value. So we will need reference counting
>> 
>> But then you'll need to acquire a lock to take the reference/remove the
>> reference which sort of defeats the purpose of trying to fast path.
>
> Atomic ops? RCU? This problem won't be solved for the first time.

Yes, there are ways to do this, but you add a fair bit of complication.

It's much simplier to make objects not have any locks and enforce all
callers to protect access.  IOW, noone except the device gets to inc/dec
reference counts.

>>> (for devices using private
>>> locks), but on the "front-line" object: the memory region. That region
>>> will block its owner from disappearing by waiting on dispatch when
>>> someone tries to unregister it.
>>>
>>> Also note that "holding a lock" is easily said but will be more tricky
>>> in practice. Quite a significant share of our code will continue to run
>>> under BQL, even for devices with their own locks. Init/cleanup functions
>>> will likely fall into this category,
>> 
>> I'm not sure I'm convinced of this--but it's hard to tell until we
>> really start converting.
>> 
>> BTW, I'm pretty sure we have to tackle main loop functions first before
>> we try to convert any devices off the BQL.
>
> I'm sure we should leave existing code alone wherever possible, focusing
> on providing alternative versions for those paths that matter. Example:
> Most timers are fine under BQL. But some sensitive devices (RTC or HPET
> as clock source) will want their own timers. So the approach is to
> instantiate a separate, also prioritizeable instance of the timer
> subsystem for them and be done.

I disagree.  I think we conver the timer subsystem to be lockless and
then let some devices acquire the BQL during dispatch.

And we have a nice thread-aware main loop available to us--glib.  We
don't need to reinvent the wheel.

Regards,

Anthony Liguori

>
> We won't convert QEMU in a day, but we surely want results before the
> last corner is refactored (which would take years, at best).
>
> Jan
>
> -- 
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-08-27 18:24, Anthony Liguori wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> On 2012-08-27 17:14, Anthony Liguori wrote:
>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>
>>>> On 2012-08-27 15:19, Anthony Liguori wrote:
>>>>> Liu Ping Fan <qemulist@gmail.com> writes:
>>>>>
>>>>>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>>>>
>>>>>> Scene:
>>>>>>   obja lies in objA, when objA's ref->0, it will be freed,
>>>>>> but at that time obja can still be in use.
>>>>>>
>>>>>> The real example is:
>>>>>> typedef struct PCIIDEState {
>>>>>>     PCIDevice dev;
>>>>>>     IDEBus bus[2]; --> create in place
>>>>>>     .....
>>>>>> }
>>>>>>
>>>>>> When without big lock protection for mmio-dispatch, we will hold
>>>>>> obj's refcnt. So memory_region_init_io() will replace the third para
>>>>>> "void *opaque" with "Object *obj".
>>>>>> With this patch, we can protect PCIIDEState from disappearing during
>>>>>> mmio-dispatch hold the IDEBus->ref.
>>>>>>
>>>>>> And the ref circle has been broken when calling qdev_delete_subtree().
>>>>>>
>>>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>>>
>>>>> I think this is solving the wrong problem.  There are many, many
>>>>> dependencies a device may have on other devices.  Memory allocation
>>>>> isn't the only one.
>>>>>
>>>>> The problem is that we want to make sure that a device doesn't "go away"
>>>>> while an MMIO dispatch is happening.  This is easy to solve without
>>>>> touching referencing counting.
>>>>>
>>>>> The device will hold a lock while the MMIO is being dispatched.  The
>>>>> delete path simply needs to acquire that same lock.  This will ensure
>>>>> that a delete operation cannot finish while MMIO is still in flight.
>>>>
>>>> That's a bit too simple. Quite a few MMIO/PIO fast-paths will work
>>>> without any device-specific locking, e.g. just to read a simple register
>>>> value. So we will need reference counting
>>>
>>> But then you'll need to acquire a lock to take the reference/remove the
>>> reference which sort of defeats the purpose of trying to fast path.
>>
>> Atomic ops? RCU? This problem won't be solved for the first time.
> 
> Yes, there are ways to do this, but you add a fair bit of complication.
> 
> It's much simplier to make objects not have any locks and enforce all
> callers to protect access.  IOW, noone except the device gets to inc/dec
> reference counts.

Pulling locks out of the devices may appear simpler on first sight but
won't be in practice. Objects can issue requests to other objects. Will
they hold its lock while taking the target's lock? Lock management is a
per-device thing, not only for performance reasons.

But I guess we will have to compare working prototypes. Many concepts
look nice on the paper, but when you implement them or even try to run
them, the interesting problems show up. Specifically those related to a
smooth transition phase.

> 
>>>> (for devices using private
>>>> locks), but on the "front-line" object: the memory region. That region
>>>> will block its owner from disappearing by waiting on dispatch when
>>>> someone tries to unregister it.
>>>>
>>>> Also note that "holding a lock" is easily said but will be more tricky
>>>> in practice. Quite a significant share of our code will continue to run
>>>> under BQL, even for devices with their own locks. Init/cleanup functions
>>>> will likely fall into this category,
>>>
>>> I'm not sure I'm convinced of this--but it's hard to tell until we
>>> really start converting.
>>>
>>> BTW, I'm pretty sure we have to tackle main loop functions first before
>>> we try to convert any devices off the BQL.
>>
>> I'm sure we should leave existing code alone wherever possible, focusing
>> on providing alternative versions for those paths that matter. Example:
>> Most timers are fine under BQL. But some sensitive devices (RTC or HPET
>> as clock source) will want their own timers. So the approach is to
>> instantiate a separate, also prioritizeable instance of the timer
>> subsystem for them and be done.
> 
> I disagree.  I think we conver the timer subsystem to be lockless and
> then let some devices acquire the BQL during dispatch.

The timer subsystem is lockless in this sense already. Its user holds
the necessary lock (BQL so far). I'm just making it multi-instance so
that the time-critical users don't need to worry about other, low-prio
timers. In that sense, I guess we are describing the same end result.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 08/27/2012 09:24 AM, Anthony Liguori wrote:
> >
> > I'm sure we should leave existing code alone wherever possible, focusing
> > on providing alternative versions for those paths that matter. Example:
> > Most timers are fine under BQL. But some sensitive devices (RTC or HPET
> > as clock source) will want their own timers. So the approach is to
> > instantiate a separate, also prioritizeable instance of the timer
> > subsystem for them and be done.
>
> I disagree.  I think we conver the timer subsystem to be lockless and
> then let some devices acquire the BQL during dispatch.

I agree with your disagreement but disagree with the rest.  The timer
subsystem should have its own internal locking that callers will not be
aware of.  Requiring devices to acquire the bql will lead to deadlocks.

Note that fine-grained locking timers will also require reference
counting: you want to call the timer expiration callback after releasing
the timer subsystem lock, so you need to make sure the callback does not
go away.

Linux manages without it (hrtimer_interrupt), so maybe we can too.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Anthony Liguori]

Avi Kivity <avi@redhat.com> writes:

> On 08/27/2012 09:24 AM, Anthony Liguori wrote:
>> >
>> > I'm sure we should leave existing code alone wherever possible, focusing
>> > on providing alternative versions for those paths that matter. Example:
>> > Most timers are fine under BQL. But some sensitive devices (RTC or HPET
>> > as clock source) will want their own timers. So the approach is to
>> > instantiate a separate, also prioritizeable instance of the timer
>> > subsystem for them and be done.
>>
>> I disagree.  I think we conver the timer subsystem to be lockless and
>> then let some devices acquire the BQL during dispatch.
>
> I agree with your disagreement but disagree with the rest.  The timer
> subsystem should have its own internal locking that callers will not be
> aware of.  Requiring devices to acquire the bql will lead to
> deadlocks.

Err, I completely mispoke there.

I meant, to say, we should convert the timer subsystem to be re-entrant
and then let some devices acquire the BQL during dispatch.

Existing users of the time API should get the BQL acquired automagically
for them.  The new API would not hold the BQL during dispatch but the
old API, implemented in terms of the new API, would acquire it on behalf
of the caller.

As a rule, if a device relies on the BQL, it must use the old APIs.  If
a device uses the new APIs, it must not acquire the BQL.

> Note that fine-grained locking timers will also require reference
> counting: you want to call the timer expiration callback after releasing
> the timer subsystem lock, so you need to make sure the callback does not
> go away.

glib simply uses a single main loop lock to deal with all of this.  I
think that's absolutely the right model.

It's best to start this conversion using very coarse locking.  There's
no need to start with ultra fine grain locking.

Regards,

Anthony Liguori

> Linux manages without it (hrtimer_interrupt), so maybe we can too.
> -- 
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Jan Kiszka]

On 2012-08-27 21:17, Anthony Liguori wrote:
> Avi Kivity <avi@redhat.com> writes:
> 
>> On 08/27/2012 09:24 AM, Anthony Liguori wrote:
>>>>
>>>> I'm sure we should leave existing code alone wherever possible, focusing
>>>> on providing alternative versions for those paths that matter. Example:
>>>> Most timers are fine under BQL. But some sensitive devices (RTC or HPET
>>>> as clock source) will want their own timers. So the approach is to
>>>> instantiate a separate, also prioritizeable instance of the timer
>>>> subsystem for them and be done.
>>>
>>> I disagree.  I think we conver the timer subsystem to be lockless and
>>> then let some devices acquire the BQL during dispatch.
>>
>> I agree with your disagreement but disagree with the rest.  The timer
>> subsystem should have its own internal locking that callers will not be
>> aware of.  Requiring devices to acquire the bql will lead to
>> deadlocks.
> 
> Err, I completely mispoke there.
> 
> I meant, to say, we should convert the timer subsystem to be re-entrant
> and then let some devices acquire the BQL during dispatch.
> 
> Existing users of the time API should get the BQL acquired automagically
> for them.  The new API would not hold the BQL during dispatch but the
> old API, implemented in terms of the new API, would acquire it on behalf
> of the caller.
> 
> As a rule, if a device relies on the BQL, it must use the old APIs.  If
> a device uses the new APIs, it must not acquire the BQL.

This makes sense.

> 
>> Note that fine-grained locking timers will also require reference
>> counting: you want to call the timer expiration callback after releasing
>> the timer subsystem lock, so you need to make sure the callback does not
>> go away.
> 
> glib simply uses a single main loop lock to deal with all of this.  I
> think that's absolutely the right model.

That depends on what is under the lock. Also, relying on locking of
external libraries is a no-go for realtime as they usually do not care
about priorities and inversion avoidance.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 08/27/2012 12:17 PM, Anthony Liguori wrote:
> Avi Kivity <avi@redhat.com> writes:
>
> > On 08/27/2012 09:24 AM, Anthony Liguori wrote:
> >> >
> >> > I'm sure we should leave existing code alone wherever possible, focusing
> >> > on providing alternative versions for those paths that matter. Example:
> >> > Most timers are fine under BQL. But some sensitive devices (RTC or HPET
> >> > as clock source) will want their own timers. So the approach is to
> >> > instantiate a separate, also prioritizeable instance of the timer
> >> > subsystem for them and be done.
> >>
> >> I disagree.  I think we conver the timer subsystem to be lockless and
> >> then let some devices acquire the BQL during dispatch.
> >
> > I agree with your disagreement but disagree with the rest.  The timer
> > subsystem should have its own internal locking that callers will not be
> > aware of.  Requiring devices to acquire the bql will lead to
> > deadlocks.
>
> Err, I completely mispoke there.
>
> I meant, to say, we should convert the timer subsystem to be re-entrant
> and then let some devices acquire the BQL during dispatch.

That would be all devices, with converted devices not acquiring it as
they are converted.

> Existing users of the time API should get the BQL acquired automagically
> for them.  The new API would not hold the BQL during dispatch but the
> old API, implemented in terms of the new API, would acquire it on behalf
> of the caller.
>
> As a rule, if a device relies on the BQL, it must use the old APIs.  If
> a device uses the new APIs, it must not acquire the BQL.

This exactly mirrors the plan with memory region conversion.

> > Note that fine-grained locking timers will also require reference
> > counting: you want to call the timer expiration callback after releasing
> > the timer subsystem lock, so you need to make sure the callback does not
> > go away.
>
> glib simply uses a single main loop lock to deal with all of this.  I
> think that's absolutely the right model.
>
> It's best to start this conversion using very coarse locking.  There's
> no need to start with ultra fine grain locking.

How does it work?  You have to drop this main loop lock to dispatch the
callback, so the data structure you use for dispatch is no longer protected.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Paolo Bonzini]

Il 27/08/2012 22:58, Avi Kivity ha scritto:
>> > It's best to start this conversion using very coarse locking.  There's
>> > no need to start with ultra fine grain locking.
> How does it work?  You have to drop this main loop lock to dispatch the
> callback, so the data structure you use for dispatch is no longer protected.

It is protected by a mixture of reference counting, thread-local storage and
careful coding to take care of re-entrancy.

I'm not too sure that we can use it, though.  Mostly because of the reasons Jan
mentioned (it may also not be precise enough, but we can fix that with our own
GSource wrapping POSIX real-time timers or Linux timerfds).

Paolo

Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Avi Kivity]

On 08/27/2012 06:19 AM, Anthony Liguori wrote:
> Liu Ping Fan <qemulist@gmail.com> writes:
>
> > From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >
> > Scene:
> >   obja lies in objA, when objA's ref->0, it will be freed,
> > but at that time obja can still be in use.
> >
> > The real example is:
> > typedef struct PCIIDEState {
> >     PCIDevice dev;
> >     IDEBus bus[2]; --> create in place
> >     .....
> > }
> >
> > When without big lock protection for mmio-dispatch, we will hold
> > obj's refcnt. So memory_region_init_io() will replace the third para
> > "void *opaque" with "Object *obj".
> > With this patch, we can protect PCIIDEState from disappearing during
> > mmio-dispatch hold the IDEBus->ref.
> >
> > And the ref circle has been broken when calling qdev_delete_subtree().
> >
> > Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> I think this is solving the wrong problem.  There are many, many
> dependencies a device may have on other devices.  Memory allocation
> isn't the only one.
>
> The problem is that we want to make sure that a device doesn't "go away"
> while an MMIO dispatch is happening.  This is easy to solve without
> touching referencing counting.
>
> The device will hold a lock while the MMIO is being dispatched.  The
> delete path simply needs to acquire that same lock.  This will ensure
> that a delete operation cannot finish while MMIO is still in flight.

Where does the lock live?  If it lives in the device, then we can't
acquire it during mmio dispatch (the device may have gone away).  If it
lives outside the device, we need to to tell the memory core about it. 
I would really like to avoid adding additional state; I think the curent
opaque should serve both to provide state to the callbacks and for life
cycle management.

> Regarding deleting a device, not all devices are capable of being
> deleted and specifically, devices that are composed within the memory of
> another device cannot be directly deleted (they can only be deleted
> as part of their parent's destruction).

This doesn't help at all.  We have devices that can be deleted, and we
would like to make the devices use the unlocked path, so we have to
account for it.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.