[Qemu-devel] [PATCH 0/5] prepare unplug out of protection of global lock

[Qemu-devel] [PATCH 0/5] prepare unplug out of protection of global lock

[Liu Ping Fan]

refer to orignal plan posted by Marcelo Tosatti,
http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg04315.html

These patches protect DeviceState's rd from reclaimer. It is neccessary
when no qemu_global_lock protects between them.

[Qemu-devel] [PATCH 1/5] qom: adopt rwlock to protect accessing dev from removing it

[Liu Ping Fan]

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

rwlock:
  qemu_device_tree_mutex

rd side:
  --device_del(destruction of device will be postphoned until unplug
    ack from guest),
  --pci hot-unplug
  --iteration (qdev_reset_all)

wr side:
  --device_add

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

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index e7fb780..b3b88c1 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -265,9 +265,11 @@ static int pci_device_hot_remove(Monitor *mon, const char *pci_addr)
         return -1;
     }
 
+    qemu_rwlock_rdlock_devtree();
     d = pci_find_device(pci_find_root_bus(dom), bus, PCI_DEVFN(slot, 0));
     if (!d) {
         monitor_printf(mon, "slot %d empty\n", slot);
+        qemu_rwlock_unlock_devtree();
         return -1;
     }
 
@@ -275,9 +277,11 @@ static int pci_device_hot_remove(Monitor *mon, const char *pci_addr)
     if (error_is_set(&local_err)) {
         monitor_printf(mon, "%s\n", error_get_pretty(local_err));
         error_free(local_err);
+        qemu_rwlock_unlock_devtree();
         return -1;
     }
 
+    qemu_rwlock_unlock_devtree();
     return 0;
 }
 
diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index 7915b45..8aec067 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -429,14 +429,18 @@ DeviceState *qdev_device_add(QemuOpts *opts)
 
     /* find bus */
     path = qemu_opt_get(opts, "bus");
+
+    qemu_rwlock_wrlock_devtree();
     if (path != NULL) {
         bus = qbus_find(path);
         if (!bus) {
+            qemu_rwlock_unlock_devtree();
             return NULL;
         }
         if (strcmp(object_get_typename(OBJECT(bus)), k->bus_type) != 0) {
             qerror_report(QERR_BAD_BUS_FOR_DEVICE,
                           driver, object_get_typename(OBJECT(bus)));
+            qemu_rwlock_unlock_devtree();
             return NULL;
         }
     } else {
@@ -444,11 +448,13 @@ DeviceState *qdev_device_add(QemuOpts *opts)
         if (!bus) {
             qerror_report(QERR_NO_BUS_FOR_DEVICE,
                           driver, k->bus_type);
+            qemu_rwlock_unlock_devtree();
             return NULL;
         }
     }
     if (qdev_hotplug && !bus->allow_hotplug) {
         qerror_report(QERR_BUS_NO_HOTPLUG, bus->name);
+        qemu_rwlock_unlock_devtree();
         return NULL;
     }
 
@@ -466,6 +472,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     }
     if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
         qdev_free(qdev);
+        qemu_rwlock_unlock_devtree();
         return NULL;
     }
     if (qdev->id) {
@@ -478,6 +485,8 @@ DeviceState *qdev_device_add(QemuOpts *opts)
                                   OBJECT(qdev), NULL);
         g_free(name);
     }        
+    qemu_rwlock_unlock_devtree();
+
     if (qdev_init(qdev) < 0) {
         qerror_report(QERR_DEVICE_INIT_FAILED, driver);
         return NULL;
@@ -600,13 +609,19 @@ void qmp_device_del(const char *id, Error **errp)
 {
     DeviceState *dev;
 
+    /* protect against unplug ack from guest, where we really remove device
+     * from system
+     */
+    qemu_rwlock_rdlock_devtree();
     dev = qdev_find_recursive(sysbus_get_default(), id);
     if (NULL == dev) {
         error_set(errp, QERR_DEVICE_NOT_FOUND, id);
+        qemu_rwlock_unlock_devtree();
         return;
     }
-
+    /* Just remove from system, and drop refcnt there*/
     qdev_unplug(dev, errp);
+    qemu_rwlock_unlock_devtree();
 }
 
 void qdev_machine_init(void)
diff --git a/hw/qdev.c b/hw/qdev.c
index af54467..ac55e45 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -230,7 +230,9 @@ static int qbus_reset_one(BusState *bus, void *opaque)
 
 void qdev_reset_all(DeviceState *dev)
 {
+    qemu_rwlock_rdlock_devtree();
     qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL);
+    qemu_rwlock_unlock_devtree();
 }
 
 void qbus_reset_all_fn(void *opaque)
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 1/5] qom: adopt rwlock to protect accessing dev from removing it

[Paolo Bonzini]

Il 25/07/2012 05:31, Liu Ping Fan ha scritto:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> rwlock:
>   qemu_device_tree_mutex
> 
> rd side:
>   --device_del(destruction of device will be postphoned until unplug
>     ack from guest),
>   --pci hot-unplug
>   --iteration (qdev_reset_all)
> 
> wr side:
>   --device_add
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/pci-hotplug.c  |    4 ++++
>  hw/qdev-monitor.c |   17 ++++++++++++++++-
>  hw/qdev.c         |    2 ++
>  3 files changed, 22 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
> index e7fb780..b3b88c1 100644
> --- a/hw/pci-hotplug.c
> +++ b/hw/pci-hotplug.c
> @@ -265,9 +265,11 @@ static int pci_device_hot_remove(Monitor *mon, const char *pci_addr)
>          return -1;
>      }
>  
> +    qemu_rwlock_rdlock_devtree();

This is not defined anywhere, is a piece missing in the patch?

Paolo

Re: [Qemu-devel] [PATCH 1/5] qom: adopt rwlock to protect accessing dev from removing it

[liu ping fan]

