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

[Anthony Liguori]

Jan Kiszka <address@hidden> writes:

> 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.

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