Patchwork [1/5] qom: adopt rwlock to protect accessing dev from removing it

login
register
mail settings
Submitter pingfan liu
Date July 25, 2012, 3:31 a.m.
Message ID <1343187070-27371-2-git-send-email-qemulist@gmail.com>
Download mbox | patch
Permalink /patch/173097/
State New
Headers show

Comments

pingfan liu - July 25, 2012, 3:31 a.m.
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(-)
Paolo Bonzini - July 25, 2012, 9:08 a.m.
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
pingfan liu - July 26, 2012, 12:56 p.m.
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
>
Avi Kivity - July 26, 2012, 1 p.m.
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.
pingfan liu - July 26, 2012, 1:14 p.m.
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
>
>
Avi Kivity - July 26, 2012, 1:15 p.m.
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?
pingfan liu - July 26, 2012, 1:21 p.m.
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
>
>
Avi Kivity - July 26, 2012, 1:46 p.m.
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.

Patch

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)