On Wed, Jul 25, 2012 at 5:08 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 25/07/2012 05:31, Liu Ping Fan ha scritto:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> rwlock:
>>   qemu_device_tree_mutex
>>
>> rd side:
>>   --device_del(destruction of device will be postphoned until unplug
>>     ack from guest),
>>   --pci hot-unplug
>>   --iteration (qdev_reset_all)
>>
>> wr side:
>>   --device_add
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  hw/pci-hotplug.c  |    4 ++++
>>  hw/qdev-monitor.c |   17 ++++++++++++++++-
>>  hw/qdev.c         |    2 ++
>>  3 files changed, 22 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
>> index e7fb780..b3b88c1 100644
>> --- a/hw/pci-hotplug.c
>> +++ b/hw/pci-hotplug.c
>> @@ -265,9 +265,11 @@ static int pci_device_hot_remove(Monitor *mon, const char *pci_addr)
>>          return -1;
>>      }
>>
>> +    qemu_rwlock_rdlock_devtree();
>
> This is not defined anywhere, is a piece missing in the patch?
>
Oh, yes, I miss the patch.  In that patch, these rwlock are just place holder.
I see there is already try to implement rwlock for qemu.
    http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg00192.html
and is it the time for introduce rwlock for qemu?

Thanks,
pingfan


> Paolo
>

Re: [Qemu-devel] [PATCH 1/5] qom: adopt rwlock to protect accessing dev from removing it

[Avi Kivity]

On 07/26/2012 03:56 PM, liu ping fan wrote:
> On Wed, Jul 25, 2012 at 5:08 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 25/07/2012 05:31, Liu Ping Fan ha scritto:
>>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>
>>> rwlock:
>>>   qemu_device_tree_mutex
>>>
>>> rd side:
>>>   --device_del(destruction of device will be postphoned until unplug
>>>     ack from guest),
>>>   --pci hot-unplug
>>>   --iteration (qdev_reset_all)
>>>
>>> wr side:
>>>   --device_add
>>>
>>
>> This is not defined anywhere, is a piece missing in the patch?
>>
> Oh, yes, I miss the patch.  In that patch, these rwlock are just place holder.
> I see there is already try to implement rwlock for qemu.
>     http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg00192.html
> and is it the time for introduce rwlock for qemu?


>From the description above, I don't see why it can't be a mutex.

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

Re: [Qemu-devel] [PATCH 1/5] qom: adopt rwlock to protect accessing dev from removing it

[liu ping fan]

On Thu, Jul 26, 2012 at 9:00 PM, Avi Kivity <avi@redhat.com> wrote:
> On 07/26/2012 03:56 PM, liu ping fan wrote:
>> On Wed, Jul 25, 2012 at 5:08 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 25/07/2012 05:31, Liu Ping Fan ha scritto:
>>>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>>
>>>> rwlock:
>>>>   qemu_device_tree_mutex
>>>>
>>>> rd side:
>>>>   --device_del(destruction of device will be postphoned until unplug
>>>>     ack from guest),
>>>>   --pci hot-unplug
>>>>   --iteration (qdev_reset_all)
>>>>
>>>> wr side:
>>>>   --device_add
>>>>
>>>
>>> This is not defined anywhere, is a piece missing in the patch?
>>>
>> Oh, yes, I miss the patch.  In that patch, these rwlock are just place holder.
>> I see there is already try to implement rwlock for qemu.
>>     http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg00192.html
>> and is it the time for introduce rwlock for qemu?
>
>
> From the description above, I don't see why it can't be a mutex.
>
Searching in the device tree (or MemoryRegion view) can be often in
parallel, especially in mmio-dispatch code path

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

Re: [Qemu-devel] [PATCH 1/5] qom: adopt rwlock to protect accessing dev from removing it

[Avi Kivity]

On 07/26/2012 04:14 PM, liu ping fan wrote:
>>
>> From the description above, I don't see why it can't be a mutex.
>>
> Searching in the device tree (or MemoryRegion view) can be often in
> parallel, especially in mmio-dispatch code path

In mmio dispatch we have a pointer to the object, we don't need to
search anything.  Is device tree search a hot path?


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

Re: [Qemu-devel] [PATCH 1/5] qom: adopt rwlock to protect accessing dev from removing it

[liu ping fan]

On Thu, Jul 26, 2012 at 9:15 PM, Avi Kivity <avi@redhat.com> wrote:
> On 07/26/2012 04:14 PM, liu ping fan wrote:
>>>
>>> From the description above, I don't see why it can't be a mutex.
>>>
>> Searching in the device tree (or MemoryRegion view) can be often in
>> parallel, especially in mmio-dispatch code path
>
> In mmio dispatch we have a pointer to the object, we don't need to
> search anything.  Is device tree search a hot path?
>
I think, we need lock to protect searching --phys_page_find()  from
deleter--DeviceClass:unmap,  so rwlock?
>
> --
> error compiling committee.c: too many arguments to function
>
>

Re: [Qemu-devel] [PATCH 1/5] qom: adopt rwlock to protect accessing dev from removing it

[Avi Kivity]

On 07/26/2012 04:21 PM, liu ping fan wrote:
> On Thu, Jul 26, 2012 at 9:15 PM, Avi Kivity <avi@redhat.com> wrote:
>> On 07/26/2012 04:14 PM, liu ping fan wrote:
>>>>
>>>> From the description above, I don't see why it can't be a mutex.
>>>>
>>> Searching in the device tree (or MemoryRegion view) can be often in
>>> parallel, especially in mmio-dispatch code path
>>
>> In mmio dispatch we have a pointer to the object, we don't need to
>> search anything.  Is device tree search a hot path?
>>
> I think, we need lock to protect searching --phys_page_find()  from
> deleter--DeviceClass:unmap,  so rwlock?

Better a lock on phys_map (because it is easily replaced by rcu, later).

I think phys_map is also better isolated, so it will be easier to find
all the placed that need protection and to avoid deadlock.


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

[Qemu-devel] [PATCH 2/5] exec.c: use refcnt to protect device during dispatching

[Liu Ping Fan]

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

acquire device's refcnt with qemu_device_tree_mutex rwlock, so we
can safely handle it when mmio dispatch.

If in radix-tree, leaf is subpage, then move further step to acquire
opaque which is the type --DeiveState.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 exec.c   |   38 ++++++++++++++++++++++++++++++++++++++
 memory.h |    2 ++
 2 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/exec.c b/exec.c
index 8244d54..d2a6d08 100644
--- a/exec.c
+++ b/exec.c
@@ -3032,6 +3032,30 @@ static void subpage_write(void *opaque, target_phys_addr_t addr,
     io_mem_write(section->mr, addr, value, len);
 }
 
