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

[Jan Kiszka]

On 2012-08-27 18:24, Anthony Liguori wrote:
> 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.

Pulling locks out of the devices may appear simpler on first sight but
won't be in practice. Objects can issue requests to other objects. Will
they hold its lock while taking the target's lock? Lock management is a
per-device thing, not only for performance reasons.

But I guess we will have to compare working prototypes. Many concepts
look nice on the paper, but when you implement them or even try to run
them, the interesting problems show up. Specifically those related to a
smooth transition phase.

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

The timer subsystem is lockless in this sense already. Its user holds
the necessary lock (BQL so far). I'm just making it multi-instance so
that the time-critical users don't need to worry about other, low-prio
timers. In that sense, I guess we are describing the same end result.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux