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

[Anthony Liguori <anthony@codemonkey.ws>]

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