+static MemoryRegionSection *subpage_get_backend(subpage_t *mmio,
+    target_phys_addr_t addr)
+{
+    MemoryRegionSection *section;
+    unsigned int idx = SUBPAGE_IDX(addr);
+
+    section = &phys_sections[mmio->sub_section[idx]];
+    return section;
+}
+
+void *get_backend(MemoryRegion* mr,  target_phys_addr_t addr)
+{
+    MemoryRegionSection *p;
+    Object *ret;
+
+    if (mr->subpage) {
+        p = subpage_get_backend(mr->opaque, addr);
+        ret = OBJECT(p->mr->opaque);
+    } else {
+        ret = OBJECT(mr->opaque);
+    }
+    return ret;
+}
+
 static const MemoryRegionOps subpage_ops = {
     .read = subpage_read,
     .write = subpage_write,
@@ -3396,13 +3420,25 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
     uint32_t val;
     target_phys_addr_t page;
     MemoryRegionSection *section;
+    Object *bk;
 
     while (len > 0) {
         page = addr & TARGET_PAGE_MASK;
         l = (page + TARGET_PAGE_SIZE) - addr;
         if (l > len)
             l = len;
+
+        qemu_rwlock_rdlock_devtree();
         section = phys_page_find(page >> TARGET_PAGE_BITS);
+        if (!(memory_region_is_ram(section->mr) ||
+            memory_region_is_romd(section->mr)) && !is_write) {
+            bk = get_backend(section->mr, addr);
+            object_ref(bk);
+        } else if (!memory_region_is_ram(section->mr) && is_write) {
+            bk = get_backend(section->mr, addr);
+            object_ref(bk);
+        }
+        qemu_rwlock_unlock_devtree();
 
         if (is_write) {
             if (!memory_region_is_ram(section->mr)) {
@@ -3426,6 +3462,7 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
                     io_mem_write(section->mr, addr1, val, 1);
                     l = 1;
                 }
+                object_unref(bk);
             } else if (!section->readonly) {
                 ram_addr_t addr1;
                 addr1 = memory_region_get_ram_addr(section->mr)
@@ -3464,6 +3501,7 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
                     stb_p(buf, val);
                     l = 1;
                 }
+                object_unref(bk);
             } else {
                 /* RAM case */
                 ptr = qemu_get_ram_ptr(section->mr->ram_addr
diff --git a/memory.h b/memory.h
index 740c48e..e5a86dc 100644
--- a/memory.h
+++ b/memory.h
@@ -748,6 +748,8 @@ void memory_global_dirty_log_stop(void);
 
 void mtree_info(fprintf_function mon_printf, void *f);
 
+void *get_backend(MemoryRegion* mr,  target_phys_addr_t addr);
+
 #endif
 
 #endif
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 2/5] exec.c: use refcnt to protect device during dispatching

[Stefan Hajnoczi]

On Wed, Jul 25, 2012 at 4:31 AM, Liu Ping Fan <qemulist@gmail.com> wrote:
> @@ -3396,13 +3420,25 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>      uint32_t val;
>      target_phys_addr_t page;
>      MemoryRegionSection *section;
> +    Object *bk;
>
>      while (len > 0) {
>          page = addr & TARGET_PAGE_MASK;
>          l = (page + TARGET_PAGE_SIZE) - addr;
>          if (l > len)
>              l = len;
> +
> +        qemu_rwlock_rdlock_devtree();
>          section = phys_page_find(page >> TARGET_PAGE_BITS);
> +        if (!(memory_region_is_ram(section->mr) ||
> +            memory_region_is_romd(section->mr)) && !is_write) {
> +            bk = get_backend(section->mr, addr);
> +            object_ref(bk);
> +        } else if (!memory_region_is_ram(section->mr) && is_write) {
> +            bk = get_backend(section->mr, addr);
> +            object_ref(bk);
> +        }
> +        qemu_rwlock_unlock_devtree();
>
>          if (is_write) {
>              if (!memory_region_is_ram(section->mr)) {
> @@ -3426,6 +3462,7 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>                      io_mem_write(section->mr, addr1, val, 1);
>                      l = 1;
>                  }
> +                object_unref(bk);

Currently object_ref()/object_unref() are not atomic.  Will you send
another patch to perform atomic increment/decrement or how will
per-device synchronization work?

Stefan

Re: [Qemu-devel] [PATCH 2/5] exec.c: use refcnt to protect device during dispatching

[liu ping fan]

On Wed, Jul 25, 2012 at 3:43 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Wed, Jul 25, 2012 at 4:31 AM, Liu Ping Fan <qemulist@gmail.com> wrote:
>> @@ -3396,13 +3420,25 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>>      uint32_t val;
>>      target_phys_addr_t page;
>>      MemoryRegionSection *section;
>> +    Object *bk;
>>
>>      while (len > 0) {
>>          page = addr & TARGET_PAGE_MASK;
>>          l = (page + TARGET_PAGE_SIZE) - addr;
>>          if (l > len)
>>              l = len;
>> +
>> +        qemu_rwlock_rdlock_devtree();
>>          section = phys_page_find(page >> TARGET_PAGE_BITS);
>> +        if (!(memory_region_is_ram(section->mr) ||
>> +            memory_region_is_romd(section->mr)) && !is_write) {
>> +            bk = get_backend(section->mr, addr);
>> +            object_ref(bk);
>> +        } else if (!memory_region_is_ram(section->mr) && is_write) {
>> +            bk = get_backend(section->mr, addr);
>> +            object_ref(bk);
>> +        }
>> +        qemu_rwlock_unlock_devtree();
>>
>>          if (is_write) {
>>              if (!memory_region_is_ram(section->mr)) {
>> @@ -3426,6 +3462,7 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>>                      io_mem_write(section->mr, addr1, val, 1);
>>                      l = 1;
>>                  }
>> +                object_unref(bk);
>
> Currently object_ref()/object_unref() are not atomic.  Will you send

We obey the rule:
           rdlock->search->ref_get,
           wrlock->remove ->ref_put
So can  it causes problem if object_ref()/object_unref()  are not atomic?
Thanx, pingfan
> another patch to perform atomic increment/decrement or how will
> per-device synchronization work?
>
> Stefan

Re: [Qemu-devel] [PATCH 2/5] exec.c: use refcnt to protect device during dispatching

[Paolo Bonzini]

Il 25/07/2012 10:12, liu ping fan ha scritto:
>>> >> +        qemu_rwlock_rdlock_devtree();
>>> >>          section = phys_page_find(page >> TARGET_PAGE_BITS);
>>> >> +        if (!(memory_region_is_ram(section->mr) ||
>>> >> +            memory_region_is_romd(section->mr)) && !is_write) {
>>> >> +            bk = get_backend(section->mr, addr);
>>> >> +            object_ref(bk);
>>> >> +        } else if (!memory_region_is_ram(section->mr) && is_write) {
>>> >> +            bk = get_backend(section->mr, addr);
>>> >> +            object_ref(bk);
>>> >> +        }
>>> >> +        qemu_rwlock_unlock_devtree();
>>> >>
>>> >>          if (is_write) {
>>> >>              if (!memory_region_is_ram(section->mr)) {
>>> >> @@ -3426,6 +3462,7 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>>> >>                      io_mem_write(section->mr, addr1, val, 1);
>>> >>                      l = 1;
>>> >>                  }
>>> >> +                object_unref(bk);
>> >
>> > Currently object_ref()/object_unref() are not atomic.  Will you send
> We obey the rule:
>            rdlock->search->ref_get,
>            wrlock->remove ->ref_put
> So can  it causes problem if object_ref()/object_unref()  are not atomic?

Yes, two CPUs can perform object_ref at the same time.

You can find a header file for atomic operations here:
https://github.com/bonzini/qemu/commit/atomics.patch

Paolo

Re: [Qemu-devel] [PATCH 2/5] exec.c: use refcnt to protect device during dispatching

[liu ping fan]

On Wed, Jul 25, 2012 at 5:18 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 25/07/2012 10:12, liu ping fan ha scritto:
>>>> >> +        qemu_rwlock_rdlock_devtree();
>>>> >>          section = phys_page_find(page >> TARGET_PAGE_BITS);
>>>> >> +        if (!(memory_region_is_ram(section->mr) ||
>>>> >> +            memory_region_is_romd(section->mr)) && !is_write) {
>>>> >> +            bk = get_backend(section->mr, addr);
>>>> >> +            object_ref(bk);
>>>> >> +        } else if (!memory_region_is_ram(section->mr) && is_write) {
>>>> >> +            bk = get_backend(section->mr, addr);
>>>> >> +            object_ref(bk);
>>>> >> +        }
>>>> >> +        qemu_rwlock_unlock_devtree();
>>>> >>
>>>> >>          if (is_write) {
>>>> >>              if (!memory_region_is_ram(section->mr)) {
>>>> >> @@ -3426,6 +3462,7 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>>>> >>                      io_mem_write(section->mr, addr1, val, 1);
>>>> >>                      l = 1;
>>>> >>                  }
>>>> >> +                object_unref(bk);
>>> >
>>> > Currently object_ref()/object_unref() are not atomic.  Will you send
>> We obey the rule:
>>            rdlock->search->ref_get,
>>            wrlock->remove ->ref_put
>> So can  it causes problem if object_ref()/object_unref()  are not atomic?
>
> Yes, two CPUs can perform object_ref at the same time.
>
> You can find a header file for atomic operations here:
> https://github.com/bonzini/qemu/commit/atomics.patch
>
Got it, thanks

> Paolo

Re: [Qemu-devel] [PATCH 2/5] exec.c: use refcnt to protect device during dispatching

[Avi Kivity]

On 07/25/2012 06:31 AM, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> acquire device's refcnt with qemu_device_tree_mutex rwlock, so we
> can safely handle it when mmio dispatch.
> 
> If in radix-tree, leaf is subpage, then move further step to acquire
> opaque which is the type --DeiveState.
> 
>  
> +static MemoryRegionSection *subpage_get_backend(subpage_t *mmio,
> +    target_phys_addr_t addr)
> +{
> +    MemoryRegionSection *section;
> +    unsigned int idx = SUBPAGE_IDX(addr);
> +
> +    section = &phys_sections[mmio->sub_section[idx]];
> +    return section;
> +}
> +
> +void *get_backend(MemoryRegion* mr,  target_phys_addr_t addr)
> +{
> +    MemoryRegionSection *p;
> +    Object *ret;
> +
> +    if (mr->subpage) {
> +        p = subpage_get_backend(mr->opaque, addr);
> +        ret = OBJECT(p->mr->opaque);
> +    } else {
> +        ret = OBJECT(mr->opaque);
> +    }
> +    return ret;
> +}
> +

You don't enforce that mr->opaque is an object.

The name 'backend' is inappropriate here (actually I don't like it
anywhere).  If we can s/opaque/object/ (and change the type too, we can
call it get_object() (and return an Object *).

>  static const MemoryRegionOps subpage_ops = {
>      .read = subpage_read,
>      .write = subpage_write,
> @@ -3396,13 +3420,25 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>      uint32_t val;
>      target_phys_addr_t page;
>      MemoryRegionSection *section;
> +    Object *bk;
>  
>      while (len > 0) {
>          page = addr & TARGET_PAGE_MASK;
>          l = (page + TARGET_PAGE_SIZE) - addr;
>          if (l > len)
>              l = len;
> +
> +        qemu_rwlock_rdlock_devtree();
>          section = phys_page_find(page >> TARGET_PAGE_BITS);

Does the devtree lock also protect the data structures accessed by
phys_page_find()?  Seems wrong.

> +        if (!(memory_region_is_ram(section->mr) ||
> +            memory_region_is_romd(section->mr)) && !is_write) {
> +            bk = get_backend(section->mr, addr);
> +            object_ref(bk);
> +        } else if (!memory_region_is_ram(section->mr) && is_write) {
> +            bk = get_backend(section->mr, addr);
> +            object_ref(bk);
> +        }

Best push the ugliness that computes bk into a small helper, and do just
the object_ref() here.

> +        qemu_rwlock_unlock_devtree();
>  
>          if (is_write) {
>              if (!memory_region_is_ram(section->mr)) {
> @@ -3426,6 +3462,7 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>                      io_mem_write(section->mr, addr1, val, 1);
>                      l = 1;
>                  }
> +                object_unref(bk);
>              } else if (!section->readonly) {
>                  ram_addr_t addr1;
>                  addr1 = memory_region_get_ram_addr(section->mr)
> @@ -3464,6 +3501,7 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>                      stb_p(buf, val);
>                      l = 1;
>                  }
> +                object_unref(bk);
>              } else {
>                  /* RAM case */
>                  ptr = qemu_get_ram_ptr(section->mr->ram_addr
> diff --git a/memory.h b/memory.h
> index 740c48e..e5a86dc 100644
> --- a/memory.h
> +++ b/memory.h
> @@ -748,6 +748,8 @@ void memory_global_dirty_log_stop(void);
>  
>  void mtree_info(fprintf_function mon_printf, void *f);
>  
> +void *get_backend(MemoryRegion* mr,  target_phys_addr_t addr);
> +

This is a private interface, shouldn't be in memory.h.


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

Re: [Qemu-devel] [PATCH 2/5] exec.c: use refcnt to protect device during dispatching

[Avi Kivity]

On 07/25/2012 01:58 PM, Avi Kivity wrote:
>>      while (len > 0) {
>>          page = addr & TARGET_PAGE_MASK;
>>          l = (page + TARGET_PAGE_SIZE) - addr;
>>          if (l > len)
>>              l = len;
>> +
>> +        qemu_rwlock_rdlock_devtree();
>>          section = phys_page_find(page >> TARGET_PAGE_BITS);
> 
> Does the devtree lock also protect the data structures accessed by
> phys_page_find()?  Seems wrong.

The right way is to object_ref() in core_region_add() and object_unref()
in core_region_del().  We're guaranteed that mr->object is alive during
_add(), and DeviceClass::unmap() ensures that the extra ref doesn't
block destruction.

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

Re: [Qemu-devel] [PATCH 2/5] exec.c: use refcnt to protect device during dispatching

[liu ping fan]

On Wed, Jul 25, 2012 at 8:27 PM, Avi Kivity <avi@redhat.com> wrote:
> On 07/25/2012 01:58 PM, Avi Kivity wrote:
>>>      while (len > 0) {
>>>          page = addr & TARGET_PAGE_MASK;
>>>          l = (page + TARGET_PAGE_SIZE) - addr;
>>>          if (l > len)
>>>              l = len;
>>> +
>>> +        qemu_rwlock_rdlock_devtree();
>>>          section = phys_page_find(page >> TARGET_PAGE_BITS);
>>
>> Does the devtree lock also protect the data structures accessed by
>> phys_page_find()?  Seems wrong.
>
> The right way is to object_ref() in core_region_add() and object_unref()
> in core_region_del().  We're guaranteed that mr->object is alive during
> _add(), and DeviceClass::unmap() ensures that the extra ref doesn't
> block destruction.
>
OK, I see. I will try in this way.  But when
memory_region_destroy()->..->core_region_del(), should we reset the
lp.ptr to phys_section_unassigned , otherwise, if using  removed
target_phys_addr_t, we will still get the pointer to invalid
MemoryRegion?

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

Re: [Qemu-devel] [PATCH 2/5] exec.c: use refcnt to protect device during dispatching

[Avi Kivity]

On 07/26/2012 04:06 PM, liu ping fan wrote:
> On Wed, Jul 25, 2012 at 8:27 PM, Avi Kivity <avi@redhat.com> wrote:
>> On 07/25/2012 01:58 PM, Avi Kivity wrote:
>>>>      while (len > 0) {
>>>>          page = addr & TARGET_PAGE_MASK;
>>>>          l = (page + TARGET_PAGE_SIZE) - addr;
>>>>          if (l > len)
>>>>              l = len;
>>>> +
>>>> +        qemu_rwlock_rdlock_devtree();
>>>>          section = phys_page_find(page >> TARGET_PAGE_BITS);
>>>
>>> Does the devtree lock also protect the data structures accessed by
>>> phys_page_find()?  Seems wrong.
>>
>> The right way is to object_ref() in core_region_add() and object_unref()
>> in core_region_del().  We're guaranteed that mr->object is alive during
>> _add(), and DeviceClass::unmap() ensures that the extra ref doesn't
>> block destruction.
>>
> OK, I see. I will try in this way.  But when
> memory_region_destroy()->..->core_region_del(), should we reset the
> lp.ptr to phys_section_unassigned , otherwise, if using  removed
> target_phys_addr_t, we will still get the pointer to invalid
> MemoryRegion?

The intent was to use rcu, so when we rebuild phys_map we build it into
a new tree, use rcu_assign_pointer() to switch into the new tree, then
synchronize_rcu() and drop the old tree.

Since we don't have rcu yet we can emulate it with a lock.  We can start
with a simple mutex around the lookup and rebuild, then switch to rwlock
or rcu if needed.

(without the lock or rcu, just changing lp.ptr is dangerous, since it is
a bit field)

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

[Qemu-devel] [PATCH 3/5] hotplug: introduce qdev_unplug_ack() to remove device from views

[Liu Ping Fan]

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

When guest confirm the removal of device, we should
--unmap from MemoryRegion view
--isolated from device tree view

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

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 0aace60..c174247 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -305,8 +305,8 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
             if (pc->no_hotplug) {
                 slot_free = false;
             } else {
-                object_unparent(OBJECT(dev));
-                qdev_free(qdev);
+                /* refcnt will be decreased */
+                qdev_unplug_ack(qdev, NULL);
             }
         }
     }
diff --git a/hw/qdev.c b/hw/qdev.c
index ac55e45..3f7dfa5 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -104,6 +104,14 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
     bus_add_child(bus, dev);
 }
 
+static void qdev_unset_parent(DeviceState *dev)
+{
+    BusState *b = dev->parent_bus;
+
+    object_unparent(OBJECT(dev));
+    bus_remove_child(b, dev);
+}
+
 /* Create a new device.  This only initializes the device state structure
    and allows properties to be set.  qdev_init should be called to
    initialize the actual device emulation.  */
@@ -194,6 +202,26 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
     dev->alias_required_for_version = required_for_version;
 }
 
+static int qdev_unmap(DeviceState *dev)
+{
+    DeviceClass *dc =  DEVICE_GET_CLASS(dev);
+    if (dc->unmap) {
+        dc->unmap(dev);
+    }
+    return 0;
+}
+
+void qdev_unplug_ack(DeviceState *dev, Error **errp)
+{
+    qemu_rwlock_wrlock_devtree();
+    /* isolate from device tree */
+    qdev_unset_parent(dev);
+    /* isolate from mem view */
+    qdev_unmap(dev);
+    qemu_rwlock_unlock_devtree();
+    object_unref(OBJECT(dev));
+}
+
 void qdev_unplug(DeviceState *dev, Error **errp)
 {
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
diff --git a/hw/qdev.h b/hw/qdev.h
index f4683dc..fc5ff02 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -47,7 +47,7 @@ typedef struct DeviceClass {
 
     /* callbacks */
     void (*reset)(DeviceState *dev);
-
+    void (*unmap)(DeviceState *dev);
     /* device state */
     const VMStateDescription *vmsd;
 
@@ -162,6 +162,7 @@ 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);
+void qdev_unplug_ack(DeviceState *dev, Error **errp);
 void qdev_free(DeviceState *dev);
 int qdev_simple_unplug_cb(DeviceState *dev);
 void qdev_machine_creation_done(void);
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 3/5] hotplug: introduce qdev_unplug_ack() to remove device from views

[Avi Kivity]

On 07/25/2012 06:31 AM, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> When guest confirm the removal of device, we should
> --unmap from MemoryRegion view
> --isolated from device tree view
> 
> +
> +void qdev_unplug_ack(DeviceState *dev, Error **errp)
> +{
> +    qemu_rwlock_wrlock_devtree();
> +    /* isolate from device tree */
> +    qdev_unset_parent(dev);
> +    /* isolate from mem view */
> +    qdev_unmap(dev);
> +    qemu_rwlock_unlock_devtree();
> +    object_unref(OBJECT(dev));
> +}

Suggest calling in _complete() instead.

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

[Qemu-devel] [PATCH 4/5] qom: delay DeviceState's reclaim to main-loop

[Liu Ping Fan]

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

iohandler/bh/timer may use DeviceState when its refcnt=0,
postpone the reclaimer till they have done with it.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 include/qemu/object.h |    2 +-
 main-loop.c           |    4 ++++
 main-loop.h           |    2 ++
 qemu-tool.c           |    4 ++++
 qom/Makefile.objs     |    2 +-
 qom/object.c          |    7 ++++++-
 qom/reclaimer.c       |   41 +++++++++++++++++++++++++++++++++++++++++
 7 files changed, 59 insertions(+), 3 deletions(-)
 create mode 100644 qom/reclaimer.c

diff --git a/include/qemu/object.h b/include/qemu/object.h
index 8b17776..b233ee4 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -958,5 +958,5 @@ int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
  */
 Object *container_get(Object *root, const char *path);
 
-
+void qemu_reclaimer_enqueue(Object *obj);
 #endif
diff --git a/main-loop.c b/main-loop.c
index eb3b6e6..f9cecc5 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -505,5 +505,9 @@ int main_loop_wait(int nonblocking)
        them.  */
     qemu_bh_poll();
 
+    /* ref to device from iohandler/bh/timer do not obey the rules, so delay
+     * reclaiming until now.
+     */
+    qemu_device_reclaimer();
     return ret;
 }
diff --git a/main-loop.h b/main-loop.h
index cedddf5..1a59a6d 100644
--- a/main-loop.h
+++ b/main-loop.h
@@ -367,4 +367,6 @@ void qemu_bh_schedule_idle(QEMUBH *bh);
 int qemu_bh_poll(void);
 void qemu_bh_update_timeout(uint32_t *timeout);
 
+void qemu_device_reclaimer(void);
+
 #endif
diff --git a/qemu-tool.c b/qemu-tool.c
index 318c5fc..34d959b 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -75,6 +75,10 @@ void qemu_mutex_unlock_iothread(void)
 {
 }
 
+void qemu_device_reclaimer(void)
+{
+}
+
 int use_icount;
 
 void qemu_clock_warp(QEMUClock *clock)
diff --git a/qom/Makefile.objs b/qom/Makefile.objs
index 5ef060a..a579261 100644
--- a/qom/Makefile.objs
+++ b/qom/Makefile.objs
@@ -1,4 +1,4 @@
-qom-obj-y = object.o container.o qom-qobject.o
+qom-obj-y = object.o container.o qom-qobject.o reclaimer.o
 qom-obj-twice-y = cpu.o
 common-obj-y = $(qom-obj-twice-y)
 user-obj-y = $(qom-obj-twice-y)
diff --git a/qom/object.c b/qom/object.c
index 00bb3b0..227d966 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -649,7 +649,12 @@ void object_unref(Object *obj)
 
     /* parent always holds a reference to its children */
     if (obj->ref == 0) {
-        object_finalize(obj);
+        /* fixme, maybe introduce obj->finalze to make this more elegant */
+        if (object_dynamic_cast(obj, "TYPE_DEVICE") != NULL) {
+            qemu_reclaimer_enqueue(obj);
+        } else {
+            object_finalize(obj);
+        }
     }
 }
 
diff --git a/qom/reclaimer.c b/qom/reclaimer.c
new file mode 100644
index 0000000..2fb3410
--- /dev/null
+++ b/qom/reclaimer.c
@@ -0,0 +1,41 @@
+/*
+ * QEMU DeviceState reclaimer
+ *
+ * Copyright IBM, Corp. 2012
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu-common.h"
+#include "qemu-thread.h"
+#include "main-loop.h"
+#include "qemu/object.h"
+
+typedef struct Chunk {
+    QLIST_ENTRY(Chunk) list;
+    Object *obj;
+} Chunk;
+
+static struct QemuMutex reclaimer_lock;
+static QLIST_HEAD(rcl, Chunk) reclaimer_list;
+
+void qemu_reclaimer_enqueue(Object *obj)
+{
+    Chunk *r = g_malloc0(sizeof(Chunk));
+    r->obj = obj;
+    qemu_mutex_lock(&reclaimer_lock);
+    QLIST_INSERT_HEAD_RCU(&reclaimer_list, r, list);
+    qemu_mutex_unlock(&reclaimer_lock);
+}
+
+void qemu_device_reclaimer(void)
+{
+    Chunk *cur, *next;
+
+    QLIST_FOREACH_SAFE(cur, &reclaimer_list, list, next) {
+        QLIST_REMOVE(cur, list);
+        object_finalize(cur->obj);
+        g_free(cur);
+    }
+}
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 4/5] qom: delay DeviceState's reclaim to main-loop

[Stefan Hajnoczi]

On Wed, Jul 25, 2012 at 4:31 AM, Liu Ping Fan <qemulist@gmail.com> wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> iohandler/bh/timer may use DeviceState when its refcnt=0,
> postpone the reclaimer till they have done with it.
>
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  include/qemu/object.h |    2 +-
>  main-loop.c           |    4 ++++
>  main-loop.h           |    2 ++
>  qemu-tool.c           |    4 ++++
>  qom/Makefile.objs     |    2 +-
>  qom/object.c          |    7 ++++++-
>  qom/reclaimer.c       |   41 +++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 59 insertions(+), 3 deletions(-)
>  create mode 100644 qom/reclaimer.c
>
> diff --git a/include/qemu/object.h b/include/qemu/object.h
> index 8b17776..b233ee4 100644
> --- a/include/qemu/object.h
> +++ b/include/qemu/object.h
> @@ -958,5 +958,5 @@ int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
>   */
>  Object *container_get(Object *root, const char *path);
>
> -
> +void qemu_reclaimer_enqueue(Object *obj);
>  #endif
> diff --git a/main-loop.c b/main-loop.c
> index eb3b6e6..f9cecc5 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -505,5 +505,9 @@ int main_loop_wait(int nonblocking)
>         them.  */
>      qemu_bh_poll();
>
> +    /* ref to device from iohandler/bh/timer do not obey the rules, so delay
> +     * reclaiming until now.
> +     */
> +    qemu_device_reclaimer();
>      return ret;
>  }
> diff --git a/main-loop.h b/main-loop.h
> index cedddf5..1a59a6d 100644
> --- a/main-loop.h
> +++ b/main-loop.h
> @@ -367,4 +367,6 @@ void qemu_bh_schedule_idle(QEMUBH *bh);
>  int qemu_bh_poll(void);
>  void qemu_bh_update_timeout(uint32_t *timeout);
>
> +void qemu_device_reclaimer(void);
> +
>  #endif
> diff --git a/qemu-tool.c b/qemu-tool.c
> index 318c5fc..34d959b 100644
> --- a/qemu-tool.c
> +++ b/qemu-tool.c
> @@ -75,6 +75,10 @@ void qemu_mutex_unlock_iothread(void)
>  {
>  }
>
> +void qemu_device_reclaimer(void)
> +{
> +}
> +
>  int use_icount;
>
>  void qemu_clock_warp(QEMUClock *clock)
> diff --git a/qom/Makefile.objs b/qom/Makefile.objs
> index 5ef060a..a579261 100644
> --- a/qom/Makefile.objs
> +++ b/qom/Makefile.objs
> @@ -1,4 +1,4 @@
> -qom-obj-y = object.o container.o qom-qobject.o
> +qom-obj-y = object.o container.o qom-qobject.o reclaimer.o
>  qom-obj-twice-y = cpu.o
>  common-obj-y = $(qom-obj-twice-y)
>  user-obj-y = $(qom-obj-twice-y)
> diff --git a/qom/object.c b/qom/object.c
> index 00bb3b0..227d966 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -649,7 +649,12 @@ void object_unref(Object *obj)
>
>      /* parent always holds a reference to its children */
>      if (obj->ref == 0) {
> -        object_finalize(obj);
> +        /* fixme, maybe introduce obj->finalze to make this more elegant */
> +        if (object_dynamic_cast(obj, "TYPE_DEVICE") != NULL) {

hw/qdev.h:#define TYPE_DEVICE "device"

This should be object_dynamic_cast(obj, TYPE_DEVICE).

Stefan

Re: [Qemu-devel] [PATCH 4/5] qom: delay DeviceState's reclaim to main-loop

[Paolo Bonzini]

Il 25/07/2012 09:03, Stefan Hajnoczi ha scritto:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> iohandler/bh/timer may use DeviceState when its refcnt=0,

It's not clear how to me.  The only reference to devices from an
iohandler/bh/timer can be in the opaque.  Now, if you have a
iohandler/bh/timer whose opaque is a DeviceState, you should bump the
refcount before setting it up, and unref after tearing it down.

See for example how hw/scsi-disk.c bumps the refcount of a request
before starting asynchronous I/O and calls unref at the end of the
asynchronous I/O callback.

Paolo

Re: [Qemu-devel] [PATCH 4/5] qom: delay DeviceState's reclaim to main-loop

[liu ping fan]

On Wed, Jul 25, 2012 at 3:37 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 25/07/2012 09:03, Stefan Hajnoczi ha scritto:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> iohandler/bh/timer may use DeviceState when its refcnt=0,
>
> It's not clear how to me.  The only reference to devices from an
> iohandler/bh/timer can be in the opaque.  Now, if you have a
> iohandler/bh/timer whose opaque is a DeviceState, you should bump the
> refcount before setting it up, and unref after tearing it down.
>
Yes, I admit refcnt is a good solution, but I think it means that we
will fix it with each device's bh. And this way seems lazy.

Thanx pingfan
> See for example how hw/scsi-disk.c bumps the refcount of a request
> before starting asynchronous I/O and calls unref at the end of the
> asynchronous I/O callback.
>
> Paolo

Re: [Qemu-devel] [PATCH 4/5] qom: delay DeviceState's reclaim to main-loop

[Paolo Bonzini]

Il 25/07/2012 10:16, liu ping fan ha scritto:
> > It's not clear how to me.  The only reference to devices from an
> > iohandler/bh/timer can be in the opaque.  Now, if you have a
> > iohandler/bh/timer whose opaque is a DeviceState, you should bump the
> > refcount before setting it up, and unref after tearing it down.
>
> Yes, I admit refcnt is a good solution, but I think it means that we
> will fix it with each device's bh. And this way seems lazy.

Most of the time the bh/timer/iohandler is created in the init function,
and deleted in the exit function, so in this case the lifecycle is even
easier to manage.

Looking at your patch again, it seems like you're implementing a
poor-man RCU.  So that's fine for a proof-of-concept, but let's replace
it with real RCU sooner or later.

Paolo

Re: [Qemu-devel] [PATCH 4/5] qom: delay DeviceState's reclaim to main-loop

[liu ping fan]

On Wed, Jul 25, 2012 at 3:03 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Wed, Jul 25, 2012 at 4:31 AM, Liu Ping Fan <qemulist@gmail.com> wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> iohandler/bh/timer may use DeviceState when its refcnt=0,
>> postpone the reclaimer till they have done with it.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  include/qemu/object.h |    2 +-
>>  main-loop.c           |    4 ++++
>>  main-loop.h           |    2 ++
>>  qemu-tool.c           |    4 ++++
>>  qom/Makefile.objs     |    2 +-
>>  qom/object.c          |    7 ++++++-
>>  qom/reclaimer.c       |   41 +++++++++++++++++++++++++++++++++++++++++
>>  7 files changed, 59 insertions(+), 3 deletions(-)
>>  create mode 100644 qom/reclaimer.c
>>
>> diff --git a/include/qemu/object.h b/include/qemu/object.h
>> index 8b17776..b233ee4 100644
>> --- a/include/qemu/object.h
>> +++ b/include/qemu/object.h
>> @@ -958,5 +958,5 @@ int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
>>   */
>>  Object *container_get(Object *root, const char *path);
>>
>> -
>> +void qemu_reclaimer_enqueue(Object *obj);
>>  #endif
>> diff --git a/main-loop.c b/main-loop.c
>> index eb3b6e6..f9cecc5 100644
>> --- a/main-loop.c
>> +++ b/main-loop.c
>> @@ -505,5 +505,9 @@ int main_loop_wait(int nonblocking)
>>         them.  */
>>      qemu_bh_poll();
>>
>> +    /* ref to device from iohandler/bh/timer do not obey the rules, so delay
>> +     * reclaiming until now.
>> +     */
>> +    qemu_device_reclaimer();
>>      return ret;
>>  }
>> diff --git a/main-loop.h b/main-loop.h
>> index cedddf5..1a59a6d 100644
>> --- a/main-loop.h
>> +++ b/main-loop.h
>> @@ -367,4 +367,6 @@ void qemu_bh_schedule_idle(QEMUBH *bh);
>>  int qemu_bh_poll(void);
>>  void qemu_bh_update_timeout(uint32_t *timeout);
>>
>> +void qemu_device_reclaimer(void);
>> +
>>  #endif
>> diff --git a/qemu-tool.c b/qemu-tool.c
>> index 318c5fc..34d959b 100644
>> --- a/qemu-tool.c
>> +++ b/qemu-tool.c
>> @@ -75,6 +75,10 @@ void qemu_mutex_unlock_iothread(void)
>>  {
>>  }
>>
>> +void qemu_device_reclaimer(void)
>> +{
>> +}
>> +
>>  int use_icount;
>>
>>  void qemu_clock_warp(QEMUClock *clock)
>> diff --git a/qom/Makefile.objs b/qom/Makefile.objs
>> index 5ef060a..a579261 100644
>> --- a/qom/Makefile.objs
>> +++ b/qom/Makefile.objs
>> @@ -1,4 +1,4 @@
>> -qom-obj-y = object.o container.o qom-qobject.o
>> +qom-obj-y = object.o container.o qom-qobject.o reclaimer.o
>>  qom-obj-twice-y = cpu.o
>>  common-obj-y = $(qom-obj-twice-y)
>>  user-obj-y = $(qom-obj-twice-y)
>> diff --git a/qom/object.c b/qom/object.c
>> index 00bb3b0..227d966 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -649,7 +649,12 @@ void object_unref(Object *obj)
>>
>>      /* parent always holds a reference to its children */
>>      if (obj->ref == 0) {
>> -        object_finalize(obj);
>> +        /* fixme, maybe introduce obj->finalze to make this more elegant */
>> +        if (object_dynamic_cast(obj, "TYPE_DEVICE") != NULL) {
>
> hw/qdev.h:#define TYPE_DEVICE "device"
>
> This should be object_dynamic_cast(obj, TYPE_DEVICE).
>
Yes, thanks.
> Stefan

[Qemu-devel] [PATCH 5/5] e1000: using new interface--unmap to unplug

[Liu Ping Fan]

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

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

diff --git a/hw/e1000.c b/hw/e1000.c
index 4573f13..4c1e141 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -1192,6 +1192,18 @@ e1000_cleanup(VLANClientState *nc)
     s->nic = NULL;
 }
 
+static void
+pci_e1000_unmap(DeviceState *dev)
+{
+    PCIDevice *p = PCI_DEVICE(dev);
+    E1000State *d = DO_UPCAST(E1000State, dev, p);
+
+    /* DO NOT FREE anything!until refcnt=0 */
+    /* isolate from memory view */
+    memory_region_destroy(&d->mmio);
+    memory_region_destroy(&d->io);
+}
+
 static int
 pci_e1000_uninit(PCIDevice *dev)
 {
@@ -1199,8 +1211,6 @@ pci_e1000_uninit(PCIDevice *dev)
 
     qemu_del_timer(d->autoneg_timer);
     qemu_free_timer(d->autoneg_timer);
-    memory_region_destroy(&d->mmio);
-    memory_region_destroy(&d->io);
     qemu_del_vlan_client(&d->nic->nc);
     return 0;
 }
@@ -1283,6 +1293,7 @@ static void e1000_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_NETWORK_ETHERNET;
     dc->desc = "Intel Gigabit Ethernet";
     dc->reset = qdev_e1000_reset;
+    dc->unmap = pci_e1000_unmap;
     dc->vmsd = &vmstate_e1000;
     dc->props = e1000_properties;
 }
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 5/5] e1000: using new interface--unmap to unplug

[Stefan Hajnoczi]

On Wed, Jul 25, 2012 at 4:31 AM, Liu Ping Fan <qemulist@gmail.com> wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/e1000.c |   15 +++++++++++++--
>  1 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 4573f13..4c1e141 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -1192,6 +1192,18 @@ e1000_cleanup(VLANClientState *nc)
>      s->nic = NULL;
>  }
>
> +static void
> +pci_e1000_unmap(DeviceState *dev)
> +{
> +    PCIDevice *p = PCI_DEVICE(dev);
> +    E1000State *d = DO_UPCAST(E1000State, dev, p);
> +
> +    /* DO NOT FREE anything!until refcnt=0 */
> +    /* isolate from memory view */
> +    memory_region_destroy(&d->mmio);
> +    memory_region_destroy(&d->io);
> +}

It's not obvious to me why a 2-stage cleanup is needed (->unmap(),
->exit()).  Explaining things a bit more in the commit description
would help.  Here's what I'm thinking:

We want to remove the memory regions at the same time as removing the
device from the tree, but ->exit() is only called when the object is
finalized.  Because of the object reference held during dispatch, the
reference might not reach 0 during hotplug and another thread could
still be running this device's code?

This series only applies this change to e1000 and piix pci hotplug.
How/when will all the other devices be converted?  Will it be safe to
leave them unconverted once dispatch really happens in parallel?

Stefan