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 <address@hidden> writes:
> 
>> On 2012-08-27 15:19, Anthony Liguori wrote:
>>> Liu Ping Fan <address@hidden> writes:
>>>
>>>> From: Liu Ping Fan <address@hidden>
>>>>
>>>> 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 <address@hidden>
>>>
>>> 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